Slashdot Mirror
Defining Useful Coding Practices?
markmcb writes "A NASA engineer recently wrote about his disappointment that despite having well-documented coding practices, 'clever' solutions still made the code he has to maintain hard to follow. This got me thinking about the overhead spent at my own company regarding our code. We too have best practices that are documented, but most seem to focus on the basics, e.g., comments, modularity, etc. While those things are good, they don't directly ensure that quality, maintainable code is written. As the author points out, an elegant one-liner coupled with a comment from a few revisions ago makes for a good headache. I'm curious what experience others have had with this, and if you've seen manageable practices that ultimately offer a lot of value to the next programmer down the line who will have to maintain the code."
"Quality" can be both objective and subjective, and it seems this post is leaning towards the latter in terms of what it's getting at... while I believe in having formalized guidelines of some kind, I've found the best way to seek out and improve the elements that can't be easily quantified is through (drum roll) code reviews/walkthroughs. It seems like these are rare, at least where I'm at, and it's hard to get buy-in when you're talking about contractors charging by the hour, but in my opinion a single quality code review can save TONS of time down the road and is necessary for projects over a certain size. Also, read "The Pragmatic Programmer" and "Code Complete" for some of the best guidance on this topic.
Let us change our traditional attitude to the construction of programs: Instead of imagining that our main task is to instruct a computer what to do, let us concentrate rather on explaining to human beings what we want a computer to do.
The practitioner of literate programming can be regarded as an essayist, whose main concern is with exposition and excellence of style.
The language which provides the capability to achieve all these goals is called COBOL. Ever heard of it?
Traditional everyone set aside time and review checked in code is hard to do, difficult, and time consuming.
On the other hand, automated code review tools are life changing. There's a bunch of tools out there, but the one I think is far and away the best is Google's Gerrit tool (http://code.google.com/p/gerrit/), which is what Google uses publicly for Android.
I cannot understate how helpful Gerrit has been in this regard. So many things that are trouble down the road are easily caught by even just one other pair of eyes. Everyone who has used Gerrit at my compnay has fallen in love with automated code reviews. It's refreshing, leads to better code, etc. I seriously could gush about Gerrit for pages.
Unless of course they meant a one-liner coupled with a comment from the previous implementation, ie, the clever programmer failed to cleverly update the comment.
The author of TFA seems somewhat confused and inexperienced.
Find free books.
That doesn't seem to be the case here; s is a "source", s->ss is the source's list of "sub-sources". So the s and ss names are just accurate abbreviations, not an example of your "x" and "xx" phenomenon. The "next" pointer for a sub-source itself being named "ss" is a little strange though.
C is a compiled language, so using long variable names has no effect on the amount of memory used at run-time
It does if you're embedding symbolic debugging information in that same binary - and running it on a system where the entire executable is loaded into memory at start, rather than paging in sections as needed.
I've done that myself. Any decent C compiler should issue at least a warning.
Take GCC, for instance:
$ cat > warning.c
int main(int argc, char *argv[])
{
unsigned int i = 5;
return argc == i;
}
$ gcc -W warning.c
warning.c: In function 'main':
warning.c:4: warning: comparison between signed and unsigned
$
If you're not requesting warnings from your compiler, then you're a fucking moron. It doesn't matter which compiler you're using.
Tom Hudson, if you're truly getting caught by that sort of a problem, then you clearly have no idea what you're doing, and I sure hope that you don't pass yourself off as a programmer of any type.
No. The language requires that Null is always zero. It also requires zero to always evaluate to false and anything non-zero to always evaluate to true.
for(ss = s->ss; ss; ss = ss->ss);
The semicolon at the end means the loop is only "processing" in the middle statement. Testing to see if ss is NULL does not processing make. Either he took liberties and left out the loop payload, or he still missed something and still doesn't understand it. It is true that in an embedded system, merely reading an address can have side effects, for example reading a register can cause hardware to perform actions, etc., and maybe that is what is what is going on here, but I doubt that is the case here since the addresses would have to be pointers to other registers and registers themselves at the same time (though it is still conceivable.)
I believe he meant to write: for(ss = s->ss; ss; ss = ss->ss) { [some kind of processing here] };, but that is not what was written, which is ironic in an article complaining about unclear writing of any kind.
I agree that the variable names aren't good for code written today, but C was written in the late 1960s when core (i.e. memory of the day) was expensive, and NASA has been around that long of course, so maybe at the time the code was written it was good code (and yes I know the size of the executable won't be bigger, but the size of the source will be, especially if s and ss are used throughout a large program, and source code lives in memory when being compiled too.)
He certainly shouldn't have needed more than a few seconds to figure out that the loop traverses a linked list.
Guns don't kill people; Physics kills people! - John Lithgow as Dick Solomon on Third Rock From The Sun
When the nodes contain multiple next pointers as they are stored in multiple lists/graphs so you need to know what list next is being traversed. So a simple 'next' is no good. Or m_pNext or anything
Regardless it's really obvious what the code does - it sets ss to a null pointer. Even without that semi-colon at the end I spent a few minutes scratching my head, thinking I was missing something because the code was so clear in it's purpose.
Actually, the short variable names make it really clear to spot what everything is doing in that context simply because they are so short, mentally digesting the statement is trivial.
TFA says that the list in question refers to source (s) and sub-source (ss)
> Wrong. You can't assume NULL=0 and have portable code, although it will most probably work.
You can if it's ANSI C. The standard dictates:
6.3.2.3 Pointers
3) "An integer constant expression with the value 0, or such an expression cast to type
void *, is called a null pointer constant. If a null pointer constant is converted to a
pointer type, the resulting pointer, called a null pointer, is guaranteed to compare unequal
to a pointer to any object or function."
If p is a pointer type, "p != 0" should always work equivalent to a null pointer.