You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by Jukka Zitting <jz...@yukatan.fi> on 2005/04/06 12:31:55 UTC
Checkstyle improvements
Hi all,
How about if I opened a Checkstyle improvement meta-issue in Jira? Like
the Javadoc issue JCR-73, it could be used as a general label for simple
style improvement changes that have no functional effects.
Checkstyle currently reports thousands of trivial problems like unused
imports in Jackrabbit. Getting rid of these trivial issues would make
the Checkstyle tool a lot more useful in spotting changes for genuine
improvements.
BR,
Jukka Zitting
Re: Checkstyle improvements
Posted by Stefan Guggisberg <st...@gmail.com>.
good idea, +1
On Apr 6, 2005 12:31 PM, Jukka Zitting <jz...@yukatan.fi> wrote:
> Hi all,
>
> How about if I opened a Checkstyle improvement meta-issue in Jira? Like
> the Javadoc issue JCR-73, it could be used as a general label for simple
> style improvement changes that have no functional effects.
>
> Checkstyle currently reports thousands of trivial problems like unused
> imports in Jackrabbit. Getting rid of these trivial issues would make
> the Checkstyle tool a lot more useful in spotting changes for genuine
> improvements.
>
> BR,
>
> Jukka Zitting
>
Re: Checkstyle improvements
Posted by Felix Meschberger <Fe...@day.com>.
Hi all,
Just my €.02 on this ..
Jukka Zitting schrieb:
>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.
>
>
I personally like inline conditionals very much because they look a lot
less cumbersome than the equivalent if-else statement.
>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?
>
>
I would vote +1 for private fields with protected accessors. It provides
better encapsulation and future-proofness. Generally I like accessors a
lot more than public/protected fields.
>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.
>
>
I would vote +1 for these to be fixed, especially the hashCode issues.
Regards,
Felix
Re: Checkstyle improvements
Posted by Elliot Metsger <em...@jhu.edu>.
I'm not a Jackrabbit developer so take my .02 for what they are worth! :)
Tobias Strasser wrote:
> you are right. just reading the javadoc for hashCode() again.
>
> * <li>Whenever it is invoked on the same object more than once during
> * an execution of a Java application, the <tt>hashCode</tt> method
> * must consistently return the same integer, provided no information
> * used in <tt>equals</tt> comparisons on the object is modified.
> * This integer need not remain consistent from one execution of an
> * application to another execution of the same application.
>
> the important part here is:
> ...provided no information used in <tt>equals</tt> comparisons on
> the object is modified....
I generally prefer to compute hash code on the same fields that equals
relies on.
> so, either we are lazy, and return '1' or we compute the hashcode
> depending on the same fields, that equals relies on.
Recognizing that in HashSets this will provide an uneven distribution of
objects in hashbuckets (the objects in the set will all have hashcode of
1) which may impact performance depending on how large the collection
is. If you return '1', perhaps a warning in the javadoc about placing
these objects in Sets.
> the problem of putting mutable objects into collections is something
> completely different.
I agree.
> cheers, tobi
>
> btw: for example, take a look at the source code of
> 'java.util.Calendar'. there the hashcode and equals also depend on
> mutable fields.
Will do, sounds like an interesting excercise!
Elliot
>
> On 4/28/05, Felix Meschberger <Fe...@day.com> wrote:
>
>>
>>>>+1 for overriding hashCode() returning a constant integer
>>>>for mutable classes. i don't fully agree with the javadoc though.
>>>>if you implement hashCode() 'correctly', i.e. calculate the hashcode
>>>>based on the objects fields, you risk losing track of data in a
>>>>hash table.
>>>>
>>>>mutable objects should imo never be managed in hash tables.
>>>>returning a constant integer is therefore fine with me for mutable
>>>>objects.
>>>>
>>>>
>>
>>What do you mean by "not be managed in hash tables" ? Of course, objects
>>whose equality state - fields compared by equals() - changes during
>>their live time should not be used as KEYS in maps. But they may of
>>course be perfectly used as VALUES.
>>
>>Now, it makes perfect sense to not use variable KEYS. This is not
>>specific to java, this is also true for any database application.
>>
>>
>>>well, when the equality depends on mutable data, the object cannot be
>>>used in sets at all.
>>>
>>
>>True, or as the JavaDoc of the Set interface states:
>>
>> Note: Great care must be exercised if mutable objects are used as
>> set elements. The behavior of a set is not specified if the value
>> of an object is changed in a manner that affects equals comparisons
>> while the object isan element in the set. A special case of this
>> prohibition is that it is not permissible for a set to contain
>> itself as an element.
>>
>>
>>>so no need to return '1' as hashcode. if the
>>>object cannot be used in set, i would rather throw a
>>>UnsuportedOperationException:
>>>
>>>public int hashCode() {
>>> thrown new UnsupportedOperationException("unable to compute hashcode");
>>>}
>>>
>>>
>>
>>There is no need for that ! Simply document the class to be mutable
>>regarding equality and the rest is in the responsibility of the programmer !
>>
>>In fact the proposed behavi is very dangerous and may lead to
>>unexplainable situations.
>>
>>Yes, I am picky on that. But this is not an issue of style but an issue
>>of correctness !
>>
>>Regards
>>Felix
>>
>>
>
>
>
Re: Checkstyle improvements
Posted by Tobias Strasser <to...@gmail.com>.
you are right. just reading the javadoc for hashCode() again.
* <li>Whenever it is invoked on the same object more than once during
* an execution of a Java application, the <tt>hashCode</tt> method
* must consistently return the same integer, provided no information
* used in <tt>equals</tt> comparisons on the object is modified.
* This integer need not remain consistent from one execution of an
* application to another execution of the same application.
the important part here is:
...provided no information used in <tt>equals</tt> comparisons on
the object is modified....
so, either we are lazy, and return '1' or we compute the hashcode
depending on the same fields, that equals relies on.
the problem of putting mutable objects into collections is something
completely different.
cheers, tobi
btw: for example, take a look at the source code of
'java.util.Calendar'. there the hashcode and equals also depend on
mutable fields.
On 4/28/05, Felix Meschberger <Fe...@day.com> wrote:
>
>
> >>+1 for overriding hashCode() returning a constant integer
> >>for mutable classes. i don't fully agree with the javadoc though.
> >>if you implement hashCode() 'correctly', i.e. calculate the hashcode
> >>based on the objects fields, you risk losing track of data in a
> >>hash table.
> >>
> >>mutable objects should imo never be managed in hash tables.
> >>returning a constant integer is therefore fine with me for mutable
> >>objects.
> >>
> >>
> What do you mean by "not be managed in hash tables" ? Of course, objects
> whose equality state - fields compared by equals() - changes during
> their live time should not be used as KEYS in maps. But they may of
> course be perfectly used as VALUES.
>
> Now, it makes perfect sense to not use variable KEYS. This is not
> specific to java, this is also true for any database application.
>
> >well, when the equality depends on mutable data, the object cannot be
> >used in sets at all.
> >
> True, or as the JavaDoc of the Set interface states:
>
> Note: Great care must be exercised if mutable objects are used as
> set elements. The behavior of a set is not specified if the value
> of an object is changed in a manner that affects equals comparisons
> while the object isan element in the set. A special case of this
> prohibition is that it is not permissible for a set to contain
> itself as an element.
>
> > so no need to return '1' as hashcode. if the
> >object cannot be used in set, i would rather throw a
> >UnsuportedOperationException:
> >
> >public int hashCode() {
> > thrown new UnsupportedOperationException("unable to compute hashcode");
> >}
> >
> >
> There is no need for that ! Simply document the class to be mutable
> regarding equality and the rest is in the responsibility of the programmer !
>
> In fact the proposed behavi is very dangerous and may lead to
> unexplainable situations.
>
> Yes, I am picky on that. But this is not an issue of style but an issue
> of correctness !
>
> Regards
> Felix
>
>
--
------------------------------------------< tobias.strasser@day.com >---
Tobias Strasser, Day Management AG, Barfuesserplatz 6, CH - 4001 Basel
T +41 61 226 98 98, F +41 61 226 98 97
-----------------------------------------------< http://www.day.com >---
Re: Checkstyle improvements
Posted by Felix Meschberger <Fe...@day.com>.
>>+1 for overriding hashCode() returning a constant integer
>>for mutable classes. i don't fully agree with the javadoc though.
>>if you implement hashCode() 'correctly', i.e. calculate the hashcode
>>based on the objects fields, you risk losing track of data in a
>>hash table.
>>
>>mutable objects should imo never be managed in hash tables.
>>returning a constant integer is therefore fine with me for mutable
>>objects.
>>
>>
What do you mean by "not be managed in hash tables" ? Of course, objects
whose equality state - fields compared by equals() - changes during
their live time should not be used as KEYS in maps. But they may of
course be perfectly used as VALUES.
Now, it makes perfect sense to not use variable KEYS. This is not
specific to java, this is also true for any database application.
>well, when the equality depends on mutable data, the object cannot be
>used in sets at all.
>
True, or as the JavaDoc of the Set interface states:
Note: Great care must be exercised if mutable objects are used as
set elements. The behavior of a set is not specified if the value
of an object is changed in a manner that affects equals comparisons
while the object isan element in the set. A special case of this
prohibition is that it is not permissible for a set to contain
itself as an element.
> so no need to return '1' as hashcode. if the
>object cannot be used in set, i would rather throw a
>UnsuportedOperationException:
>
>public int hashCode() {
> thrown new UnsupportedOperationException("unable to compute hashcode");
>}
>
>
There is no need for that ! Simply document the class to be mutable
regarding equality and the rest is in the responsibility of the programmer !
In fact the proposed behavi is very dangerous and may lead to
unexplainable situations.
Yes, I am picky on that. But this is not an issue of style but an issue
of correctness !
Regards
Felix
Re: Checkstyle improvements
Posted by Jukka Zitting <jz...@yukatan.fi>.
Hi,
Very good points, both Stefan and Tobias.
Tobias:
> well, when the equality depends on mutable data, the object cannot be
> used in sets at all. so no need to return '1' as hashcode. if the
> object cannot be used in set, i would rather throw a
> UnsuportedOperationException:
Sounds good. OK if I add the following snippet to accompany the
overridden equals methods?
/**
* Unsupported operation. Instances of this class are mutable and
* should therefore not be managed in a hash table. See the linked
* message and the related discussion thread for details.
*
* @return nothing, throws an exception instead
* @throws UnsupportedOperationException always thrown
* @see
http://article.gmane.org/gmane.comp.apache.jackrabbit.devel/1899
* @see Object#hashCode()
*/
public int hashCode() throws UnsupportedOperationException {
thrown new UnsupportedOperationException(
"unable to compute hashcode");
}
PS. Longer term issue: Would it be better to migrate to some other
method like the JCR isSame() instead of equals() in these cases? The
equals and hashCode operations are pretty fundamental to Java objects
and messing with their semantics is an obvious gotcha. The standard
reference equality and hash code implementation of Object.hashCode()
works well also for mutable objects.
BR,
Jukka Zitting
Re: Checkstyle improvements
Posted by Tobias Strasser <to...@gmail.com>.
> > > regarding missing hashCode(): i intentionally do never override hashCode()
> > > for mutable objects. hashCode() should imo only be implemented for
> > > immutable objects.
> +1 for overriding hashCode() returning a constant integer
> for mutable classes. i don't fully agree with the javadoc though.
> if you implement hashCode() 'correctly', i.e. calculate the hashcode
> based on the objects fields, you risk losing track of data in a
> hash table.
>
> mutable objects should imo never be managed in hash tables.
> returning a constant integer is therefore fine with me for mutable
> objects.
well, when the equality depends on mutable data, the object cannot be
used in sets at all. so no need to return '1' as hashcode. if the
object cannot be used in set, i would rather throw a
UnsuportedOperationException:
public int hashCode() {
thrown new UnsupportedOperationException("unable to compute hashcode");
}
cheers, tobi
--
------------------------------------------< tobias.strasser@day.com >---
Tobias Strasser, Day Management AG, Barfuesserplatz 6, CH - 4001 Basel
T +41 61 226 98 98, F +41 61 226 98 97
-----------------------------------------------< http://www.day.com >---
Re: Checkstyle improvements
Posted by Stefan Guggisberg <st...@gmail.com>.
On 4/27/05, Jukka Zitting <jz...@yukatan.fi> wrote:
> Hi,
>
> Stefan wrote on protected member variables:
> > therefore i suggest to disable the related Checkstyle check.
>
> Sounds fine to me, especially since the protected member variable
> pattern is so common in Jackrabbit.
>
> In fact this pattern reflects the more general design style in
> Jackrabbit and in the JCR API as well. In many places Jackrabbit uses
> classic inheritance for common functionality between classes. Realizing
> this was a major step in getting to understand the Jackrabbit internals.
> Based on this I intend to create and add (sooner or later :-) some class
> inheritance diagrams as a part of the JCR-73 Javadoc improvement task.
>
> > regarding missing hashCode(): i intentionally do never override hashCode()
> > for mutable objects. hashCode() should imo only be implemented for
> > immutable objects.
>
> As Felix already noted:
>
> There's always the catch that you won't be able to use HashSets or
> similar data structures to reliably keep track of such objects (two
> "equal" objects might have different hash codes). Thus, if nothing else,
> I'd suggest using the following placeholder code to implementing the
> hashCode() methods. It certainly isn't fast but satisfies the
> equals/hashCode contract for all equals() implementations.
>
> /**
> * Always returns <code>1</code>. Implemented as a simple
> * placeholder to satisfy the equals/hashCode contract. This
> * method should be properly implemented if instances of
> * this class need to be managed in a hash table.
> *
> * @return always <code>1</code>
> * @see Object#hashCode()
> */
> public int hashCode() {
> return 1;
> }
+1 for overriding hashCode() returning a constant integer
for mutable classes. i don't fully agree with the javadoc though.
if you implement hashCode() 'correctly', i.e. calculate the hashcode
based on the objects fields, you risk losing track of data in a
hash table.
mutable objects should imo never be managed in hash tables.
returning a constant integer is therefore fine with me for mutable
objects.
cheers
stefan
>
> > 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.
>
> Agreed. My goal with JCR-97 is not to blindly follow guidelines but to
> get rid of the simple issues so that the Checkstyle report is actually
> useful in enforcing a common coding style and locating potentially
> troublesome constructs (like the hashCode issue). A newcomer like me can
> also use the report as a list of gotchas to watch out for.
>
> BR,
>
> Jukka Zitting
>
>
Re: Checkstyle improvements
Posted by Jukka Zitting <jz...@yukatan.fi>.
Hi,
Stefan wrote on protected member variables:
> therefore i suggest to disable the related Checkstyle check.
Sounds fine to me, especially since the protected member variable
pattern is so common in Jackrabbit.
In fact this pattern reflects the more general design style in
Jackrabbit and in the JCR API as well. In many places Jackrabbit uses
classic inheritance for common functionality between classes. Realizing
this was a major step in getting to understand the Jackrabbit internals.
Based on this I intend to create and add (sooner or later :-) some class
inheritance diagrams as a part of the JCR-73 Javadoc improvement task.
> regarding missing hashCode(): i intentionally do never override hashCode()
> for mutable objects. hashCode() should imo only be implemented for
> immutable objects.
As Felix already noted:
There's always the catch that you won't be able to use HashSets or
similar data structures to reliably keep track of such objects (two
"equal" objects might have different hash codes). Thus, if nothing else,
I'd suggest using the following placeholder code to implementing the
hashCode() methods. It certainly isn't fast but satisfies the
equals/hashCode contract for all equals() implementations.
/**
* Always returns <code>1</code>. Implemented as a simple
* placeholder to satisfy the equals/hashCode contract. This
* method should be properly implemented if instances of
* this class need to be managed in a hash table.
*
* @return always <code>1</code>
* @see Object#hashCode()
*/
public int hashCode() {
return 1;
}
> 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.
Agreed. My goal with JCR-97 is not to blindly follow guidelines but to
get rid of the simple issues so that the Checkstyle report is actually
useful in enforcing a common coding style and locating potentially
troublesome constructs (like the hashCode issue). A newcomer like me can
also use the report as a list of gotchas to watch out for.
BR,
Jukka Zitting
Re: Checkstyle improvements
Posted by Stefan Guggisberg <st...@gmail.com>.
On 4/27/05, Elliot Metsger <em...@jhu.edu> wrote:
> Felix Meschberger wrote:
> > Hi,
> >
> > Just a small clarification...
> >
> > Stefan Guggisberg schrieb:
> >
> >> regarding missing hashCode(): i intentionally do never override
> >> hashCode()
> >> for mutable objects. hashCode() should imo only be implemented for
> >> immutable objects.
> >>
> > Unfortunately this is not in line with the general contract of the
> > hashCode and equals methods, which says - amongst other things - that
> > for two objects being equal according to equals the hashCode must be the
> > same. If you do not overwrite the hashCode method whenever you overwrite
> > the equals method, you will get into troubles. Josh Bloch has written a
> > nice text on this issue in his famous book.
>
> yes, you run into problems when inserting objects that override equals()
> but not hashcode() into Set's for example.
>
well that's exactly the problem. if your override equals() and hashCode() in a
mutable class you e.g. risk losing track of data in a hashtable. that's
why i intentionally did not override hashCode() in order to hint that this
class should not be used in hashtables etc.
on second thought i realize that i had better overriden hashCode()
returning a constant integer.
cheers
stefan
Re: Checkstyle improvements
Posted by Elliot Metsger <em...@jhu.edu>.
Felix Meschberger wrote:
> Hi,
>
> Just a small clarification...
>
> Stefan Guggisberg schrieb:
>
>> regarding missing hashCode(): i intentionally do never override
>> hashCode()
>> for mutable objects. hashCode() should imo only be implemented for
>> immutable objects.
>>
> Unfortunately this is not in line with the general contract of the
> hashCode and equals methods, which says - amongst other things - that
> for two objects being equal according to equals the hashCode must be the
> same. If you do not overwrite the hashCode method whenever you overwrite
> the equals method, you will get into troubles. Josh Bloch has written a
> nice text on this issue in his famous book.
yes, you run into problems when inserting objects that override equals()
but not hashcode() into Set's for example.
Re: Checkstyle improvements
Posted by Felix Meschberger <Fe...@day.com>.
Hi,
Just a small clarification...
Stefan Guggisberg schrieb:
>regarding missing hashCode(): i intentionally do never override hashCode()
>for mutable objects. hashCode() should imo only be implemented for
>immutable objects.
>
>
Unfortunately this is not in line with the general contract of the
hashCode and equals methods, which says - amongst other things - that
for two objects being equal according to equals the hashCode must be the
same. If you do not overwrite the hashCode method whenever you overwrite
the equals method, you will get into troubles. Josh Bloch has written a
nice text on this issue in his famous book.
>i suggest to disable the related Checkstyle check.
>
>
Therefore I strongly oppose to removing that 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.
>
>
>
Regards
Felix
Re: Checkstyle improvements
Posted by Stefan Guggisberg <st...@gmail.com>.
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
>
>
Re: Checkstyle improvements
Posted by Jukka Zitting <jz...@yukatan.fi>.
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?
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.
PS. Is there a reason why the Checkstyle report is disabled in
project.xml?
BR,
Jukka Zitting