3

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?

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

7 Answers 7

9

You could write it more compact, 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;
}
Sign up to request clarification or add additional context in comments.

6 Comments

This is an elegant idea and it respects position, thanks!
@FlickaMin if you want to update up and increment base at the same time, final.base += up = Math.min(40, 4 * strength)
The second approach won't work, note that final.base is not always incremented...
@Shidersz, that's why i added a disclaimer.
I think,you do not understand me. I believe you forgot to replace final.base += Math.min(40, 4 * strength); by this final.base = final.base + (operator === "+" ? Math.min(40, 4 * strength) : -Math.min(40, 4 * strength)); or maybe by this: final.base += (operator === "+" ? 1 : -1) * Math.min(40, 4 * 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 complex 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);

Comments

2

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;

Comments

1

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 +

2 Comments

As I said just before, position is really important so I can't do something like this. If the operator is "plus" I must get the up after incrementing, if the operator is "minus" it's after.
yeah you're right @num8er. This will only work if the operator is either + or -. Good catch, I'm adding an edit
1

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
})

Comments

0

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;

1 Comment

True! I think there are much better answers above!
0

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;

Comments

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.