Bad code has lots of unique characters. In this article, we’ll look at each one and what they are. We look at more general code smells.
Too Much Information
Well-defined modules should be small and allow us to do a lot with little code. They don’t offer many functions to depend upon, so coupling is loose.
A bad module has lots of functions that we have to call to get something done, so coupling is high.
Things that are exposed to modules should be minimized. Data and implementation should be hidden except when they can’t be. We shouldn’t create lots of instance variables that are inherited by other classes.
Utility functions should be hidden from other modules. In the end, we should be able to reference a few things to do a lot to minimize coupling.
Dead code should definitely be removed from our codebase.
It’s ignored and it’s never updated to follow the latest conventions. Therefore, it should be removed from our code. Our tests should check if it works without the dead code.
Variables and functions should be closed to where they’re used so that we don’t have to do lots of scrolling to trace our code.
Functions and classes are upper camel case. Variables are camel case. Constants are upper case. Consistently named things are easier to modify.
We should remove anything that clutters our code. Useless code should be removed. Unused entities should be gone.
Everything should be well-organized, clean, and free of clutter.
Coupling should always be minimized so things that shouldn’t be coupled together stay uncoupled. The less one thing has to know about another, the better.
They serve no purpose and making changes is harder because we have to deal with all the coupling whenever we make changes. When we get something to work, we should clean up our code so that we don’t have these situations.
The methods of a class should be interested in the variables and functions of the class they are in, and not the ones from other classes.
We should reference as little code from external sources as possible.
The following example shows what feature envy is:
We have the
ShapeCalculator class that references the
Rectangle class a lot. We call its constructor and instance variables.
However, we shouldn’t do this because it’s referencing too much from the
Rectangle class. We can remove references to the instance variables as follows:
As we can see, we didn’t have to touch the internals to get the area of a rectangle. It’s much better to not reference the
width from a
Rectangle instance if we don’t have to.
This is because when we make changes to the
Rectangle class, we have to change a lot of things in the
ShapeCalculator class if we do reference these entities.
For example, when we change the names of
width to something else, then we have to change them everywhere.
Boolean arguments for selecting functionality in a function is bad. It’s hard to know what
false means when we select it. We should split a function into two if we do need a selector argument. It’s just as bad as any other argument.
Avoiding boolean arguments is one of the clear-cut criteria to separate a function into multiple functions. If we do need selectors, they should be something clearer like strings or integers.
We want our code to be as clear to our readers as possible. Therefore, naming should reveal the intent of identifiers.
x is a bad name since it tells us nothing, but
numApples is good since we know what it means. We know that it stores a number of apples.
Code should be placed where we expect it to be. For example,
PI should belong to a
Math class as a constant. We shouldn’t be clever about where to put certain functionalities. The place we put it should be intuitive to the reader.
Names of functions should tell us where to put our code. For instance,
getHoursWorked should be in the
Employee class because it belongs to an employee.
Static methods should only be used on functions that don’t operate on an instance. So,
getHoursWorked shouldn’t be a static method in the
Employee class, since the number of hours worked belongs to the employee.
Something that’s suitable for static methods is the ones that don’t care about the instance of the class it operates on.
For example, the
Math.min method should be a static method because we don’t need the
Math instance for anything.
When writing clean code, we have to think about many things. However, most of them are common sense. We should write code that is clear and expose as little to the outside as possible to reduce coupling.
Names should be clear so everyone knows what we mean. Finally, things have to placed where they make sense.