directory-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Pierre-Arnaud Marcelot ...@marcelot.net>
Subject Re: Coding rules, some more things to discuss
Date Wed, 31 Aug 2011 09:48:15 GMT

On 31 août 2011, at 11:06, Kiran Ayyagari wrote:

> On Wed, Aug 31, 2011 at 12:38 PM, Pierre-Arnaud Marcelot
> <pa@marcelot.net> wrote:
>> On 30 août 2011, at 15:32, Emmanuel Lecharny wrote:
>> 
>>> Hi guys,
>> 
>> Hi,
>> 
>>> we just had a private convo with Pierre-Arnaud about coding rules. We are not
following exactly the same type of rules in Studio and in ADS, which is quite normal. There
are some reason why there is a divergence.
>> 
>> As we've seen in our private convo with Emmanuel, the divergence is very very subtle
and it's mostly a divergence on "unwritten" rules that we can't find in our coding standards
documentation...
>> The rest of rules are clearly well followed (except some very very old parts of code
that haven't been touch for years now).
>> 
>>> I think we need to discuss a few things here.
>>> 
>>> Currently, we have a small coding standard page : https://cwiki.apache.org/confluence/display/DIRxDEV/Coding+standards
>>> 
>>> It's pretty simple, with not a lot of rules. Both ADS and Studio are more or
less following those rules which were established a long time ago (there are still some very
old files in ADS which are not following those rules, but with more than 3000 files on the
project, we won't spend one month reviewing all of those files one by one...)
>> 
>> Same thing for Studio.
>> Some pretty old files may not be following *all* the rules.
>> 
>>> I'd like to add a few more rules, at least for ADS, and suggest that Studio keep
a slightly different sets of rules, but in any case, I'd like to see all the rules added to
the wiki.
>>> 
>>> Here is what I think would be good for ADS :
>>> - add a blank line before each 'if', 'for', 'do', 'switch', 'case' unless the
previous line is a '{'
>> 
>> In most cases I agree, but I find some cases where I prefer leaving the if close
to the previous expression.
>> Especially in cases where I get a variable and I want to test something on it just
after.
>> Here's an example:
>>>> // Testing variable
>>>> SomeType variable = anotherVariable.getVariable();
>>>> if ( variable.hasFlag() )
>>>> {
>>>> [...]
>> 
>> In that particular case, IMO, it helps grouping expressions for a better readability.
>> 
>>> - get rid of trinary operator ( expr ? op1 : op2 )
>> 
>> I would prefer keeping it as it's very handy for variable nullity checks.
>> 
>> Here's an example:
>>>> return ( ( variable == null ) ? "null" : variable );
>> 
>> 
>> I prefer the compact format instead of this:
>>>> if ( variable == null )
>>>> {
>>>>     return "null";
>>>> }
>>>> else
>>>> {
>>>>     return variable;
>>>> }
>> 
>> Now, if I'm the only one liking it, I will refrain myself from using it in the future...
;)
>> 
>>> - add a blank line before each 'return'
>> 
>> +1
>> 
>>> - in if ( expr ), we should use '(' and ')' for expressions containing an '=='
or any other logical connector
>> 
>> +1
>> 
>>> We also may want to add some rules for pom.xml.
>> 
>> +1
>> Even though I think we already share the same rules, having them written is always
a plus. Especially for newcomers.
>> 
>>> Typically, what I'd like to see is a blank line between each element like <dependency>.
Here is an example :
>>> 
>>> <dependencies>
>>> <!-- Apache Directory Studio library plugins dependencies -->
>>> <dependency>
>>> <groupId>org.apache.directory.shared</groupId>
>>> <artifactId>shared-ldap-model</artifactId>
>>> <scope>provided</scope>
>>> </dependency>
>>> 
>>> <dependency>
>>> <groupId>org.apache.directory.shared</groupId>
>>> <artifactId>shared-util</artifactId>
>>> <scope>provided</scope>
>>> </dependency>
>>> 
>>> This is to separate all the items which have the same dame, for clarity sake.
>> 
>> Why not.
>> I liked the idea of grouping a set of dependencies under a common "label" like this
"Apache Directory Studio library plugins dependencies" in your example.
>> But adding a blank line doesn't really break either...
>> So, +1.
>> 
>> One more thing I'd like to add to pom.xml guidelines, I really like when dependencies
are ordered in alphabetical order.
>> In Studio, we deal with a lot of dependencies for each project (mostly Eclipse dependencies
+ a few others) and having them ordered REALLY helps when looking for something, IMO.
>> 
> won't this be a bit laborious to order the dependencies by name

Hopefully, most of the pom.xml files are already almost following this path.
I think this is somehow a natural thinking.
At least for me. Even though, I admit it, I think I'm a little too maniac about ordering things
in code (at home and in life too...)
Maybe, I should consult someone... ;)

> ( we have Ctrl + F anyway :)

Haha, yeah, that's true.
One big advantage I also see by the alphabetical ordering of dependencies is that it allows
you to catch duplicated dependencies easily, which could then be a problem if you think you
removed a dependency and it's still there because it was duplicated.
I had this problem personally a few times already.


>>> For Studio, I let Stefan and Pierre-Arnaud define the rules they prefer to use,
as i'm not working often on its code.
>> 
>> For the sake of a better interaction and simplicity, I think we should share the
same rules across the whole Directory project.
>> As I'm mostly the only dissident on some of the facts above, I can and will adapt
myself.
>> Not a big deal (except for the trinary operator... ;) ).
>> 
>> Regards,
>> Pierre-Arnaud
>> 
>> 
>>> Any comments ?
>>> 
>>> --
>>> Regards,
>>> Cordialement,
>>> Emmanuel Lécharny
>>> www.iktek.com
>>> 
>> 
>> 
> 
> 
> 
> -- 
> Kiran Ayyagari


Mime
View raw message