I am currently reviewing the review. I've found a few minor issues, am still collecting them.
A few notes in general:
1. In general, the changes are good and beneficial. I think that is important to say in advance.
2. These changes should have been communicated to the project manager of the Optimization project (me). In particular since you there are changes to the core, and in running projects (i.e. packaging). Also a new class was added to org.mmbase.util. All this is no problem in and of itself, but the project maintainer should have been informed, so it can be documented (as step 11 in the Optimixation project) and the list would be informed that changes wre being made. This prevents comments later on.
3. A few of the comments you made are valid and should possibly be included in the MMBase code conventions. I'll look into these.
4. A few changes may have been a bit hasty - I can't tell until I further investigated, will mail later. These are all minor issues though.
Specific comments:
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.
Agreed. Before we cahneg anything though, it is important to determine where it is used now and what changign teh implementation would mean for backward compatibility.
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.
Or, in some cases, log.trace().
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.
Agreed, we should look into this closely. Noet tahts oem code simply doesn't 'want' to throw exceptions at all. We should re-evaluate if that is desireable, posisbyl ona a case-by-case basis.
Specify the minimum visibility scope that a method requires.
This is MMBase code convention. Some code has already been marked (with @scope) for changing.
Note that not everyone fully agrees with this pracice, and in some cases (i.e. objects that are intended to just hold data and only have fields) it may actually be useful.
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.
Also included in the MMBase code conventions. The main issue here is backward compatibility with older code that uses/expects Vector and Hashtable.
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.
Though not always, i.e. java.util or the interface packages, and the mmbase core package are much used and I don't think it adds much to specify all classes.
There are two ways to write error-free programs; only the third one works.
And only sometimes.
-- Pierre van Rooden Mediapark, C 107 tel. +31 (0)35 6772815 "Anything worth doing is worth overdoing."
_______________________________________________ Developers mailing list [email protected] http://lists.mmbase.org/mailman/listinfo/developers
