[logback-dev] field encapsulation in logback classes

[ceki]

Hi Carl,

While reading "Clean code: A Handbook of Agile Software Craftsmanship"
by Robert C. Martin, I often had to admit to myself that logback code
violates several of the rules mentioned in the book. I take pride in
the fact that the project's code is fairly DRY. On the other hand, it
fails in the "one level of abstraction per function" and the "no
surprises" departments. Method and class names could have been better
chosen as well, especially in unit tests.

As for mutable fields in SyslogAppenderBase and elsewhere, I should
note that most fields are "package private" which, compared to the
'public' modifier, greatly limits exposure. Moreover, "package
private" is considered more restrictive than 'protected'. Thus,
content with "package private", I tend to leave out the private
keyword out of laziness. Nevertheless, it's not a practice I actually
advocate or would want to defend further. In other words, I agree that
your proposal constitutes better practice.

There are probably many other improvements that could be applied to
the code. Writing good code takes several iterations. Whenever you can
improve the code, please come forward as you have done with field
encapsulation.


On 25.04.2013 20:14, Harris, Carl wrote:

 > There are apparently many examples of fields in logback classes that
 > are not private to the class implementation.  For example,
 > SyslogAppenderBase contains seven examples of mutable fields that are
 > not private.  Best practice would suggest that unless there's a really
 > good reason to break encapsulation by exposing such fields, they
 > should be private.  In the case of SyslogAppenderBase there appears to
 > be no reason for exposing most of these fields, apparently the
 > "private" modifier was omitted as an oversight.  Properly marking
 > fields as private is a significant aid to project contributors, in
 > that the scope of the field is explicit and limited.  Moreover, it is
 > much easier to contemplate making changes to a properly encapsulated
 > class.
 >
 > I propose that as a general practice, logback contributors should mark
 > all mutable fields as private and expose properly documented accessor
 > methods with appropriate access modifiers as a better alternative to
 > allowing field state to directly accessed and possibly modified.
 >
 > Comments or opposing opinions?

 > carl
>
-- 
Ceki
65% of statistics are made up on the spot