Author: John Au-Yeung

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 for judgment of whether the code is decent quality or not by looking at it.

In this article, we’ll look at some code smells of JavaScript functions, including too many parameters, long methods, identifier length, returning too much data, and long lines of code.

Too Many Parameters

Functions can have too many parameters. If they have too many, it makes the function more complicated when reading it and calling it. It probably means that we can clean up the code in some way to make this easier to read.

For example, the following function takes many parameters:

const describeFruit = (color, name, size, price, numSeeds, type) => {
  return `${fruitName} is ${fruitColor}. It's ${fruitSize}. It costs ${price}. It has ${numSeeds}. The type if ${type}`;
}

6 parameters are probably too many. We can clean this up by passing in an object instead:

const describeFruit = (fruit) => {
  return `${fruit.name} is ${fruit.color}. It's ${fruit.size}. It costs ${fruit.price}. It has ${fruit.numSeeds}. The type if ${fruit.type}`;
}

As we can see, it’s much cleaner. We don’t have to worry about passing in many arguments.

It also fits better on the screen since it’s shorter.

5 parameters are probably the maximum that should be in a function.

Excessively Long Identifiers

Identifiers that are too long are hard to read. If they can be shorter without losing any information then make them shorter.

For example, we can shorten the following variable declaration:

let colorOfAFruit = 'red';

into:

let fruitColor = 'red';

by removing the Of and A to make it shorter. It doesn’t change the meaning or remove any information. However, it’s shorter so we type less and get the same results.

Excessively Short Identifiers

Identifiers that are too short are also a problem. This is because identifiers that are too short don’t capture all the meaning of the entity that we define.

For example, the following variable name is too short:

let x = 'red';

In the code above, x is too short since we have no idea what it means by looking at the variable name.

Instead, we should write:

let fruitColor = 'red';

So that we know the variable is the color of a fruit.

Excessive Return of Data

Functions that return data we don’t need isn’t good. A function should only return what’s needed by outside code so that we don’t expose extra stuff that isn’t needed.

For example, if we have the following function:

const getFruitColor = (fruit) => {
  return {
    ...fruit,
    size: 'big'
  };
}

We have getFruitColor function with the size property, which isn’t relevant to the color of the fruit. Therefore, it isn’t needed and shouldn’t be returned with the object.

Instead, we should return a string with the fruit color as follows:

const getFruitColor = (fruit) => fruit.color;

The code above is much cleaner and only returns the fruit color as suggested by the name of the function.

Excessively Long Line of Code

Lines of code that are too long make the codebase hard to read, understand and debug.

Code that is so long that they don’t fit in the screen probably should be broken into multiple lines.

For example, instead of writing:

$("p").html("This is a paragraph").css("color", "red").css("text-align", "center").slideUp(1000).slideDown(1000);

We should instead write:

$("p")
  .html("This is a paragraph")
  .css("color", "red")
  .css("text-align", "center")
  .slideUp(1000)
  .slideDown(1000);

This is much cleaner and doesn’t overflow the screen.

Each line of code shouldn’t be more 100 characters so that they can be read without scrolling on any screen.

Code formatters can rearrange the lines so that they’re shorter in most cases.

Conclusion

Having too many parameters in a method makes passing in data hard since it’s easy to miss some items. It also makes the method signature excessively long.

Identifiers should just be long enough to identify the information we need. Also, it shouldn’t be so short that we don’t get enough information from the identifier.

A function should only return the items that we need and no more. This makes using the function easy since there’s less data to handle and not expose extra information that we don’t want to expose.

Finally, long lines of code should be broken into multiple lines so that they’re easier to read and change. Code formatters can break code into multiple lines automatically.

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 for judgment of whether the code is of decent quality or not by looking at it.

In this piece, we’ll look at some code smells of JavaScript classes, including lazy classes, excessive use of literals, too many conditionals and loops, orphaned variables and constants, and using too many variables instead of objects.

Lazy or Freeloader Class

A lazy or freeloader class is a class that does too little. If it doesn’t do much, it probably shouldn’t be added since it’s mostly useless.

We should find a way to put whatever is in the lazy class into a place that has more stuff. A class that has only one or two methods probably isn’t too useful.

Excessive Use of Literals

Using literals too much isn’t a good idea because repeating them will bring in more chances for errors. This is because we have to change each of them when we change code if there are too many of them.

Literals should be externalized into their own files and scripts. More dynamic data should be stored in databases where possible. This makes localization easy if needed.

For example, if we need to place the URL of Medium in multiple places in our code, we should have one constant with the URL rather than repeating it in multiple places.

So instead of:

const mediumRequest = fetch('[http://medium.com'](http://medium.com%27));
//...
const mediumJsUrl = '[https://medium.com/topic/javascript](https://medium.com/topic/javascript)';

We should instead write:

const MEDIUM_URL = '[http://medium.com'](http://medium.com%27);
const mediumRequest = fetch(MEDIUM_URL)
//.,.
const mediumJsUrl = `${MEDIUM_URL}/topic/javascript`;

This is better because we didn’t repeat [https://medium.com](https://medium.com/topic/javascript) in multiple places. Making changes then becomes easier.

Cyclomatic Complexity

Cyclomatic complexity means that there are too many conditional statements and loops in our code.

The complexity can arise in different ways. Loops and conditionals can be nested too deeply. More than two levels of nesting is probably too much and hard to read.

There can also be too many conditionals and loops that aren’t nested.

For example, instead of writing something like:

We should instead write:

We eliminated the nesting and moved some deeply nested code into its own function. This increases readability, and separating code into its own functions makes it easier to test.

Also, using loop control statements — like continue and break— in addition to return can help with controlling the flow of the code a lot without deeply nesting conditional statements with many lines inside.

Orphaned Variable or Constant Class

These are classes that have a collection of constants that belong elsewhere rather than in their own class.

This needs changing because it doesn’t make sense to put them in a place where they aren’t used. It’s not intuitive for anyone reading the code.

For example, if we have a class that has the following variable and we have the following classes:

Then the 'red' in the Color class is a better fit in the Apple class since we’re only using it for Apple instances.

So we can instead write:

class Apple {
  constructor() {
    this.color = 'red';
  }
}

And not bother with having a Color class.

Data Clump

A data clump is a situation where we have too many variables passed around together in various parts of a program. This means that we should group these together into their own objects and pass them together.

For example, if we have a bunch of variables that we pass into a function as follows:

We should rewrite this so that all the variables are in an object instead and change the signature of the function to accept the object. As we can see, there are six parameters, which is too many. More than five is probably too many parameters for a function in most cases.

Also, we had to write out all the variables and they’re related, so we can group them into fields and reference the whole object instead of each variable separately.

This lowers the chance of missing variable references anywhere since grouping the variables into one object means that we only have to deal with one variable instead of six.

We also repeated the word fruit a lot.

We can eliminate all the repetition and reduce the number of variables and parameters we have to deal with by writing the following instead:

Now, we only have one object and parameter to deal with instead of several variables and parameters. It’s much easier on the eyes and more organized. Also, it’s harder for us to forget to reference some variables in our code since we have only one to deal with.

Conclusion

We shouldn’t have classes that do little or orphaned variables and constants. Also, too many loops and conditions — especially if they’re nested deeply — are hard to read and follow.

If we have many variables that are related to each other, they should be grouped into objects so that they’re easy to reference and deal with.

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:

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:

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.

Photo by Wynand Uys on Unsplash

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:

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!

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 for judgment of whether the code is decent quality or not by looking at it.

In JavaScript, code smells still apply. Since there’re old constructs that should be eliminated, there’re additional issues that may arise from looking at old code.

In this article, we’ll look at some code smells using JavaScript code as examples. The code smells we look at include duplicate code, complex code, shotgun surgery, and classes that are too large.

Common Code Smells

Duplicate code

Duplicate code is code that is repeated in different places doing the same thing. Pieces of code that are very similar may also be considered duplicate.

This is a problem because we have to change multiple pieces of code when we have to change duplicate code. Also, it’s easy to forget that they’re there by other developers.

This is why the don’t repeat yourself (DRY) principle is emphasized a lot. It saves developers time.

For example, if we have

let numApples = 1;

in a code file, then we use that everywhere if we want to get the number of apples.

If we have repeated code that’s run many times, then we use a loop. If we want to run a snippet of code in multiple places but not repeated, then we can write a function and run that.

For instance, if we have something like:

let str = '';
str += 'foo';
str += 'bar';
str += 'baz';

We should instead write:

const arr = ['foo', 'bar', 'baz'];
let str = '';
for (let word of arr) {
  str += word;
}

If we want code to be available in multiple modules, we export it and import it elsewhere.

For example, we can write the following in a JavaScript module called module.js and import the foo function elsewhere:

export foo = () => 'foo';

and call it as follows:

import { foo } from 'module';
const bar = foo();

Code That’s Too Complex

If there’s a simpler solution, we should go for that instead of writing something more complex.

Simple solutions are easy to read and change.

For example, if we have:

let str = '';
str += 'foo';
str += 'bar';
str += 'baz';

to create the string 'foobarbaz' , we can simplify it to:

let str = ['foo', 'bar', 'baz'].join('');

Shotgun Surgery

Shotgun surgery is a change that requires code to be multiple pieces of code to be changed.

This is a problem because changes are required in multiple places, which makes making the change difficult. It’s also easy to miss multiple places.

It’s usually caused by adding code that’s run in multiple places in the wrong place. There’s usually a better way to change code than to write code that’s used in multiple places.

For example, if we have an Express app and we want to add logging to it, we shouldn’t do the following:

const express = require('express');
const bodyParser = require('body-parser');

const app = express();

app.use(bodyParser.json());
app.use(bodyParser.urlencoded({ extended: true }));

app.get('/foo', (req, res) => {
  console.log('foo called');
  res.json({ message: 'foo' });
});

app.get('/bar', (req, res) => {
  console.log('bar called');
  res.json({ message: 'bar' });
});

app.get('/baz', (req, res) => {
  console.log('baz called');
  res.json({ message: 'baz' });
});

app.listen(3000, () => console.log('server started'));

The code above isn’t good because we have to add console.log to every route.

Instead, we should refactor the console.log into a route middleware that’s run before the code route code runs:

const express = require('express');
const bodyParser = require('body-parser');

const app = express();

app.use(bodyParser.json());
app.use(bodyParser.urlencoded({ extended: true }));

app.use((req, res, next) => {
  if (req.path.substring(1)) {
    console.log(`${req.path.substring(1)} called`);
  }
  next();
});

app.get('/foo', (req, res) => res.json({ message: 'foo' }));

app.get('/bar', (req, res) => res.json({ message: 'bar' }));

app.get('/baz', (req, res) => res.json({ message: 'baz' }));

app.listen(3000, () => console.log('server started'));

As we can see, the code above is much cleaner and it achieves the same thing as before, except that we only have one console.log instead of one for each route.

If we have more routes, then it’s going to be even harder to change the logging code. For example, if we want to replace console.log with some logging library, then it’s going be way harder if we put the logging code into every route.

Large Classes

Any class that’s large is going to be hard to maintain. Classes are supposed to create objects that only do one thing. We don’t want classes that are so big that they’re doing many things.

An object that’s too large is called a god object. A god object is an object that does too much. It’s very big and it does many things.

Each class is only supposed to solve small problems. If they’re large, then they’re probably doing too much.

Another issue that may arise from having classes that are too large is that different parts of the class depend on each other. The tight coupling creates lots of headaches when sorting out the code.

To solve this, we should the big class into smaller classes that does different things. Common functionality between them can be refactored into a parent class.

For example, if we have the following Employee class:

class Employee {
  eat() {
    //...
  }
  drink() {
    //...
  }
  work() {
    //...
  }
  sleep() {
    ///...
  }
  walk() {
    //...
  }
}

this is an example of a class that’s doing to much because it has methods that aren’t directly relevant for employees.

Instead, we can break the class into two classes as follows:

class Person {
  eat() {
    //...
  }
  drink() {
    //...
  }
  sleep() {
    ///...
  }
  walk() {
    //...
  }
}

class Employee extends Person {
  work() {
    //...
  }
}

Now the Employee only has the method that’s relevant to employees, and that’s the work method. We move the methods that are common to all people to the Person class so we can reuse them and declutter the Employee class.

It’s easy to clean up code and make it easy to read and change by following some basic principles. First, move duplicate code to a common location and reuse the code from the common location. Second, don’t create classes that do too many things. Third, if we need to make a code change in many places at once, there’s probably a better way to do it. Finally, don’t make code that’s too complex.