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
