There are only two types of issues in the codebase.
Some issues, like bugs, are so bad that you should remove them immediately. They cause the application to behave incorrectly and usually lead to a loss in time or money for end-users.
The other type of issue is less subtle. These issues are called code smells.
A code smell is a metaphoric term for a pattern in the application code that indicates a likely problem. It could be a symptom of a bad design or a sign of an impending problem. A typical example of a code smell is a duplicated code, a long method, or a long class.
Like the kitchen, the code is full of nasty odors that don’t smell nice.
Leave them long enough, and they become permanent problems. That’s why it’s essential to have a periodic cleaning routine to rid your code of those smells.
This article will explain the 21 most common code smells. For each code smell, you will see the problem, how to identify a code smell, and how to fix it.
What are the code smells?
We’re all familiar with the word “smell”: a smell can be either good or bad. For example, the smell of coffee means “good”. The smell of rotten eggs means “bad”. So, when it comes to code, what does it mean when it “smells” bad?
The term “code smell” was coined by Kent Beck and popularized by the Refactoring book of Martin Fowler.
Code smell can be defined as an indication that something about a code section is not right.
That doesn’t necessarily mean the code has bugs. It just means that the code is not in the best shape, and it could be improved to improve its code quality, readability, and maintainability.
Code smells are signs in your code that you should investigate once you detect them.
Based on the result of the investigation after the detection, there are three outcomes:
- No action – you detect a code smell, but you decide to do nothing about it. This can happen when you see a code smell but ignore it for the time being.
- Discard the code smell – There are times when you see a code smell, but you decide to discard it. There are two possible reasons for that. The first one is that it is impossible to refactor the existing code. Maybe the code is in a really bad place, and you would need to add more tests around it first before you refactor it. Or you are not currently working on that area in the code. The second reason for discarding the detected code smell is if the detection was wrong. For example, you see a really large method, but upon further inspection, you realize that it doesn’t make much sense to split it into smaller methods. As the statements in the existing method are highly coupled and belong together.
- Refactor the code and remove the code smell – If you have the time and skills to refactor the code smell and improve the code, you should do it. Throughout this article, I will suggest refactoring for every type of code smell you can stumble upon in the code.
Software developers often don’t like to talk about code smells. But they are present in almost every codebase. And you should periodically check and fix them.
Ok, enough for the introduction. Let’s see the usual suspects!
Duplicated code
Problem: Duplicate code is a bad code smell. It is one of the most common smells in code because it is almost human nature to copy code. You find the solution you need in the existing code, paste it into the new location, and change a few lines. And done. Problem solved.
The lack of understanding of duplicated code’s purpose can lead to a deeper problem appearing in the application. There are many reasons why duplicate code is bad. First, when you have duplicate code, you risk introducing the same bug to the new place. And when you have duplicate code, you are never really sure that you have found all the bugs in one place.
Next, if you need to change some code to add a new feature, but you have the same code elsewhere, you also need to remember to go to all those other places and see if you need to make the change there. And there are chances that you won’t remember where are those other places in your code. Having duplicate code in your codebase does not help the application’s maintainability.
Solution: Use Extract Method refactoring to move the code to a new method, which you can call from different places. Other solutions include Pull Up Method, the Form Template Method and Substitute Algorithm refactorings.
Long method
Problem: Long method is a method that contains too many lines of code.
How many are too many?
It is hard to specify a specific number, but if a method has more than 50 lines, it is more likely that it is too long.
But the point is not in the number of lines. It’s in the work it does.
If the method does more than one thing, you should consider changing it because smaller methods are easier to understand. The best solution is to move some logic into a separate method.
For example, take a look at the following method:
//list of available smartphone results
List<string> smartphones = new List<string>()
{
"Samsung Galaxy S20",
"Pixel 2",
"Pixel 3",
"Pixel 4",
"iPhone XR",
"iPhone 12",
"iPhone 12 Pro",
"iPhone 12 Pro Max"
};
//long method that we will refactor
public void PerformSearch()
{
bool continueSearch = true;
while (continueSearch)
{
//user enters the term
Console.Write("Search for smartphone: ");
string keyword = Console.ReadLine();
var results = smartphones
.Where(phone => phone
.ToLower()
.Contains(keyword.ToLower()));
//if there are resuls, they are displayed in the output
if (results != null)
{
Console.WriteLine("Here are the matched results.\n");
foreach (var result in results)
{
Console.WriteLine(result);
}
}
else
{
Console.WriteLine("No results found.");
}
//this asks user if he wants to search again
//valid responses are Y and N
//the program stops if the answer is N
string continueSearchResponse;
do
{
Console.Write("\nMake another search (y/n)?: ");
continueSearchResponse = Console.ReadLine();
if (continueSearchResponse.ToLower() == "n")
{
continueSearch = false;
break;
}
if (continueSearchResponse.ToLower() != "y")
{
Console.WriteLine("Invalid response.");
}
} while (continueSearchResponse.ToLower() != "n"
&& continueSearchResponse.ToLower() != "y");
}
Console.Write("Thanks for searching!");
}
It’s tough to understand what it does without spending some time going through the code.
Take a look at the same method but improved using the Extract Method refactoring:
public void PerformSearch()
{
bool continueSearch = true;
while (continueSearch)
{
SearchForSmartphones();
continueSearch = ShouldContinueWithSearch(continueSearch);
}
Console.Write("Thanks for searching!");
}
It is simpler and easier to understand. If you need to know about the specifics of the code, you would then navigate to the SearchForSmartphones and ShouldContinueWithSearch methods.
Long methods are a code smell that you need to refactor. Long methods can make adding new features or modifying existing ones hard. Before you attempt to add a new feature, first, you need to spend some time familiarising yourself with the method. And if the method is long, you need to spend more time investigating what the method does.
Long methods are also more difficult to understand, challenging to test, and harder to reuse.
Solution: Use Extract Method or Introduce Method Object refactoring.
Large class
Problem: Large class is a term used by software engineers to describe classes with many lines of code. These classes have been used for a long time. Some developers call this type of class a “god class.” This class does many things and has thousands of lines of code.
For example, take a look at the following class declaration:
public class TeacherHomeController : Controller
{
public ActionResult Index() { }
public ActionResult QuizList(int? pageId, int? course_id, int? category_id, string title) { }
public ActionResult QuizDetail(int? quiz_id) { }
public ActionResult StudentResultList(int? pageId, int? quiz_id) { }
public ActionResult EditQuizDetail(quiz FormData) { }
public ActionResult AddQuiz() { }
public ActionResult AddQuiz(quiz FormData) { }
public ActionResult DeleteQuiz(quiz FormData) { }
public ActionResult StudentResultDetail(int? quiz_id, int? student_id) { }
public ActionResult AddQuestion(quiz_questions FormData) { }
public ActionResult EditQuestion(quiz_questions FormData) { }
public ActionResult DeleteQuestion(quiz_questions FormData) { }
public ActionResult Courses(int? pageId, string title, int? status, int? course_category_id) { }
public ActionResult StudentsList(int? pageId, string user_name, string email, int? student_id) { }
public ActionResult AssignmentList(int? pageId, int? course_id, string title) { }
public ActionResult DeleteAssignment(assignments FormData) { }
public ActionResult AddAssignment() { }
public ActionResult AddAssignment(assignments FormData) { }
public ActionResult AssignmentDetail(int? assignment_id) { }
public ActionResult EditAssignmentDetail(assignments FormData) { }
public ActionResult StudentAssignmentResultList(int? pageId, int? assignment_id) { }
public ActionResult StudentAssignmentResultDetail(int? assign_answers_id, int? student_id) { }
public ActionResult GiveAssignmentMarkToStudent(assignment_answers FormData) { }
public ActionResult StudentListInCourses(int? pageId, int? course_id, string user_name, int? student_id) { }
}
It has almost 800 lines of code and 24 methods. By looking at the method signatures, you can see the class is doing a lot of work.
The larger the class, the more code will contain, and the more likely it is that the class is starting to do too much. As a rule, classes should only do one thing. This is also known as a single responsibility principle. But when they start doing more, they become difficult to maintain.
While many factors can cause a class to grow in size, one of the most common factors is static methods. Static methods get called from different parts of your solution. And that’s why, when you need to add a new method, you add a new static method to this “god class.”
This design smell can be a sign of poor system design, and you should refactor the class. You need to move some responsibilities to a different class. If the class is in the inheritance hierarchy, see if you can move some code to a base class or a subclass.
Solution: Use Extract Class or Move Method refactoring to have smaller classes.
Long parameter list
Problem: A long parameter list is a code smell because it can be a sign that the method does too much. What qualifies as a long parameter list? Again, that depends on the context and the code you are working with.
In his book, Clean Code, Robert C. Martin (who goes by his stage name Uncle Bob) writes that you should refactor a method that has more than three parameters. This is a bit aggressive statement. Three or four parameters might be just fine. However, if the method has more than four parameters, you should start asking some questions about it.
For example, if you have a long parameter list like this:
public void SaveHomeAddress(string name,
string homeAddress,
string country,
string email,
string fileLocation)
{
}
You can refactor it using Introduce Parameter object refactoring:
public void SaveHomeAddress(AddressDetails addressDetails)
It is good to simplify the parameter list by making it shorter and organizing the parameters. A long parameter list can cause problems in the form of confusion and more difficult debugging. It is also harder to call the method from other places if it has five, six, or more different parameters you need to pass to it.
Another place where you can see a long parameter list is a constructor. Consult this article if you want to get rid of massive constructors once and for all.
Solution: Introduce Parameter Object, Preserve Whole Object, Replace Parameter With Method Call refactoring.
Divergent change
Problem: The divergent change code smell is when the same code is being changed in different ways for different reasons. This can indicate that the code is not well organized or that it is not serving its purpose well.
As an example, take a look at the TeacherHomeController one more time. It can change because of different reasons:
- a change to the quiz list management
- a change to the student assignments feature
- a change to the student list feature
Let’s say you need to change the code that deals with quiz list management. If you are not careful or don’t have enough tests, you can break other features. For example, the students list feature.
When that kind of bug happens, it is a strong indication that something is wrong with how code is structured. You should probably separate the above features into separate classes. This is not always easy, but it’s a refactoring often worth doing.
Solution: Use Extract Class refactoring to move different features to different classes.
Shotgun surgery
Problem: As mentioned before, code smells are often indicators of problems in our codebase and need to be dealt with to keep our code healthy and maintainable.
One such code smell is shotgun surgery. It occurs when you perform some changes in one place to fix a bug or add a feature. But you also need to make changes in several other places for a feature to function correctly.
The problem with this is that it can add unnecessary complexity to the codebase, making it brittle and hard to work in. You could fix the original problem in the codebase with a few lines of code, but with the shotgun surgery, this becomes much more complicated.
You can also notice this during a code review. For example, if there are changes in many files to fix a minor bug, this might indicate a shotgun surgery.
Shotgun surgery is a code smell that behaves opposite of divergent change.
Solution: Use Move Method and Inline Class refactoring to bring code from different places together.
Feature Envy
Problem: Have you ever written code that accesses data or methods from another class more often than its own? If so, you may be suffering from feature envy.
Feature envy is a code smell that occurs when a class accesses the data or methods of another class more often than its own. This indicates that the class’s functionality is too closely tied to another class.
For example, take a look at the following code:
private void CalculateSadness(WordDictionary wordDictionary,
List<string> sadness)
{
wordDictionary._sadnessScore = 0;
for (int i = 0; i < sadness.Count; i++)
{
if (wordDictionary.SadnessWordsContain(sadness[i]))
{
int val;
wordDictionary.SaddnessWords.TryGetValue(sadness[i], out val);
wordDictionary._sadnessScore += val;
}
}
}
This method uses a method, property and a field from the WordDictionary class. If you think you may have feature envy, you can do a few things to remedy the situation:
- Take a look at the code in question and identify the classes referenced most often.
- Consider moving any relevant methods to those classes. This can help to reduce code duplication and improve code organization.
- You can move the code to an entirely new class, both the code that’s suffering from feature envy and the code that the troublesome code calls.
Solution: Use the Extract Method and Move Method to move the troublesome code to another class.
Data clumps
Problem: Data clump is a code smell that occurs when three or more data items always appear together in the code.
Data clumps can be an indication that the code is not well organized. For example, if every time you use a customer’s name in the code, you also need to use the customer’s address and phone number, then you have a data clump:
internal void AddCustomer(string customerName, string address, string phoneNumber) { }
internal void UpdateCustomer(string customerName, string address, string phoneNumber) { }
internal void SetCustomerEmail(string customerName, string email) { }
The code above shows that customerName, address, and phoneNumber are used in multiple places. Data clumps can also make code harder to change. If the same group of data values appears in multiple places throughout the code, you probably need to make changes in several places every time you change something related to one item.
Solution: The solution is to store these items in a single class. Use Extract Class to group the items. Then use Introduce Parameter Object refactoring to eliminate data clumps from the method parameters. If you can’t group all method parameters, use Preserve Whole Object to group at least some of them.
Primitive obsession
Problem: Primitive types are the building blocks of any programming language, and they are essential for writing efficient and readable code. However, when primitive types are used excessively, it can lead to what is known as “primitive obsession.”
This code smell occurs when a class or module contains too many references to primitive types. This can make the code difficult to understand and maintain, leading to errors. For example, think about the subtle error that can occur if you pass the order ID to the method that expects the customer ID. Yikes!
To avoid primitive obsession, create classes for primitive types that are logically connected. For example, create a Money class to group currency and amount fields. Likewise, create a DataRange class for DateTime types that mean start date and end date.
By encapsulating primitive types in objects, code can be more readable and maintainable. In addition, you can prevent errors while implementing new features.
Solution: Group primitive fields together using Extract Class.
Switch statements
Problem: Switch statements are often overlooked as a code smell. They are one of the most basic conditional statements in any C# application. As a result, developers left them alone and didn’t overthink them.
However, in larger applications, the same switch statement, which has the same cases, may appear in multiple places throughout the codebase. Such repetition suggests that there is a lack of real structure or organization. As a result, when you add a new case to one switch statement, you will almost certainly have to go to other places and add the same clause, the same switch branch.
Luckily, there is a solution to this problem: polymorphism.
By implementing polymorphic design principles, developers can create more robust and flexible code structures, reducing the need for unnecessary repetition in their applications. With polymorphism, each switch case goes to a separate class.
After that, you only have one switch statement to return the appropriate class. This allows developers to take full advantage of existing functionality without unnecessarily repeating the same switch statement.
Solution: Use Replace Type Code with State/Strategy or Replace Type Code with Subclasses. After that, use the Replace Conditional with Polymorphism refactoring.
Parallel Inheritance Hierarchies
Problem: Parallel inheritance hierarchies happen in the code in the following case: every time you make one subclass or implement one interface, you also need to make another subclass or create a class that implements another interface. The second class is created further down the chain to keep things consistent and maintainable.
This can lead to code complexity and excessive repetition problems and can easily result in issues such as duplication of effort or maintenance burden shifting between different developers.
Solution: Inspect if you can eliminate one hierarchy by using Collapse Hierarchy and Move Method refactorings to combine them into a single hierarchy.
Lazy class
Problem: A class with minimal functionality can be seen as a code smell.
At its core, a lazy class is simply a class with one or two methods that you could move to another class.
This type of class is often seen as a placeholder for future features, even though that future never comes. A lazy class doesn’t deserve to exist because dealing with a class that has minimal functionality can be confusing.
Solution: Use the Inline Class refactoring to remove the lazy class.
Speculative generality
Problem: Speculative generality refers to code that is overengineered. It can manifest in a variety of ways, such as:
- using generic classes
- using design patterns to solve simple problems
- using abstract class structures or overgeneralizing the solution.
In general, these design decisions are viewed as a waste of time and effort since they often result in premature abstraction that is later discarded or refactored.
To avoid this problem, many developers opt instead to practice the principle of YAGNI, which stands for “You ain’t gonna need it.” YAGNI promotes a focus on short-term requirements and building only what is strictly necessary with minimal overhead.
You can create a solid, functional code that fulfills the immediate needs without overengineering and bloat. Thus, rather than focusing on overengineering and overextending yourself with speculative generality, you should prefer more pragmatic design choices that help you focus on delivering working functionality quickly and efficiently.
Solution: If you want to eliminate unnecessary abstract classes, use Collapse Hierarchy. You can eliminate unnecessary delegation with Inline Class. Use Remove Parameter to remove the unused method parameter.
Temporary field
Problem: Using temporary private fields that only get initialized in some scenarios is considered a code smell.
Such instance variables can make the resulting code difficult to understand. That’s because instance variables used only in specific scenarios are generally inconvenient and confusing for developers to work with.
These instance variables are sometimes used to store an intermediate result during the execution of a complicated algorithm. Instead of passing it as a method parameter, you store the result in a private field. This can further add to the complexity of understanding the code. You need to track both values in the method parameters and the private fields.
That kind of implementation leads to less maintainable and understandable code.
Solution: Use Extract Class to group private fields and method parameters when it makes sense. Then use Introduce Parameter Object to pass all necessary data while executing a complicated algorithm.
Message chains
Problem: A message chain is a class that calls an object which calls another object, and so forth. This code smell can lead to many maintenance problems down the line.
The message chain can look similar to this:
customer.LastInvoice.Order.OrderId
For example, if the way how you access one class in the chain needs to be changed, all the places where that message chain is will need to be changed as well. This can lead to wasted time and effort, and it can be hard to keep track of all the changes.
Additionally, message chains can make code more difficult to understand and debug. If you suspect that your code has a message chain code smell, it’s worth taking some time to refactor your code. Breaking up the message chains can make your code more manageable and easier to work with.
Solution: Use the Hide Delegate refactoring throughout the chain.
Middle man
Problem: When writing clean, efficient code, it can sometimes be easy to fall into the trap of creating a class responsible for delegating calls between different parts of your application. While this may seem like a good way to keep things organized, it can actually lead to middle man code smell.
Middle man class often results in high levels of encapsulation and excessive use of delegation. The methods inside this class rely on outside functionality rather than containing the necessary logic within itself.
Solution: Use Inline Method to eliminate methods that don’t do much.
Inappropriate intimacy
Problem: When classes get too intimate with each other, it creates what is known as an “inappropriate intimacy” code smell. This can happen when classes are tightly coupled, meaning they rely on each other too much.
As a result, they become difficult to change or break apart.
Inappropriate intimacy can lead to problems down the line, so it’s essential to be aware of this code smell and take steps to prevent it. One way to do this is to create more independent classes from each other. It is easier to reuse and change an independent class in the future.
Solution:
- Use Move Method refactoring to reduce the intimacy in the code.
- Try to use Change Bidirectional Association to Unidirectional refactoring to reduce communication between classes.
- If the classes share the common functionality, use Extract Class to move the functionality to a new home.
Alternative classes with different interfaces
Problem: When classes have different method signatures but almost the same behavior, it can be challenging to maintain and modify code. This is because developers need to remember which classes have which methods and how those methods differ.
In addition, it can be confusing to know where to add new functionality. This is not a common code smell, but it can occur in large codebases where multiple teams work on the same solution.
Solution:
- Use Move Method code refactoring to move methods from one class to another.
- Parameterize methods that are similar to remove redundant methods.
- Remove the class from which you have removed the methods.
Refused bequest
Problem: Refused bequest happens when subclasses don’t use methods and fields inherited from the parent class. That can happen for a variety of reasons. For example, maybe the subclasses are doing something different than the parent, or maybe the subclasses were added later, and the developer wasn’t aware of the parent’s methods.
Either way, it’s important to take a close look at refused bequests and decide whether they’re problems or not. In some cases, they’re perfectly fine – but in others, they might indicate a more significant problem.
Solution: Create a new sibling class and use Push Down Method and Push Down Field to move all unused methods to the sibling class. As a result, the parent will only contain common methods.
Comments
Problem: As a developer, you might think that code comments can be helpful when the code is difficult to read or when the code’s functionality is not worth the time to figure out.
A good comment is one that is written and improves the readability of the code. It explains why the code does what it does.
A bad comment is a comment that has been added to explain to the programmer what the code does and has not added any real value to the code. As a result, they are often redundant or not needed. Even worse, when you change the code, you also need to update the comments, which doesn’t happen all the time. And then the comment tells one story, but the code tells another story. And this leads to confusion.
Code comments are often a code smell for a more extensive problem – an improper design. If you find yourself commenting the code more than refactoring it to a proper solution, it may be time to revisit your design to see how you can improve it.
Solution: Use comments. They are good things. But use them sparingly.
Dead code
Problem: Dead code is the term used to describe any code that is no longer used. This typically includes all those defined and unused class variables or injected dependencies through the constructor. This also includes a method parameter passed to the method, but it’s not used anywhere inside the method. Dead code is also a method that has no references.
Dead code does not do much harm. But it complicates the code. It adds another piece of logic you need to read and understand what it does even though it’s not used anymore.
The amount of dead code in a project can indicate how well the project is managed, how much the team cares about the technical debt, and how well the team is communicating.
One warning. It is a mistake to believe that if the code has not been touched in a while, it should be safe to remove it. If the code is referenced by another code, then it’s used and should be left unharmed.
Solution: Many IDEs such as Visual Studio run checks all the time to see what code is referenced by other code and what code is not used. Visual Studio will offer you suggestions to remove the unused code.
Use this help as much as possible, but verify on your own that you can remove the code.
Conclusion
In conclusion, your code is an ever-changing beast. But, like with all other beasts, it can smell from time to time. This post has identified the 21 most common code smells.
I’ve also given you tips on how to get rid of them. So, go forth, and make your code clean and beautiful.
I know, I know.
There is not always enough time to spend in an extensive refactoring process, especially with larger codebases that require more changes.
And you shouldn’t set aside time just for refactoring purposes. But if you do one small refactoring every day, that alone should be enough to eliminate the code smells and improve your code.