You are viewing a plain text version of this content. The canonical link for it is here.
Posted to kerby@directory.apache.org by Emmanuel Lécharny <el...@gmail.com> on 2015/07/02 10:30:00 UTC
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: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 !
Emmanuel
Re: Code quality & formatting rules
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 02/07/15 11:36, Zheng, Kai a écrit :
> 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.
It does :
Title: Table widget
Notice: Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
.
http://www.apache.org/licenses/LICENSE-2.0
.
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
But please feel free to doublecheck.
> 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.
Absolutely right. Test code does not need the level of Javadoc we use
for real code, especially for methods. I usually just add a Class
header, and a method header which just tells what is the method testing.
Nothing fancy.
>
>>> 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.
We have a Jenkins server that build Kerby every day, and it provides a
good set of rules violation :
https://analysis.apache.org/dashboard/index/212397
RE: Code quality & formatting rules
Posted by "Zheng, Kai" <ka...@intel.com>.
>> Is it plain Sun/Oracle conventions?
I'm not very sure, but guess so. When we enforce the convention and see some few exceptions, then we could correct.
Thanks Stefan!!
Regards,
Kai
-----Original Message-----
From: Stefan Seelmann [mailto:mail@stefan-seelmann.de]
Sent: Friday, July 03, 2015 2:30 AM
To: kerby@directory.apache.org
Subject: Re: Code quality & formatting rules
On 07/02/2015 11:36 AM, Zheng, Kai wrote:
> 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.
For Studio I just added a block quote, but of course it is visible:
https://github.com/apache/directory-studio
> 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.
I can take care of adding checkstyle check that lets the build fail when there is any violation. Are there already formatting rules defined for Kerby? I think they differ from other Directory projects. Is it plain Sun/Oracle conventions?
Kind Regards,
Stefan
Re: Code quality & formatting rules
Posted by Stefan Seelmann <ma...@stefan-seelmann.de>.
On 07/02/2015 11:36 AM, Zheng, Kai wrote:
> 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.
For Studio I just added a block quote, but of course it is visible:
https://github.com/apache/directory-studio
> 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.
I can take care of adding checkstyle check that lets the build fail when
there is any violation. Are there already formatting rules defined for
Kerby? I think they differ from other Directory projects. Is it plain
Sun/Oracle conventions?
Kind Regards,
Stefan
RE: Code quality & formatting rules
Posted by "Zheng, Kai" <ka...@intel.com>.
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:elecharny@gmail.com]
Sent: Thursday, July 02, 2015 4:30 PM
To: kerby@directory.apache.org
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: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 !
Emmanuel
Re: Code quality & formatting rules
Posted by Kiran Ayyagari <ka...@apache.org>.
On Thu, Jul 2, 2015 at 4:30 PM, Emmanuel Lécharny <el...@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