Every software developer will eventually develop his own sense of how and when he should refactor his code, in order to develop that sense, that intuition I’ve assembled a list of code smells that should indicate its time you refactor your code.

Naming

Software developers have to name their files, variables, functions, arguments, classes, etc.. basically they name everything. That is why naming is a skill you must conquer, and take it seriously.

Finding a good variable name that you can understand what it holds and what it does could be hard to find, that is why it is ok to refactor our names when you find a better name.

A good name could save hours of confusion in the future when you come back and try to fix a bug. If you find yourself struggling to find a good name it could be a good indication that the code is not well designed and it holds issues you need to repair.

The following code shows an example of bad naming which makes it harder to understand what it does.

The following examples were taken from the book “Clean Code” by Robert C. Martin and converted to Javascript.

Let’s say that we are working on a Mine Sweeper game, and the argument theList represents our game’s board.

Can you understand what the following function does in our Mine Sweeper game?

function getThem(theList) {
  let list = [];

  for (let i of theList) {
    if (i[0] == 4) {
      list.push(i);
    }
  }

  return list;
}

This code is extremely implicit, it does not tell you why it does what it does, here are the problems with it:

1. The function name is unclear, what does “getThem” refer to?

2. What is the significance of the number 4?

3. Why do we only check the array’s zeroth index?

4. Was I supposed to know that theList contains other lists/arrays?

Those are all questions that could have been answered by simply taking the time to name properly.

Now let’s refactor our code and see the difference:

function getFlaggedCells(gameBoard) {
  let flaggedCells = [];

  for (let cell of gameBoard) {
    if (cell[STATUS_VALUE] == FLAGGED) {
      flaggedCells.push(cell);
    }
  }
  return flaggedCells;
}

First of all, we now know the purpose of the function is to get every flagged cell in our game’s board, a significant improvement.

I’m still not able to automatically see that each cell of the game board is an array as of itself, that is because Javascript is not a typed language, but later we can see again that the cell is being used as an array.

We can also understand that the purpose of the zeroth index in the cell is to hold the status of the cell, and that the number 4 was intended to indicate a flagged cell.

I believe you can now see the power of caring when we name our variables, functions, etc..

Even today when I have to finish a task on time, I sometimes use bad names just to get the job done. I try to come back later and refactor but I sometimes forget or I’m too busy fixing other bugs.

An example of a bad name I wrote is: “setDetailsForModalAndToggle”

The name clearly SCREAMS that the function does two different things at once, it sets the details for the details modal, and it also toggles the modal at the same time.

A name like this could be a good indication that the function needs refactoring.

You should split the function into two different functions, one named “setModalDetails” and the other “toggleModal”, by doing so we decouple the set details action from the toggle action.

This change will make our functions more readable and easier to work with independently.

By now you understand how naming could drastically affect the time you spend reading / understanding / debugging your code.

The names you choose should always tell you what the function does or what the variable holds, without the need for comments.

Do not be afraid of long names, as long as they are expressive and declarative enough.

Course by Sagi Liba,
Author of this blog

The DRY Principal Don't Repeat yourself

One of the first things you learn about refactoring is that duplicated code is bad practice.

Duplicated code is bad practice because every time you read a copy of that code you won’t really know if its the same code or if there is a slight difference, also any changes made to one copy you will probably want to make to the other copies, therefore it wastes your time and will make for an unreliable code base.

To tackle duplicate code you would extract it into a new function and call it where necessary.

One of my friends just started learning React, he reminded me of the way I used to write my components when I started my first position as a junior front-end developer.

Here is an example of a page displaying a list of cards:

AN APPLICATION SHOWING A LIST OF COUNTRIES IN CARDS

import React from "react";
import "./styles.scss";
import Card from "./Card";

export default class App extends React.Component {
  render() {
    return (
      <div className="app-container">
        <Card country="Israel" temperature="28" image="http://..." />
        <Card country="Brazil" temperature="35" image="http://..." />
        <Card country="Iceland" temperature="-12" image="http://..." />
      </div>
    );
  }
}

First of all, I’d like to say that there is nothing wrong with the above code, and for a short readable component, rendering this way might still be acceptable.

The problem starts when we have a component displaying many other components like modals, cards, etc.. and by doing so it makes it much harder to understand our code.

A solution I prefer is to create an array of objects with the necessary data and map over the elements to create the cards, I find it much cleaner, easier to read, and shorter in terms of lines of code inside our render function.

AN APPLICATION SHOWING A LIST OF COUNTRIES IN CARDS

import React from "react";
import "./styles.scss";
import Card from "./Card";

export default class App extends React.Component {
  render() {
    const cards = [
      { country: "Israel", temperature: "28", image: "http://..." },
      { country: "Brazil", temperature: "35", image: "http://..." },
      { country: "Iceland", temperature: "-12", image: "http://..." },
    ];
    return (
      <div className="app-container">
        {cards.map((card) => {
          return (
            <Card
              country={card.country}
              temperature={card.temperature}
              image={card.image}
            />
          );
        })}
      </div>
    );
  }
}

Handling Long Functions, Components, etc...

To be able to maintain a large-scale application the code must be comprised of small units ( functions ) that should only do one thing.

That for me is the main guideline when I write code, I remember the difference I saw when I’ve moved to a new project that followed this principle, suddenly every piece of code seemed easier to understand, and fixing bugs was more about understanding the logic than spending time understanding the code.

One of the basic principals of functional programming is that a function should only do one thing, and avoid creating side-effects. I’m not going to get into functional programming right now but you can understand that when you do more than one thing at a time the code gets more complicated.

You can also understand that an application comprised of a singular purpose functions will be made of smaller units of code.

The longer your classes / function are the harder you have to work to understand them later. Let’s take for example working with React’s components, my rule of thumb is that a component should not exceed 200 lines of code, if it does I usually split it into sub-components or use a library file/move the logic to the store.

This way the component is still readable and if there are any bugs I can debug it at the appropriate locations instead of trying to understand a component with hundreds of lines of code for me to figure out.

Commenting Our Code

When our code gets longer and more complicated the need to write comments that explain our code will arise, if you feel the need to comment your code then it is a good indication that you should refactor it instead.

The type of comments I’m talking about are multi-line comments that are solely there to explain what the following code is doing.

Whenever you encounter such a comment you should write a declarative function instead, that its name is based on that comment.

Even a single line of code that needs explanation is a good candidate to replace with a function.

Remember that everything you do, should be done in moderation, commenting sounds like good advice, but if it is meant to explain what the entire code does then you should rethink how you write your code.

Global Data

Immutable global data that is guaranteed never to be changed is relatively safe, but mutable global data can become very problematic, it can be modified from anywhere in the codebase, when you try to debug an error it could be very difficult to find which part of the code has changed the global data you are using.

To tackle mutable global data we can use set and get functions instead of accessing it directly, that is how we can follow how it is used and where with ease.

Let’s say we have an application that manages a factory, and we have a default global factory owner that we use throughout our application.

let defaultOwner = { firstName: "Sagi", lastName: "Liba" };

We can refactor the above code with a get and set functions as said before:

let defaultOwnerData = { firstName: "Sagi", lastName: "Liba" };

export function defaultOwner() {
  return defaultOwnerData;
}
export function setDefaultOwner(arg) {
  defaultOwnerData = arg;
}

Based on how you would use this global data we can improve it even further, if the global data should not change, then you should return a copy of the data so that any changes made to it later will not affect the shared global data.

The improvement when shared global data should not be changed:

let defaultOwnerData = { firstName: "Sagi", lastName: "Liba" };

export function defaultOwner() {
  return Object.assign({}, defaultOwnerData);
}
export function setDefaultOwner(arg) {
  defaultOwnerData = arg;
}

The greater the scope of the global data that you use the more important it is to encapsulate it.

Reduce Surface Area

When working with global data it is important to make sure that the person going over your code is aware that the data being used/manipulated is global.

This might seem weird at first, but one of the things I like to do in order to show that I’m working with global data is to declare new variables pointing to that global data at the start of the function, that’s how I know that this data is global.

Let’s say we have a global array of default user privileges:

export const defaultPrivileges = [
  "create_articles",
  "edit_articles",
  "delete_articles",
];

When I’m going to work with that global data, I’m going to create a variable at the top that points to that global reference.

function canUserPerformAction(privilege) {
  const defaultPrivileges = defaultPrivileges;

  let result = false;

  // Is user logged in
  // ...

  // Is it an admin user
  // ...

  // Check his default privileges
  if (defaultPrivileges.includes(privilege)) {
    result = true;
  }

  // Other Calculations
  // ...

  return result;
}

The main benefits of using this approach are:

• Debugging: when you have a bug related to the user’s default privileges you don’t have to start wandering around your code and going to other files, you can start by looking at the variable we have set pointing to that global data, Only if that is not enough then you must go to its original location.

• Slight performance improvement, the Javascript engine does not have to perform a scope chain lookup until it reaches the global defaultPrivileges, it is already available in the current scope.

Performance Consideration - Avoid Global Lookups

To increase the performance of our code we should always pay extra attention to global variables and functions, they are always more expensive to use than local ones because they involve a scope chain lookup.

The following examples were taken from the book “Professional Javascript For Web Developers” by Matt Frisbie.

function updateUI() {
  let imgs = document.getElementsByTagName("img");

  for (let i = 0, len = imgs.length; i < len; i++) {
    imgs[i].title = `${document.title} image ${i}`;
  }

  let msg = document.getElementById("msg");
  msg.innerHTML = "Update complete.";
}

This function is perfectly fine when working on a small number of images, the only problem is the number of references to the global document object.

When you consider that there could be dozens or even hundreds of images to update in the UI each time, these global document lookups will shortly become expensive, each time requiring a scope chain lookup.

We can refactor our code by creating a local variable that points to the document object, it will decrease the number of scope chain lookups to just one, ensuring it will run faster.

function updateUI() {
  let doc = document;
  let imgs = doc.getElementsByTagName("img");

  for (let i = 0, len = imgs.length; i < len; i++) {
    imgs[i].title = "${doc.title} image ${i}";
  }

  let msg = doc.getElementById("msg");
  msg.innerHTML = "Update complete.";
}

A good rule of thumb is to store any global object that is used more than once in a function as a local variable.

Some of the advice given in this article is well known for some and missed by others.

I hope this article was of value to you as a developer, now you can give back by liking the IRYL Facebook page below 🙂