drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chris Westin <cwes...@yahoo.com>
Subject Re: Loggers should be private
Date Tue, 03 Mar 2015 18:53:24 GMT
Re: "If they are
private you get an unused exception unless you suppress the warning. Making
them package private means no warning and no suppression required."

Given the enormous number of other warnings that are present, this seems
pretty weak. And yes, I've been cleaning up all those warnings as I go
along -- removing unused imports, variables, and members. We I find an
unused (now) private logger, I just comment it out. If someone needs it,
they can uncomment it. We really need to keep the code clean; all the
warnings were obscuring real problems I found (and fixed) with findbugs.

Re: "Does this include making existing ones private as well? It would be
nice if
those who are touching a piece of code make non-private loggers private."

That's what I've been doing. That's how this topic came up, in a code
review.


On Mon, Mar 2, 2015 at 8:51 PM, Ted Dunning <ted.dunning@gmail.com> wrote:

>
> Sound like a good idea.
>
> Sent from my iPhone
>
> > On Mar 2, 2015, at 19:26, Chris Westin <cwestin@maprtech.com> wrote:
> >
> > I've noticed in the Drill codebase that loggers are being declared static
> > final, but not private. Based on my past experience, this doesn't match
> > common
> > practice.
> >
> > Loggers should be private:
> >
> > (1) It's common practice. I've never seen otherwise in the past. A quick
> > search turned up these examples in the top search results:
> > - http://www.vogella.com/tutorials/Logging/article.html
> > -
> >
> http://examples.javacodegeeks.com/core-java/util/logging/java-util-logging-example/
> >
> > (2) It prevents accidentally using the logger in a derived class. I've
> been
> > sent off to the wrong place a few times when looking at logs for the
> Drill
> > code
> > base, because some derived classes have (accidentally?) used their
> > super-classes' loggers. Since the loggers aren't private, this isn't
> > prevented.
> > This gave me the wrong information about where to go looking for messages
> > associated with a problem.
> >
> > (3) Loggers are meant to be associated with exactly one class; that's why
> > they
> > take the class as an argument. Since they're not meant to be used by
> another
> > class, there's no reason for them to have greater visibility. (There
> > certainly
> > aren't any examples of code that use "OtherClass.logger....") In order to
> > reduce the fragility of code, we reduce the visible surface of classes
> (aka
> > "encapsulation"), and that means giving everything the least visibility
> that
> > it requires -- in this case, "private."
> >
> > (4) Private loggers reveal when they aren't being used, at least in IDEs.
> > When they're private, and they aren't used, we can comment them out
> > //  private final static Logger logger = ....;
> > and avoid allocating them when we don't need them. If we need them, then
> we
> > can uncomment them.
> >
> > Does anyone object to making all the loggers in the Drill codebase
> private
> > from now on?
> >
> > Chris Westin
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message