> > BasicStepField
> > - return number1.doubleValue() == number2.doubleValue();
> > + return Double.doubleToLongBits(number1.doubleValue()) ==
> Double.doubleToLongBits(number2.doubleValue());
>
> Is there a specific reason to change this? I.e. is double
> comparion in java unreliable?
Yes they are. Just compare 0.0 with -0.0 or NaN with NaN. This code is used
in an equals method. If I left it how it was then I had to changed the
hashCode() to do (x == 0.0 ? 0L : Double.doubleToLongBits(x)). Equals and
hashCode methodes have to behave the same way. The general contract for the
hashCode method states that equal objects must have equal hash codes.
Looking into the javadoc I am even wrong in my implementation. If both
objects are of type Double then we should call the normal equals method.
> > TypeRel.java
> > // make sure only MMObjectNode's are added
> > public boolean add(Object object) {
> > - return super.add((MMObjectNode) object);
> > + return super.add(object);
> > }
> >
> > // make sure only MMObjectNode's are added
> > public boolean add(Object object) {
> > - return super.add((MMObjectNode) object);
> > + return super.add(object);
> > }
>
> The casting here is explicit to ensure that 'object' is an
> MMObjectNode.
> Should probably be set back? Not sure if it the test is really needed.
Not so nice of me to leave the comment. And if the method should do what the
comment says then I don't like the previoue implementation either. If it is
not an MMObjectNode then the JVM will throw a ClassCastException which looks
like a programming error. A better implementation would then be
If (!(object instanceof MMObjectNode)) {
throw new IllegalArgumentException("Only objects of type
MMObjectNode are allowed");
}
This will imform a developer using the code that it is not allowed to throw
in something else and that he does not have to investigate why a
ClassCastException was thrown. This piece of code makes the comment
obsolete.
> > RemoteGenerator.java:
> > - private static Vector getSubClasses(XMLClass xmlClass) {
> > + static Vector getSubClasses(XMLClass xmlClass) {
> >
> > - private static Vector getSuperClasses(XMLClass xmlClass) {
> > + static Vector getSuperClasses(XMLClass xmlClass) {
>
> Why were these methods made package?
Because according to the compiler they should be. Private methods can only
be accessed by the class itself. An inner class shouldn't be allowed to
access them. The compiler is nice enough to correct our code to remove the
private modifier or create a package local method to let the inner class use
the private methods of the parent class.
> Another thing, imports like:
>
> import java.io.BufferedWriter;
> import java.io.File;
> import java.io.FileOutputStream;
> import java.io.OutputStreamWriter;
> import java.io.Writer;
> import java.util.ArrayList;
> import java.util.Enumeration;
> import java.util.HashMap;
> import java.util.Iterator;
> import java.util.Map;
>
> I would rather avoid. The MMBase code conventions specify
> that you should, in general, use package imports if you use
> more than 3 imports from a package.
> While you can deviate from this when using obscure packages,
> I don't think such import lists are really needed with
> java.io or java.util.
> It's more a matter of taste than that it actually affects
> functionality, but endless import lists are not always
> beneficial for readability or maintenance.
I received this remark earlier from an mmbase developer. I don't like the
code conventions at this point. Three imports is not a lot and imo it is
more vital from how many packages you are importing (less is better).
I like to scan import listings and get a first impression how good the code
is in the class. Eg. Based on this listing Collections aren't used as
desired in the last few releases of java (Enumeration is present and
interface List is missing). The io implementation is questionable too: File,
FileOutputStream, OutputStreamWriter could maybe replaced by a Fileriter?
Again, endless import listings are a sign of bad design. There are too many
dependencies to other parts of the system or the class is in the wrong
place.
Nico
_______________________________________________
Developers mailing list
Developers@lists.mmbase.org
http://lists.mmbase.org/mailman/listinfo/developers
