I think this pattern is a holdover from using log4j 1. In that version, you
added an if check to avoid unnecessary string concatenation if debug was
enabled.
1. if (logger.isDebugEnabled()) {
2. logger.debug("Logging in user " + user.getName() + " with
birthday " + user.getBirthdayCalendar());
3. }
Log4j2 lets you pass a format string, so you can just do this:
logger.debug("Logging in user {} with birthday {}", user.getName(),
user.getBirthdayCalendar());
These examples come from the log4j2 docs:
https://logging.apache.org/log4j/2.0/manual/api.html
-Dan
On Fri, Sep 7, 2018 at 10:50 AM, Galen O'Sullivan <gosulli...@apache.org>
wrote:
> Hi all,
>
> I've noticed a pattern in Geode where we wrap a log call in a check at the
> same level, such as:
>
> if (logger.isDebugEnabled()) {
> logger.debug("cleaning up incompletely started
> DistributionManager due to exception", r);
> }
>
> Is there any reason to do this? To my mind, it's an extra conditional that
> should essentially be the first check inside `logger.debug(...)` anyways,
> and it complicates the code and makes it less readable. I've even seen
> places in the code which have `if (logger.isDebugEnabled())
> logger.trace(...))` and such.
>
> I would like to propose that unless there is a compelling reason to use
> this pattern, we remove all extra checks entirely.
>
> Galen
>
