31 code smells you must be familiar with


Before we dive into learning about some of the most common types of code smells, let’s first answer the important question:

What are Code Smells?

Code smells are common programming characteristics that might indicate a problem in the code. Many times, the problem may be clear and visible. Other times, the problem may result in a future problem or a deeply rooted problem. Other times, there may not even be a problem in the first place.

code smell is a hint that something has gone wrong somewhere in your code. 

Wards Wiki

Are code smells bad?

Yes, but also no. Code smells are “most likely” bad, but the term is often misused and misunderstood. Not all “signs of code smells” are necessarily “signs of bad code.”

The code smell metaphor originates from Wards Wiki, and they stress:

Note that a CodeSmell is a hint that something might be wrong, not a certainty. A perfectly good idiom may be considered a CodeSmell because it’s often misused, or because there’s a simpler alternative that works in most cases. Calling something a CodeSmell is not an attack; it’s simply a sign that a closer look is warranted.

Wards Wiki

So while not all of these common characteristics may always indicate that a code smell is present, it is very important that all software engineers be very familiar with the different types of code smells. By knowing what code smells are and how to sniff them out, you can help yourself write clean code by ensuring you’re not introducing these 31 code smells into your software.

Dispensables

Dispensable Code Smells are generally unnecessary code that should be removed from the source. They range in the form of:

  • Comments
  • Duplicate Code
  • Lazy Class
  • Dead Code
  • Speculative Generality
  • Oddball Solution

1. Comments

Comments are code smells

Are you commenting your code? There’s no question it’s important to comment your code, but it’s more important to know why you’re commenting your code.

Are you commenting your code because the algorithm you created is confusing as all hell? (come on, we’ve all been there).

That right there is NOT the correct way to comment your code. In fact, that’s not even the correct way to code. If you need instructions to follow along with your confusing code, then it’s confusing code that needs to be refactored.

Comments in code should not have to explain how your code works. Your code should be easy enough to understand for anyone with a bit of programming knowledge to be able to follow along.

So when should you use comments then? Comments are meant to explain why your code is doing what it’s doing. Why a certain algorithm is there or doing something may not be as clear to other programmers if they aren’t as familiar with the system.

Common concerns during a code review might reveal:

  • “Why do you use function X instead of function Y?
  • “When would this conditional ever get executed?
  • “Why did you rewrite this function like this?”

The answers to many of these types of questions may not always be clear, even with the cleanest code. Answering these types of questions or describing this type of rationale is not only completely acceptable in comments, but also encouraged to help future programmers of your code.

SSL Certificate for just $8.88 with Namecheap

2. Duplicate Code

Duplicate Code is Code Smell. Follow the DRY principle

We’re all familiar with the D.R.Y. principle, which stands for Don’t Repeat Yourself. Yet many times we still find duplicate code in our code bases. Why is this?

One of the most common scenarios I’ve found is because of a rushed deadline. Often times programmers will need a “one off” solution of a function they’ve already created or seen.

However, they need to slightly change the algorithm and also not break the existing functionality that depends on it. So they will just copy and paste that code into a new function for their specific needs, either due to a short deadline or out of pure laziness.

The term duplicate code doesn’t have to refer to code that’s just copy and paste (or nearly copy and paste) either. Sometimes a developer may have good intentions on writing clean code but may have not even realized they just created duplicate code.

This often happens when multiple team members are working on a similar problem and not communicating properly, each creating their own version to the solution without realizing it.

Hey wait!!! If you want to learn some refactoring tips for cleaning up duplicate code, then I’d highly suggest you buy the book “Clean Code” by Robert “Uncle Bob” Martin. Click here to purchase it on Amazon!

3. Lazy Class

lazy class is a common code smell

The more, the merrier? Not when it comes to constructing software!

Every additional class adds more complexity to the project. If you identify a class that isn’t entirely too useful and not pulling its own weight in the project, then consider collapsing the class or possibly combining it with an existing class (just keep in mind the Single Responsibility Principle).

4. Dead Code

dead code is a code smell

Alas, you come across a class that doesn’t appear to be hooked up to anything. Is it being used? Are there any other classes depending on this class to remain functional? Is it dead code???

Jeff Atwood advocates to “ruthlessly delete code that isn’t being used. That’s why we have source control systems!” on his blog over at CodingHorror.

I tend to agree with him. It’s frustrating coming across old code that you have no clue what it’s doing in the project in the first place. Worse if you actually take the time to try and refactor any of it when it turns out it wasn’t even being used in the first place.

How does dead code end up in a project? Most of the time code becomes dead as a result of refactoring and/or software requirements change. The developers either forget to clean up the old code or didn’t even know about the old code in the first place.

Dead code can also take the form of old commented out code. During code refactoring, some developers may leave the old code in there but commented out in fear that they may need to revert back to the old changes. Again to Jeff’s point, THAT’S THE EXACT REASON WE USE VERSION CONTROL!

Jeff’s stance on just deleting the old code makes sense. You will (should) know if it breaks anything if all of your test cases still pass after you delete the code. Plus, in the event that your test cases didn’t catch something and the application does break after deleting the presumed dead code, you can always roll back the changes in your versioning control system anyways! Then it really does give you a reason to refactor and document the code so the next developer doesn’t try and make the same mistake as you!

5. Speculative Generality

speculative generality is a code smell

Speculative Generality is similar to dead code in that it’s unused code. However instead of being code that once lived and then died, speculative generality is code that never even lived in the first place.

Often times developers will put in code as a “placeholder” for a future requirement, either as directed by management or with good intentions thinking it’s what the customer/ end user would actually want.

However, this code just ends up cluttering the already crowded project. As time goes on, these speculative classes end up getting forgotten, buried, and lost in the documentation as they never made their way into hard requirements.

As a result, no one remembers what these classes are for and they’re just left in the code base adding to the complexity, wasting valuable time as developers analyze them to try and understand their existence.

In short, don’t create classes based on speculative generality. Create classes that solve the current requirements. Nothing less, nothing more.

6. Oddball Solution

Oddball Solution Code Smell

What is an oddball solution? Imagine having two solutions to one problem. One of these solutions is the oddball solution.

Often times this occurs from poorly duplicated code or similar classes with different interfaces. More often than not, the way to fix this code is to just remove the oddball solution (the duplicate code that’s used less throughout the project).

Sometimes, you may need to keep the duplicated classes, i.e. you need to keep the oddball solution. Perhaps an existing class may provide the functionality required by a client, but its interface may not be what the client expects.

In such cases, the existing interface needs to be converted into
another interface, which the client expects, preserving the reusability
of the existing class. The Adapter Pattern would help cleanup these types of scenarios.

The Adapter pattern suggests defining a wrapper class around the object with the incompatible interface. It converts the interface of a class into another interface the clients expect. The Adapter Pattern lets classes work together that couldn’t otherwise because of incompatible interfaces.

SSL Certificate for just $8.88 with Namecheap

Bloaters

Bloater Code Smells occur when code is way bigger than it should be, but you don’t exactly know “when” it happened, it just kind of creeps up on you. Usually, the project started out with some rock solid code, but as the life of the program continues to age, new requirements come in, and different programmers cycle through the code base, the code smells start trickling in as more and more code gets added to the same old classes. No one honors the Programmer Boy Scout Rule – leave the code base camp cleaner than you found it!

Many of the common examples of bloater code smells include:

  • Classes that are entirely too big (Large Class)
  • Methods and functions that are also entirely too big (Long Method)
  • Way too many parameters for a given function (Long Parameter List)
  • Too many primitives and constants that should be encapsulated together in an object (Primitive Obsession)
  • Same group of data being repeated in several different places (Data Clump)

7. Large Class

Large Classes are Code Smells

Are you familiar with SRP (Single Responsibility Principle)? I’ve written a blog article about SRP, and it’s also heavily documented in Uncle Bob’s Clean Code book, as well as StackOverflow, and many other places on the internet. Basically what I’m saying is if you don’t know the Single Responsibility Principle then you’re wrong.

As the name suggests, the SRP tells us that a single class should only do one single thing. Or as Uncle Bob puts it, should only have one single reason to change.

The SRP is one of the simplest of the principles, and one of the hardest to get right. Conjoining responsibilities is something that we do naturally. Finding and separating those responsibilities from one another is much of what software design is really about.

Robert “Uncle Bob” Martin

I’m sure you see where I’m going with this now. Large classes are most likely the result of developers violating the Single Responsibility Principle. More often than not, a large class is doing more than one thing.

As time goes on and the program grows, classes tend to get bigger and more bloated. As new requirements come in, software engineers need to consider if the code they’re adding to an existing class really belongs in the class, or if they would benefit from creating a separate class or possibly even restructuring the existing class and breaking it down into smaller classes.

Large classes are just naturally harder to read, harder to understand, and harder to maintain. Keep your classes small and adhere to the Single Responsibility Principle.

8. Long Method

Long Methods may indicate a code smell

Much like large classes, long methods and long functions are generally harder to read than shorter ones. Everyone has a different target number for their “maximum lines of code” a function should be, some say 20 lines long, other 10 lines, and few even only 5 lines long.

Realistically though, the only important thing to keep in mind when writing functions is to ensure that each method should only manage one responsibility.

A method should be as long as it needs to be. However, most can agree that anything above 25 lines of code should make you start asking questions. There may be cases where a 50 or 100 line method is necessary, but more often than not you should be able to break down a function into smaller chunks to make the code more readable.

9. Long Parameter List

Long Parameter Lists almost always are considered a code smell

Do you have functions that take so many parameters you need to wrap the parameter list down to the next line?

public void printPerson(String firstName, String lastName, String middleName, 
  String maidenName, String babyName, String nickName, 
  String thirdFavoriteDinosaur) {
...
}

Functions that suffer from long parameter lists are harder to read and tend to be more complex, i.e. they’re doing more than they’re supposed to be doing.

Remember, functions should be small enough to handle one and only one thing, but large enough to do that one thing well (however most of the time you should be able to handle that one thing in under 10 lines of code).

When functions or methods exceed 4+ parameters, then there’s likely 1 of 2 problems going on:

  1. there’s more than one thing happening in the function
  2. the parameters are tightly related and should be grouped together in a data object

If there’s more than one thing happening in the function, then it should be pretty obvious that you need to refactor the function into smaller, more targeted functions.

If the method truly is only doing one thing and you do need all those parameters, then chances are you’d benefit from combining those parameters together in a data object.

10. Primitive Obsession

Primitive Obsession as a code smell

Here’s a code smell that really starts to creep up on you, primitive obsession. As the name probably indicates, this occurs when there’s a bunch of unnecessary primitive variables.

The reason this one creeps up on you is because it usually only starts with one or two primitives. It’s like sneaking a piece of chocolate while on a strict diet, “just one little piece won’t hurt.”

// user data capture form
String username;

Then the next release is under development and management realizes you never added fields to capture the user’s first and last name. So you add them in real quick.

// user data capture form
String username;
String firstName;
String lastName;

Then more subsequent release cycles and marketing starts breathing down your neck asking why you’re not capturing email or address information.

// user data capture form
String username;
String firstName;
String lastName;
String addressLine1;
String addressLine2;
String city;
...

I’m sure you see where this is going. No one is naturally constructing primitive obsession from the get-go (hopefully anyways). It just sort of happens as new requirements come in and the programmers don’t know where to store the data.

31 code smells you must be familiar with 1

Take a page from the Programmer Boy Scouts Rule, leave the code base camp cleaner than you found it. When you start to see this bloating happen, take the proactive action to introduce a parameter object or create a whole object to represent the data and clean up this code smell.

SSL Certificate for just $8.88 with Namecheap

11. Data Clumps

data clump is a code smell

Much like primitive obsession, data clumps are data that always seems to cling onto each other. They’re always hanging out together, getting themselves into trouble. It’s often hard to pull these data away from each other. You finally have the nerve to tease them “if you like them so much then why don’t you marry them?”

“Actually, what a great idea!” they rejoice.

Too lame an analogy? Ok true, well what about that old proverb then “birds of a feather flock together” or something like that?

Basically what I’m saying is, just group like-data together in a class. For much of the same reasons for many of the bloater code smells, data clumps just make your code harder to read and understand.

Why should the next programmer have to piece together different yet related data just to find out they’re all doing. Group them together in a class with a meaningful class name to represent the data.

Abusers

Abusers are more commonly referred to as Object-Orientation Abusers. This category represents cases where the solutions do not fully exploit the possibilities of good object-oriented design. Many times, these code smells are a result of novice programmers who fail to identify when and where to put their design patterns to good use.

Object-Orientation Abuser Code Smells:

  • Switch Statements
  • Temporary Field
  • Refused Bequest
  • Alternate Classes with Different Interfaces
  • Combinatorial Explosion
  • Conditional Complexity

12. Switch Statements

switch statements are often a code smell

If you’ve been a developer for any amount of time I’m sure you’ve heard (hopefully) that switch statements are often preferred over if-else statements. If you’ve been a developer for anything longer than any amount of time then I’m sure you’ve heard (hopefully) that switch statements are bad in OOP.

So are switch statements a code smell in Object Oriented Programming?

Here’s the thing, switch statements aren’t bad, but they’re also not good. It’s like saying a fork is bad but a spoon is good. Both are useful for their intended purpose. And switch statements have their own intended purpose. The problem arises when newer developer abuse switch statements when they should be using a design pattern instead.

Consider this scenario for example:

function void fight(int characterType) {
switch (characterType):
  case CHARMANDER:
    doFireStuff();
    break;
  case BULBASOUR:
    doPlantStuff();
    break;
  case SQUIRTLE:
    doWaterStuff();
    break;
  case PIKACHU:
    doElectricStuff();
    break;
}

This is a great example of when not to use a switch statement.

(By the way, I’m about to attempt to go into a Pokemon reference when I’m not that much of a Pokemon fan. So 2 things to note before we proceed: 1. if you’re a true Pokemon fan, please don’t get too offended when I screw something up, this is only for an example, and 2. if you’re not a Pokemon fan and have no clue what the above code really means, just know that there are like hundreds of different types of Pokemon and each can do different types of things, some have “fire” action moves, others have “water” action moves, etc.)

So why is the above example bad? When starting out we only have 4 different character types here, but what happens when we get more characters? We’ll need to add more and more cases to our switch statement.

What’s worse is that this is only for fighting moves. We may have different functions for other action types where we’d have a very similar switch statement in each one. Maybe each character type talks differently, moves differently (some only walk, some can swim, some can fly), maybe they all even sleep differently. I don’t really know. The point is, we would have a duplicated switch statement structure for different functions to accommodate all the different character types.

For reference, the above code would benefit from something like the Factory Design Pattern or Strategy Design Pattern, where we can use polymorphism create different object types or apply different algorithms to an object dynamically. But that’s beyond the discussion of this blog post. Read the Design Patterns article to learn more!

So then, when should we use a switch statement? The example above shows a program that’s switching on a different object “type”. You’re essentially hardcoding something that would be better handled by polymorphism. Switching on a value is usually alright, as long as switching logic is confined to one place (i.e. avoid the Duplicate Code smell).

function Character getCharacterType(int userSelectedCharacter) {
  switch (userSelectedCharacter) {
    case CHARMANDER:
      return new Charmander();
    case BULBASOUR:
      return new Bulbasour();
    case SQUIRTLE:
      return new Squirtle();
    case PIKACHU:
      return new Pikachu();
    default:
      throw new InvalidPokemonException(); 
  }

  function void fight(Character character) {
    character.performFight();
  }
}

The example above is a very basic view of the Factory Method Design pattern in action. Here we’re only using a switch statement once, which handles creating the certain character type that we need. So we’re switching on a value which will then create a new object type.

Then we use polymorphism to invoke each action. Each character type (Pikachu, Charmander, etc.) extends the Character interface, so each character will implement their own fighting action, their own talking action, etc. Again, this is beyond the scope of the Switch Statement code smell.

13. Temporary Field

SSL Certificate for just $8.88 with Namecheap

14. Refused Bequest

Refused bequest is when a subclass doesn’t use any (or at least most) of its inherited functionality. Ok, now let’s bring out the grammar police because I think this term was originally defined wrong.

Jumping outside of the programmer world of terminology here, the word “inheritance” refers to the property (here “property” doesn’t necessarily have to mean real estate) that you receive upon someone else’s death. The world “bequest” refers to the property you give to someone upon your death. So personally, I think this code smell should really be renamed to “Refused Inheritance” instead of “Refused Bequest” but I guess what’s done is done. Perhaps some of you can prove me wrong in the comments below. Let me hear your arguments on why it was named Request Bequest and not Refused Inheritance.

Anyways, if you see a class that inherits from another class, but isn’t actually using much of the inherited properties, then why is it inherited in the first place?

Programmers love jumping to inheritance to solve all the world’s problems, when in reality I think inheritance is dangerously over-used in Object Oriented Programming.

It’s usually done with the right intentions. “I need a separate class that also wants to use this function, so I’ll just inherit from this class to avoid code duplication!”

However, when that one function is the only thing being used, it can lead to a whole mess of other problems down the line, not to mention it’s just confusing to other programmers who need to one day maintain the system.

Use inheritance only when it actually makes sense. Class Cat is an Animal. Class Dog is an Animal. Those are great examples of inheritance. Class Piano is not a Computer, and Class Computer is not a Piano, despite both objects having keys that a user will interact with.

15. Alternative Classes with Different Interfaces

16. Combinatorial Explosion

17. Conditional Complexity

Conditional Complexity is a code smell when 10 or more if-else statements are in one large conditional block

Large and complex condition blocks are a common code smell sign of, well, conditional complexity.

You could make the case for this one to be a part of the Bloaters category, as usually these start out relatively small then grow with the life of the program as new requirements and features are brought in. However, that category was already a little bloated (pun intended).

Plus, this one is similar to Switch Statements anyways, and per the Bad Code Smells Taxonomy, it should be part of The Object-Orientation Abusers.

Regardless of what category we put this code smell in, I know we’re all familiar with this one. We see it all too often, conditional blocks of 10+ if-else statements.

A lot of times, this really resulted due to a lack of design planning. More often than not, these large conditional blocks could be cleaned up by implementing a design pattern such as the decorator, strategy, or state.

Couplers

Coupler Code Smells, as the name indicates, couples code together too tightly that makes change difficult. Software is supposed to change. That’s literally the name, “soft” + ware. If it wasn’t meant to change then it would just be “hard” + ware. By coupling software together too tightly, you make change impossible, and have just effectively re-invented hardware.

Here are some common Coupler Code Smells:

  • Inappropriate Intimacy
  • Indecent Exposure
  • Feature Envy
  • Message Chains
  • Middle Man

18. Inappropriate Intimacy

19. Indecent Exposure

SSL Certificate for just $8.88 with Namecheap

20. Feature Envy

21. Message Chains

22. Middle Man

Preventers

Preventers are more commonly referred to as The Change Preventers. These smells hinder change for further developing software. They include:

  • Divergent Change
  • Shotgun Surgery
  • Parallel Inheritance Hierarchies

23. Divergent Change

24. Shotgun Surgery

25. Parallel Inheritance Hierarchies

SSL Certificate for just $8.88 with Namecheap

26. Solution Sprawl

The code smell of a solution sprawl

Solution Sprawl is “the identical twin brother of Shotgun Surgery as described by Fowler and Beck.

You need to add or remove a feature from a project, but in order to do so must change many different components in many different classes. This is the sign of a code smell.

Or put another way by Jeff Atwood, “if it takes five classes to do anything useful, you might have solution sprawl.”

If the solution sprawl is caused by many different objects being created, but the creation logic is sprawled out in many different places, consider refactoring with the Factory Design Pattern.

Other notable mentions

Many of the above mentioned code smells derived from many places, including the Refactoring book and the simplified taxonomy of code smells.

However there are more code smells in the world of software engineering. here are some of the other code smells, inspired by various sources including CodingHorror and personal experiences.

They include:

  • Inconsistent Names
  • Uncommunicative Name
  • Type Embedded in Name
  • Magic Numbers
  • Incomplete Library Class

27. Inconsistent Names

28. Uncommunicative Name

29. Type Embedded in Name

30. Magic Numbers

31. Incomplete Library Class

Adam Allard

Father of 2, husband of 1, developer of many. I'm a Software Engineer for Northrop Grumman creating web applications for the DoD. Currently I'm primarily working with Java and Angular based applications, although I have some years of experience in various languages and frameworks, including Python & Django, C# & Xamarin, PHP, and Bootstrap. My hobbies include time with my family, wondering when the Green Bay Packers will win their next Super Bowl, drinking over-priced beer, and of course learning and teaching.

Leave a Reply

Your email address will not be published. Required fields are marked *

Recent Content