INT OR BOOLEAN
if (use_tls != 0) {
/* Code having to do with TLS */
}
So, what is "use_tls"? It's obviously somewhat of a flag - if it's non-zero then we need to do TLS stuff - but is it a count? If it's a count, then it should have been named "use_tls_cnt", or maybe just "tls_cnt". If it's being used as a boolean, then it should have tested as a boolean:
if (use_tls) {
/* Code having to do with TLS */
}
And yes, this is actual code that I've worked on. It was being used as a boolean, and should have been tested as one. Is this a big deal? No, like I said, I might not have brought it up in a review. But I am a believer that the code should communicate information to the reader. The boolean usage tells the reader more than the numeric version (and is easier on the eyes as well).
MALLOC AND FREE
state_info = OUR_MALLOC_1(sizeof state_info_t);
/* Code using state_info */
free(state_info);
Again, actual code I've worked on. We have a code macro for malloc which does some useful things. But the author couldn't think of anything useful to do with free so he didn't make a free macro. He should have. There have been at least two times (that I know of) when it would have been tremendously useful. One time we just wanted to log a message for each malloc and free because we suspected we were double-freeing. Another time we thought that a freed structure was being accessed, and we wanted free to write a pattern on the memory before freeing.
"No problem," you say, "just create that macro and name it free." Nope. There are other mallocs: OUR_MALLOC_2, OUR_MALLOC_3, etc (no they aren't actually named that). And some of the code doesn't use any of the macros, it just calls malloc directly! For that second feature (writing a pattern before freeing), you need special code in the malloc part as well as the free part. These all should have been done consistently so that OUR_MALLOC_1 only works with OUR_FREE_1, etc. That would have allowed us to do powerful things, but as it is, we couldn't.
UNNATURAL CONDITIONAL ORDER
I bet there is an official name for this:
if ('q' == cmd) break;
I've always felt that code should be written the way you would explain it to somebody. Would you say, "If q is the command, then exit the loop." No. You would say, "If the command is q, then exit the loop." I've seen this construct a lot where they put the constant in front of the comparison operator, and it is almost always awkward.
Oh, I know there's a good reason for it: it's a form of defensive programming. Consider:
if (cmd = 'q') break;
Oops, forgot an equals. But the compiler will happily assign 'q' to cmd, test it against zero, and always do the break. Swap the variable and the constant and you get a compile error.
But come on. Gcc supports the "-parentheses" option which warns the above. In fact, gcc supports a *lot* of nice optional warnings, which people should be using all the time. "-Wall" is your friend!
DECLARE CLOSE TO FIRST USE
Ok, I'm more of a recent convert here. I've written hundreds of functions like this:
void my_funct(blah blah blah)
{
int something = 0;
int something_else = 1;
.... and another 10 or 15 variables
In the REALLY old days, you *had* to declare all local variables immediately after the function's open brace. But modern C compilers let you declare variables anywhere you want. Variables should be declared and initialized close to their first use. This has two advantages: as the reader is reading the code and he encounters a variable, he can see right away what its type and initial value is without having to scroll up and back down again. But more importantly, when the user sees the variable declared, he knows that the variable is not used anywhere above that declaration. Whereas if it is declared at the top of the function, he has to examine all the function code above the first use to make sure that it isn't secretly being changed from its initial value.
POINTLESS COMMENTS
i++; /* increment i */
Oh, thank goodness somebody taught me what the "++" operator does! Note to programmer: you don't need to teach me the language.
fclose(file[i]); /* close the ith file */
Are you sure? I thought the "fclose" function *opened* a file. Note to programmer: you don't have to teach me the standard library functions. They all have man pages.
session_cleanup(client[i]); /* clean up the client session */
OK, session_cleanup() probably doesn't have a man page. But the comment doesn't contain any more information than the function and variable names!
One might add comments to say what the variable "i" is for and what the files in the "filep[]" array are for. That at least gives information that a reader might not know. But even those should not be comments, they should be descriptive variable names.
One might add comments to say what the variable "i" is for and what the files in the "filep[]" array are for. That at least gives information that a reader might not know. But even those should not be comments, they should be descriptive variable names.
Most comments I've encountered should simply be removed as unnecessary clutter that makes it hard to read the code. The best comments don't try to explain *what* the code is doing, they explain *why* it is being done. Usually that is pretty obvious, so only give the "why" explanations when the answer may not be obvious.
client_num++;
/* Usually we would close the log file *after* we clean up the
* session, in case the cleanup code wants to log an error. But
* the session_cleanup() function is legacy and is written to
* open and close the log file itself on error. */
fclose(log_file[client_num]);
session_cleanup(client[client_num]);
That's better.
PETTY PEEVE
"Really Steve? Brace indention? What are you, 12?"
(grumble) I know, I should have grown past this way back when I realized that emacs v.s. vi just doesn't matter. And really, I don't much care between the various popular styles. If writing from scratch, I put the open brace at the end of the "if", but I'm also happy to conform to almost any style.
But I just recently had the pleasure of peeking inside the OpenSSL source code and I saw something I hadn't seen for many years:
But I just recently had the pleasure of peeking inside the OpenSSL source code and I saw something I hadn't seen for many years:
if (blah)
{
code_to_handle_blah;
}
It's called "Whitesmith" and supposedly has advantages. But those advantages seem more philosophical than practical ("the alignment of the braces with the block that emphasizes the fact that the entire block is conceptually (as well as programmatically) a single compound statement"). Um ... sorry, I prefer actual utility over making a subtle point. Having the close unindented makes it easier to find the end of the block. The jargon file claims that, "Surveys have shown the Allman and Whitesmiths styles to be the most common, with about equal mind shares." Really? I've been a C programmer for over 20 years with 7 different companies, and none of those places used Whitesmith. This is only the second time I've even seen it. Equal mind share? Here's a survey which has Allman 17 times as popular as Whitesmith. I think jargon file got it wrong.
And yeah, I know that if I were immersed in it for a few months, I wouldn't even notice it any more. Just like emacs v.s. vi, it doesn't really matter, so long as a project is internally consistent. But hey, this is *my* rant, and I don't like it! So there.
