On Fri, Mar 6, 2009 at 11:47 AM, Vincent Siveton <[email protected]> wrote: > 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
isLoggable(Level.INFO) implies isLoggable(Level.OFF), because Level.OFF is Integer.MAX_VALUE. Configuring logging to be at "Level.OFF" implies Level.SEVERE, Level.WARNING, etc.. will all not be logged. > >> Third, it's a little unusual to bother with checking isLoggable() > > it depends on the conventions of projects on which I contribute... It shouldn't be a per-project convention - this is just basics of how JUL works. Wrapping a trivial call to log() inside an isLoggable() call is a waste of time, since the log() methods all check isLoggable() themselves. It's only if you're doing work (creating an array, string concatenation) that it makes any sense to check isLoggable(). >> 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? The Java style page should be limited to stylistic decisions, which this isn't. This is just understanding the Java libraries, which would take thousands of pages to describe. ;) Cheers, Adam > > Cheers, > > Vincent >
