Bad code, Readable code
Bad code. Words that ring in our developer ears every time we hear it.
We all know about it. You have written it. I have written it. We have all written it.
What is it though? How do you know whether or not the code you write is "good" or "bad"? Is it simply code that your scrum master would consider engineering excellence? Perhaps it is just bug free code, or code written in the fastest amount of time. Surely the company that employ you would value spending the least amount of time on a "task" the most, so that would obviously be the "best code", right?
No, I would go so far to claim- nay. I would go so far to state that bad code is code that is difficult to read and understand.
"You must be crazy! Surely code that is stable/bug free is far more important!"
Aha! You would be wrong! -that is, unless the stable/bug free code is code that never ever see the light of day again by another human being.
Then I would give you a gold star and commend you for your work
Realistically speaking we will more than likely be coming back to the code at some point in time. Perhaps not by you, but by another developer. Having then to read and understand the code all over again.
Every time you are spending more than a few seconds, minutes or, Thanos forbid; hours trying to read and understand a method, you are witnessing bad code in action. It is simply not written with readability in mind.
Less talk, more walk
Let us look at a very small and heavily simplified code example, and I will do my best to narrate the thought processes we all have had at some point.
Keep in mind that this is just one example that barely scratches the surface of the substance found in "bad code". There is never just one way of writing bad code, as there is never just one way of writing good code.
I would encourage you to note down the good and the bad in this code snippet before continuing, so that we may share notes
class Main {
public static void main(String[] args) {
NumberUtil numberUtil = new NumberUtil();
double r = 10;
double h = 2;
double v = numberUtil.getVolume(r, h);
double sa = numberUtil.getSurfaceArea(r, h);
System.out.println(v);
System.out.println(sa);
}
}
class NumberUtil {
public double getVolume(double r, double h) {
return Math.PI * r * r * h;
}
public double getSurfaceArea(double r, double h) {
return 2.0 * Math.PI * r * (r + h);
}
public double getSquared(double n) {
return n * n;
}
}
Is this the bad code, or is this the good code?
We have a Main class that contains the famous public static void main (psvm) method.
It uses a NumberUtil in order to call two methods; getVolume and getSurfaceArea.
NumberUtil also have a third method that does not appear to be in use; getSquared.
We are setting two variables; r and h, which are used as parameters in the two methods used. Knowing the method names, we can with a high degree of certainty be sure that r is radius. Perhaps it could be radian, if it is an incomplete shape, but it is most likely the radius. h must be height then. Cannot calculate volume without an height, right?
Looking in the getVolume method, we see that we are using PI * r * r * h, so we are probably working with some sort of circular shape doodad. Same variables are used in getSurfaceArea too.
We are also setting sa and v. Knowing the method names used to set them, we can be sure that sa means Surface Area, and v means Volume.
Then we print out those variables at the end. We use the r and h, to get sa and v in order to print them out. Wonderfully easy.
What is wrong?
Well, technically nothing was wrong with this code. It works. There is no way to produce bugs in this code without changing or moving things around.
It is the perfect code.
I lied. There are many elements of "wrong" in this code. The "wrong" is that this code is "bad code".
Bad code is wrong. Don't do it.
Let us look at the variables again. We have r, h, v, sa.
There! You just did it again!
In order to identify what r, h, v, and sa is, you had to form a mental Map<String, String> in order to identify what they are. A mental map.
r = radius,
h = height,
v = volume,
sa = sand area. Or was it surface area? Woops.
In your mind you now have an unnecessary Map<String, String> in memory. It is taking attention away from what is really important. We dont want to keep mental map of the variables being read throughout the code. It is extra work.
No one wants to scroll through the code, only to find abbrevations, then scrolling up, finding out what it is, and then scroll down again.
"But using abbrevations shortens the time I use to type!"
Nu-uh. Anyone that is not you will guaranteed spend more time figuring out what each and every abbrevation stands for; at least once each. Even you, when taking longer breaks from the code, will have to rebuild your mental Map<String, String> when coming back to the code.
Imagine if this example was longer, more complex, where the variables might have been used in several places! An unnecessary use of your finger strength. Save it for the gym instead.
No, the obvious solution to this is to refactor and rename the variable names so they become readable.
class Main {
public static void main(String[] args) {
NumberUtil numberUtil = new NumberUtil();
double radius = 10;
double height = 2;
double volume = numberUtil.getVolume(radius, height);
double surfaceArea = numberUtil.getSurfaceArea(radius, height);
System.out.println(volume);
System.out.println(surfaceArea);
}
}
class NumberUtil {
public double getVolume(double radius, double height) {
return Math.PI * radius * radius * height;
}
public double getSurfaceArea(double radius, double height) {
return 2.0 * Math.PI * radius * (radius + height);
}
public double getSquared(double n) {
return n * n;
}
}
Ahh, our minds can now rest and be assured that volume is volume, surfaceArea is sandArea- I mean surfaceArea; and that the remaining variables are what they are supposed to be. No need to drag the entire world of context into the picture.
If I were to ask you; what exactly, is the shape of the "volume" we are calculating?
Let us rename it.
Let us see, NumberUtil has three methods, getVolume, getSurfaceArea and getSquared... hold on. What does getSquared has to do with the other methods?
(Spoiler: Nothing)
What we have here, is a class with an unuseful and generic name. Let us fix that- and while we are at it, let us clean the methods a bit.
class Main {
public static void main(String[] args) {
double radius = 10;
double height = 2;
Cylinder cylinder = new Cylinder(radius, height);
double volume = cylinder.getVolume();
double surfaceArea = cylinder.getSurfaceArea();
System.out.println(volume);
System.out.println(surfaceArea);
}
}
class Cylinder {
private double radius;
private double height;
public Cylinder(double radius, double height) {
this.radius = radius;
this.height = height;
}
public double getVolume() {
return Math.PI * radius * radius * height;
}
public double getSurfaceArea() {
return 2.0 * Math.PI * radius * (radius + height);
}
}
We were working with a cylinder all along!
Now there is no doubt what this is. No prior math experience needed. No need to read into the function in order to figure out the equation. We have Cylinder that takes in a radius and height in order to be constructed, and it will return the information you need when asked.
Sweet.
What else could possibly be wrong here? Its readable, right?
Well, it could be better. We have several lines of code that does something when put together. We have created a Cylinder so that we may fetch the Volume and Surface Area from it, for the purpose of printing it out.
"Tell, do not ask": Fameous words that can be read throughout several Uncle Bob books and videos.
Why go through the trouble of doing the work ourselves, when we can simply tell something what to do!
class Main {
public static void main(String[] args) {
double radius = 10;
double height = 2;
Cylinder cylinder = new Cylinder(radius, height);
cylinder.printVolume();
cylinder.printSurfaceArea();
}
}
class Cylinder {
private double radius;
private double height;
public Cylinder(double radius, double height) {
this.radius = radius;
this.height = height;
}
public double getVolume() {
return Math.PI * radius * radius * height;
}
public double getSurfaceArea() {
return 2.0 * Math.PI * radius * (radius + height);
}
public void printVolume() {
System.out.println(getVolume());
}
public void printSurfaceArea() {
System.out.println(getSurfaceArea());
}
}
Now it is clear as reading text on paper what is going on.
We have a cylinder with radius 10, and height 2.
We are then printing out the volume and surface area.
All without needing to look at how we do it.
Tell, do not ask.
Comparison
Let us compare the "before" and the "after".
class Main {
public static void main(String[] args) {
NumberUtil numberUtil = new NumberUtil();
double r = 10;
double h = 2;
double v = numberUtil.getVolume(r, h);
double sa = numberUtil.getSurfaceArea(r, h);
System.out.println(v);
System.out.println(sa);
}
}
class NumberUtil {
public double getVolume(double r, double h) {
return Math.PI * r * r * h;
}
public double getSurfaceArea(double r, double h) {
return 2.0 * Math.PI * r * (r + h);
}
public double getSquared(double n) {
return n * n;
}
}
class Main {
public static void main(String[] args) {
double radius = 10;
double height = 2;
Cylinder cylinder = new Cylinder(radius, height);
cylinder.printVolume();
cylinder.printSurfaceArea();
}
}
class Cylinder {
private double radius;
private double height;
public Cylinder(double radius, double height) {
this.radius = radius;
this.height = height;
}
public double getVolume() {
return Math.PI * radius * radius * height;
}
public double getSurfaceArea() {
return 2.0 * Math.PI * radius * (radius + height);
}
public void printVolume() {
System.out.println(getVolume());
}
public void printSurfaceArea() {
System.out.println(getSurfaceArea());
}
}
In a more complex software, I would most likely have opted for a wrapper class in order to provide additional functionality to the Cylinder - instead of adding print methods directly on it.
Oh, where did the getSquared method go? I threw it away. Because it was a SPAMM (Stupid Pointless and Meaningless Method). Guess what, if your coworker will need that single method days from now, weeks from now, or years, s/he can go fish in the git commit logs.
Key Points
The key points to take away from this is;
- Do not use abbreviations
- Separate responsibilities
- Tell, do not ask
- Get rid of SPAMM, mercilessly
- Avoid generic naming of classes and methods
- Keep learning, and share what you learn
Recommended Resources
All of these are highly recommended if you wish to dive deeper into the rabbit hole, to uncover ways of handling bad code, as well as to learn and practice writing better code.
Books
- Clean Code: A Handbook of Agile Software Craftsmanship, by Robert C. Martin (Uncle Bob)
- Working Effectively with Legacy Code, by Michael Feathers
- The Pragmatic Programmer: From Journeyman to Master, by Andrew Hunt & David Thomas