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

[Steve Underwood]

If you don't like * the way it is, don't use it. If you want to make it 
better, submit patches.

If you have the time to write such a long whiney and pointless message, 
you probably have the time to do something useful. Try that instead. 
Sure the Asterisk code is messy in lots of ways, but it actually exists 
in a useful state because the developers focussed on functionality. Most 
of the neat and stylish programs never make it to a working state. If 
you want to make useful criticism, look at the structural issues in *. 
There are many of those which could be far better. Do something useful 
about those, and people will be impressed.

Regards,
Steve


Luigi Rizzo wrote:

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