Simplify Nested Code

One of the most common pieces of feedback I leave in code reviews, at least in the realm of code readability, is to reduce nesting. In general, the more deeply-nested code is, the more complex it is to track mentally. This is especially the case with if statements.

A have placed a code snippet below; which you can imagine might appear as part of a larger function. This code is doing something relatively simple: setting a status message based on a few criteria. Unfortunately, while the message itself is simple, getting your head around all the different execution paths is not. Understanding this code means you have to think about error state, loading state, an isActive flag, an isDefault flag, and whether other items in the array are active.

if (item) {
  if (item.isActive || (item.isDefault && !items.some((i) => i.isActive))) {
    message = 'Active';
  } else {
    message = 'Inactive';
  }
} else {
  if (hasError) {
    message = 'Error loading item';
  } else if (isLoading) {
    message = 'Loading...';
  } else {
    message = 'No item selected';
  }
}

We’ve all seen code similar to this. We’ve probably all seen code even worse.

Often, code like this evolves over time. It may have been very simple initially, but as needs arose or edge cases emerged, developers addressed them by adding new conditionals.

I’m not going to say this code is wrong, per se, because it does work and it’s not so obtuse as to be indecipherable. In fact, this is a perfectly reasonable first draft when writing code. But it has some weaknesses that could be improved.

Return early ¶

All the complexity in this code lies in the conditional logic. One of the conditionals is made more complicated by a combination of several values plus an inline function. The most difficult part of understanding this code lies in following all the branching ways execution may flow.

If you were to pick up this code to make edits, you would have to carefully read through these conditionals and try to keep all the possible paths in mind. It’s a lot of little things you need to keep in your working memory while you make changes.

To minimize this mental load, it’s best to reduce the number of conditionals, and to remove else clauses entirely if possible.

The best technique to do this is to return early.

If the block of code is part of a longer function, enclose it in a new function of its own. Return out of that function as various cases are handled. Doing this results in code more like the following:

const getMessage = () => {
  if (item) {
    if (item.isActive || (item.isDefault && !items.some((i) => i.isActive))) {
      return 'Active';
    }
    return 'Inactive';
  }

  if (hasError) {
    return 'Error loading item';
  }
  if (isLoading) {
    return 'Loading...';
  }
  return 'No item selected';
};

const message = getMessage();

Now, there are no else statements required. Each time a case is handled, the message value is returned, and that condition can be disregarded for the rest of the function. The nesting has been simplified, making the code a lot more linear.

Handle edge cases first ¶

At the beginning of the new function, checks are made to see if we’re on the “happy path”, and if so the function returns the expected value ('Active' or 'Inactive'). After that, all the edge cases are handled.

This approach requires two nested conditionals at the beginning of the function. Instead, it’s often simpler to reverse the order and handle the edge cases first. The errors and loading states are a quick check in this case, so make those checks as the very first thing in the function, moving the core logic of the logic below them:

const getMessage() => {
  if (hasError) {
    return 'Error loading resource';
  }
  if (isLoading) {
    return 'Loading...';
  }
  if (!item) {
    return 'No item selected';
  }

  if (item.isActive || (item.isDefault && !items.some((i) => i.isActive))) {
    return 'Active';
  }
  return 'Inactive';
}

const message = getMessage();

Now, when working on the meat of the function, you can know that edge cases are dealt with and dismiss them from your mind. This frees up your mental energy to focus only on the core logic you’re concerned with. This is especially useful when the logic is more complicated.

This code is the exact same number of lines as the original, but the logical flow is much simpler.

Simplify boolean logic ¶

There’s one final way you can simplify this code further.

Boolean logic is not intuitive. The computer is excellent at it, but it does not come naturally to us. Think about a simple example:

if (a && b) {
  doX();
} else {
  doY();
}

When does the else case execute?

It executes when the conditional is false, or if (!a || !b). Reversing the logic mentally means negating each value and changing the and to an or.

It gets even more complicated with three or more values. When does the statement if (a && b || !c) evaluate to false? Can you figure it out instantly by looking at it, or do you need to trace through each piece of it carefully to be sure?

If this sort of logic is part of two or three nested conditionals, and you find a new edge case in your code, how confident will you be that you’re placing the new code in the right part of the conditional? How confident will you be adding a new variable d to the conditionals and understanding how it impacts the existing logic?

This kind of change to code is extremely prone to bugs.

When I encounter a complex conditional, I always look to see if there’s a way to simplify it down and assign it to a variable with an easy to understand name.

In the code above, this certainly means moving the inline function out of the if statement:

const noActiveItems = !items.some((i) => i.isActive);

I’d also consider setting the entire conditional to a single variable, called isActive. This gets me a final version of the code that looks like this:

const getMessage = () => {
  if (hasError) {
    return 'Error loading resource';
  }
  if (isLoading) {
    return 'Loading...';
  }
  if (!item) {
    return 'No item selected';
  }

  const noActiveItems = !items.some((i) => i.isActive);
  const isActive = item.isActive ?? (item.isDefault && noActiveItems);
  if (isActive) {
    return 'Active';
  }
  return 'Inactive';
};

const message = getMessage();

Now, when the next developer comes along, this should be much faster to skim and understand.

Edge cases are dealt with, and can be disregarded before the core logic is considered. The final if (isActive) is easy to understand without even reading the two lines above it. If the developer needs to look closely at how isActive is determined, they can, but if that’s not a part of the logic they’re looking to change, they can almost completely ignore the lines that assign values to noActiveItems and isActive.

This code flows linearly, has no deep nesting, and requires much less focused effort to parse. Life is just a bit easier for developers maintaining it, and any new changes will be less prone to bugs as a result.