Monday, June 13, 2011

pedantic coding

(hopefully this is obvious, but these opinions are my own and should not be construed to represent those of my employer or the zookeeper project. both sometimes, erroneously :), disagree with me.)

lately a open source project i'm involved with, zookeeper, was the subject of a rant about clean code.

i'm a big believer in clean code and best practices. some of those best practices also include things like "if it aint broke, don't fix it".

one reason to write clean code is to avoid bugs, but another important reason is to make the code maintainable. if best practices are followed, the next developer who subscribes to these same practices can jump into the code quickly.

as i've worked on different code bases, i have found some really nice reasons to break best practices. for example, in the linux code base there is this common pattern:
void some_function() {
        ... initial section ..
        if (error) goto out1;
        ... more setup ...
        if (error) goto out2;
        ... finish the work ...
out2:
        ... cleanup from more setup ...
out1:
        ... cleanup from initial section ...
}  
the above code breaks the cardinal rule of C "goto considered harmful". so harmful in fact that some programmers don't even know that C has a goto. i do not use gotos myself; however, that pattern is extremely useful in the kernel where setup and teardown of structures can be complicated and can cause big problems if done incorrectly. i wouldn't call such code unclean.

other code bases have similar issues and still subscribe to the "no goto" rule. one alternate way to achieve the above is:
void some_function() {
        do {
                ... initial section ..
                if (error) break;
                do {
                        ... more setup ...
                        if (error) break;
                        ... finish the work ...
                } while(0);
                ... cleanup from more setup ...
        } while(0);
        ... cleanup from initial section ...
}
this form has many of the benefits as the earlier code and doesn't use goto. the while(0) construct does look a bit strange when you first look at it though. it's also not as easy to see where the break is going to take you. also, the indentation gets quite deep. personally i like the gotos better.

it can get a bit out of control though. i just finished teaching a graduate OS class on Linux. there is one particularly hairy piece of code that is central to the cache manager that really shows why gotos are scary! checkout do_generic_file_read in the linux kernel. it's pretty out of control, but it is also doing something pretty complex. i'm not sure that making the code clean by removing all the gotos and refactoring to get smaller functions would make it better. it may, but what we have works well, and once you read through it you realize that it isn't that hard to follow.

so where am i going with all of this? i think we need to differentiate between clean code and pedantic code. i propose that clean code is something that follows effective programming rules with an eye toward readability and maintainability, while pedantic code strictly follows effective programming rules without regard to readability and maintainability. clean code has a degree of subjectivity and may occasionally violate a rule, while pedantic code can be objectively and precisely measured.

some of this is summed up nicely in the introductory chapter of the book Clean Code:
We are willing to claim that if you follow these teachings, you will enjoy the beneļ¬ts that we have enjoyed, and you will learn to write code that is clean and professional. But don’t make the mistake of thinking that we are somehow “right” in any absolute sense. There are other schools and other masters that have just as much claim to professionalism as we. It would behoove you to learn from them as well.
there is also the issue of working code vs new code. there is always a trade off between making minimal changes to code to fix a bug, improve performance, or add some other improvement, and cleaning up code. especially with a mature code base, minimal changes minimize the chance of introducing new bugs. (fixing a bug and introducing a worse new bug with the fix is terribly tragic!) cleaning up code can really help long-term maintenance, if done correctly, but it must be weighed against the big short-term risk of code instability and bugs. it is great to occasionally bite the bullet and rip out the old and put in the shiny new, but you need to do it when the old has quiesced a bit and you can have a shiny new release that people will test and use with caution.

i would like to conclude with these ideas: clean is not an objective universal metric, there are different views of clean; it helps expose bugs, but it doesn't make them go away; code maturity needs to balanced against code cleaning; and, as i mentioned earlier, cleanliness is something to strive for, but don't be pedantic about it. (yes, i purposely ended that clause with a preposition :) i don't think you should declare something not production worthy based solely on your definition of cleanliness. i would love to see a cleaner, according to my definition of clean, version of  do_generic_file_read, but i don't hesitate to rely on it everyday for my livelihood.