Code Smells in JavaScript
Tags: javascript, tutorial, beginners, webdev
Published: 2020-02-05 00:40:16

**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:

```javascript
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:


```javascript
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:

```javascript
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!