6

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) {
  // ...
}

1 Answer 1

12

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
  • 5
    +1 but would also add that if (shouldShowSpinner()){ is alot more readable than if (fsm.state === 'fetching' && isEmpty(listNode)) {. Jan 5, 2017 at 3:20
  • @Maybe_Factor: if (shouldShowSpinner) would also be easy to read, if it actually worked. Jan 5, 2017 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, 2017 at 16:10
  • Additionally, shouldShowSpinner() is more testable, especially if the criteria for showing spinners changes.
    – Doug
    Jan 12, 2017 at 0:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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