jackrabbit-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Guggisberg <stefan.guggisb...@gmail.com>
Subject Re: Checkstyle improvements
Date Wed, 27 Apr 2005 15:46:29 GMT
On 4/27/05, Jukka Zitting <jz@yukatan.fi> wrote:
> Hi all,
> 
> After a couple of largish commits I've now fixed most of the trivial
> Checkstyle warnings (see JCR-73). The remaining 241 warnings (excluding
> Javadoc issues) are more related to code design than code formatting.
> I'd like to raise these issues for comments before unilaterally messing
> with the code.
> 
> We already discussed one issue related to inline conditionals in
> private. Checkstyle suggests to avoid all inline conditionals, but
> especially in some equals() methods they are very handy.
> 
> Another, more essential issue is related to the use of protected member
> variables. Checkstyle currently reports 161 protected member variables
> in Jackrabbit. The canonical suggestion is to make the variables private
> and provide accessor methods as needed. Should Jackrabbit follow this
> guideline or should we simply disable the related Checkstyle check?

hmm... i think fields from outside a class should only be accessed
through an accessor method, i.e. obj.getAttr() rather than obj.attr;
i tried to follow this rule. 

on the other hand i don't see the value in unconditionally requiring 
a derived class to access base class fields through an accessor.

checkstyle complains e.g. about the the 'id' member in ItemImpl:

    protected final ItemId id;

an accessor would imo be overkill.

therefore i suggest to disable the related Checkstyle check.

> 
> There are also some isolated issues like equals() methods without
> corresponding hashCode() methods and switch statements without default
> cases that might cause unexpected trouble in some situations.

regarding missing hashCode(): i intentionally do never override hashCode()
for mutable objects. hashCode() should imo only be implemented for 
immutable objects. 

i suggest to disable the related Checkstyle check.

in general: i think that checkstyle is a good tool that helps to improve the 
quality and consistency of the code base. but we should use common 
sense when interpreting the recommendations. blindly following all the 
recommendations or or trying to achieve 0 reported issues is imo not 
worthwhile.

> 
> PS. Is there a reason why the Checkstyle report is disabled in
> project.xml?

checkstyle reporting too many issues maybe?;) or shorter build times...

cheers
stefan

> 
> BR,
> 
> Jukka Zitting
> 
>

Mime
View raw message