As a software developer, it’s our responsibility to not only write code that works but rather write code that’s maintainable. Martin Fowler in his book Refactoring: Improving the design of existing code defines a code smell as:
A surface indication that usually corresponds to a deeper problem in the system
Refactoring is a process of improving the internal structure of our code without impacting its external behavior. Ideally, we should refactor old code while adding new features. It will save us some time as compared to trying to do it all at once.
Fowler’s book is an excellent resource that helps us identify some common code smells and eliminate them. We should also avoid these code smells as we write code to cater to new requirements.
In this tutorial, we’ll explore a few of them.
1. Comments:
We should ideally be writing code that speaks for itself. Having a lot of comments is considered a bad practice. When we use a lot of comments, they often get out of sync over time. They sometimes also acts as a deodorant for a poorly designed system.
If we have a good design and have named our classes, methods, and variables correctly, the code will easily convey its purpose to another developer.
Some developers like to sign their name over a new class they create. Personally, I don’t promote this as tracking an author can be easily done using any version control system.
Comments can prove to be useful for some cases but let’s use it judiciously.
2. Duplicate Code:
Duplicated code is the code smell we see when we have a similar code spread across multiple places in our codebase. It is a badly structured code and we should find some way to extract the common functionality out in a separate method.
The problem with duplicated code is that if there’s a change that’s to be done, all those files will have to be modified to accommodate it. There is a possibility that we miss out updates in a few blocks of code.
Let’s try to stick to the D.R.Y. (Don’t Repeat Yourself) principle wherever possible. As per the D.R.Y principle, we should not rewrite a feature or function that’s already written.
3. Long Method:
We should avoid having long methods, it’s a bad code smell. Too long methods are hard to read and it becomes difficult to accommodate new changes to it. How long is too long is often debatable among developers. Personally, I prefer to stick to a rule of method size should not go beyond fifteen lines of code. For most of the cases, this rule works perfectly well for me.
Whenever I see myself violating this rule, I ask myself ‘Is this method doing just one thing (SRP principle)?’. If not, I then try logically splitting my method into something that makes more sense.
Though it’s sometimes fine to have a long method, the constraint being we should have enough reasons to justify it.
4. Large Class:
As expected, the next on our list is a large class code smell. Large classes are often also referred to as ‘God classes’ or ‘Blob or blackhole classes’.
We often encounter this code smell in large systems. As the system grows, some classes end up supporting a lot of functionalities added to it over a period of time. It’s a good idea to catch this code smell as early as possible. If a class becomes way too large, it’ll take a lot of time and effort to fix it later.
As per the Single-Responsibility Principle(SRP), a class must do exactly one thing and do that well. When adding some code to an existing class, let’s use our developer instinct and ask ourselves – ‘Does this class should really be supporting this functionality?’. If no, it’s better to place it elsewhere.
5. Long Parameter List:
Yet another similar code smell is long parameter lists. A method will a long parameter list can be difficult to use and increases the chance of wrong mappings due to oversight:
1 2 3 4 | public void doSomething(String name, int id, String deptCode, String regNumber) { ...} |
The solution here is to introduce parameter objects that capture a context. So, we can refine the above method as:
1 2 3 | public void doSomething(Student student) { ...} |
Here, we have achieved proper encapsulation.
6. Data Class:
A data class is a class that only contains the data members along with their getters and setters:
1 2 3 4 5 6 7 8 | public class Student { private int id; private String name; //constructor, getters and setters } |
This usually indicates that it might not be a good abstraction.
Though we create parameter objects to solve ‘Long parameter’ code smell, we should ideally design classes that do more than just storing data.
We should ask questions like: ‘Can I add some functionality to this class which is currently being handled elsewhere?’
Sometimes, we’ll realize that duplicated code smell occurs as we have handled the functionality of these data classes across several places in our codebase.
7. Divergent Class:
A divergent class code smell occurs when we realize that we have to change a class in many different ways, for many different reasons.
As we discussed earlier, classes should have only one specific purpose. If so, we have fewer reasons to make a change to a class and less variety of changes to be implemented in them.
If we find ourselves changing a class in multiple ways, then it’s a good indicator that the responsibilities of this class need to be broken up into separate classes.
8. Message Chains:
Message chain is a code smell where we are calling a method on an object and then calling another method on that returned object and so on:
1 | int id = obj.getDept().getSubDept().getHOD().getId(); |
Long message chains make our systems rigid and harder to test independently.
It usually also violates the Law of Demeter, which specifies which methods are allowed to be called for a good object-oriented design.
9. Shotgun Surgery:
Shotgun surgery is a code smell that occurs when we realize we have to touch a lot of classes to make a change for one simple requirement. When touching a lot of places in our codebase, it’s more likely to introduce bugs and break an existing implementation.
For a well-designed system, a small change will ideally require a localized change to one or two places. Although, this is pretty hard to achieve and some changes at times require a shotgun surgery no matter how well we design our code.
We can resolve the shotgun surgery code smell by moving the methods around. If a change requires us to modify methods across several classes, we should ask ourselves: ‘Should these methods be consolidated in one or two classes?’ and then let our developer instinct guide us.
10. Feature Envy:
Feature envy is a code smell that occurs when we have a method that’s more interested in the details of other classes than the class it is in.
If two or more methods are always talking to one another, chances are they must be part of the same class.
11. Inappropriate Intimacy:
When two classes depend too much on one another through two-way communication, it is an inappropriate intimacy code smell.
Having two-way communication among classes make them tightly coupled. We should at the very least factor out some methods to a separate class and aim to remove the cycle. We should design classes that are easier to understand and maintain.
12. Primitive Obsession:
As the name suggests, we sometimes rely too much on primitive types. Although we need primitives in our code, they should exist at the lowest levels of the code.
We should avoid overusing primitives and define suitable classes wherever required.
13. Speculative Generality:
At times, we over-engineer things like defining a superclass or some code that is not needed currently but we feel it might be useful someday. This code smell is known as speculative generality.
Agile Development promotes having Just In Time Design. Our designs should remain simple and should be just enough to support current functionality. User requirements often change rapidly, and so, we should introduce generalizations only when necessary. Otherwise, we may end up wasting our time on designs that end up not being ever utilized.
14. Refused Request:
A refused request code smell occurs when a subclass inherits something but doesn’t need it.
If subclasses are inheriting things they don’t use, they may not be an appropriate subclass for the superclass:
01 02 03 04 05 06 07 08 09 10 11 12 13 | public class Bird { void fly() { System.out.println("Flying!!"); }} public class Ostrich extends Bird { void fly() { throw new IllegalStateException("An ostrich can't fly"); }} |
Clearly, an Ostrich can’t fly and so this is an example of refused request code smell. We can deal with this code smell in one of the following ways:
- Either, don’t define unwanted behavior in the superclass, Or
- Create them to be separate stand-alone classes
Conclusion:
In this tutorial, we looked at a few code-smells and learned how to avoid and handle them.
This list obviously isn’t exhaustive but can prove to be a quick starter guide.
Published on Java Code Geeks with permission by Shubhra Srivastava, partner at our JCG program. See the original article here: Identifying Code Smells In Java Opinions expressed by Java Code Geeks contributors are their own. |
This is a good list. However, I would like to suggest that data objects are not necessarily bad, and may be a sign of better data management. Isolating model/dto/transfer/dao objects from rich objects is often highly preferred.
I’d add mutability, especially in input params, as a huge code smell, as well.
agreed