Code cleaning + simple rules

[Emmanuel Lecharny <el...@gmail.com>]

Hi guys,

with Felix's help, I was able to fix some bad code (he has generated a
lot of reports using PMD, CheckStyle, etc).

There are some specific pieces of code we could improve. I just got
through the hashCode and equals method, and there are pretty
inconsistant. Here are the very basic rules we should follow all over
the code (I didn't invented them, Joshua Bloch exposed them in his
very valuable book)

1) Equals methods :
a) compare this with obj, and return true if they are equal ( if (this
== obj ) return true ) ). This is for efficiency
b) do a if ( ! ( obj instanceof <class> ) ) return false
c) cast obj to <class>
d) do whatever operation needed to compare obj and this members

This is the most efficent way to write an equals method. There is no
need to check (obj == null ), as the obj instanceof will take care of
that

2) Hashcode
to be consistent, this is the way to write this method :

int h = 37;

for each element
  h = h * 17 + element.hashcode()

return h

3) We have a HashCodeBuilder class which should be replaced by a
direct hashCode method

4) We should have a toString() method for each class, or at least for
each data structure. This is helpful for debugging purpose

5) Use logs as much as possible, and use common sense when using them
(use error when needed, debug when needed, etc...). Usually, log an
error when catching an exception is good policy...

Have fun !

-- 
Regards,
Cordialement,
Emmanuel Lécharny
www.iktek.com

Re: Code cleaning + simple rules

[Alex Karasulu <ak...@apache.org>]

Thanks Felix I will use this utility class as well for toString().  It will
help make toString() consistent.

Alex

On Dec 23, 2007 1:06 PM, Emmanuel Lecharny <el...@gmail.com> wrote:

> > Checkstyle (I can't find, but it'd be anyway just about formatting)? Do
> you mean CPD (Copy/Paste Detector) ;-)
>
> Yeah, I don't exactly recall all the tools you are using ;) I usually
> don't use those tools ;)
>
>
> > > 4) We should have a toString() method for each class, or at least for
> > > each data structure. This is helpful for debugging purpose
> >
> > In most cases you just want to show the values of your class fields. If
> so you may want to have a look at commons-lang
> > [1] and especially on its ToStringBuilder [2] function. It simplifies
> life really. Just add
> >
> > public String toString() {
> >    return ToStringBuilder.reflectionToString(this);
> > }
> >
> > to your class and your done.
>
> Thanks for the trick ! I gonna try (and it will save me a lot of time,
> for sure ;)
>
> --
> Regards,
> Cordialement,
> Emmanuel Lécharny
> www.iktek.com
>

Re: Code cleaning + simple rules

[Emmanuel Lecharny <el...@gmail.com>]

> Checkstyle (I can't find, but it'd be anyway just about formatting)? Do you mean CPD (Copy/Paste Detector) ;-)

Yeah, I don't exactly recall all the tools you are using ;) I usually
don't use those tools ;)


> > 4) We should have a toString() method for each class, or at least for
> > each data structure. This is helpful for debugging purpose
>
> In most cases you just want to show the values of your class fields. If so you may want to have a look at commons-lang
> [1] and especially on its ToStringBuilder [2] function. It simplifies life really. Just add
>
> public String toString() {
>    return ToStringBuilder.reflectionToString(this);
> }
>
> to your class and your done.

Thanks for the trick ! I gonna try (and it will save me a lot of time,
for sure ;)

-- 
Regards,
Cordialement,
Emmanuel Lécharny
www.iktek.com

Re: Code cleaning + simple rules

[Felix Knecht <fe...@apache.org>]

Emmanuel Lecharny schrieb:
> Hi guys,
> 
> with Felix's help, I was able to fix some bad code (he has generated a
> lot of reports using PMD, CheckStyle, etc).

Checkstyle (I can't find, but it'd be anyway just about formatting)? Do you mean CPD (Copy/Paste Detector) ;-)
Anyway, findbugs report will show you what you missed or what can be really dangerous.

> 
> There are some specific pieces of code we could improve. I just got
> through the hashCode and equals method, and there are pretty
> inconsistant. Here are the very basic rules we should follow all over
> the code (I didn't invented them, Joshua Bloch exposed them in his
> very valuable book)
> 
> 1) Equals methods :
> a) compare this with obj, and return true if they are equal ( if (this
> == obj ) return true ) ). This is for efficiency
> b) do a if ( ! ( obj instanceof <class> ) ) return false
> c) cast obj to <class>
> d) do whatever operation needed to compare obj and this members
> 
> This is the most efficent way to write an equals method. There is no
> need to check (obj == null ), as the obj instanceof will take care of
> that
> 
> 2) Hashcode
> to be consistent, this is the way to write this method :
> 
> int h = 37;
> 
> for each element
>   h = h * 17 + element.hashcode()
> 
> return h
> 
> 3) We have a HashCodeBuilder class which should be replaced by a
> direct hashCode method
> 
> 4) We should have a toString() method for each class, or at least for
> each data structure. This is helpful for debugging purpose

In most cases you just want to show the values of your class fields. If so you may want to have a look at commons-lang
[1] and especially on its ToStringBuilder [2] function. It simplifies life really. Just add

public String toString() {
   return ToStringBuilder.reflectionToString(this);
}

to your class and your done.

[1] http://commons.apache.org/lang/
[2] http://commons.apache.org/lang/api-release/org/apache/commons/lang/builder/ToStringBuilder.html.

> 
> 5) Use logs as much as possible, and use common sense when using them
> (use error when needed, debug when needed, etc...). Usually, log an
> error when catching an exception is good policy...
> 
> Have fun !
>