← back to articles

Untangling Spaghetti Code: Writing Maintainable JavaScript

Save article ToRead Archive Delete · Log out

17 min read · View original · sitepoint.com

This article was peer reviewed by Tom Greco, Dan Prince and Yaphi Berhanu. Thanks to all of SitePoint’s peer reviewers for making SitePoint content the best it can be!

Almost every developer has had the experience of maintaining or taking over a legacy project. Or, maybe it’s an old project which got picked up again. Common first thoughts are to throw away the code base, and start from scratch. The code can be messy, undocumented and it can take days to fully understand everything. But, with proper planning, analysis, and a good workflow, it is possible to turn a spaghetti codebase into a clean, organised, and scalable one.

More from this author

I’ve had to take over and clean-up a lot of projects. There haven’t been many I started from scratch. In fact, I am currently doing exact that. I’ve learned a lot regarding JavaScript, keeping a codebase organised and — most importantly — not being mad at the previous developer. In this article I want to show you my steps and tell you my experience.

Analyse the Project

The very first step is to get an overview of what is going on. If it’s a website, click your way through all the functionality: open modals, send forms and so on. While doing that, have the Developer Tools open, to see if any errors are popping up or anything is getting logged. If it’s a Node.js project, open the command line interface and go through the API. In the best case the project has an entry point (e.g. main.js, index.js, app.js, …) where either all modules are initialized or, in the worst case, the entire business logic is located.

Find out which tools are in use. jQuery? React? Express? Make a list of everything that is important to know. Let’s say the project is written in Angular 2 and you haven’t worked with that, go straight to the documentation and get a basic understanding. Search for best practices.

Understand the Project on a Higher Level

Knowing the technologies is a good start, but to get a real feel and understanding, it’s time to look into the unit tests. Unit testing is a way of testing functionality and the methods of your code to ensure your code behaves as intended. Reading — and running — unit tests gives you a much deeper understanding than reading only code. If they are no unit tests in your project, don’t worry, we will come to that.

Create a Baseline

This is all about establishing consistency. Now that you have all information about the projects toolchain, you know the structure and how the logic is connected, it’s time to create a baseline. I recommend adding an .editorconfig file to keep coding style guides consistent between different editors, IDE’s, and developers.

Coherent indentation

The famous question (it’s rather a war though), whether spaces or tabs should be used, doesn’t matter. Is the codebase written in spaces? Continue with spaces. With tabs? Use them. Only when the codebase has mixed indentation is it necessary to decide which to use. Opinions are fine, but a good project makes sure all developer can work without hassle.

Why is this even important? Everyone has it’s own way of using an editor or IDE. For instance, I am a huge fan of code folding. Without that feature, I am literally lost in a file. When the indentation isn’t coherent, this features fails. So every time I open a file, I would have to fix the indentation before I can even start working. This is a huge waste of time.

// While this is valid JavaScript, the block can't
// be properly folded due to its mixed indentation.
 function foo (data) {
  let property = String(data);

if (property === 'bar') {
   property = doSomething(property);
  }
  //... more logic.
 }

// Correct indentation makes the code block foldable,
// enabling a better experience and clean codebase.
function foo (data) {
 let property = String(data);

 if (property === 'bar') {
  property = doSomething(property);
 }
 //... more logic.
}

Naming

Make sure that the naming convention used in the project is respected. CamelCase is commonly used in JavaScript code, but I’ve seen mixed conventions a lot. For instance, jQuery projects often have mixed naming of jQuery object variables and other variables.

// Inconsistent naming makes it harder
// to scan and understand the code. It can also
// lead to false expectations.
const $element = $('.element');

function _privateMethod () {
  const self = $(this);
  const _internalElement = $('.internal-element');
  let $data = element.data('foo');
  //... more logic.
}

// This is much easier and faster to understand.
const $element = $('.element');

function _privateMethod () {
  const $this = $(this);
  const $internalElement = $('.internal-element');
  let elementData = $element.data('foo');
  //... more logic.
}

Linting Everything

While the previous steps were more cosmetic and mainly to help with scanning the code faster, here we introduce and ensure common best practices as well as code quality. ESLint, JSLint, and JSHint are the most popular JavaScript linters these days. Personally, I used to work with JSHint a lot, but ESLint has started to become my favorite, mainly because of its custom rules and early ES2015 support.

When you start linting, if a lot of errors pop up, fix them! Don’t continue with anything else before your linter is happy!

Updating Dependencies

Updating dependencies should be done carefully. It’s easy to introduce more errors when not paying attention to the changes your dependencies have gone through. Some projects might work with fixed versions (e.g. v1.12.5), while others use wildcard versions (e.g. v1.12.x). In case you need a quick update, a version number is constructed as follows: MAJOR.MINOR.PATCH. If you aren’t familiar with how semantic versioning works, I recommend reading this article by Tim Oxley.

There is no general rule for updating dependencies. Each project is different and should be handled as such. Updating the PATCH number of your dependencies shouldn’t be a problem at all, and MINOR is usually fine too. Only when you bump the MAJOR number of your dependencies, you should look up what exactly has changed. Maybe the API has changed entirely and you need to rewrite large parts of your application. If that’s not worth the effort, I would avoid updating to the next major version.

If your project uses npm as dependency manager (and there aren’t any competitors) you can check for any outdated dependencies with the handy npm outdated command from your CLI. Let me illustrate this with an example from one of my projects called FrontBook, where I frequently update all dependencies:

Image of npm outdated

As you can see I have a lot of major updates here. I wouldn’t update all of them at once, but one at a time. Granted, this will take up a lot of time, yet it is the only way to ensure nothing breaks (if the project doesn’t have any tests).

Let’s Get Our Hands Dirty

The main message I want you to take with you is that cleaning up doesn’t necessarily mean removing and rewriting large sections of code. Of course, this is sometimes the only solution but it shouldn’t be your first and only step. JavaScript can be an odd language, hence giving generic advice is usually not possible. You always have to evaluate your specific situation and figure out a working solution.

Establish Unit Tests

Having unit tests ensures you understand how the code is intended to work and you don’t accidentily break anything. JavaScript unit testing is worth its own articles, so I won’t be able to go much into detail here. Widely used frameworks are Karma, Jasmine, Mocha or Ava. If you also want to test your user interface, Nightwatch.js and DalekJS are recommended browser automation tools.

The difference between unit testing and browser automation is, that the former tests your JavaScript code itself. It ensures all your modules and general logic work as intended. Browser automation, on the other hand, tests the surface — the user interface — of your project, making sure elements are in the right place and work as expected.

Take care of unit tests before you start refactoring anything else. The stability of your project will improve, and you haven’t even thought about scalability! A great side effect is not being worried all the time that you might have broken something and didn’t notice.

Rebecca Murphey as written an excellent article on writing unit tests for existing JavaScript.

Architecture

JavaScript architecture is another huge topic. Refactoring and cleaning up the architecture boils down to how much experience you have with doing that. We have a lot of different design patterns in software development, but not all of them are a good fit where scalability is concerned. Unfortunately I won’t be able to cover all of the cases in this article, but can at least give you some general advice.

First of all, you should figure out which design patterns are already used in your project. Read up about the pattern, and ensure it’s consistent. One of the keys to scalability is sticking to the pattern, and not mixing methodologies. Of course, you can have different design patterns for different purposes in your project (e.g. using the Singleton Pattern for data structures or short namespaced helper functions, and the Observer Pattern for your modules) but should never write one module with one pattern, and another one with a different pattern.

If there isn’t really any architecture in your project (maybe everything is just in one huge app.js), it’s time to change that. Don’t do it all at once, but piece by piece. Again, there is no generic way to do things and every project setup is different. Folder structures varies between projects, depending on the size and complexity. Usually — on a very basic level — the structure is split up into third-party libraries, modules, data and an entry-point (e.g. index.js, main.js) where all your modules and logic gets initialized.

This leads me to modularization.

Modularize Everything?

Modularization is by far not the answer to the great JavaScript scalability question. It adds another layer of API that developers have to get familiar with. This can be worth the hassle though. The principle is splitting up all your functionality in to tiny modules. By doing that, it is easier to solve problems in your code and to work in a team on the same codebase. Every module should have exactly one purpose and task to do. A module doesn’t know about the outside logic of your application, and can be reused in different locations and situations.

How do you split up a large feature with lots of tightly connected logic? Let’s do this together.

// This example uses the Fetch API to request an API. Let's assume
// that it returns a JSON file with some basic content. We then create a
// new element, count all characters from some fictional content
// and insert it somewhere in your UI.
fetch('https://api.somewebsite.io/post/61454e0126ebb8a2e85d', { method: 'GET' })
  .then(response => {
    if (response.status === 200) {
      return response.json();
    }
  })
  .then(json => {
    if (json) {
      Object.keys(json).forEach(key => {
        const item = json[key];
        const count = item.content.trim().replace(/\s+/gi, '').length;
        const el = `
          <div class="foo-${item.className}">
            <p>Total characters: ${count}</p>
          </div>
        `;
        const wrapper = document.querySelector('.info-element');

        wrapper.innerHTML = el;
      });
    }
  })
  .catch(error => console.error(error));

This is not very modular. Everything is tightly connected and dependent on the other pieces. Imagine this with larger, more complex functions and you would have to debug this because something breaks. Maybe the API doesn’t respond, something changed inside of the JSON or whatever. A nightmare, isn’t it?

Let’s separate out the different responsibilities:

// In the previous example we had a function that counted
// the characters of a string. Let's turn that into a module.
function countCharacters (text) {
  const removeWhitespace = /\s+/gi;
  return text.trim().replace(removeWhitespace, '').length;
}

// The part where we had a string with some markup in it,
// is also a proper module now. We use the DOM API to create
// the HTML, instead of inserting it with a string.
function createWrapperElement (cssClass, content) {
  const className = cssClass || 'default';
  const wrapperElement = document.createElement('div');
  const textElement = document.createElement('p');
  const textNode = document.createTextNode(`Total characters: ${content}`);

  wrapperElement.classList.add(className);
  textElement.appendChild(textNode);
  wrapperElement.appendChild(textElement);

  return wrapperElement;
}

// The anonymous function from the .forEach() method,
// should also be its own module.
function appendCharacterCount (config) {
  const wordCount = countCharacters(config.content);
  const wrapperElement = createWrapperElement(config.className, wordCount);
  const infoElement = document.querySelector('.info-element');

  infoElement.appendChild(wrapperElement);
}

Alright, we have three new modules now. Let’s see the refactored fetch call.

fetch('https://api.somewebsite.io/post/61454e0126ebb8a2e85d', { method: 'GET' })
  .then(response => {
    if (response.status === 200) {
      return response.json();
    }
  })
  .then(json => {
    if (json) {
      Object.keys(json).forEach(key => appendCharacterCount(json[key]))
    }
  })
  .catch(error => console.error(error));

We could also take the logic from inside the .then() methods and separate that, but I think I have demonstrated what modularization means.

If !modularization What Else?

As I already mentioned, turning your codebase in tiny modules adds another layer of API. If you don’t want that, but want to keep it easier for other developers to work with your code, it’s absolutely fine to keep functions larger. You can still break down your code into simpler portions and focus more on testable code.

Document Your Code

Documentation is a heavily discussed topic. One part of the programming community advocates for documenting everything, while another group thinks self-documenting code is the way to go. As with most things in life, I think a good balance of both makes code readable and scalable. Use JSDoc for your documentation.

JSDoc is an API documentation generator for JavaScript. It is usually available as a plugin for all well-known editors and IDE’s. Let’s go through an example:

function properties (name, obj = {}) {
  if (!name) return;
  const arr = [];

  Object.keys(obj).forEach(key => {
    if (arr.indexOf(obj[key][name]) <= -1) {
      arr.push(obj[key][name]);
    }
  });

  return arr;
}

This function takes two parameters and iterates over an object, which then returns an array. This might not be an overly complicated method, but for someone who hasn’t written the code it might take a while to figure out what is going on. Additionally, it’s not obvious what the method does. Let’s start documenting:

/**
 * Iterates over an object, pushes all properties matching 'name' into
 * a new array, but only once per occurance.
 * @param  {String}  propertyName - Name of the property you want
 * @param  {Object}  obj          - The object you want to iterate over
 * @return {Array}
 */
function getArrayOfProperties (propertyName, obj = {}) {
  if (!propertyName) return;
  const properties = [];
  Object.keys(obj).forEach(child => {
    if (properties.indexOf(obj[child][propertyName]) <= -1) {
      properties.push(obj[child][propertyName]);
    }
  });
  return properties;
}

I haven’t touched much of the code itself. Just by renaming the function and adding a short, yet detailed comment block, we’ve improved the readability.

Have An Organized Commit Workflow

Refactoring is a huge mission on its own. To be able to always rollback your changes (in case you break something and only notice later), I recommend committing every update you make. Rewrote a method? git commit (or svn commit, if you work with SVN). Renamed a namespace, folder or a few images? git commit. You get the idea. It might be tedious for some people to do, but it really helps you clean up properly and get organized.

Create a new branch for the entire refactoring effort. Don’t ever work on master! You may have to do quick changes or upload bug fixes to the production environment and you don’t want to deploy your (maybe untested) code until it is tested and finished. Hence it is advised to always work on a different branch.

In case you need short update how all this works, there is an interesting guide from GitHub on their version control workflow.

How To Not Lose Your Mind

Besides all the technical steps required for a clean-up, there is one important step I rarely see mentioned anywhere: not being mad at the previous developer. Of course, this doesn’t apply to everyone, but I know that some people experience this. It took me years to really understand this and get over it. I used to get pretty mad at the previous developers code, their solutions and just why everything was such a mess.

In the end, all that negativity never got me anywhere. It only results in you refactoring more than necessary, wasting your time, and maybe breaking things. This just makes you more and more annoyed. You might spend extra hours and nobody will ever thank you for rewriting an already working module. It’s not worth it. Do what is required, analyse the situation. You can always refactor tiny bits every time you go back to a module.

There are always reasons why code is written the way it is. Maybe the previous developer just didn’t have enough time to do it properly, didn’t know better, or whatever. We have all been there.

Wrapping It Up

Let’s go over all steps again, to create a checklist for your next project.

  1. Analyse the project
  1. Create a baseline
  1. Cleaning up
  1. Don’t lose your mind

I really hope this article has helped you. Let me know if you struggle with any of the steps, or maybe have some good advice that I didn’t mention!

Log in with GitHub account

Create a token at your GitHub settings page and paste it in the text field below