directory-kerby mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kiran Ayyagari <kayyag...@apache.org>
Subject Re: Code quality & formatting rules
Date Thu, 02 Jul 2015 08:48:27 GMT
On Thu, Jul 2, 2015 at 4:30 PM, Emmanuel L├ęcharny <elecharny@gmail.com>
wrote:

> 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:dev@directory.apache.org">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 !
>
++1, clean code == less maintenance (and is a joy to read and work with ;)

>
> Emmanuel
>
>
>


-- 
Kiran Ayyagari
http://keydap.com

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message