Riddler Challenge 01: The case with the switch

The riddler did it again. He used his superior knowledge of JavaScript to create this incredible piece of code:

const riddlerChallenge01 = (items = []) => {
  const options = [{ value: 'name' }];
  const defaultName = items.find(val => val.isEnabled);
  let output = '';

  switch (true) {
    case Boolean(defaultName):
      output = 'defaultName';
      break;
    case options.length > 0:
      output = options[options.length - 1].value;
      break;
    default:
      break;
  }

  return output;
};

Take your time to appreciate the marvel of the inner workings and the beauty of the solution. We will rewrite the code that the logic is simpler and the next developer can understand what is going on.

First Step: Analysis

Line for line you will have to look for clues and understanding of what the code actually does. While we are doing this, I make mental notes on what to improve.

I’ll add some comments to the code that really stands out and can be improved. Ideally, we would write these comments in the code review so the riddler can fix the issues on his own.

const riddlerChallenge01 = (items = []) => {
  const options = [{ value: 'name' }]; // An array with a single item -> probably should be converted to a simpler variable
  const defaultName = items.find(val => val.isEnabled); // Should use Array.some
  let output = '';

  switch (
    true // Incredible that this passed a code review, with a fixed value the switch is the wrong construct
  ) {
    case Boolean(defaultName): // The Boolean check should be written as defaultName !== unknown
      output = 'defaultName'; //Convert to constant string
      break;
    case options.length > 0: //options.length > 0 => is always true
      output = options[options.length - 1].value; //The array never changes - the string could be used directly
      break;
    default: //unneeded default case
      //unreachable code
      break; 
  }

  return output;
};

Second step: Add tests

By reading the code we could figure out that the function returns a string. We now write tests that simulate all the possible outcomes of this function. This way we get a clear definition of what the functionality of the code is.

This will ensure that we do not break existing functionality when we start refactoring the code.

it('works for no items', () => {
  expect(riddlerChallenge01([])).toEqual('name');
});

it('works with an enabled item', () => {
  expect(riddlerChallenge01([{ isEnabled: true }])).toEqual('defaultName');
});

it('works with an disabled item', () => {
  expect(riddlerChallenge01([{ isEnabled: false }])).toEqual('name');
});
it('works with an mixed items', () => {
  expect(riddlerChallenge01([{ isEnabled: true }, { isEnabled: false }])).toEqual('defaultName');
});

Refactor the code

Step 1: Use a if-statement instead of a switch

The current switch statement works because of the case Boolean(defaultName). Depending on the input it is either true, and then the case gets executed. It does not execute the second case because of the break;.

If Boolean(defaultName) evaluates to false, then the switch case will always execute options.length > 0 as it always evaluates to true. This in turn means that the default-case cannot be reached and is not needed.

The correct way to write this logic is with a simple ‘if statement’.

const riddlerChallenge01 = (items = []) => {
  const options = [{ value: 'name' }];
  const defaultName = items.find(val => val.isEnabled);
  let output = '';

  if(defaultName !== undefined) {
      output = 'defaultName';
  } else {
      output = options[options.length - 1].value;
  }

  return output;
};

Step 2: Remove the options Variable

The variable options probably had multiple values in the past, and is now just a hangover from an older version of the code. As the array only contains a single item and never gets modified > the array should be converted to a string.

In this case we can simply use the variable directly, as the array is not even referenced at any other place in the code.

const riddlerChallenge01 = (items = []) => {

  const defaultName = items.find(val => val.isEnabled);
  let output = '';

  if(defaultName !== undefined) {
      output = 'defaultName';
  } else {
      output = 'name';
  }

  return output;
};

Step 3: Optimize Variable names and readability

The variable name defaultName is misleading as it indicates that it is a string but it used as an boolean. This in turn means that it is better to use Array.some() that returns a boolean instead of Array.find() that returns the object.

I would also rename the variable output to appTitle to make it more clear why we are saving this string.

const riddlerChallenge01 = (items = []) => {  
  let appTitle = 'name';

  const useDefaultName = items.some(val => val.isEnabled);
  if(useDefaultName) {
      appTitle = 'defaultName';
  }

  return appTitle;
};

Note: I choose to remove the else branch of the code as well. This is mostly to mimic the switch mechanism more closely. If you would want to extend it with another case then you would just add another if block.

When you are refactoring code, your primary goal should be that the functionality stays the same, while the code gets more readable and if possible more efficient than before.

What do you think? How would you have rewritten this piece of code?