2017-02-06

How to improve code readability

It’s the first edition of my “Last Week” posts, where I don’t have any external links I want to share with you. So what I’ll do instead is give an example of typical JavaScript code and how I’d improve it to make it easier to understand.

Original Code

This is a function I reviewed some time ago. When reading the following code, try to analyse how you do it.

Are there certain lines where you have to think longer than others? Do you have to go back after reading a line? What makes you stop?

function renderResults(response) {
  hidePaginationArrows();

  let html;
  if (response.hits) {
    const results = response.hits.map(formatSearchResult);
    html = formatSearchTitle(`Results for: ${response.query}`);
    html += results.join("");
  } else {
    html = formatSearchTitle(`No results found for: ${searchInput.value}`);
  }

  documentationContent.innerHTML = html;
}

This code is okay. The naming of the variables is fine. It does what it is supposed to do and isn’t especially hard to understand.

I have written similar code and sometimes do on a first iteration. Since I got more into functional programming I always refactor code like this.

Let’s start improving it!

Remove unnecessary mutations

The first thing I’d change when refactoring code like this is the unnecessary use of mutation. It’s very easy to spot thanks to the let keyword.

Once you reach the line with this mutable variable, you have to keep this state in your head for the following lines. Due to the combination of mutability and a condition in the line below, your brain has to spend more cognitive energy to compare how each branch mutates this variable.

Let’s apply the “return early” pattern to improve this:

function renderResults(response) {
  hidePaginationArrows();

  if (response.hits) {
    documentationContent.innerHTML =
      formatSearchTitle(`Results for: ${response.query}`) +
      response.hits.map(formatSearchResult).join("");
    return;
  }

  documentationContent.innerHTML = formatSearchTitle(
    `No results found for: ${searchInput.value}`
  );
}

Notice how there is nothing to keep in your head anymore. One can read the code from top to bottom without comparing the different branches. The branch for the situation when response.hits exists ends the function execution with the return statement. The other branch follows below in case the condition evaluates to false.

This allows the reader to skip a branch that he/she does not care about. The possibility to skip code while reading is very important. Imagine having multiple branches in a function that you can safely skip while searching for the root cause of a bug.

Do one thing only

Looking at the body of the function one could notice that it does more than its name suggests. First it transforms some data to HTML and then it renders this HTML on the screen by appending it to the DOM.

Below I’ve split up this flow into two functions that only do what they say. I’ve also pulled out the hidePaginationArrows function, since it probably does something with the DOM as well.

Separating side effects from data transformation is one major principle in functional programming.

JavaScript has no type system or pure functions, so it is harder to figure out which functions have side effects. Any function could do more than using its inputs and returning a value. Mutating input values is also a side effect.

One clear indicator of impure functions are functions without a return value. These always have a side effect of some kind, e.g. calling some external library or mutating its inputs. Otherwise what is the point of calling a function, if it doesn’t do anything?

const html = getSearchResultsHtml(response);
hidePaginationArrows();
renderSearchResults(html);

function getSearchResultsHtml(response) {
  if (response.hits) {
    return (
      formatSearchTitle(`Results for: ${response.query}`) +
      response.hits.map(formatSearchResult).join("")
    );
  }
  return formatSearchTitle(`No results found for: ${searchInput.value}`);
}

function renderSearchResults(searchResultsHtml) {
  documentationContent.innerHTML = searchResultsHtml;
}

Note how you could stop reading the code after the first three lines to understand what it does from a high level perspective.

All major steps of the procedure are explicit now. Something else we’ve achieved is that one wouldn’t need to touch the render function if the format of the data changes. The separation of concerns has been improved.

Make dependencies explicit

The next thing I noticed is how getSearchResultsHtml uses the variable searchInput from “somewhere” (the current scope). It also probably displays the search term using different variables: response.query and searchInput.value.

renderSearchResults uses implicit imports too. We can change this function to require the HTML element as an input. This makes it a generic render function, which takes an element and html to render something on the screen.

const html = getSearchResultsHtml(searchInput.value, response.hits);
hidePaginationArrows();
render(documentationContent, html);

function getSearchResultsHtml(searchTerm, hits) {
  if (hits) {
    return (
      formatSearchTitle(`Results for: ${searchTerm}`) +
      hits.map(formatSearchResult).join("")
    );
  }
  return formatSearchTitle(`No results found for: ${searchTerm}`);
}

function render(element, html) {
  element.innerHTML = html;
}

The first three lines are now also explicit about the data and instances they depend on.

You might think about pulling out formatSearchResult from getSearchResultsHtml too. The question is what you’d achieve from it. Do you want to make it possible to replace the formatting of a single search result? What does the function do if you make this part configurable? How would you name it? I’ll leave this thought experiment to you.

Handle exceptions first

My final improvement is handling the exception first in getSearchResultsHtml. Exceptions tend to be shorter than the success case. The reader of this function can then skip the exception handling and quickly get to the main logic.

const html = getSearchResultsHtml(searchInput.value, response.hits);
hidePaginationArrows();
render(documentationContent, html);

function getSearchResultsHtml(searchTerm, hits) {
  if (!hits) {
    return formatSearchTitle(`No results found for: ${searchTerm}`);
  }
  return (
    formatSearchTitle(`Results for: ${searchTerm}`) +
    hits.map(formatSearchResult).join("")
  );
}

function render(element, html) {
  element.innerHTML = html;
}

Analysis of the final result

Compared to the original code at the top, the high level logic is explicit in the first three lines and separate from its implementation.

Data transformation has been separated from side effects. Dependencies are explicit and could be changed without touching the implementation inside of the functions. The code for conditionally visualising the search results is shorter. The render function is generalised and could be reused.

One could still think about better names for variables, functions or try to find out what hidePaginationArrows does, but I will stop here. This is an example to show how you can think about writing more readable code.

Keep in mind that the patterns I used are guidelines and not rules. As a programmer you’ll have to decide on a case by case basis if it makes sense to apply them. There are always advantages and disadvantages, even when refactoring code.

In general though I believe that in most cases these principles guide you towards writing code, that is easier to understand.

How to improve code readability

#Original Code

#Remove unnecessary mutations

#Do one thing only

#Make dependencies explicit