how to refactor this multi-nested if else block

Question

I have this kind of multi-nested if-else block. My understanding is that there is a 'data-driven' approach that can help eliminate the need for it and trim down the code, however, I'm not experienced with it in a large way yet, so can anyone help me refactor this code to work in the 'data-driven' approach?

function (arg1) {
  if(this.thing[arg1]){
    // there is a valid property for this arg1
    if(this.thing.a){
      // there exists an 'a' propertie also
      if(this.thing.a.arg1 == arg1){
        // the a property has a property is the same as the arg1    
        // if this 'a' has a number higher than 0, avoid doing anything
        if(this.thing.a.number > 0){
        }else{
          // 'a' number was 0 or lower, so we do something
          this.thing.a = this.thing[arg1];
          this.thing.a.arg1 = arg1;
        }
      }else{
        // the' a' is not the arg1
        // so we want to use the current arg1!
        // but only if its 'number' is lower than 1
        if(this.thing.a.number > 0){
        }else{
          // 'a' number was 0 or lower, so we do something
          this.thing.a = this.thing[arg1];
          this.thing.a.arg1 = arg1;
        }
      }
    }else{
      // there is no thing.a so we set it to arg1 
      this.thing.a = this.thing[arg1];
      this.thing.a.arg1 = arg1;
    }
  }
}

I recommend this book: http://martinfowler.com/books/refactoring.html (It changed my whole coding style) — Valamas, Jul 17 '15 at 23:09

score 2 · Accepted Answer · edited Jul 17 '15 at 23:50

Pretty sure your logic boils down to this:

if (this.thing[arg1]) {
    //confirm or set a
    this.thing.a = this.thing.a ? this.thing.a : this.thing[arg1];

    //if a.arg1 is not thing[arg1] and a.number is less than 1
    if (this.thing.a.arg1 !== this.thing[arg1] && this.thing.a.number < 1) {
        this.thing.a = this.thing[arg1];
        this.thing.a.arg1 = arg1;   
    }
}

Things you should watch out for:

This:

if(someNumber > 0){
   //do nothing
} else {
   //do something
}

is never going to be right. Don't create empty blocks, change your expression, like so:

if (someNumber < 1) {
    //do something
}

You repeat this block of code 3 times. Stay DRY (don't repeat yourself)

this.thing.a = this.thing[arg1];
this.thing.a.arg1 = arg1;

If you notice you are repeating code like this, step back and look at how you can change your logical flow so that you only have to write the code once.

thanks for this! i made one small edit: in the second if nest, i want `if (this.thing.a.arg1 !== this.thing[arg1] && this.thing.a.number < 1)` , before adding that `.arg1` to `thing.a` the first time around this code runs it didn't fire my event properly, adding that in worked. PS: `if(someNumber >0){ /*do nothing*/}` actually I 'do nothing' currently but plan to add something in there — RenaissanceProgrammer, Jul 17 '15 at 23:51

how to refactor this multi-nested if else block

1 Answers1