Re: [Developers] Code cleanup
On Feb 4, 2005, at 9:52 AM, Nico Klasens wrote: [zapped my remarks] What is the reason to have more committors then we have now? There are a lot of people who have the right to commit, but only a few have used it in the last months. This last year does contradict with the remark you make. Some people who were willing to become a committor were denied, because they did not contribute enough as a known mmbase developer. The above is not very pragmatic and realistic. I did the changes in the present where only a few commitors are active on the repository. I don't understand why I should take possible future situations into account in my current actions. I will deal with it when it is there. You have 2 valid points there lets look at them on why the mmc and me (as ex mmc) was worried and pushed for more developers and changes of some the rules. Very few of the current commitors really commit into the core, So why do we need more of them ? Well the reverse is that if for some reason these 4 or 5 people for whatever reason stop core development will stop. Most of these people don't have 'work on the core' as a job title. imho we are living on borrowed time. The idea is with more commitors the changes are we will find more very active developers who could and will be ready to replace that small group if needed. The only other way i can see is just hope for the best or slowly move to the state where the about 30 sponsers seeing this and create 4 or 5 job titles that say 'work on mmbase core' (removing them from any companies project cycle and making mmbase their only focus). Your second point on but you (and others have voted down new commitors), Personally i voted down 1 not because of him but because of the timing of the call (it was done when i left the mmc and we had a vote running for my replacement). I since then asked the person in question (since he works at my company) if he wanted me to create a new call and he declined. The other negative votes seem to be 'ive not seen any code from him or i don't know him' they are right in a way since we put that as demands on becomming a commitor !! I know the mmc has been trying to find a way out, my personal view is work on project level, allow the projectleader to add developers on his own. This also makes more and more sense since we more and more work in layers and applications. But this seems to be hard to enforce in cvs so if this is done it will probably be a model based on trust where every assigned developer can access the whole cvs and we frown and ask for comment from the projectleader if a developer commits outside of the project(s) the developer is assigned. I personally see your action in this light i feel it would have been better if you had a highlevel ok from the project leader on that project even if it was a small email or phone call. Again this is my view but i would be for lowering the bar on allowing people in cvs to near zero (a project leader who wants you in his team would be enough) but i also feel that this can only work when we have some basic rules like stay within the scope of the project and problems and questions will be aimed at the projectleader. The timimg of mine was okay I think. Not a lot of new functionality projects are running and one big step in the cleanup project was just finished, And to be very pragrmatic, most of the changes (if not all) would easily be merged by CVS into changed code. You still feel you did something wrong you didn't, My point is that the work should (and is imho) part of the cleanup project and it would have been nice and made more sense if he knew you where about to do it. One of the reasons is that if someone else does it and f***up i would have asked him what happend? [zapped some] Maybe my perception on what a committor is allowed to do on the HEAD is much more loose then it should be. I will keep that in mind next time. Well its not a bad idea to talk about these issues and make them more clear for all. The end result for all is that we allow developers to add useful things to mmbase and be happy. Why do we need more developers? For what do we need them? What issue is the community trying to solve with mmbase at the moment? Mmbase already solved the issue to store content. It is a content toolkit which can be used to build many types of systems. So mmbase is done ? Well we have a high commit count compared to most opensource projects so something is not done i guess. Ive talked about this before i feel we as developers need to rethink what MMBase is. Since there is more than just the core. Most of the work seems to be done above the core these days in either frameworks (finished cms systems) or tools and services. I think its time we who build the product !!! had a much better idea of what MMBase is and what we want it to be in the future. My guess is its more than just the core and if that is the case we need a better think about these.
RE: [Developers] Code cleanup
A few notes in general: 1. In general, the changes are good and beneficial. I think that is important to say in advance. And they were easy to do without expected side effects. 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. I expected this note, because I have had this one already a couple of times. First, I didn't apply code conventions and javadoc as step 11 describes. I removed a lot of unused lines of code and improved some methods. Second, this is exactly why I don't actively develop on the mmbase codebase. When I am at work I implement applications on top of mmbase. It does what I need so I don't have to modify the code. Sometimes, I have my weak moments and develop things in my own time. I want to do it when I have the time and not when the mmbase community wants my time. IOW I don't want to wait for approval for something I believe does not have any major impact. I am not going to wait a couple of days before I am allowed to do it. The moment has passed and I probably will spend my time in some other way. Third, I did this on the head. It is THE place to have many changes to the code. Users (other developers) of mmbase trust us to change to code in a good way. They know when they switch to a new major version (I think the 1.x are) that they have to do some work to get it working again, but that it will be a better system as the previous version. Fourth, IMO, changing user functionality should be commnicated, but refactoring code to have a more robust and stable system could happen at anytime. There are a lot of good experienced developers watching mmbase who don't want to go through all the bureaucratic procedures to change some methods they know could be better. 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. Thank you, While I am writing this we might need another document for people who want to contribute to mmbase and want a quick start. A hacker's guide which tells how to participate and where documentation is and what rules apply. Such a document could contain something like the following. * Participating in the community - short description hwo to communicate help * What to read * Directory layout * Coding style - explaining most important ones and redirect to full list * Document everything - which parts of the system should be documented * Error message conventions * Other conventions * Exception handling * Automated tests * Writing test cases before code * Writing log messages * Patch submission guidelines * Filing bugs / issues * Commit access * Release numbering, compatibility, and deprecation * Stabilizing and maintaining releases 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. I don't think so. Some of them don't comply fully with the mmbase coding standards, but I didn't do it blindly. 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. Backward compatibility? Fixing the equals will make the system more robust and everyone wants that. 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(). That's even better ;) 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
Re: [Developers] Code cleanup
First, I didn't apply code conventions and javadoc as step 11 describes. I removed a lot of unused lines of code and improved some methods. Second, this is exactly why I don't actively develop on the mmbase codebase. When I am at work I implement applications on top of mmbase. It does what I need so I don't have to modify the code. Sometimes, I have my weak moments and develop things in my own time. I want to do it when I have the time and not when the mmbase community wants my time. IOW I don't want to wait for approval for something I believe does not have any major impact. I am not going to wait a couple of days before I am allowed to do it. The moment has passed and I probably will spend my time in some other way. I totally agree here. What you did was a positive and good thing. Every commiter was given a certain amount of thrust and I don't think you abused this. Worse IMHO are commitors who do contribute code that does not fit to the stanards. I hope you find some more weak moments, but I can understand if you have lost the taste. ___ Developers mailing list Developers@lists.mmbase.org http://lists.mmbase.org/mailman/listinfo/developers
Re: [Developers] Code cleanup
Nico Klasens wrote: Fourth, IMO, changing user functionality should be commnicated, but refactoring code to have a more robust and stable system could happen at anytime. There are a lot of good experienced developers watching mmbase who don't want to go through all the bureaucratic procedures to change some methods they know could be better. I agree. If every committor would only once a year do what you did, the code would look a lot nicer already. backward compatibility again. Yes, we have to provide a durable api, but aren't we allowed to change the api between major releases? I'm never sure about that. Of course it does happen sometimes. But I think we may sometimes be a bit (too) reluctant here. Michiel -- Michiel Meeuwissen mihxil' Mediacentrum 140 H'sum[] () +31 (0)35 6772979 nl_NL eo_XX en_US ___ Developers mailing list Developers@lists.mmbase.org http://lists.mmbase.org/mailman/listinfo/developers
Re: [Developers] Code cleanup
On Feb 2, 2005, at 10:26 AM, Michiel Meeuwissen wrote: Nico Klasens wrote: Fourth, IMO, changing user functionality should be commnicated, but refactoring code to have a more robust and stable system could happen at anytime. There are a lot of good experienced developers watching mmbase who don't want to go through all the bureaucratic procedures to change some methods they know could be better. I agree. If every committor would only once a year do what you did, the code would look a lot nicer already. backward compatibility again. Yes, we have to provide a durable api, but aren't we allowed to change the api between major releases? I'm never sure about that. Of course it does happen sometimes. But I think we may sometimes be a bit (too) reluctant here. Michiel The whole point is not if the work is nice we all agree that it is and we thank you for the work the point is that we want more commitors if we do that it means more people can at on any random day change 30 classes as part of cleanup. That itself is again not a problem unless they all do it at the same time and they are all done at the high level of work you did. The MMC for that reason has to have 'some' idea of what is going on. This is the reason why there is a cleanup project and a simple email to the projectmanager with a general note of 'i want to make changes on the whole code base for imports, scoop etc etc fo the next few days) would have been enough. This limits the changes over overlapping work of very unlucky timing and it also means that lesser commitors (since we want to have more commitors) don't get the feeling you can do whatever you like. Im sorry but if such simple email to the projectmanager is too much you should not work in a opensource community or any other team. You have to see that in order to at least have some safeguards at least some communication is needed this has nothing to do with your work but a general comment. MMBase is a lot more open then for example linux or other big opensource projects. If you feel i am too hard then well lets have a developers meeting on topics like this since i know the mmc has been having a hard time on how to handle these issues and how to make them so that we get more developers get the feeling they are welcome and respected for their work if you see this as a attack on yourself then something is very wrong and we need to talk about it as a group. Lets not forget the mmc also puts in alot of their private time and they are not out to get you, like you they want to have a big happy family of developers, Daniel. ___ Developers mailing list Developers@lists.mmbase.org http://lists.mmbase.org/mailman/listinfo/developers
Re: [Developers] Code cleanup
Nico Klasens wrote: 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 How nice :-) 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. That a very odd one indeed, and I may be the culprit here. I would like to base equals always on the number. That is not the case now (ussually the equals of Object is used). But you can override it (which must as usual for every method in MMObjectNod be done in MMObjectBuilder), because sometimes it is absolutely essential that equals is based on the number. I would be +1 for generalizing this behaviour, because it would e.g. ensure that the same node does not appear more then once in the node-cache. 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) Of course. But I am still not completely detemeined on the use of private, protected en package on certain methods. Sometimes we see that methods are made package only to make sure that the test-cases have access, which always stroke me as a kind of odd reason. Furthermore there often can be some discussion whether a method must be private or protected. When private, you can easier safely change the implementation of the class, but when protected, you often offer greater flexibility for extension. It can be very irritating if methods were made private which you would very much like to call or override in your exension. I figure that common sense should determin what to choose. If you envision a complete change of the class in which the method may get lost, because it is very specific or perhaps oddly implmented, I'd make it private. If the methods looks good, and it is imaginable that someone wants to change or use its behavior for some reason, I normally go for protected. But I wonder if we good agree on some rules of thumb for this? 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. And if I may add, the logically most apt one. E.g. I think that NodeManager.getFields() should have returned a Set or SortedSet, and not a List, because it is logically impossible that a builder has two the same fields. Michiel -- Michiel Meeuwissen mihxil' Mediacentrum 140 H'sum[] () +31 (0)35 6772979 nl_NL eo_XX en_US ___ Developers mailing list Developers@lists.mmbase.org http://lists.mmbase.org/mailman/listinfo/developers
Re: [Developers] Code cleanup
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 Developers@lists.mmbase.org http://lists.mmbase.org/mailman/listinfo/developers
Re: [Developers] Code cleanup
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. See the MMBase code conventions: http://www.mmbase.org/?docnr=33153portal=199project=14804backtemplate=%2Fdevelopment%2Fprojects%2Fproject.jsptemplate=%2Fincludes%2Fdoc_index.jsp -- Pierre van Rooden Mediapark, C 107 tel. +31 (0)35 6772815 Anything worth doing is worth overdoing. ___ Developers mailing list Developers@lists.mmbase.org http://lists.mmbase.org/mailman/listinfo/developers
Re: [Developers] Code cleanup
Pierre van Rooden wrote: See the MMBase code conventions: http://www.mmbase.org/?docnr=33153portal=199project=14804backtemplate=%2Fdevelopment%2Fprojects%2Fproject.jsptemplate=%2Fincludes%2Fdoc_index.jsp or: http://www.mmbase.org/docs/backenddevelopers/codingstandards.html which should perhaps be 'leading', because generated from CVS. Michiel -- Michiel Meeuwissen mihxil' Mediacentrum 140 H'sum[] () +31 (0)35 6772979 nl_NL eo_XX en_US ___ Developers mailing list Developers@lists.mmbase.org http://lists.mmbase.org/mailman/listinfo/developers
Re: [Developers] Code cleanup
Pierre van Rooden wrote: Michiel Meeuwissen wrote: And if I may add, the logically most apt one. E.g. I think that NodeManager.getFields() should have returned a Set or SortedSet, and not a List, because it is logically impossible that a builder has two the same fields. Except that the fields returned by getFields() have a certain order (which depends on the passed parameter). Hence List is (currently) more useful, as you would need Comparators to get SortedSet to work properly. True. So not a very good example. A Set with predictable iteration order then. There is no seperate interface for that, but there are Set implementations which garantee that. Michiel -- Michiel Meeuwissen mihxil' Mediacentrum 140 H'sum[] () +31 (0)35 6772979 nl_NL eo_XX en_US ___ Developers mailing list Developers@lists.mmbase.org http://lists.mmbase.org/mailman/listinfo/developers
