Polishing some codes #63
Conversation
| log.debug(String.format("No %s found.", MapperFactoryBean.class.getName())); | ||
| if (log.isDebugEnabled()) { | ||
| log.debug(String.format("No %s found.", MapperFactoryBean.class.getName())); | ||
| } |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for merging !!
I agree with your opinion 👍
2 participants
I've polished some codes.
+operationisDebugEnabledPlease review.
Thanks.