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