Monday, May 11, 2009

I see lots of code that is more complicated than it needs to be.

When writing struct classes, this one guy always adds an elaborate hashCode() method, in which he combines the hashcodes of all the fields, and always adds an elaborate equals() method, in which he compares all the fields, and always adds an elaborate toString() method. Of course, when new fields are added, they aren't always added to the hashCode(), equals(), and toString() methods. The hashCode() result would still be okay, but the equals() would be incorrect, though it's never actually used. And the overly elaborate toString() doesn't matter anyhow, since the only thing it affects are the debug logs.

In another case, I was coding some module that depended on some custom HTTP header, as well as the module system. So I asked that all the HTTP headers be passed in, as some future module might need some other custom HTTP header. But, this other guy writes this elaborate code to read a configuration parameter for that one HTTP header, and extract that one HTTP header, put that one HTTP header into the Map<String,String[]> that was intended to hold all the HTTP headers, and pass that in. As it turns out, the module needed 2 custom HTTP headers. Fortunately, I had already ripped out that elaborate code and passed in all the headers.

There's also this enum that has a constructor that takes a String that is supposed to be the name of a class. All the enum elements are constructed with hard-coded Strings of class names of classes that are known at compile time. The only thing that String is used for is in Class.forName(). So, it would be much simpler to just have the constructor take a Class as the argument. Furthermore, since the String can't be checked at compile time like the Class could be, there is a stupid validate() method that does Class.forName() on that String and logs an error if the class is not found. Furthermore, since all the classes have to be a subclass of a known class, after the Class.forName() is done, a Class.asSubclass() call is done, which is another stupid run-time check that could be done at compile-time by declaring the enum constructor argument to be Class<? extends KnownClass>.

No comments:

Post a Comment