Hi Adam, 2009/3/6 Adam Winer <[email protected]>: >> } catch (IllegalArgumentException e) { >> response.setStatus(HttpServletResponse.SC_BAD_REQUEST); >> - logger.log(Level.INFO, e.getMessage(), e); >> + if (!logger.isLoggable(Level.OFF)) { >> + if (logger.isLoggable(Level.INFO)) { >> + logger.log(Level.INFO, e.getMessage()); >> + } else { >> + logger.log(Level.INFO, e.getMessage(), e); >> + } >> + } > > This code pattern is strange, for a few reasons: > > if logger.isLoggable(Level.INFO) returns false, there's no reason > calling log at Level.INFO. It's a no-op. So kill that branch.
I am not an expert of JUL, so I trust you. > Second, I don't see a reason to check both Level.OFF and Level.INFO - > just check Level.INFO. If Level.OFF is set, isLoggable(Level.INFO) > will return false. again not a JUL expert but without jvm property java.util.logging.config.file ie without a level specified isLoggable(Level.INFO) = true isLoggable(Level.OFF) = true > Third, it's a little unusual to bother with checking isLoggable() it depends on the conventions of projects on which I contribute... > unless you're performing significant work within the logging statement > (string concatenation as the usual example). There's none of that > here (unless you had an exception class that had overridden > getMessage(), which is odd.) > > Basically, those seven lines can be reduced to: > > logger.log(Level.INFO, e.getMessage()) > ... or just: > logger.info(e.getMessage()); I like this kind of productivity :) I will fix it in trunk and branch. WDYT to update the java style page in the wiki? Cheers, Vincent
