Hello Tom! My comments inline...
> -----Original Message----- > From: Tom Morris [mailto:[EMAIL PROTECTED] > Sent: den 25 maj 2006 21:46 > To: [email protected] > Subject: RE: [argouml-dev] Coding conventions > > Oops, meant to comment on this piece of the process: > > > > 3. enforce conformance by asking the author to make revisions to > > > non-conforming commits rather than changing their code for them > > > > In your item 3. I have been trying to avoid this kind of > > pointing finger because when I attempted it, the result was > > not satisfying. Instead I have set up systems to check things > > automatically hoping that everyone would pursue this. Because > > the number of checkstyle warnings have increased, one could > > argue that I have failed also in this. We need someone with > > better socials skills than I have to do the "asking". > > Did everyone commit to following the process you put in place? This is a very difficult question. I'd say that the answer is yes. In the same time I am disappointed in that new code and code changes are frequently committed that don't follow the rules. 27 new violations are added the last week: argouml/src/model-mdr/src/org/argouml/model/mdr/CommonBehaviorFactoryMDR Impl.java:572: warning: Line is longer than 80 characters. argouml/src_new/org/argouml/ui/cmd/InitMenusLater.java:97: warning: Line is longer than 80 characters. argouml/src_new/org/argouml/ui/SplashScreen.java:53:20: warning: Variable 'paintCalled' must be private and have accessor methods. argouml/src_new/org/argouml/uml/ui/UMLListCellRenderer2.java:163: warning: if at indentation level 12 not at correct indentation, 16 argouml/src_new/org/argouml/uml/ui/UMLListCellRenderer2.java:164: warning: if child at indentation level 16 not at correct indentation, 20 argouml/src_new/org/argouml/uml/ui/UMLListCellRenderer2.java:165: warning: if child at indentation level 16 not at correct indentation, 20 argouml/src_new/org/argouml/uml/ui/UMLListCellRenderer2.java:166: warning: if child at indentation level 16 not at correct indentation, 20 argouml/src_new/org/argouml/uml/ui/UMLListCellRenderer2.java:166: warning: method call child at indentation level 16 not at correct indentation, 20 argouml/src_new/org/argouml/uml/ui/UMLListCellRenderer2.java:167: warning: for at indentation level 16 not at correct indentation, 20 argouml/src_new/org/argouml/uml/ui/UMLListCellRenderer2.java:167:20: warning: 'for' is not followed by whitespace. argouml/src_new/org/argouml/uml/ui/UMLListCellRenderer2.java:168: warning: for child at indentation level 20 not at correct indentation, 24 argouml/src_new/org/argouml/uml/ui/UMLListCellRenderer2.java:168: warning: method call child at indentation level 20 not at correct indentation, 24 argouml/src_new/org/argouml/uml/ui/UMLListCellRenderer2.java:169: warning: if at indentation level 20 not at correct indentation, 24 argouml/src_new/org/argouml/uml/ui/UMLListCellRenderer2.java:170: warning: if child at indentation level 24 not at correct indentation, 28 argouml/src_new/org/argouml/uml/ui/UMLListCellRenderer2.java:170: warning: method call child at indentation level 24 not at correct indentation, 28 argouml/src_new/org/argouml/uml/ui/UMLListCellRenderer2.java:171: warning: if rcurly at indentation level 20 not at correct indentation, 24 argouml/src_new/org/argouml/uml/ui/UMLListCellRenderer2.java:172: warning: for rcurly at indentation level 16 not at correct indentation, 20 argouml/src_new/org/argouml/uml/ui/UMLListCellRenderer2.java:173: warning: if child at indentation level 16 not at correct indentation, 20 argouml/src_new/org/argouml/uml/ui/UMLListCellRenderer2.java:174: warning: if rcurly at indentation level 12 not at correct indentation, 16 argouml/src_new/org/argouml/uml/ui/UMLListCellRenderer2.java:175:48: warning: '{' is not preceded with whitespace. argouml/src_new/org/argouml/uml/ui/UMLModelElementListModel2.java:130: warning: Line is longer than 80 characters. argouml/src_new/org/argouml/uml/ui/UMLModelElementListModel2.java:131:57 : warning: ';' is preceded with whitespace. argouml/src_new/org/argouml/uml/ui/UMLModelElementListModel2.java:298: warning: if child at indentation level 16 not at correct indentation, 20 argouml/src_new/org/argouml/uml/ui/UMLModelElementListModel2.java:299: warning: if child at indentation level 16 not at correct indentation, 20 argouml/src_new/org/argouml/uml/ui/UMLModelElementListModel2.java:299: warning: method call child at indentation level 16 not at correct indentation, 20 argouml/src_new/org/argouml/uml/ui/UMLModelElementListModel2.java:300: warning: if child at indentation level 16 not at correct indentation, 20 > Is this the > most important thing that needs fixing to make ArgoUML a success? Yes. The readability and quality of the code is vital if we should be able to lower the threshold for new developers to understand the code to an acceptable level. > The > answers to those questions would influence what is the best approach going > forward. Having one person going around cleaning up after everyone else > is > an untenable approach in general and in this specific case, because it's > the > project leader, it's taking effort away from planning and other critical > leadership tasks. I get the feeling that this whole discussion is misdirected. If the big question is really "what Linus is not doing", then it is a very big. > From a tools point of view, improvements which would encourage better > behavior include: 1) personal stats instead of group stats, 2) email or > other notification when a developer's commit caused counts to increase or > exceed some threshold, 3) tracking stats over time so trends can be > identified, ... probably lots of other stuff. I don't know any tools for 1) w.r.t. static code checking. 2) is complicated because of the set up. I would prefer to have the mails sent from the tigris host but that is not possible. 3) has been available for the past couple of years from the Xenofarm setup. > Let's take a step back though and look at what we're trying to achieve. > Are > we trying to drive some number to zero or are we trying to improve the > quality of the code base or are we trying to improve the quality of the > functionality delivered to the user? They're related to a greater or less > degree, but we shouldn't lose sight of the end goal -- the user experience. > > Focusing solely on the metric results in comments like: > > /** > * @return the result > */ > > which improves the metric without improving the code at all, but a > single-minded focus on this has bigger impacts. > > A recent commit consisted of completely cloning a method and then > commenting > out a few lines leaving two methods which are 90% duplicates. When the > style review was done, all the indentation, wrapping, and punctuation was > cleaned up without addressing the biggest code quality problem -- the > cloned > methods. That's not an effective use of resources. The regular checkstyle checks notices that some developers commit code in violation with the coding standards. You notice that some developers make apparent bad judgments when working with the code. I don't have any solution to this so unless the "culprit" notices this and fixes it and the person noticing points out the problem, the code will deteriorate. I don't want the code to deteriorate. I want it to be always improved in all conceivable aspects. As long as developers will continue to make mistakes (i.e. forever) I hope that we all will help in fixing those mistakes. /Linus --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
