I was wondering on how I can refactor that? I'm repeating myself I feel this is not the best way to write it :

if (operator === "+") {
    strength += 2;
    up = 4 * strength;
    if (up > 40) up = 40;
    final.base += up;
} else if (operator === "-") {
    up = 4 * strength;
    if (up > 40) up = 40;
    final.base -= up;
    strength -= 2;
}

I don't really see a way to properly refactor that since position is important. Is there a way to clean this function?

I was wondering on how I can refactor that? I'm repeating myself I feel this is not the best way to write it :

if (operator === "+") {
    strength += 2;
    up = 4 * strength;
    if (up > 40) up = 40;
    final.base += up;
} else if (operator === "-") {
    up = 4 * strength;
    if (up > 40) up = 40;
    final.base -= up;
    strength -= 2;
}

I don't really see a way to properly refactor that since position is important. Is there a way to clean this function?

• javascript

• 2 The usual way to refactor out a large else-if ladder is to loop through an associative array. – Robert Harvey Commented Feb 8, 2019 at 20:42
• Have a look here: blog.wax-o./2015/05/… – Robert Harvey Commented Feb 8, 2019 at 20:43
• If nothing else, the if (up > 40) up = 40; doesn't rely on the conditional at all. – jhpratt Commented Feb 8, 2019 at 20:44
• 2 After some refactoring i think your original code is better. – Salman Arshad Commented Feb 8, 2019 at 20:45
• 1 stackoverflow./questions/5834318/… – Teemu Commented Feb 8, 2019 at 20:50

7 Answers 7

You could write it more pact, if you do not use up later by using Math.min.

if (operator === "+") {
    strength += 2;
    final.base += Math.min(40, 4 * strength);
} else if (operator === "-") {
    final.base -= Math.min(40, 4 * strength);
    strength -= 2;
}

My answer is not refactoring of if..else - it's about thinking ahead how Your app can grow, it's about making right choices.

In large apps with plex logic You'll have to abstract methods to make You code more flexible.

For example how about having Operations class that abstracts if..else switch, which You can extend?

class Operations {
  static plus (base, strength) {
    base = parseInt(base);
    strength = parseInt(strength);
    
    strength += 2;
    base += Math.min(40, 4 * strength);
    
    return [base, strength];
  }
  
  static minus (base, strength) {
    base = parseInt(base);
    strength = parseInt(strength);
    
    base -= Math.min(40, 4 * strength);
    strength -= 2;
    
    return [base, strength];
  }

  static do (operation) {
    const operators = {
      '+' : Operations.plus,
      '-' : Operations.minus
    }

    const args = Object.values(arguments).slice(1);
    
    if (!operators[operation]) {
      return args;
    }

    return operators[operation].apply(null, args);
  }
}

   
let final = {base: 10};
let strength = 10;
let newBase, newStrength;

console.log('Before. base:', final.base, 'strength:', strength);

// NO IF ELSE ON OPERATOR (:
[newBase, newStrength] = Operations.do('+', final.base, strength);
strength = newStrength;
final.base = newBase;

console.log('After "+" operation. base:', final.base, 'strength:', strength);

[newBase, newStrength] = Operations.do('-', final.base, strength);
strength = newStrength;
final.base = newBase;

console.log('After "-" operation. base:', final.base, 'strength:', strength);

I will do something like this, for preserve up variable:

if (operator === "+")
{
    up = Math.min(4 * (strength += 2), 40);
    final.base += up;
}
else if (operator === "-")
{
    final.base -= (up = Math.min(4 * strength, 40));
    strength -= 2;
}

If you don't need up variable, can be simplified to this:

if (operator === "+")
{
    final.base += Math.min(4 * (strength += 2), 40);
}
else if (operator === "-")
{
    final.base -= Math.min(4 * strength, 40);
    strength -= 2;
}

If you don't need up variable and also you only have + and - operators, then you can go like this:

strength += (operator === "+") ? 2 : 0;
final.base += (operator === "+" ? 1 : -1) * Math.min(4 * strength, 40);
strength -= (operator === "-") ? 2 : 0;

To solve the duplication problem, you can add a multiplier factor since the majority of what's changing here is just the sign.

let multiplier = 1;
if (operator === "-")
    multiplier = -1;

up = 4 * strength;
strength += multiplier * 2;
if (up > 40) up = 40;
final.base += multiplier * up;

Note

This will only work if operator is either - or +. If it's something like *, this will act as if the operator is +

you could put the operations in an object :

const obj = {
    "+" : {
        strength : prevStrength => prevStrength + 2,
        finalBase: (prevFinalBase , up) =>  prevFinalBase + Math.min(40, 4 * strength)
    },  
    "-" : {
        strength : prevStrength => prevStrength - 2,
        finalBase: (prevFinalBase , up) => prevFinalBase - Math.min(40, 4 * strength)
    }
}

strength = obj[operator].strength(strength);
finalBase = obj[operator].finalBase(finalBase);

var operator = "+";
var strength = 3;
var finalBase = 5;

const obj = {
  "+": {
    strength: prevStrength => prevStrength + 2,
    finalBase: (prevFinalBase, up) => prevFinalBase + Math.min(40, 4 * strength)
  },
  "-": {
    strength: prevStrength => prevStrength - 2,
    finalBase: (prevFinalBase, up) => prevFinalBase - Math.min(40, 4 * strength)
  }
}

strength = obj[operator].strength(strength);
finalBase = obj[operator].finalBase(finalBase);

console.log({
  strength,
  finalBase
})

You could consider writing the logic as a series of conditional ternary operator statements:

let isAdd = operator === '+'
strength += isAdd ? 2 : 0;
up = 4 * strength;
if(up > 40) up = 40;
final.base += isAdd ? up : (-1 * up)
strength -= isAdd ? 0 : 2;

How about this ?

 /*ASCII Code For "-" = 45, "+" = 43*/
 operator = (44-operator.charCodeAt(0));
 if(operator > 0) strength += 2;
 up = 4*strength;
 if(up > 40) up = 40;
 final.base = final.base + (operator*up);
 if(operator < 0) strength -= 2;