DEV Community

Discussion on: JS Refactoring Combo: Replace Nested If-Else with Guards

Collapse
 
lgrammel profile image
Lars Grammel • Edited

While I like the object construct and use it in many cases where the values are constant, in this particular case here, it would change the runtime behavior and could lead to unnecessary overhead and bugs. With it, all three functions deadAmount(), separatedAmount(), and retiredAmount() would be called regardless of the actual value of result. Those computations have overhead, could error out, and could have side effects.

Also, in cases where the values are constant and using an object lookup would work, using ?? instead of || for the default value would be even better. If the result for the case is 0, || would trigger because 0 is falsy and normalPayAmount() would be returned, which could be a bug. With ?? this would not happen.

Collapse
 
rkallan profile image
RRKallan

Using ?? or || depends on expected behaviour.
Your argument could be also the another way. So which is better to use depends on expected behavior

Thread Thread
 
lgrammel profile image
Lars Grammel • Edited

In the original code it was function calls. ?? does not change the behavior in this case, || does if the function returns 0. I'm assuming that the functions return an amount tho, so point taken.

Thread Thread
 
rkallan profile image
RRKallan

if we look to the original code then my assumption isDead, isSeparated & isRetired are booleans.

So in the solution provided as object result needs to be set.

a possible solution could be

function getPayAmount() {
    const cases = {
        deadAmount,
        separatedAmount,
        retiredAmount,
        normalPayAmount,
    };
    let result = "normalPay";

    if (isDead) result = "dead";
    if (isSeparated) result = "separated";
    if (isRetired) result = "retired";

    return cases[`${result}Amount`]();
}

Enter fullscreen mode Exit fullscreen mode
Thread Thread
 
lgrammel profile image
Lars Grammel • Edited

Agreed. Overall this goes back to my point that an object lookup is not the best refactoring here, since there is a risk that it'll change behavior and introduce bugs. Plus it adds unnecessary cognitive complexity.

Btw, I'm a huge fan of using the object lookup pattern when there is a need for it, e.g. when there are many entries or when the entries need to be assembled. In this example tho I think simple if statements are way easier to understand.

Thread Thread
 
rkallan profile image
RRKallan

Agree that if statements with return is easier in this case

Collapse
 
lgrammel profile image
Lars Grammel

Small bonus: this variant would work without the downsides. However, I think it's harder to understand than the approach with guard clauses / early returns:

const cases = {
  isDead: deadAmount,
  isSeparated: separatedAmount,
  isRetired: retiredAmount
};
return (cases[result] ?? normalPayAmount)();
Enter fullscreen mode Exit fullscreen mode