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

[Simo Sorce]

On Fri, 2008-10-31 at 15:56 -0400, Dmitri Pal wrote:
> Hello,
> 
> In addition to the coding guidelines published on freeIPA.org the 
> following (currently undefined) rules are suggested:
> 
> *Spaces:*
> 
> No tabs all indentation 4 spaces.

ack

> *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 :-)

> *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.

> * 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

> * Declarations of external functions

Should not be needed, usually headers should contain foreign functions
definitions.

> * 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.

> * Implementation of the exposed functions from the module
> * Implementation of the static functions

Given the above comment, static functions usually come first.

> 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.

> *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.

> 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 :-)

> *Naming of the variables: *
> 
> 
> There are multiple different styles that developers usually use to name 
> variables. It can be very short like “ctx” or “”buf”; more explicit 
> “context” or “buffer” ; detailed using underscore “hash_context” or 
> “input_buffer”; detailed using capital letters “hashContext” or 
> “inputBuffer” all these ways of naming variables are acceptable though 
> low case with underscore is preferable and highly recommended.

+1

>  The 
> developers should not start variables with capital letter. It is 
> beneficial and thus suggested to use self explanatory names for 
> variables that carry significant information. Variables that are used to 
> perform a simple operation and act as a temporary storage can have 
> shorter names.

+1

> 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.

> *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.

> * 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.

> 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.

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)
{


Do not put spaces before or after parenthesis:

OK:  int foo(int bar, int baz);
NOT OK: bad ( arg1 , arg2 );


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);

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.


Use only C style comments /* blah */ and not C++ style comment: // blah


Add to these the coding style predicaments already available on
freeipa.org :-)

Simo

-- 
Simo Sorce * Red Hat, Inc * New York