Skip to content

Polishing some codes #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 23, 2016
Merged

Polishing some codes #63

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions ...nfigure/src/main/java/org/mybatis/spring/boot/autoconfigure/MybatisAutoConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public void checkConfigFileExists() {
if (this.properties.isCheckConfigLocation() && StringUtils.hasText(this.properties.getConfigLocation())) {
Resource resource = this.resourceLoader.getResource(this.properties.getConfigLocation());
Assert.state(resource.exists(), "Cannot find config location: " + resource
+ " (please add config file or check your Mybatis " + "configuration)");
+ " (please add config file or check your Mybatis configuration)");
}
}

Expand Down Expand Up @@ -167,16 +167,18 @@ public void registerBeanDefinitions(AnnotationMetadata importingClassMetadata, B
scanner.setResourceLoader(this.resourceLoader);
}

List<String> pkgs = AutoConfigurationPackages.get(this.beanFactory);
for (String pkg : pkgs) {
log.debug("Using auto-configuration base package '" + pkg + "'");
List<String> packages = AutoConfigurationPackages.get(this.beanFactory);
if (log.isDebugEnabled()) {
for (String pkg : packages) {
log.debug("Using auto-configuration base package '" + pkg + "'");
}
}

scanner.setAnnotationClass(Mapper.class);
scanner.registerFilters();
scanner.doScan(StringUtils.toStringArray(pkgs));
scanner.doScan(StringUtils.toStringArray(packages));
} catch (IllegalStateException ex) {
log.debug("Could not determine auto-configuration " + "package, automatic mapper scanning disabled.");
log.debug("Could not determine auto-configuration package, automatic mapper scanning disabled.");
}
}

Expand Down Expand Up @@ -206,7 +208,9 @@ public static class MapperScannerRegistrarNotFoundConfiguration {

@PostConstruct
public void afterPropertiesSet() {
log.debug(String.format("No %s found.", MapperFactoryBean.class.getName()));
if (log.isDebugEnabled()) {
log.debug(String.format("No %s found.", MapperFactoryBean.class.getName()));
}
Copy link
Member

@hazendaz hazendaz Apr 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kazuki43zoo I don't really see any savings in this specific one. I understand the prior want to check for debug since there is an otherwise expensive looping on logging. However, that isn't the case here. I don't think this is going to amount to much in savings. Am I missing something?
Copy link
Member Author

@kazuki43zoo kazuki43zoo Apr 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @hazendaz , thanks for your comment.

In this case, your understanding and opinion is correct.
However, i think general coding style to use the isDebugEnable method when debug message is dynamically.

What do you think ?
Copy link
Member

@hazendaz hazendaz Apr 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree if the logging hit was needed and generally I see string format in a logger as bad practice. After looking at the class a bit, I see an obvious flaw in logging being used. It's using apache commons logging (legacy) but also has slf4j available. If we switched this to use slf4j instead, the cost can be incurred inside the debug statement rather than external to it. However, that is beyond this pull request so for the time being this is ok. I'm merging it now. Thanks.
Copy link
Member Author

@kazuki43zoo kazuki43zoo Apr 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for merging !!
I agree with your opinion 👍
}
}

Expand Down