Thanks Emmanuel again for sorting this out. Some lazy guys like me should definitely be more careful and keep the mentioned points in mind before commit. Some notes here.
>> The README.md file has none. I don't know if github markdown supports comment or not. Need to see if doable for the READMEs. About Javadoc, I'm wondering if we would allow some exceptions for test codes, because from my point of view, we could make the class names and methods good long enough readable and understandable. >> To be more spot on : it's absolutely critical to add Javadoc in Interfaces : >> this is what get exposed to the public. Sure! Absolutely. I'm going to add missing ones for the existing APIs. About coding styles, I agree totally. Need to fix the found issues immediately. Sometime it would be great to have checking styles to be enforced so such exceptions can be found soon once they get in. Regards, Kai -----Original Message----- From: Emmanuel Lécharny [mailto:[email protected]] Sent: Thursday, July 02, 2015 4:30 PM To: [email protected] Subject: Code quality & formatting rules Hi guys, first of all, I'm impressed by the dedication of all you guys. It's definitively a living project ! That being said, I have had some time lateluy to look at the code, and I have a few remarks about the overall quality. Please don't take it as a blame, but much more as a reminder that we value quality at The ASF because it improves maintenability. - The AL 2.0 header is mandatory in every non-binary files we commit. The README.md file has none. - Javadoc : ok, it's clear to everyone that writing javadoc is boring, especially for those of us not being english native speaker. Oh, wait ! None of us is an english native speaker !(weird, isn't it ?) So don't be ashamed for you bad english : it only improves if you practice ! To be more spot on : it's absolutely critical to add Javadoc in Interfaces : this is what get exposed to the public. For classes implementing the interface, that's quite simple, 3 rules : o If a method is implementing an interface's method, simply add a {@inheritDoc} tag in your method header. That will give a hint to the reader o If a method is private, you can just add an explaination on what does the method. You may add teh @param, @return, etc, but it's not mandatory. o Any other method *must* be documented, its parameters documented, its return documented, and its excecption documented. getters and setters will generally be created by your IDE, with the correct Javadoc (at least in Eclipse, when you tell the IDE to generae the source for them). Classes : Header is mandatory, with Template param documented : /** * A Class used to store the comparator and labelProvider used by the TableWidget. * * @author <a href="mailto:[email protected]";>Apache Directory Project</a> * @param <E>The element being handled bu the decorator */ public abstract class TableDecorator<E> extends LabelProvider implements Comparator<E> { ... Fields : add a Javadoc documentation : /** The Dialog instance */ private AddEditDialog<E> dialog; Last, not least, DO IT BEFORE COMMITTING ! I stress that out because once it's commited, the message you send is "I'm too lazy to do it, please do it for me". You are not lazy, I know that (ok, I'm so I can't blame you for committing to fast, but I try hard to amend myself ;-) - Code quality : That's a difficult part. We have a code format, which is the Java code convention, for Kerby, AFAIR (we already have discussed it at the very beginning, if my memory is correct you wanted to stay with that instead of switching to the Directory code style, which is perfectly ok). Although, it's not respected everywhere : @Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; KerberosTime time = (KerberosTime) o; return this.getValue().equals(time.getValue()); } (KerberosTime) This is not good. The pb with this code might be troublesome later, if some one do something like : One if (o == null || getClass() != o.getClass()) return false; Log.print( "Instances are different" ); Ok, this is a trivial and not very helpful example, and some might say that it's unlikely to happen. Guys, I'm 50 years old, I'm coding since I'm 15, and trust me : I *have* seen such a problem, but with way bigger consequences than just a Log being printed everytime two instances are equals. => don't forget { and } around a block of code Another thing in this code : are you sure you know everything about operator precedence ? I don't. Pleas have a look at http://introcs.cs.princeton.edu/java/11precedence/ In other words : if ((o == null) || (getClass() != o.getClass())) is always safer than if (o == null || getClass() != o.getClass()). - Overall code quality : Always consider that your code is likely to be run in a multi-threaded environement. If you have a doubt, ASK. The community at large might help selecting the right data structure. It's a complicated mater, better have more than two eye balls looking at the code when it comes to concurrency ! Know which method of the JAVA API is thread safe, and which one is not. It's documented. Things like SimpleDateFormat is treatherous : it' snot thread safe, and it's not performant. One might thing that declaring it as a static field would speed up a lot the code, but if you don't protect it against concurrent access, you'll get bitten ! This is not in your code, but it's just an example : synchronized ( KerberosUtils.UTC_DATE_FORMAT ) { kerberosTime = KerberosUtils.UTC_DATE_FORMAT.parse( date ).getTime(); } with : public static final SimpleDateFormat UTC_DATE_FORMAT = new SimpleDateFormat( "yyyyMMddHHmmss'Z'" ); That's all for today, feel free to comment ! Emmanuel
