logging-log4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nick Williams <nicho...@nicholaswilliams.net>
Subject Re: Final (again) (was Re: r1501104)
Date Wed, 10 Jul 2013 01:13:34 GMT
I see.

I imagine we could have this discussion every time a new developer comes on board. :-)

So, I see the points they're making, and I don't necessarily disagree with all of them ipso
facto. I, personally, always try to make instance variables final when appropriate. However,
I also think that each developer knows best the intent of his code, so if we want to put this
"always use final when possible" practice into place, the best way to do that is to train
developers to use it. Put it in the developer guidelines for contributors and committers,
etc. This blanket changing of dozens of files that the changer has no knowledge of and isn't
even reviewing individually concern me.

And that doesn't even address the fact that these changes increase the Checkstyle errors.
The additional of the six characters per variable (in a method with many parameters, that's
a lot of characters) causes some lines of code to exceed the 120 character limit and trigger
Checkstyle.

Finally, even /if/ I were to agree to blanket changes like this, I think they were done at
the wrong time. At $work, we have a policy about such changes: Blanket reformats and policy
changes are always made at the /beginning/ of a development cycle (immediately after a release)
as opposed to at the /end/ of a development cycle (immediately before a release, as GG has
done with MANY changes in the last few days). While such changes /shouldn't/ change program
logic, tools (Eclipse) are only as good as the people who write them (don't get me started
on my opinion of Eclipse) and can make mistakes. We shouldn't be doing things like this right
before a release.

My $0.02.

Nick

On Jul 9, 2013, at 7:14 PM, Ralph Goers wrote:

> This topic was covered in an email thread that started with this email - http://mail-archives.apache.org/mod_mbox/logging-log4j-dev/201301.mbox/%3CCACZkXPwsDyD-X%2B5wHuq4X26wzCZnsHJ%3Ds5rLJNKe71XYyg6LfA%40mail.gmail.com%3E.
 Unfortunately I don't know how to link the whole thread from the apache archives but you
can find it at http://marc.info/?t=134978818500007&r=1&w=2.
> 
> I happen to agree with you, as you will see from the thread.  I find it more annoying
that many, many files were changed at once with no warning.  I also suspect Gary does this
through something automated in Eclipse, which means he doesn't see the comments that say "don't
do that".
> 
> Ralph
> 
> On Jul 9, 2013, at 4:41 PM, Nick Williams wrote:
> 
>> Gary,
>> 
>> Earlier today you committed r1501104 with the following comment:
>> 
>>> Add final modifier to private fields.
>>> Add final modifier to method parameters.
>>> Add final modifier to local variables.
>> 
>> I've seen you do similar things before, too. Can you explain a little bit why you
do this? There's certainly no compiler value--the compiler for a long time has been capable
of determining whether a variable is ever changed and optimizing appropriately. All it seems
to do to me is clutter the code...significantly. It makes the code harder to read, IMO.
>> 
>> Can you justify the intentional mass change of this nature?
>> 
>> Thanks,
>> 
>> Nick
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
>> For additional commands, e-mail: log4j-dev-help@logging.apache.org
>> 
> 


Mime
View raw message