Code Smells in JavaScript

Subscribe to my email list now at http://jauyeung.net/subscribe/

Follow me on Twitter at https://twitter.com/AuMayeung

Many more articles at https://medium.com/@hohanga

Even more articles at http://thewebdev.info/

In programming, a code smell is a characteristic of a piece of code that indicates there may be deeper problems. It’s a subjective characteristic used to judge whether code is of decent quality by looking at it.

In this article, we look at more code smells in JavaScript code, including feature envy, and classes that are too intimate.

---

Feature Envy

Feature envy means that one class uses too many features of another class. This means that most of one class reference something in another class.

This is especially applicable to using fields from one in class in another class excessively

For example, if we have the following:

class Rectangle {
  constructor() {
    this.height = 1;
    this.width = 1;
  }
}
class ShapeCalculator {
  constructor() {
    this.rectangle = new Rectangle();
  }
  getRectangleArea() {
    return this.rectangle.height * this.rectangle.width;
  }
}

The ShapeCalculator class uses lots of fields from Rectangle. We get all the fields from Rectangle in the ShapeCalculator class when we don’t have to.

This breaks encapsulation in the Rectangle class since we exposed the fields height and width when we don’t have to.

Instead, we should avoid using the height and width fields from the Rectangle object in the ShapeCalculator class and write a method in the Rectangle class and call it in the ShapeCalculator class as follows:

class Rectangle {
  constructor() {
    this.height = 1;
    this.width = 1;
  }
  getArea() {
    return this.height * this.width;
  }
}
class ShapeCalculator {
  constructor() {
    this.rectangle = new Rectangle();
  }
  getRectangleArea() {
    return this.rectangle.getArea();
  }
}

This is much better because all we reference in the ShapeCalculator from the Rectangle class is the getArea method. We no longer have to worry about changes to the height and width fields since they’re encapsulated in the getArea method.

As long as getArea returns a number with the Rectangle's area, we aren’t concerned about the fields or the implementation of the methods in the Rectangle class.

---

Inappropriate Intimacy

A class that has dependencies on the implementation details of another class is considered to be too intimate.

We don’t want classes that depend on the implementation of the other classes because this means that we have to change both classes when the first class change.

There isn’t enough encapsulation of the fields or implementation of the methods.

The lack of encapsulation may arise from exposing the fields of the class. Letting us set field values directly is probably not good in most cases.

Writing code that adheres to some interface is impossible because it’s very hard to change a class to follow an interface since so many other classes depend on the implementation of the given class.

Too many interactions between classes also cause confusion because the workflow is hard to trace. This is the object-oriented version of spaghetti code.

Any changes in a class that other classes are too dependent on also creates problems when we change them since they’re so tightly coupled, we have to change all the classes that depend on it to change it. This exacerbates the problem of maintainability of code, in addition to spaghetti code.

For example, if we have the following classes:

class Box {
  constructor() {
    this.length = 1;
    this.width = 1;
    this.height = 1;
  }
  getSurfaceArea() {
    return 2 * (this.height * this.width) + 2 * (this.height * this.length) + 2 * (this.length * this.width)
  }
}
class ShapeCalculator {
  constructor() {
    this.box = new Box();
  }
  getBoxSurfaceArea() {
    return this.box.getArea();
  }
  getBoxVolume() {
    return this.box.length * this.box.width * this.box.height;
  }
}

We can see the ShapeCalculator uses the Box classes fields and methods a lot.

In the ShapeCalculator class, we reference the Box's length, width, height properties, and the getSurfaceArea method.

If we change anything in the Box class, we also have to change it in the ShapeCalculator. For instance, if we rename everything in the Box class as follows:

We have to make the same changes in the ShapeCalculator calculator. This is a complete nightmare and it’s not even that complex. Imagine if our classes had more fields and methods — it quickly becomes unmaintainable.

The interaction is also confusing. Some fields are used in ShapeCalculator for calculations and some methods are used straight from the Box class.

What we should do instead is not reference any fields in the Box class in the ShapeCalculator class and instead change the Box class to provide all the methods needed by the ShapeCalculator class, as follows:

Then the ShapeCalculator can be rewritten as:

As we can see, the ShapeCalculator class only references the methods of the Box class, instead of having a mix of field and method references from the Box class.

Now we don’t have to worry whether the field names are changed in the Box class, since we never used them.

When we write classes, we should only expose what we need. Most fields can probably be hidden so they won’t be used by external classes. This reduces the effort to maintain the code since they won’t have to be changed in all the classes that use them.

It keeps everything modular and self-contained. Also, we only expose methods that we need to use and hide others like helper methods from the public for the same reason.

Too much coupling and referencing between classes is no good!