Secret Stylecheck Tests

Hello,

so I read this ominous post:

Now on one hand I would like to get full points for this but on the other hand I have no interest in watching all C lectures again at 2x speed in order to find everything we deigned to be “good quality code” in the lecture (especially if it happens in an off-hand remark to an unrelated topic).

I can gleam two things from this answer:

  1. Putting function bodies in headers is considered bad style.
  2. It is a good idea to format all .c and .h files I touched with clangd.*

So what else should we look out for? I am not asking for the program (or its parameters) that will check our style in the eval tests but I think it would be fair to say what we should look at.

* To do this go into one of your source files, right click into the text field, choose Format Document With... and from the selection that appears choose the option clangd.

The project description explicitly states:

It is important to keep the code readable and easy to understand. This includes writing comments, formatting the
code, using functions instead of code duplication, and using indentation. In this project, such properties are required
and contributed with one point to the overall score.

Yes, this follows by the concept of a header file.

I neither confirm nor deny that any of the following things are tested nor am I stating that this list enumerates all good coding practices:

  • short comments at sensible locations stating the high-level intent of your code
  • sensible variable names
  • no code duplication
  • no redundant patterns like if(a) return true; else return false;
  • no intricate control flow without the need for it
  • clear control flow instead of goto and labels
  • indentation for readability
  • local variables where it is suited to avoid duplication or complex expressions
  • avoid weird pointer arithmetic (arrays and similar is okay but one can do crazy things)
  • no definition of new types inline in functions
  • no inline assembler
  • no if splitting
  • unneeded magic numbers
  • very large and complicated functions
  • excessively long identifiers

It is in your own interest to write good quality code for multiple reasons:

  • it is less error-prone
  • you will be able to read it even if you come back after a weekend not working on it
  • if you write bad code no one will hire you
  • it makes teamwork easier later on
  • it is easier to change and refactor it

You may say that some of these points (hiring, teamwork) are not important to you right now. But if you directly learn how to code probably it is easier to keep that habit. Our goal is not to teach you only to write programs that work somehow.

If you manage to code the logic, you should also be able to estimate whether your code is good or bad.
If you satisfy the public tests, avoid code duplication, and code with common sense (avoid some of the bad points from above), you will be fine.

You could also enable “Format on Save” using the enable_autoformat.sh script or in the settings of VSCode.

1 Like

Thanks for this elaborate reply. I understand why good coding practices are important but they are still subjective and hence need to be spelled out. For example the google style reference you linked some time ago also discourages use of Exceptions (don’t remember if it was for Java or C++) but most people would disagree. Hence my question above, “good” style varies on who you are talking too.

I have two questions on this list:

  1. Is goto forbidden under any circumstance? I understand that excessive use of goto can make code completely unreadable, but I use a goto statement to continue out of a nested loop in my guess function. I consider this to be good style because otherwise I would have to use a bool to manage this which would certainly make the code less readable. See the example at the end for elaboration of this idea.
  2. What is “if splitting”?

Here is the code for the first point without a goto:

do {
    for ( ... ) {
        if ( ... ) {
            bool f = true;
            break; // this breaks us out of the for loop
        }
    }
    if (f) {
        continue; // now we finally get to go back to the start
    }
} while (true);

and here is the very elegant way of dealing with this using goto:

loop:
    do {
        for ( ... ) {
            if ( ... ) {
                goto loop; // this goes directly to the next round
            }
        }
    } while (true);

Also a lot of those points you mentioned can not be accurately “measured” by a computer, so I assume our code is going to be read by actual people?

There are some good use cases of goto.
For instance, a multi-break as you mentioned.
In such a case, a goto is okay.
But one should not replace a while with two goto or jump between functions.

With this point, I meant confusing conditional patterns like

if(cond){
  A
}
if(cond){
  B
}
if(cond){
  A
}
if(!cond){
  B
}
if(!cond){
  A
} else {
  B
}
if(cond){} else {
  B
}


Tangent to exceptions:

The guide was for C++.
It states:

On their face, the benefits of using exceptions outweigh the costs, especially in new projects.

There are some restrictions however when it discourages exceptions. For instance in old code.
They advocate against using exceptions when writing code for google or to interface with google.

Our advice against using exceptions is not predicated on philosophical or moral grounds, but on practical ones.

Things would probably be different if we had to do it all over again from scratch.

That said, there are some cases when exceptions are commonly frowned upon.
For instance, when iterating over an array and accessing element i-1, one could check for the case i=0 and handle it explicitly.
One could also let Java throw an index out of bounds exception, catch it and handle the case this way.
The second method is more verbose, harder to read, and contradicts the meaning of an exception as something that should not happen (because we expect it to happen here).

Some of the measures are easy to check, some need sophisticated algorithms, some of them are automatically checkable in most cases and need review for a few edge cases, but some would need manual review.
That said there are a few hundred students in this course.
Therefore, we will not review all code manually.

1 Like

Thanks for elaborating. Let me get the if stuff straight:

if(cond){
  A
}
if(cond){
  B \\ This is dead code and will never be executed
}
if(cond){
  A
}
if(!cond){
  B // This will be executed but using an else clause would be more elegant
}
if(!cond){ // Ok, I guess you are supposed to switch A and B... not a big issue
  A
} else {
  B
}
if(cond){} else { // Now it would be better to check !cond and not have an else at all
  B
}

What about chained if clauses?

if (cond_1) {
    doSomething_1();
}
...
if (cond_n) {
    doSomething_n();
}

This goes under the assumptions that the cond_i are “independent”/“unrelated” in a sense instead of a “partitioning” of true.

Hope I am not being too annoying :smile:

B is not dead code. But the two ifs could be combined into one.
Nevertheless, the programmer wrote two ifs leading the reader to wonder:

  • is the condition really the same here or was there a mistake
  • maybe B could be dead code
  • does cond have side effects leading to weird behavior like different values in consecutive executions
  • why is there code duplication of the condition => mistakes if the condition should be changed
if(cond){
  A;
  B
}

Your explanation for the other three is correct.

Chained If clauses are usually not bad code.
In some cases, they might even be considered the most elegant solution.
For instance consider a simple login function:

if(username exists) {
  if(password correct) {
    if(has access) {
      grant access
    } else {
      return error("not permitted");
    }
  } else {
    return error("invalid password");
  }
} else {
  return error("invalid user");
}

is quite verbose with all the nesting.

if(! username exists) {
  return error("invalid user");
}
if(!password correct) {
  return error("invalid password");
}
if(!has access) {
  return error("not permitted");
}
grant access

is easier to read.

One exception is the case (where the conditions are not independent)

if(x==A) {
   ...
}
if(x==B) {
   ...
}
if(x==C) {
   ...
}
...

where a switch should be used.

Not at all :slight_smile: .