Loggers should be private

[Chris Westin <cw...@maprtech.com>]

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

Re: Loggers should be private

[Chris Westin <cw...@yahoo.com>]

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 <te...@gmail.com> wrote:

>
> Sound like a good idea.
>
> Sent from my iPhone
>
> > On Mar 2, 2015, at 19:26, Chris Westin <cw...@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
>

Re: Loggers should be private

[Ted Dunning <te...@gmail.com>]

Sound like a good idea. 

Sent from my iPhone

> On Mar 2, 2015, at 19:26, Chris Westin <cw...@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