Hello developers, This weekend I reviewed and cleaned the code a little of MMBase. After reviewing I decided to only do some of the stuff now and leave the rest for another afternoon. When I reviewed the code I came to the conclusion that the quality wasn't as bad as I have seen in some other codebases. Some best practices should still be applied to the code and I'd like to inform everyone about these.
Usage of object methods equals(), hashCode() and others ============================================================================ =========== The object class has only a few methods and most of them are final. The ones which are not final could be overriden, but a lot of programmers don't implement them correctly and violate the contract. See http://www.macchiato.com/columns/Durable5.html and http://www.macchiato.com/columns/Durable6.html for common mistakes in implementing equals(), hashCode() and clone(). One of the mistakes is to implement equals and forget to implement hashCode. This leads to not finding elements in a collection and it has performance penalties. I fixed a few in the MMBase codebase, but I probably missed a couple. One equals method I didn't like at all, was the one in MMObjectNode, but I haven't changed that one. We really should discuss what this should be. Usage of exception ============================================================================ =========== There are catch blocks in the code without an implementation. Some are commented, but most of them are not. A good practice is to add log.debug("", exception) when nothing else should happen. When debug is turned on then the exception will show up which makes debugging much easier. Mmbase really has to improve in exception handling in the internals. Most of the time Throwable, Error, RuntimeException or Exception are caught instead of the more specific types. If they are not caught the declared throws class mentions the generic exception class. This would represent a loss of possibly important information. The throws claus should be as specific as possible without grouping of exceptions. Some of the code throws an Error, Exception or RuntimeException. A more specific type would already contain more information why the exception is thrown and won't catch stuff it shouldn't catch. This will also prevent other code from catching generic exceptions. Usage of modifiers (Public, protected, private, final and synchronize) ============================================================================ =========== Using public, protected, private or package scope Specify the minimum visibility scope that a method requires. THE trick in OO is to hide as much of the implementation secrets as possible. The visibility modifiers are there to accomplish this. Remember that there are four scopes, not two (public and private) Declaring fields to be public is usually the wrong thing to do, since it does not protect the user of the class from changes in class implementation. Declare fields as private. If fields need to be accessed by the user of the class, then provide the necessary get and set methods. Don't declare the field public for performance reasons. The JIT compiler in the JVM will optimize the get and set methods and the field might be accessed directly. Using final clearly communicates the intent and flags items which are simpler in behaviour. Final says, "If you are looking for complexity, you won't find it here." The final keyword has more than one meaning : * a final class cannot be extended * a final method cannot be overridden * final fields, parameters, and local variables cannot change their value once set In the last case, "value" for primitives is understood in the usual sense, while "value" for objects means the object's identity, not its state. Once the identity of a final object reference is set, it can still change its state, but not its identity. Synchronized methods and blocks (as the visible part of object monitors) are a handy construct to ensure exclusiveness. In fact, it is so handy that people often think it's sufficient to turn all methods of a class into synchronized ones, in order to ensure thread safety. Well, it might be "safe," but sometimes in a very deadlocked way. The idiom of choice is to synchronize only local operations, but then we really can get along with it for threaded versions of patterns like SINGLETON or OBSERVER. Declaration of overspecific variables ============================================================================ =========== Like I wrote above, OO is all about encapsulation and hiding implementations. This also applies for methods and variables. A change in OO code should have a minimal impact on the code. The idiom of choice here is to choose the highest level of abstraction appropriate to the problem. This means that only the interfaces (like List and Map) for collections should be used for method return types, method parameters, fields and local variables. Imports ============================================================================ =========== Unused imports are not an issue in java runtime. The java bytecode contains the fully quallfied names on all places. The compiler uses the import statements to lookup the simple names in a source file. When code is written with only simple names in the code an interesting thing occurs. Developers can determine a classes dependencies by simply reading its import statements. While not an exact science having the imports at the beginning of each type declaration can provide other developers with insight into what types the code is using. IOW removing unused imports is only useful for the developers maintaining the code. Some conclusion can be drawn from these import listing. - many imports of the same package usually means that the class is in the wrong package. - high number of imports can indicate a high degree of coupling within an object. In most cases. the class implements more then one piece of functionality and should be divided into multiple classes. Nico -------------------------------------------------------------------------- There are two ways to write error-free programs; only the third one works. _______________________________________________ Developers mailing list [email protected] http://lists.mmbase.org/mailman/listinfo/developers
