Clean code exercises - part 2

After years of reading other people's code in code reviews, I've developed an 'eye' to catch bad code, and I think you can develop it too by reading the scenarios I've designed.

The following examples aren't necessarily flawed code, I mean, imagine they don't have any bugs and they do the job, but the aren't as maintainable as they could be. Read each example, try to identify the problem and think what would you do to solve it.

Scenario 4

const chemicalSymbols = {
  Sodium: "Na",
  Hydrogen: "H",
  Helium: "He",
  Oxigen: "O",
};

function getSymbol(name) {
  const symbol = chemicalSymbols[name];
  if (symbol) {
    return symbol;
  }

  console.log("symbol not found");
  return "not found";
}

What's the problem?

(don't read until you are done with above's code)

The problem in this scenario is the lack of error handling. If a chemical symbol is not found, it returns a string which isn't helpful for the caller of the function, because it can't easily determine if the symbol was found or not. This message can then propagate through the chain of functions and cause undesired behavior.

Solution

A way to fix this, is to have better logs and return something that is of a different type as the regular return value. For example null instead of a string.

function getSymbol(name) {
  const symbol = chemicalSymbols[name];
  if (symbol) {
    return symbol;
  }
  console.log(`getSymbol:: symbol not found for ${name}`);
  return null;
}

But a much better alternative is to throw an error. This way if there's a problem, the error will prevent the propagation and crash the program if it's not handled. You can always use try/catch to handle the error.

function getSymbol(name) {
  const symbol = chemicalSymbols[name];
  if (symbol) {
    return symbol;
  }
  throw new Error(`symbol not found for ${name}`);
}

Scenario 5

function getDisplayImage(article, watermark) {
  let image
  if(article.image && article.displayImage) {
    if(watermark){
      image = applyWatermark(article.image, watermark)
    } else {
      image = article.image
    }
  } else {
    image = null
  }

  return image
}

What's the problem?

We have three possible responses with nested if statements in this function. This is a bad practice, because it makes the code more complex and harder to maintain. This example is relatively simple to understand, but in more complex methods, it can become really hard to understand the logic behind it.

Solution

A useful pattern to break this logic into smaller and more understandable chunks is to "return early". We can do de inverse comparison of the first if and return null immediately. Then the other statements don't need to be nested, as they don't need to check anymore for the article images.

function getDisplayImage(article, watermark) {
  let image
  if(!article.image || !article.displayImage) {
    return null
  }
  if(watermark){
    return applyWatermark(article.image, watermark)
  }
  return image
}

Scenario 6

if (
  typeof AUTH_ENABLED === 'string' &&
  AUTH_ENABLED === 'true' &&
  !skipAuthentication &&
  user.email
) {
  console.info('Welcome back, ' + user.name);
}

What's the problem?

There are too many boolean comparisons. It's really hard to understand what's happening at a first glance. If something in this block needs to change, it is going to be super complicated to do so and without introducing any bugs.

Solution

You can simplify the evaluation with named Booleans. By assigning the value to a constant with a meaningful name, you can end up with something that reads almost like plain English and is easier to understand.

const authEnabled = typeof AUTH_ENABLED === 'string' && AUTH_ENABLED === 'true';
const authIsEnforced = authEnabled && !skipAuthentication;
const userLoggedIn = user && user.email;

if(authIsEnforced && userLoggedIn) {
  console.info('Welcome back, ' + user.name);
}

And that was the last exercise for this post. Remember that you can review the exercises for part 1 of the series here.