drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Barclay <dbarc...@maprtech.com>
Subject Re: Loggers should be private
Date Thu, 05 Mar 2015 20:10:00 GMT
Jacques Nadeau wrote:
> It's a dumb answer but there was a reason. I think they should be available
> in every class so I add them to the my new class template. 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.

Why not include the warning-suppression annotation on the declaration
of the logger field in your template that inserts that logger
declaration?


Daniel


 > ...
> On Mar 2, 2015 8:03 PM, "Steven Phillips" <sphillips@maprtech.com> wrote:
>
>> I have no objection to making the loggers private, and your reasons make
>> sense to me. But I do wonder if there was a reason for not making them
>> private to begin with.
>>
>> On Mon, Mar 2, 2015 at 12:27 PM, Aditya <adi@apache.org> wrote:
>>
>>> Sounds good to me.
>>>
>>> On Mon, Mar 2, 2015 at 11:28 AM, Chris Westin <chriswestin42@gmail.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
>>>>
>>>
>>
>>
>>
>> --
>>   Steven Phillips
>>   Software Engineer
>>
>>   mapr.com
>>
>


-- 
Daniel Barclay
MapR Technologies

Mime
View raw message