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:39:28 GMT

On 31 août 2011, at 10:52, Felix Knecht wrote:

> Hi Pierre-Arnaud,
> 
> On 08/31/2011 10:33 AM, Pierre-Arnaud Marcelot wrote:
>> Hi Felix,
>> 
>> On 31 août 2011, at 10:07, Felix Knecht wrote:
>> 
>>> On 08/31/2011 09:08 AM, Pierre-Arnaud Marcelot 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.
>>> 
>>> I absolutely agree. On which tag (groupId | artifactId) would you order them?
No matter wich one we take it should be the first tag after<dependency>, so for artifactId
it would be
>>> 
>>> <dependency>
>>>  <artifactId>shared-ldap-model</artifactId>
>>>  <groupId>org.apache.directory.shared</groupId>
>>> </dependency>
>>> 
>>> Ordering by groupId would make it possible to group then under a common "label".
>> 
>> My personal preference would be to keep the structure of a dependency element the
way it is used at the moment (groupId tag first then and artifactId in second), like this:
>> <dependency>
>>  <groupId>${groupId}</groupId>
>>  <artifactId>${artifactId}</artifactId>
>> </dependency>
>> 
>> I would order dependencies by groupId first and then, in the case of an identical
groupId, I would order them via their artifactId.
>> 
>> That's why I think we should stick with the order groupId, artifactId.
>> But I might be wrong. ;)
> 
> I don't think that you're wrong, I totally agree with this.
> 
>> 
>> Of course, ordering rules should not applied on "labels" themselves and "labels"
could be ordered the way we want, according to the importance of each "label".
>> BUT, in each label, the dependencies should be ordered by alphabetical order.
>> 
>> This scheme would allow to have different regroupments ("labels") ordered by importance
and each of the regroupments dependencies would be classified by alphabetical order.
>> 
>> Does it make sense?
> 
> I'm not that positive of these regroupments ("labels").
> - Having an alphabetic ordering I'd assume to find a dependency according the ordering
- this might not be the case because of the regroupments.
> - Where do overridden transitive dependencies belong? Into the regroupment of it's parent?
Into a regroupment labeled "miscellaneous"?
> - What are the criterias about the importance of a label?
> 
> That's why I'd have them ordered as you proposed (groupId/artifactId) but not regrouped
by importance.

Actually, this concept of regroupments isn't something new and that's something that is already
in place in both ApacheDS and Studio pom.xml files.
In ApacheDS, we use to have ApacheDS' own modules dependencies first, then Shared dependencies,
then Commons dependencies, etc. etc...

But your questions above are perfectly valid...

I do believe that we really have a tendency to organize things into groups (like files in
folders) and that can be (and already is) the case for dependencies in our pom files.

Some dependencies gets regrouped automatically because they share a common groupId for example.
But we might like to regroup two or more dependencies that are far away in terms of alphabetical
order but that are related in terms of purpose.
An example to that would be our logging dependencies.
Here's an excerpt from ApacheDS' pom.xml (line 898-916):
>>       <!-- Logging dependencies -->
>> 
>>       <dependency>
>>         <groupId>org.slf4j</groupId>
>>         <artifactId>slf4j-api</artifactId>
>>         <version>${slf4j.api.version}</version>
>>       </dependency>
>> 
>>       <dependency>
>>         <groupId>org.slf4j</groupId>
>>         <artifactId>slf4j-log4j12</artifactId>
>>         <version>${slf4j.log4j12.version}</version>
>>       </dependency>
>> 
>>       <dependency>
>>         <groupId>log4j</groupId>
>>         <artifactId>log4j</artifactId>
>>         <version>${log4j.version}</version>
>>       </dependency>


Some of these dependencies would have been far away from each other when classified by alphabetical
order but here, regrouped as common "Logging dependencies" set, I think they make more sense.

Regards,
Pierre-Arnaud

> Regards
> Felix
> 
>> 
>>>> 
>>>>> 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
>>>>> 
>>>> 
>>> 
>> 
> 


Mime
View raw message