[Asterisk-Dev] [Rant] [long] - code style and quality

[Luigi Rizzo]

I am new to the project, but i find the coding style used in many
of the components of asterisk very hard to follow and maintain. The
examples i supply below are not to point fingers but just to put
facts behind my claims.

The request I make, which I hope you will agree with after reading
the rest of this email, is that developers should be more careful
about code quality, and should actively try to fix things as they
encounter functions that are more than 100 lines or code that
looks like cut&pasted from other parts.
These changes are not gratuitous, they help revealing potential
bugs, and make the program enormously more maintainable.

List with examples follows, in case someone agrees feel free to
put this stuff in the 'coding style' file

	cheers
	luigi

LIST OF COMMON STYLE BUGS

1. in many cases we have blocks of this kind:

	if (condition) {
		bla bla bla...
		... 100+ lines of code ...
	} else {
		ast_log(LOG_WARNING, "too bad, can't do that");
		return -1;
	}

   see e.g. apps/app_voicemail.c function sendmail() or its clone
   sendpage(), channels/chan_sip.c:sip_new(),
   channels/chan_sip.c:build_peer(), and more.

   Also similar (not ending with a return but a break;)
   channels/chan_zap.c:zt_handle_event() in e.g.
	case ZT_EVENT_WINKFLASH:
		case SIG_FXOKS:
   and so on.

   The more readable way to write these statements is to put
   the very short block first.

	if (!condition) {
		ast_log(LOG_WARNING, "too bad, can't do that");
		return -1;
	}
	bla bla bla...
	... 100+ lines of code ...
	
   The return/break, when present, even saves the need for an extra
   level of indentation making This way one immediately sees the
   simple case, and can focus on the complex one without having to
   remember that there might be an alternate path of unknown
   complexity.

2. Huge source files and functions.

   Several source files are between 4k and 10k+ lines of code.
   A large number of functions are 300-700+ lines long.
   E.g. channels/chan_zap.c:zt_handle_event()  at 700 lines,
   channels/chan_zap.c:zt_read() about 350, 
   pbx.c:ast_pbx_run() 330 lines,
   apps/app_voicemail.c:load_config() about 370,
   apps/app_voicemail.c:vm_execmain() almost 400, and so on

   This is way beyond good programming practice.
   I cannot believe that they couldn't be made shorter or broken up
   in subfunctions to make them more readable.
   Part of the problem is #3, #4, and #5 below, but a lot comes
   from carelessly adding code to the body of functions without
   considering readability.

3. Unused language features.

   The C language has the conditional expression operator, and there
   is no reason not to use it when it can reduce the amount of
   programming errors and ease readability.

   A sample from zt_read():

	if (ps.rxisoffhook)
	{
		p->subs[index].f.subclass = AST_CONTROL_RADIO_KEY;
	}
	else
	{
		p->subs[index].f.subclass = AST_CONTROL_RADIO_UNKEY;
	}

   could be written as

	p->subs[index].f.subclass = ps.rxisoffhook ?
		    AST_CONTROL_RADIO_KEY : AST_CONTROL_RADIO_UNKEY

   and not just because it is a lot shorter, but because when someone
   reads the code immediately understand that the test affects only
   the right hand side of the expression, not the left hand side.
   And because by writing the code this way the programmer does not
   have a chance to write, by mistake, the LHS in two different ways.

4. No use of functions for similar blocks of code

   Also from zt_read line 4093:

        p->subs[index].f.frametype = AST_FRAME_NULL;
        p->subs[index].f.datalen = 0;
        p->subs[index].f.samples = 0;
        p->subs[index].f.mallocd = 0;
        p->subs[index].f.offset = 0;
        p->subs[index].f.subclass = 0;
        p->subs[index].f.delivery.tv_sec = 0;
        p->subs[index].f.delivery.tv_usec = 0;
        p->subs[index].f.src = "zt_read";
        p->subs[index].f.data = NULL;

   and repeated almost completely in the same function at line 4295
   (with some fields forgotten, i believe).
   And in the same file, lines 3261 and 3967.
   Clearly, this would deserve a frame_init() function, if not already there.

5. Abuse of cut&paste programming

   Examples:
   apps/app_voicemail.c functions sendmail() and sendpage()
   have probably 60 lines in common (in addition to issues #1 and #3
   mentioned above) clearly pasted from one place to another.

   channel.c:ast_channel_walk_locked() has a block of 12 lines
	if (ast_mutex_trylock(&l->lock)) {
		...
	}
   replicated twice. The second instance is completely useless, as
   by code inspection you realize that you can jump to the first one.

   channel.c:ast_safe_sleep(x, y) is exactly the same as
	ast_safe_sleep_conditional(x, y, NULL, NULL)
   it does not make sense to rewrite the body - the optimization
   in avoiding the test in the loop (see also #6 below) is negligible
   compared to the overhead of the subsequent ast_waitfor() which
   calls ast_waitfor_n() which calls ast_waitfor_nandfds()  which
   finally calls the system call poll().


6. Abuse of micro optimizations

   In the current state of the code, micro optimizations to save a
   handful of clock cycles for function call overhead is pointless
   especially when it causes code bloat or renders the source less
   readable or portable.

   Example #1:
   pbx.c has a long and obscure macro, EXTENSION_MATCH_CORE, which is
   called only twice by two very similar functions. It would be
   trivial to merge them (perhaps using wrappers to differentiate)
   and get rid of the macro.

   Example #2:
   ast_copy_string() ends with a __builtin_expect() to check if it
   is necessary to decrement dst to write the terminator. Do you really
   believe the hint to the compiler (because this is what it does)
   helps a lot after the function call overhead ? If anything,
   one should optimize the loop perhaps by relying on the os/compiler
   library to do the strncpy in an efficient way eg. a word at
   a time...