Over a million developers have joined DZone.

Bad, Bad Code! (And Tips to Avoid It)

DZone 's Guide to

Bad, Bad Code! (And Tips to Avoid It)

You're never too experienced for a refresher on code smells and clean code. Here are some helpful tips to make sure your code is readable for you and those who follow.

· Java Zone ·
Free Resource

Due to my work as an instructor, I get to review a lot of code written by students. Their code's quality is different, of course, and most of them write 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 identification – my goal is to learn from these examples. Sometimes, poorly written code is the best lesson you'll ever get. So let's see a marvelous example: 

a = 8;b=8;c=8;

Before you go on reading, try to figure out what is wrong with this code — what are its breaking points?

How many problems have you found?

Let's start analyzing 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 alike, 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 jas 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 feel 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 is about – any guess is as good. Things may get worse – the number "8" is written six times in the code, which means that if we want to change its value to 7, it would be very error-prone because we'll have to locate every occurrence of 8 and validate that we didn't miss any of them.

The solution is, of course, using constants. We could define a constant like:

final int EARLIEST_HOUR = 8;

And then the code would have looked like:


Now let's talk for a moment about this code's semantics: What is this code suppose to do? Now that we have a constant, it looks clearer – the "if" tests 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 spot the code's lines that would execute if the conditional is true?

Before we do that, it's good to remember that Java is blind to white spaces. That 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 put a tab before the line so it would be clear that this line belongs to the "if" statement. But here is the question again – What really belongs to the if's body? Here's the answer, the if's body is bold: 



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's supposed 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 wanted.

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:


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, is that if I can avoid bugs ahead by writing more organized and clearer code – I do it, and use 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 a one-line statement (and then 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?
java ,code smell ,clean code

Opinions expressed by DZone contributors are their own.

{{ parent.title || parent.header.title}}

{{ parent.tldr }}

{{ parent.urlSource.name }}