[Freeipa-devel] Suggested additions to codding standard page on freeIPA wiki.

[Dmitri Pal]

Simo Sorce wrote:
> On Fri, 2008-10-31 at 15:56 -0400, Dmitri Pal wrote:
>   
>
>> *Length of line: *
>>
>> Not more than 100-110 characters. Just keep it reasonable. Longer lines 
>> are harder to deal with due to different sized of monitors and different 
>> editors used.
>>     
>
> I am for strict adherence to not more than 80 chars per line.
> While I myself used to use up to 120 chars I found out that 80 lines is
> more usable in many situations, including debugging python code/C code
> on a console, letting people use their tools of choice, and yes, fitting
> 3 terminals in a row :-)
>
>   
I think 80 is to short with the modern monitors and screens.
We should not limit us but try no to be overwhelmingly long

>> *Module structure: *
>>
>> The module should be structured in the following order:
>> * Copyright boilerplate
>> * Includes
>> * Local defines – used only inside module. Global/shared defines should 
>> be in the header
>> * Local module typedefs - Global/shared typedefs should be in the header
>>     
>
> I would like to discourage use of typdefs unless really necessary, like
> when a very basic type is defined. I find the code is much more
> comprehensible if a struct is immediately recognizable as such as well
> as pointers.
>
>   
I think this is should be formulated as preference with suggestion not 
to use typedefs.


>> * Module variable declarations – variables that are global for the module.
>>     
>
> Global variables should be discouraged as well, they make for very poor
> code. Should be used only if no other way can be found. They tend to be
> not thread/async safe
>
>   
Agree. But if there are some they should be covered.


>> * Declarations of external functions
>>     
>
> Should not be needed, usually headers should contain foreign functions
> definitions.
>
>   
Agree. The word usually is crucial here. Sometimes you have collisions 
on the headers in this case you need to include things explicitly. But 
this is more an indication of the bad header design.


>> * Declarations of static functions
>>     
>
> I find it much easier to declare functions only if they need to be
> referenced before the body of the function is defined. Otherwise just
> use the ordering in the file to avoid declarations.
>
>   
I agree but this should be a preference not a rule.

>> * Implementation of the exposed functions from the module
>> * Implementation of the static functions
>>     
>
> Given the above comment, static functions usually come first.
>
>   
I kind of disagree. This should be a preference.


>> It is not required but recommended to group and position functions in 
>> the module so that it is easier to understand.
>>     
>
> This is very controversial, for me easier to understand is when the
> innermost function is first in the file and the outermost is last, but
> for some that seem an unnatural bottom to top ordering.
>
>   
No it is not. I think this allows you to follow your natural flaw of 
logic and me mine.
I prefer implementing the functions that are main to the module first 
and stub out the functions that I need.
So naturally I read the code the same way. I do not care about the 
details and low level functions that act as helpers.
I want to see and concentrate on the core functionality and not dig it 
into the module to find it scattered somewhere in the middle or in the 
bottom.

I guess this is a personal preference so I would suggest leaving it as 
it was spelled.

>> *Comments: *
>>
>> Sections (groups of functions) can be separated with a line of comments 
>> like this:
>>
>> /************ Main Interface Functions ****************/
>>
>> Or this
>>
>> /********* Static Internal Functions **********/
>>     
>
> Is it really necessary to state this? Static functions are usually
> pretty obviously static ... Also I usually prefer to keep static
> functions related to a specific public function close to said function
> so that there is no need to jump around the file when checking what a
> related function does.
>
>   
Yes. I think it will. It will help you with your approach to processing 
code to read the code I write and vice versa.
These things are very helpful when you face the code that is organized 
in the way it is not intuitive for you to digest.

>> Each function must have a comment describing what this function does and 
>> returns.
>> There is no specific format for this but it is a good practice to 
>> describe what parameters mean and what return codes function returns 
>> under what conditions.
>>
>> Inside the function comments should help reader to digest the logic of 
>> the function. They should answer the main question “why is this line of 
>> code here and what it tries to accomplish?”. Not everything should be 
>> commented but practice shows that comments giving the hint about what 
>> the developer what trying to accomplish with this or that construct 
>> usually turn out to be pretty helpful.
>>     
>
> This is fine as long as we don't have things like:
>
> int i = x +1; /* add 1 to x and assign it to i */
>
> Let's avoid useless comments :-)
>
>   

Agree.

>
>   
>> Do not use Hungarian notation (prefixing variable with the type). The 
>> only exception to the rule is pointers. The following example 
>> illustrates the case:
>>
>> void function_that_does_something(data_set inventory) {
>> int number_items =0;
>>
>> err=get_number_of_items_in_set(inventory, number_items);
>> ...
>> }
>>
>> int get_number_of_items_in_set(data_set inventory, int* p_number_items) {
>> ...
>> }
>>     
>
> This definition is redundant, int * already defines the variable as pointer.
> Please let's avoid the insane Hungarian Notation completely.
>
>   
>> In this case the variable in the second function is related to the 
>> variable in the calling function but it is a pointer. This is useful 
>> since naming variables same will cause confusion with type (especially 
>> if there will be cutting and pasting of code which I use to do a lot), 
>> and naming them differently will break the relation. Like having 
>> number_items in one function and item_count in the other and one is 
>> actually a pointer to another is hard to work with. Prefixing with "p_" 
>> solves the issue nicely.
>>     
>
> No, the compiler solves any issue nicely, it will tell you if you are
> using the wrong type.
>
>   

There are cases when these things caused a lot of trouble. Not 
explicitly indicating pointers where variables are both pointers and 
just variables IMO makes the code much harder to read and maintain.
I just makes things harder for no value but if others insist I would not 
resist. Any other opinions?


>> *Names of the functions: *
>>
>> Looking at the code (server.c for example) I have seen all sorts of 
>> names for functions used in one module:
>>
>> setup_signals(void)
>> BlockSignals(true, SIGPIPE);
>> poptGetNextOpt(pc)
>>
>> We should try to stick to one style which will be low case, multi word, 
>> _ separated like:
>> monitor_task_init(struct task_server *task);
>>     
>
> The later is the preferred form, poptGetNextOpt and BlockSignals come
> from external libraries, so we have no control over them.
>
>   
Yes.


>> * Declarations *
>> Major declarations such as typedef's, stuct tags, etc. deserve to be 
>> identifiable on their own in some fashion.
>> Often this was done with an initial cap, but since we're using 
>> lower_case_with_underscores then I think the convention of appending 
>> "_t" is a good way to go and is consistent with the Posix and kernel 
>> conventions.
>>     
>
> Only if we are defining a new type, something very rare. Again I am for
> avoiding typedefs altogether except for rare cases.
>
> If I need a structure to hold something a bout a socket this is how it
> should be defined:
>
> struct socket_stuff {
>     int foo;
>     long bar;
>     struct baz;
> };
>
> no typdefs.
>
>   
Well... I kind of disagree but I can live with that.



>> Other name formats should not be used unless it is an external function 
>> we do not have control over.
>>
>> The wiki page will be updated based on this discussion.
>>     
>
> Thanks.
>
>
> I'd like to add some more style related conventions I tend to use.
>
> Function definitions:
>
> Always define a function on one line with the opening parenthesis on the
> next line.
> Example:
> struct foobar *foo_bar_fn(int foo, int bar)
> {
>
> Do not put the "{" on the same line or the return type on a previous
> line like.
>
>   

+1
> Example of NOT ok declaration:
> struct foobar *
> foo_bar_fn(int foo, int bar) {
>
> if the function declaration does not fit 80 chars, put arguments after
> the first on following lines and indent them to start after the "("
>
> Example:
> int very_long_function_name_that_makes_arguments_not_fit_80(int foo,
>                                                             int bar)
> {
>
>
>   
+1
> Do not put spaces before or after parenthesis:
>
> OK:  int foo(int bar, int baz);
> NOT OK: bad ( arg1 , arg2 );
>
>
>   
+1
> Avoid complex variable initializations (like calling functions) when
> declaring variables like:
>
> 	int foobar = get_foobar(baz);
>
> but split it in:
> 	int foobar;
>
> 	foobar = get_foobar(baz);
>
>   
+1
> This makes it simpler to follow code in a debugger, and also avoid
> forgetting to test the return value of the function for errors.
>
>
> Always declare all variables at the top of the function, normally try to
> avoid declaring local variables in internal loops.
>
>   
+1
> Use only C style comments /* blah */ and not C++ style comment: // blah
>
>
>   
+1
> Add to these the coding style predicaments already available on
> freeipa.org :-)
>
>
>   
I am planning to add this back to freeIPA.org

Any other opinions and suggestions?

> Simo
>
>