[taspeotis]

> Every line is documented C

Every line? Oh dear [1]:

    /* Set verbose flag */
    verbose++;
    ...
    /* Set advanced flag */
    advanced = 1;
    ...
    /* Set expansion level */
    expansion = atoi(argv[++i]);
    ...
    /* Initialize game */
    init_game(&my_game);

I assume the verbose flag is incremented because there's multiple levels of verbosity, whereas the advanced flag is a boolean, and i is incremented because expansion is not a command line flag (rather, an option). But I wouldn't know that from the comments! Especially useful to know if like, verbosity actually has multiple levels, like -v and -v -v or -v -v -v and so on ... instead of knowing that init_game is ... init'ing the game.

Compare all those comments to this [2], which is actually a spectacularly useful comment.

Signal to noise ratio!

[1] https://github.com/bnordli/rftg/blob/master/src/learner.c

[2] https://github.com/bnordli/rftg/blob/master/src/replay.c#L94

[kstrauser]

Yikes, I've gotta agree:

    /* Exit */
    exit(1);

Well, yes, I see that. It might have been helpful to extend that to "Exit with an error status", perhaps, but as-is it tells you literally nothing that the code doesn't.

[teknopaul]

IMHO Coding guides should ban "SBO comments" Statin the Bleedin Obvious. Let devs decide what SBO is, usually its bleedin obvious.

[dror]

Actually, this line:

/* Set expansion level */

    expansion = atoi(argv[++i]);

Is an example of ugly code. They're doing multiple things in one line, and in a confusing order.

# Increment i by 1.

# Grab the command line argument in position i

# Convert from string to integer.

# Assign to the variable expansion.

Note that there's no error checking to handle strings that don't convert to ints.

Arguably, any serious C developer can read this on the fly, but I seriously object to code with side-effects like the above ++i. It's often confusing to read and change.

I'm not criticizing the original author, more nit-picking on this as an example of great code.

[GuB-42]

I don't have a problem with that code. It is idiomatic C code, like *p++ or flag &= ~mask. ++i is a bit less common than i++ but nothing unusual. The lack of of error checking is more concerning though. It can be justified though, and it brings me to the next point.

The comment is the worst part. It is useless, it literally repeats the most obvious part of the code. A good comment would be something like "there is no error checking because arguments are already validated in the GUI, also 0 is an acceptable default".

For me, comments should not repeat what the code is doing. Comments are for expressing what is in the the mind of the developer that cannot be expressed in code. Example : "constant found using empirical testing", "Hermite spline formula" or "required on Windows". (Edit: the Hermite spline example is actually a bad one, a better idea would be to write a function called hermite_spline_formula instead)

If some coding rule require you to comment, find something to say about your code that isn't your code. It will also help you think more about what you just wrote.

[mannykannot]

> The lack of of error checking is more concerning though. It can be justified though...

How so, in this case? It could lead to undefined behavior.

> The comment is the worst part.

Useless comments may be irritating, but, as a bad practice, they are not comparable to skipping checks for possible errors and creating the possibility for undefined behavior.

[GuB-42]

It is not undefined behavior. atoi() returns 0 if no valid conversion could be found. Assuming argv[++i] is not out of bounds of course.

Useless comments are bad in part because they are sometimes not updated as the code changes, and no code analyzer will catch that. There is a reason some people say that comments are lies.

Most common example of what I've seen:

  // set foo to x
  foo = x;
  // set foo to x
  bar = x;

[mannykannot]

> Assuming argv[++i] is not out of bounds of course.

Well, you cannot assume that - look at the code (there are links to it) - there are several different ways in which it will go out of bounds.

What's more, even if it were the case that the worst that could happen is that the variable gets set to zero and the program terminates gracefully without doing anything, that would not amount to a justification for not checking for an error, it would merely be an observation that, fortuitously, this particular bug is relatively harmless, perhaps doing nothing worse than leaving the user unnecessarily puzzled over what went wrong.

Even the example you give here (which is not actually to be found in the code in question, and so could not possibly be a candidate for the worst thing about it) is relatively harmless compared to potentially causing undefined behavior, or (what is sometimes at least as bad) defined behavior that apparently works but which gives subtly wrong answers. And, by the way, code can lie too: identifiers can make claims about the semantics of a variable or function that are not always true. The only way to avoid that is to use meaningless identifiers.

The more you try to brush this off and argue that comments are worse than actual errors, the more obvious it becomes that your priorities are askew here.

[GuB-42]

Looking at the source, yeah, you are totally right. I was just looking at the lines out of context.

I was just thinking of cases where atoi(argv[++i]) was justified. Turns out I was overthinking it, it is just terrible code. No feedback on error, crash when the last argument is missing a parameter, static array with no bounds checking, a potentially leaky strdup. Comments are the least of the problems. It is a code smell though. A well justified one in that case.

[prasanthmj]

It seems to follow the notion that more comments = better code. If the symbol names are explanatory enough, the code itself will be self-explanatory. Then all that must be commented about will be special cases handled/not handled and why a certain decision is taken about handling in a certain way.

Often, "every single line is commented" results from blindly following a certain "Standard" that demands that. Eventually generating meaningless comments all over the code. Then the important points never gets mentioned in the comments or just gets buried in the noise.