Bad, Bad Code!
Due to my work as an instructor, I get to review a lot of code lines written by students. The code's quality is differ of course, and most of them writes just fine, but sometimes I run into piece of code that is so erroneous and buggy that it's literally unforgettable. I'm not trying to make fun of someone, hence all examples will be anonymous and without any identifications – my goal is to learn from these examples. Sometimes, a poorly written code is the best lesson you'll ever get. So let's see a marvellous example:
if(a<8&&b<8&&c<8)
a = 8;b=8;c=8;
Before you go on reading, try to figure out what is wrong in this code, what are its breaking points.
How much problems have you found?
Let's start analysing it. The first thing that catches the eye is the density of the 'if' clause. The conditionals (there are three of them) are written without any spaces. Since the & operator is used right near the 8 and they both look quite a like, this line becomes almost unreadable. So here is problem number 1 – spacing. You better add spaces to your code. Spaces between statements, before and after operators and so on. I would've written this line with spaces like that:
if(a < 8 && b < 8 && c < 8)
And I think it already looks a little better.
Problem number 2 – anyone who has ever written even a few lines of code felt the harm of "magic numbers". Magic numbers are numbers that appear in the code, and since they are just numbers, they are not self-descriptive and actually meaningless to the reader. They are also very hard to change if needed. If you didn't felt at least uneasy with the number "8" that appears in the code, you probably haven't written enough code in your life . You are welcome to guess the meaning of the number. Since I was the one who gave this exercise to the students, I know what "8" means (in this case it's the earliest possible hour for worker shift in a factory), but for those who doesn't know what is this code about – any guess is as good. The things may get worse – the number "8" is written 6 times in the code, which means that if we want some day to change its value to 7 it would be very error-prone mission, because we'll have to locate every occurrence of 8, and validate we didn't miss any of them.
The solution is of course using constants. We could define a constant like that –
final int EARLIEST_HOUR = 8;
And then the code would have looked like that:
if(a < EARLIEST_HOUR && b < EARLIEST_HOUR && c <
EARLIEST_HOUR)
a = EARLIEST_HOUR; b = EARLIEST_HOUR; c = EARLIEST_HOUR;
Now let's talk for a moment about this code's semantics: What does this code suppose to do? Now that we have a constant it looks clearer – the "if" test three variables – a, b and c. If the value in all three of them is smaller than the earliest hour, they all get the earliest hour value assigned to them. This kind of testing is very common and should prevent illegal input situations. But look at the if's body. Can you circle the code's lines that would execute if the conditional is true?
Before we circle, it's good to remember that Java is blind to white spaces. It means that as far as the compiler is concerned, spaces are almost always optional and not mandatory (you must put at least one space between two words but not more than that). Which also means that the statements in Java are terminated when the compiler sees the ';' sign, and not by the end of the line. Now let's get back to the code beneath the conditional. It's written in one line. This student even made here a good thing and put a tab before the line so it would be clear that this line belongs the "if" statement. But here is the question again – What does really belong to the if's body? Here's the answer, the if's body is bold:
if(a < EARLIEST_HOUR && b < EARLIEST_HOUR && c < EARLIEST_HOUR)
a = EARLIEST_HOUR; b = EARLIEST_HOUR; c = EARLIEST_HOUR;
That's all. Only the assignment to "a" is the conditional's body. What about the rest of the statements? They have nothing to do with the "if", which means they would executed whether the conditional is true or false. If the student meant that when the conditional is true, all three assignments would be executed, he got a severe logical error – the code doesn't do what it supposes to do, but it's very hard to find it.
So, let's add two more problems to the previous two –
Problem number 3 – only one assignment would be executed in the conditionals, which is in contrary to what the programmer has meant.
Problem number 4 – Writing more than one statement in a line is really a bad habit. It's not an error – Java allows it – but it makes you see things that aren't there.
This is how the code should have looked like:
if(a < EARLIEST_HOUR && b < EARLIEST_HOUR && c < EARLIEST_HOUR) {
a = EARLIEST_HOUR;
b = EARLIEST_HOUR;
c = EARLIEST_HOUR;
}
In the bottom line – this piece of code is really bad because it contains a lot of problems that may fail the programmer and exposes him to errors and bugs. My recommendation, which I strongly believe from my earliest days as a programmer – if I can avoid bugs ahead by writing more organized and clearer code – I do it, and the practical conclusion – spacing, constants, indentation and so on.
Here are some follow up questions for those who are interested:
In the last code sample I've shown the conditional's body contains three statements. How could we write the three assignments and still stay in one-line statement (and then to avoid the curly brackets)?
I've talked about "8" as an unexplained "magic number" but I haven't said a word about the variables' names – a, b, c. What do you think? Are those names wrong?
What do you think will happen if only one of the variables – a, b or c – is smaller than 8?
