Christopher Seiwald's Seven Pillars of Pretty Code argues that code that looks good, works good:
This is a companion discussion topic for the original blog entry at: http://www.codinghorror.com/blog/2006/06/pretty-code-ugly-code.html
This is a companion discussion topic for the original blog entry at: http://www.codinghorror.com/blog/2006/06/pretty-code-ugly-code.html
I’ve never been a fan of block aligned variables in C either. I remember working on some code that had this and I was almost afraid to touch it. Any changes involved hitting space an awful lot to get things to align. It may look OK when you’re done, but in the process of editing, it’s annoying.
And I second the notion that make.c seems ugly. But then again, I do work with Lisp almost exclusively.
To each, their own. Arguing about which language to use is completely irrelevant to the point at hand and is likely to spark a religious war. People who are Java programmers will try to solve every problem in Java. People who are C programmers will tend to do the same with C. People who are C# programmers will tend to do the same with C#. Perl programmers will do the same with Perl.
In my opinion, it’s always better to use an inferior tool that you are very well acquainted with than it is to use the best tool for the job when you haven’t the faintest idea what to do with it!
I have some comments about make.c and his seven pillars, but I’ll save those for a follow-up comment
Comments on the pillars:
-
Unless the original style is virutally non-existant! I’ve worked with plenty of code where I ended up reformatting the entire file before even beggining to deal with it.
-
Better yet: two or more blocks of code that do the same or similar things should be ONE block of code, possibly with some flags to distinguish the behaviour. It’s better to invest the extra time to deal with eliminating extraneous code than to deal with every time that the alternate code becomes out of date.
-
However, code structure oftentimes dictates the overall behavior. if indentation is called for, use it.
On the make.c:
Never ever use /* */ as comment blocks! Unless you have a decent editor (not always an option), these type of comment blocks makes it virtually impossible to easily comment out a block of code for testing (barring adding things like “if (0) {” which can quickly cause problems)
make0 should be broken up into separate functions.
“Don’t even think of deciphering this.”
Not something I’d like to see in a style exemplar.
The biggest problem with make.c is the humongous function. Break that function up, and the need for comments (which quickly tend to go out-of-date) is greatly reduced.
As for the pillar that states that code that does identical things should look the same – well, that’s true if a single function should look like itself.
Shouldn’t we be searching for something prettier on the inside? Such as a higher level language?
Although a higher level language might be a solution to some problems, I don’t believe it will make code fundamentally prettier. People do horrible things with English after all 
I don’t believe it will make code fundamentally prettier
But isn’t pretty formatting a relative constant across all languages? We can line up variable names in FORTRAN just as well as we can in Ruby.
#1 is only really feasible if you’ve got nice code to start with. The work I’ve been doing lately my code has looked to me like islands of sanity in the morass of terrible programming practice that makes up the rest of the code.
Heh, I always forget block comments are /* /, I always use / //*/. That way you just have to add an extra slash in front of the first one to uncomment, and remove it to recomment.
But it’s a pain if you try to comment block comments. I’ve never quite understood why you can’t nest them.
I liked your article and the discussion in the comments. I too wasn’t that impressed with the code (although, of course, it could have been much better).
My major criticisms:
- too much commenting, and in multi-line blocks.
I hate the following kind of things.
/*
- gets the attenuation
*/
Almost all C compilers let you use //, and if not, then put it on one line dammit.
-
related – putting the parameters each on their own, indented line. I found he was wasteful of screen real estate.
-
HUGE indentation increment
-
too frequent line breaks on booleans
-
Mixing of blocking (with {}) conditional, and multi-lining or not
-
in reponse to others, I think i j are very rarely good ideas. I think iLight, iCol, iRow work very well, don’t require much typing, and reduce the likelihood that variables will be re-used for different purposes.
-
Not distinguising NULL, 0, and FALSE
-
Not so keen on his spacing (none after keywords, one after # and before include
I like #1 a lot: I worked for a while recently on a system that was over 10 years old and still being extended. I’d submit that one of the reasons for its longevity was its consistent design and style. I didn’t particularly like either, but it was easy to drop in at any point in the code and figure out what was going on.
Much of the rest I’m less certain about. I don’t much like commented blocks of code, for example: why not just refactor out a new routine? Ditto megadentation: just extract the innards. More than once if really necessary.
Oh, and I work in C#, VB and Ruby. Admittedly I’d probably stick with Ruby given the choice, but I’m quite happy in the others.
I thought I was a harsh critic of code, but MAN, what a bunch of code nazis you guys are! I don’t like C much because it’s impossible to write ANY code that involves string parsing or dynamic memory management and not have it look ugly. But IMHO this code (probably because it avoids string handling / dynamic allocation) is pretty good for C code.
When I look at code, my #1 criteria is no more than “can I read it, understand it, and if need be, safely modify it?” I had no problem reading and understanding this code, even if it uses a coding style I see rather infrequently. The only thing that bothered me (momentarily) was the use of “p” for “parent” and “t” for “target”, but now that I think about it, it’s rather elegant. These two variable names are crucial for understanding the code, and the fact that they’re given such short names makes them stand out whenever they are used.
And as far as maintainability goes, I think he did a very good job. After reading the code, I don’t have much fear that changes made in one place would need to be made in many places, nor do I fear that the overall design is creaking and liable to fall apart with the next feature addition.
Most of the things mentioned here are total nitpicks that don’t affect readability or maintainability in the slightest. I feel as though a lot of commentators here are just regurgitating old shopworn aphorisms in an attempt to feel smarter than the original coder.
“There are functions named make and make0. Ouch.”
What’s so difficult to understand about this? What better way is there to illustrate the relationship between the two functions? One is obviously a more public function which uses the internal, oddly named function to do most of the heavy lifting. What would you have named it? make_guts, maybe? make_pass1? As long as make0 never calls back into make (which is true here), I’m fine with the naming scheme.
“Better yet: two or more blocks of code that do the same or similar things should be ONE block of code, possibly with some flags to distinguish the behaviour.”
I agree with you. And with Seiwald. Repetition is bad. Parallelism is good.
(or would you have me say: Repetition_Or_Parallelism_Is_Good_Or_Bad(bool fRepetition))?
“The biggest problem with make.c is the humongous function.”
Not really. It’s straight line code. Chopping it up into a half-dozen mini-functions wouldn’t add a bit of clarity; it’d just force you to yo-yo around to see what’s happening.
Now, if you have a 50-line if or while statement, THAT’s a real problem. Ditto code that sets a flag at the top and checks it 100 lines later. “Write short functions” is good advice which prevents these REAL sins from being committed. But since he’s not doing that here, I can’t complain.
(If it really bothers you, just put empty braces around each commented “step”, and visualize them as anonymous functions which, for readability’s sake, have been inlined).
“Never ever use /* */ as comment blocks! Unless you have a decent editor (not always an option), these type of comment blocks makes it virtually impossible to easily comment out a block of code for testing.”
Steve, meet #if 0. #if 0, meet Steve.
(Actually, I feel that commenting-out code for testing purposes may be a suboptimal practice, but I’ll elide that discussion for now).
“This is my biggest pet hate of some (not many) C programmers programming style! WHY do you hit return after the data type in a function declaration??”
Why not? Is it provably any better or any worse than any other arbitrary style? I don’t use this style myself, but I don’t yell at the people who do.
“Over-commenting, or comments of suspect usefulness, make things worse.”
Agreed. But if you think this is over-commenting, you should try reading some of the code I see on a daily basis. (I love it when people document the fact, for example, that you can safely call “delete” with a null pointer). In my view this coder uses comments effectively to show the overall design and structure of the program, and not to document details which are more obvious in C than they are in English.
“Keywords deserve their own space.”
Of all arbitrary rules to make. I personally put the space outside the parenthesis, but he’s an inside-whitespace man. Ain’t no harm in it. Some people don’t put spaces around the parens at all. Does it honestly make it harder to read, or does it just “irk” you because you’re not used to it? For me, I can read anything as long as they put whitespace around the operators (like and ||).
“I found he was wasteful of screen real estate.”
What’s the problem? Is there some global whitespace shortage crisis going on that I should know about? Use all the screen real estate you want – they’ll make more.
(Yes, at times there’s a benefit at keeping a complicated algorithm readable in one screen of code … but that’s not going on here, is it? The important thing is that the coder doesn’t force you to scroll around to find the declaration of a variable (usually they’re in the tightest scope already), to find the antecedent of an “else” clause or to find the terminating condition of a while loop).
I took this article to mean that he was saying if you format things consistently and in a way that makes the code easy to read, then the result is that it is easier to read and maintain, not that it works better. Based on the fact that you were able to find some things (presumably pretty quickly) in his make.c that you disliked, his formatting of the file must have made it relatively easy. [I am imagining you didn’t spend a great deal of time looking through his file.]
I thought I was a harsh critic of code, but MAN, what a bunch of code nazis you guys are!
his formatting of the file must have made it relatively easy
One of the points of this post was to highlight the meaninglessness of formatting choices.
Sure, there are ways to arrange code pathologically so nobody can understand it, eg:
http://www.codinghorror.com/blog/archives/000291.html
But assuming that everyone has made reasonable formatting choices, how do you make code easier to understand?
You switch to a better language. In other words, beauty is more than skin (formatting) deep.
“Breaking up functional units allows you to focus only on the parts you actually care about … I don’t want to wade through a hundred lines that have nothing to do with what I’m interested in.”
He DID break his code up into functional units.
Didn’t you see rule number three? “Break code into logical blocks so that each does a single thing or single kind of thing.”
Do you see any unnecessary dependencies or code blocks that extend longer than ten lines (the distance between one of his commented sections and another)?
More to the point, do you find that you actually have to wade through each subsection? Because I don’t. With his clearly commented subsections, I find it extremely easy to skip over the levels of detail I don’t want to get involved in. For me, actually, it’s MUCH easier than seperate subroutines, given that I don’t have to jump around in the file – intellisense editor or not.
Maybe you’re under the impression that the ONLY way to seperate code into logical blocks is to split them into single-use subroutines. But it’s not true. Anonymous blocks are just as effective at conveying the cohesion of function steps – if not more so.
static TARGETS *
make0sort( TARGETS *chain )
This is my biggest pet hate of some (not many) C programmers programming style!
WHY do you hit return after the data type in a function declaration?? There seems to be no reason.
Anyway rant over…concerning number 1 on the list. I agree this is a good idea, but what if the style is terrible? Am I supposed to follow the previous developer, like a lemming over a cliff?..
Re: Alyosha’s comments
“… [snip] I suggested not using /* */ …”
“Steve, meet #if 0. #if 0, meet Steve.”
Although I have certainly used #if 0, the advantage of this:
/*
…some code to comment out
*/
versus this:
#if 0
…some code to comment out
#endif
is in terms of visualization. Most of the editors I have used can easily color code the comment block. Not so much with the #if 0 … #endif block. Makes it far easier to clean up after you’re done, though again… using the technique and tools you are used to is better than using a superior tool you haven’t mastered.
In addition to commenting out code for testing, I very often use the blocks for commenting out sections of code that are works in progress or code segments that are likely to be deleted, but until I decide to do so, it’s useful to be able to restore the lines quickly.
(yes, yes… I know that you can use source code control systems for doing the same thing, but I usually try to avoid messing up the code repository with my own personal brain spewings)
"But assuming that everyone has made reasonable formatting choices, how do you make code easier to understand?
You switch to a better language. In other words, beauty is more than skin (formatting) deep."
Agreed on this point. C is way too verbose for me to consider doing serious development in it. C++, if properly used, can be a much more terse language – but more often than not, it’s misused and in practice is only marginally better than C++. C#, Java, Python, perl … all of these are wonderfully expressive languages that I enjoy writing (if not running). I think, if I got to study it long enough, I would find Erlang to be the same way.
But I think there’s a danger in swinging too far the other way on the terseness specturm. Lisp-like languages make it possible to “reprogram” your language, and make it orders of magnitude much more expressive. The practical problem is that in doing so, the rules of usage and the total vocabulary you need to have memorized in order to be fluent expand exponentially. Way out at the extreme end, APL is a great example of a totally expressive, but utterly unreadable language to anyone who hasn’t spent five years studying it.
-
Keep your Lisp bigotry to yourself.
-
If you use Lisp, you almost certainly use Emacs. If you use Emacs, you will find that it has a world-class auto-indentation system. I don’t know why anyone would be pushing “space”.
-
The line “I want to indent my code myself” is idiotic. You have MUCH better things to be doing than aligning code manually. Suck it up, accept the small problems with auto-indentation, and write code.