Intive Blog

A review of the most common code smells

On the article Code smells and refactoring we dealt with code smells, but not in-depth actually. We said that with a sharp eye we could spot them to know when refactoring becomes necessary and we have defined them as superficial indicators of some kind of issue in the system, structures that can be seen in the code and suggest the possibility of a refactor. But, which are they? What are they like? How to identify them? How to work them out? We intend to answer all these in the following article.


Types and subtypes of code smells

  • Bloaters: They stand for something that has grown at such a rate that it is complex both to grasp and manage. In general, they stock up through time and cannot be detected until the program evolves. They’re:
  1. Long Method: Anyone can notice that the longer the procedure, the more difficult to understand it. Still, most developers are reluctant to cut down on methods too much because they are afraid of having too many and, when reading the code, they might need to be constantly changing the context to know what a submethod does. The key for small methods to be easy to understand is to name them properly, with a simple abstraction that allows us to know what they do. By knowing this, we will not need to check its body and change the context. So, Martin Fowler (see book at the end of the doc) suggests that we should be more aggressive when breaking down methods. How to identify it? If we think it necessary to comment on the code of a method to make it easier to understand, we should probably remove it and put it in a new method to make it more expressive. How to solve it? 99% of the times we will use the refactor Extract Method. If local parameters and variables interfere, we can use other refactors to use objects of parameters and replace temporal variables with methods.
  2. Large class: How to identify it? A class tries to take too many responsibilities, when it should follow the principle of single responsibility, that is, to fulfill only one of the software features and to be well encapsulated. In general, they show too many instance variables and duplicated code among methods. How to solve it? We must use the refactor Extract Class to extract a group of variables that can go together with a descriptive name. Thus, we will prevent primitive obsession (which will be tackled later). We can also extract subclasses if part of the behavior can be used in different forms, or to extract an interface if a list of behaviors for the client is required. Preserve whole object may come in handy if there are parameters that prevent the class from being extracted.
  3. Primitive Obsession: It is actually a symptom that generates bloats rather than a bloat itself. When there is primitive obsession there are not small classes for small entities (such as phone numbers) and many constants are used. Since this looks easier than creating a new class, they start stocking up. Data clumps also lead to bloating: different parts of the code use the same groups of variables. This happens when lots of copy & paste is performed, so avoiding bad practices is key in order not to generate bloats. How to identify it? By the use of constants for the information codification and the use of chain constants as field names for its use as in data fixing. How to work it out? If we see a good range of primitive fields, we can group some in their own class or move the behavior associated to that data also to the same class. To do so, we can test replace value with object data. When data are coded in a difficult way in the variables, we can replace type code with classes or with subclasses. If we observe fixes among variables, we should perform the matrix replacement with object.
  4. Long Parameter List: They come up when a method is too long or when we try to make classes independent. But when delegating less among classes, we need to use too many parameters for the method to have what it requires to work. Anyway, you should never pass to a method everything it requires, but only the necessary to achieve what it requires. How to identify it? When we have more than three or four parameters for a method. How to work it out? We should check the past values as parameter. Generally, it is only necessary to replace the parameter with a call to another class and instead of passing a list of variables as parameters, we must send the entire object. Sometimes many parameters can be grouped in a single object before being sent, such as a date range rather than start and end.
  • Object-Orientation Abusers: The solution for them does not use all the possibilities that object oriented programming features. For instance, a switch in objects is, let’s say it, appalling. Polymorphism should be used then. Let’s go through some object-orientation abusers.
  1. Refused bequest: How to identify it? A subclass uses only some of the methods and attributes of its superclasses. Somebody, very likely, wanted to reuse code, but the hierarchy is misused since classes are totally different. How to work it out? If the heritage makes no sense, we should replace it with delegation to the object. If it makes sense, a superclass should be extracted to obtain the variables and the behavior we are looking for.
    Bequest means that a subclass rejects the father class behavior that it does not need.
  2. Temporary field: How to identify it? An object has instance variables which are used only at times. . This is strange because we expect the object to need all its variables and it is difficult to understand why it is there if it does not need it. How to work it out? We can extract a class where all the temporal variables are and the behavior using them. Conditional code can also be eliminated using an object for the Null value, a very commonly used refactor.
  • Change Preventers: These are smells that prevent software change or expansion. They are:
  1. Divergent Change: This implies that we have only one class to be modified when many different changes are made. Shotgun surgery (the next to be explained) is the opposite: we need to modify many classes when we make a single change in the system. That is why, according to Fowler and Beck, classes and possible changes must have a one-to-one relationship.
  2. Shotgun surgery: How to identify it? Making any modification requires many small changes in many different classes. How to work it out? Move all the similar behaviors to only one class, in this way each will have its own responsibility. Remember the principle of single responsibility. If an appropriate class to move the behavior to does not exist, you must create one.
  3. Parallel hierarchies of heritage: How to identify them? When creating a subclass for a class and realizing that there is no other way but to create a new subclass for another class. How to work them out? You should “un-duplicate” the hierarchy class. Then, make mentions of a hierarchy refer to cases of another. Eventually, remove the hierarchy of the class which is referred to using Move Method and Move Field.
  •  Couplers: They contribute to an excessive coupling among classes or it shows what happens when the compiling is replaced by an excessive delegation. They’re:
    1. Feature Envy
    2. Inappropriate Intimacy
    3. Message Chains
    4. Middle Man

The first three are of a high coupling, whereas Middle Man represents a problem that may come up when trying to avoid high coupling with constant delegation. Inappropriate intimacy occurs when two classes are too much coupled between them.

Feature Envy: We spot it when an object is more interested in another object than in itself. This mainly happens when there is “data envy”. This is easy to solve: Methods should be moved to the class which is being envied. This is not always so easy, we should bear in mind that a method may include behavior of several classes. To avoid this, it is ideal to keep responsibilities and encapsulation of the classes. Middle Man is identifiable because the only job it performs is to delegate tasks to other classes. It should not exist. The solutions lie in removing the class and calling the final object. If the class has other own responsibilities, we should remove the methods and do the same. (remove middle man).

We hope that after this article, identifying code smells becomes easier in the future, to actually know when to perform refactoring.

Santiago Pugliese

He is a developer in intive – FDV since February 2016 and leads the Backend Team. He also works wit Node.js. He studies the Systems Engineering degree at the National Technological University (NTU). He is a Technical Electronic and Electromechanical. He stands out for its creativity and when he is not working, he plays harmonica, piano or draws.

Add comment