5
\$\begingroup\$

I get an object with categories, first category has 2 types. Both types are in the first and second position but sorting is not constant.

I have to return category 1 with playLevel === 2 when bonusPlus is true and category 1 with playLevel === 1 otherwise.

Here is the code:

const getFirstCategory = (bonusPlus, prizeCategories) => {
  if (bonusPlus) {
    if (prizeCategories[0].playLevel === 2) {
      return [prizeCategories[0]];
    }
    return [prizeCategories[1]];
  }
  if (prizeCategories[0].playLevel === 1) {
    return [prizeCategories[0]];
  }
  return [prizeCategories[1]];
};

How can I reduce / beautify this code?

\$\endgroup\$
  • 1
    \$\begingroup\$ As per the How to Ask guidelines, please explain what "this code" does, and what the parameters represent. \$\endgroup\$ – 200_success Mar 27 '17 at 13:41
  • \$\begingroup\$ Do I understand correctly that you want the price category where playLevel === (bonusPlus ? 2 :1)? \$\endgroup\$ – JollyJoker Mar 27 '17 at 15:02
12
\$\begingroup\$

To make @kfx's answer more readable:

const getFirstCategory = (bonusPlus, prizeCategories) => {
  const bonusPlusLevel = bonusPlus ? 2 : 1;
  if (prizeCategories[0].playLevel === bonusPlusLevel) {
    return [prizeCategories[0]];
  }
  return [prizeCategories[1]];
};
\$\endgroup\$
  • 1
    \$\begingroup\$ just a little suggestion: const bonusPlusLevel \$\endgroup\$ – joc Mar 27 '17 at 14:44
  • 1
    \$\begingroup\$ Yeah, I'm not a JS guy... \$\endgroup\$ – kyrill Mar 27 '17 at 14:46
5
\$\begingroup\$

Using the ternary operator and an extra variable would allow to reduce the size of the code:

const getFirstCategory = (bonusPlus, prizeCategories) => {
  var index = (prizeCategories[0].playLevel === (bonusPlus ? 2 : 1) ? 0 : 1);
  return [prizeCategories[index]];
};
\$\endgroup\$
  • 12
    \$\begingroup\$ The double ternary operator is a bit unreadable, isn't it? \$\endgroup\$ – kyrill Mar 27 '17 at 12:39
  • 2
    \$\begingroup\$ I agree with @kyrill. You have reduced the code, but uglified it \$\endgroup\$ – ediblecode Mar 27 '17 at 14:28
3
\$\begingroup\$

I would start by leaving the unnecessary condition brackets to make the program flow more obvious :

const getFirstCategory = (bonusPlus, prizeCategories) => {
  if (bonusPlus) {
    if (prizeCategories[0].playLevel === 2) {
      return [prizeCategories[0]];
    } else {
      return [prizeCategories[1]];
    }
  } else {
    if (prizeCategories[0].playLevel === 1) {
      return [prizeCategories[0]];
    } else {
      return [prizeCategories[1]];
    }
  }
};

Then use a variable for the result :

const getFirstCategory = (bonusPlus, prizeCategories) => {
  let firstCategory;
  if (bonusPlus) {
    if (prizeCategories[0].playLevel === 2) {
      firstCategory = [prizeCategories[0]];
    } else {
      firstCategory = [prizeCategories[1]];
    }
  } else {
    if (prizeCategories[0].playLevel === 1) {
      firstCategory = [prizeCategories[0]];
    } else {
      firstCategory = [prizeCategories[1]];
    }
  }
  return firstCategory;
};

Then mutualise the use of [prizeCategories[i]] :

const getFirstCategory = (bonusPlus, prizeCategories) => {
  let index;
  if (bonusPlus) {
    if (prizeCategories[0].playLevel === 2) {
      index = 0;
    } else {
      index = 1;
    }
  } else {
    if (prizeCategories[0].playLevel === 1) {
      index = 0;
    } else {
      index = 1;
    }
  }
  return [prizeCategories[index]];
};

Then mutualise the index double initialisation :

const getFirstCategory = (bonusPlus, prizeCategories) => {
  let lvlToCheck;
  if(bonusPlus) {
      lvlToCheck = 2;
  } else {
      lvlToCheck = 1;
  }
  let index;
  if (prizeCategories[0].playLevel === lvlToCheck) {
    index = 0;
  } else {
    index = 1;
  }
  return [prizeCategories[index]];
};

You can then make the code shorter (but not necessaraly more readable) with a ternary operator :

const getFirstCategory = (bonusPlus, prizeCategories) => {
  const lvlToCheck = bonusPlus ? 2 : 1;
  let index;
  if (prizeCategories[0].playLevel === lvlToCheck) {
    index = 0;
  } else {
    index = 1;
  }
  return [prizeCategories[index]];
};

Or even two :

const getFirstCategory = (bonusPlus, prizeCategories) => {
  const lvlToCheck = bonusPlus ? 2 : 1;
  let index = (prizeCategories[0].playLevel === lvlToCheck) ? 0 : 1;
  return [prizeCategories[index]];
};

You can shorter the code even more but I would stop there (if not before).

\$\endgroup\$
1
\$\begingroup\$

You can take advantage of the fact that when adding a boolean to a number, the boolean will be coerced to 0 or 1. That lets you simplify the function down to a single expression:

const getFirstCategory = (bonusPlus, prizeCategories) => [
    prizeCategories[(prizeCategories[0].playLevel === 1 + bonusPlus) ? 0 : 1]
];

You could golf the condition ? 0 : 1, but I wouldn't recommend it.

\$\endgroup\$
  • 1
    \$\begingroup\$ Why not? It's already on the way there. (bonusPlus, prizeCategories) => [prizeCategories[1 - (prizeCategories[0].playLevel === bonusPlus+1)], correct? \$\endgroup\$ – kyrill Mar 28 '17 at 0:25
0
\$\begingroup\$

Although not strictly necessary, I would just use the Array's find function:

const getFirstCategory = (bonusPlus, prizeCategories) => {
  const bonusPlusLevel = bonusPlus ? 2 : 1;
  return prizeCategories.find( (cat) => {
    return cat.playLevel == bonusPlusLevel;
  });
};

(Borrowed partially from @kyrill)

\$\endgroup\$
  • 1
    \$\begingroup\$ This is not correct. You presume there is at least one element cat such that cat.playLevel == bonusPlusLevel, which might not be true. Moreover you presume that find searches the array from index 0 up, although this is a fairly valid assumption. \$\endgroup\$ – kyrill Mar 27 '17 at 17:51
  • 1
    \$\begingroup\$ I think all the solutions make that assumption in one way or another, though the requirements aren't entirely clear. The spec for find is to return the first match. I would love it if JS did include a bsearch style find but it doesn't :( \$\endgroup\$ – Marc Rohloff Mar 27 '17 at 22:35
  • 1
    \$\begingroup\$ None of the solutions makes either of the assumptions. BTW to do a binary search, the array would have to be sorted, which is kind of an overkill for array of two elements... \$\endgroup\$ – kyrill Mar 27 '17 at 22:50
  • \$\begingroup\$ @kyrill I suspect the assumption actually is true. "Both types are in the first and second position but sorting is not constant." would indicate you always have playLevel 1 and 2. \$\endgroup\$ – JollyJoker Mar 28 '17 at 8:15
  • 1
    \$\begingroup\$ @kyrill That's exactly the point; if my interpretation is correct, you either have prizeCategories[0].playLevel === 1 && prizeCategories[1].playLevel === 2 or prizeCategories[0].playLevel === 2 && prizeCategories[1].playLevel === 1 \$\endgroup\$ – JollyJoker Mar 28 '17 at 8:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.