3

According to this clean-code guide you should encapsulate conditionals:

function shouldShowSpinner() {
  return fsm.state === 'fetching' && isEmpty(listNode);
}

if (shouldShowSpinner()) {
  // ...
}

Why not just write:

const shouldShowSpinner = fsm.state === 'fetching' && isEmpty(listNode)

if (shouldShowSpinner) {
  // ...
}
10

Because

const shouldShowSpinner = fsm.state === 'fetching' && isEmpty(listNode)

is only evaluated once. The assignment operator = stores the result of a single evaluation of the boolean expression.

This is fine if the state of the boolean expression never changes during code execution in the relevant scope, but will produce wrong results if it does change, in which case you will get the now inaccurate result of the original calculation.

  • 4
    +1 but would also add that if (shouldShowSpinner()){ is alot more readable than if (fsm.state === 'fetching' && isEmpty(listNode)) {. – Maybe_Factor Jan 5 '17 at 3:20
  • @Maybe_Factor: if (shouldShowSpinner) would also be easy to read, if it actually worked. – Robert Harvey Jan 5 '17 at 15:57
  • And the corollary to this is, if you want to correctly check if (shouldShowSpinner) later on, you wind up rewriting this code each time you need to check it. That's the perfect candidate for encapsulation. – mmathis Jan 5 '17 at 16:10
  • Additionally, shouldShowSpinner() is more testable, especially if the criteria for showing spinners changes. – Doug Jan 12 '17 at 0:56

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.