How to Identify Code Smells

Code may not have a nose, but it can still smell

Written by Schubert, Software Engineer.

Most of the time as a developer your work does not consist of writing code (That’s shocking I know, but hear me out first). A lot of the time you have to read someone else’s code, review it, and work on it as required. That means identifying mistakes, bugs, bad behavior and... code smells.

What’s this, you ask? Well, a ‘code smell’ is essentially a surface-level sign that something may not be right with the programming. It’s a whiff of weakness in the code that should be investigated further; an indication that there may be a deeper problem affecting performance or stability.

There are several types of code smells that developers should be aware of. As a program becomes ‘richer’ and more complex, you might identify a lot of problems if you know what to look out for. So here I want to show you some of the main code smells and how to detect them.

1. Bloaters

Bloaters are pieces of code - like methods or classes - that have grown into such huge proportions that they have become a nightmare to work with. When you see a ‘bloater’ in the code you’re reviewing your nose should start to twitch. Usually, this type of code smell doesn’t pop up right away - a bloater is something that grows and accumulates over time as a program evolves (particularly when no developer has made an effort to point out the potential issue).

What are the signs and symptoms of bloaters?

• Large Classes/God class antipattern

A class contains many fields, methods and lines of code. As the software program grows larger and more complex, developers often find it less appealing to create new functionalities using new classes. Instead, they prefer to add or overwrite methods within the same class. Eventually, this class has too many responsibilities, and becomes known as a ‘God object’.

• Long Methods
  
  

A method with too many lines of code could indicate a problem. There’s no specific rule, but if you see a method with more than ten lines, it is time to be suspicious of it. As with large classes, developers often think it is harder to implement a new method than just adding a couple of extra lines to an existing one. The result is usually oversized methods and spaghetti code.

• Primitive obsession
  
  This is usually caused by programmer laziness and it is pretty common as well. It involves the overuse of primitives rather than small classes or enums for simple tasks, like ranges, currency, or string constants as field names in arrays. How many times do you see this kind of code in your classes FIRST_ARRAY_ELEMENT = 0 for referring to what is the first element of an array? Using primitives like int, bool, short, double, char, float, etc. excessively undermines the quality of your code. Another symptom of primitive obsession is when you’re forced to write overriding logic which should probably be in its own class.

var phone = string;
          // Extract the area code
          var areacode = "";
          let index = phone.LastIndexOf("-", StringComparison.Ordinal);
          if (index > 0 &amp;&amp; index < phone.Length)
                 areacode = phone.Substring(0,index);
          return areacode

A ‘bloater’ programmer would think: “Well, I don’t want to create a whole new class for just an attribute”. But this moment of weakness might cause a lot of problems in the future.

• Long parameter list

In every single software program, you are going to have a method/class that holds the most valuable algorithm - the one that is at the core of your business. From time to time programmers are asked to add a second path to this logic. Fine. The easiest thing to do is just add another parameter. But then this process iterates again and again as the program keeps growing, which means a longer and longer parameter list that becomes harder and harder to understand. As a general rule, the ideal number of parameters is zero, but if you see three or more it should be a concern and be refactored. Now, which of these look better to you?

function generateBillOrder(userId, name, address, shipingAddress, hasPaymentMode, email, currency, region, postalCode, phone, shipVia, articles, promoCodes, ...)

or

function generateBillOrder(user, shoppingCart)

• Data clumps
  
  This occurs more often than it should. Sometimes, due to bad programming structure, developers may find portions of repeated code in several places. Those clumps are there mainly because of the legendary copy-paste programming technique.

2. Dispensables

This is a concept that most programmers fail to understand. They include too much pointless information in the program, which is damaging for the health of the code. Not having these dispensables would improve readability, proficiency and make the code easier to maintain.

Here are some examples:

• Excessive comments
  
  There could be a method, a class, or variables with a bunch of explanatory comments. This actually hurts a program over time, as other programmers do not feel the need to update them. And then the comments become confusing to read and add no valuable information. What do you think about this?
  
  

  //Class Square
  class Square { //This is the name of the class
  let height //Variable to store height value
  let width //Variable to store width value
  constructor(height, width) { //Class constructor with two params
    if(height !== width){ //Check if height and width are equal, condition to be an square
        alert('Error');  //User notification if the condition false
        continue //Break the whole constructor
    }
    this.height = height; //Assign height from parameter to class attribute
    this.width = width;  //Assign width from parameter to class attribute
  } //End of Class constructor
  } //End of Class Square
  

This is awful to read. Good quality code should be self-explanatory and will not require much useless information. Now check the code below - looks a lot better, right?

class Square { 
  let side
  constructor(side) { 
    this.side = side;
  }
}

• Duplicate code
  
  

When two or (hopefully not) more pieces of identical code are used in different places, something smells. No more explanation required.

• Dead code

How many times do you find variables, methods, parameters, classes, imports that are no longer required but still there? This is dead code and it smells bad. You should always remove it.

• Speculative generality

This is a part of nearly every programmer’s mindset and it is really hard to ignore. Thinking about the future appeals to us - we like that feeling of being prepared for the future with some “just in case” code that would help in case a new feature is implemented. But this thinking usually just leaves us with a lot of unused classes and methods. Some of these remain fossilized in our code, forever waiting for a feature that was never added. Over time, others will find it hard to understand their presence and support them.

• Data class / Brainless class

A brainless class is one that contains only attributes and the basic methods for accessing them (getters/setters). At the end they become nothing more than a storage of data that are used by other classes. So what is the point of those classes if they do not contain any additional functionality and cannot operate their own data? This is a clear violation of the Object-Oriented Programming (OOP) paradigm. This smell can be hard to detect since brainless classes do not typically bring any attention to themselves so programmers may not feel the need to review them.

3. Couplers

An unspoken rule in programming is making code as decoupled as possible while remaining cohesive. Often programmers will find excessive coupling between classes - this group of smells comes from programmers’ need to create entities in order to delegate everything.

• Feature envy

How many times do you find methods that access data from another object to do their job? Programmers will often find pieces of code that access data from another class more often than they access their own data. This might be caused by refactoring gone wrong, which moved data to another class but left the behavior behind.

• Inappropriate intimacy

Like the previous code smell, this might be detected when one class overuses fields from another class. When classes get too interconnected, a programmer will usually struggle to decide where to place methods and variables because both seem to be a good spot. Remember that, by definition, classes should know as little as possible about each other.

• Message chains
  

This happens more often than we’d like. And it’s important to identify the issue quickly, because after a while, message chains build dependency across all of the classes or methods involved. How often do you see this?

methodA()->methodB()->methodC()->methodD()->E.

• Middle man, Class in the middle
  

As programmers, we like to have a delegation order for everything. But most of the time we end up creating a class whose only function is to be called and then call another class. In the end they become empty shells that only delegate. At that point existential questions should enter your mind: why does this class even exist? SPOILER ALERT: It shouldn’t.

--

So these are some of the code smells I wanted to show you. I’m sure many of them are familiar to you already, but from time to time we need to remind ourselves of the things we need to look out for to improve our code.

If you want to stay up to date with all the new content we publish on our blog, share your email and hit the subscribe button.

Also, feel free to browse through the other sections of the blog where you can find many other amazing articles on: Programming, IT, Outsourcing, and even Management.