Javascript - Replace lots of if statements

Question

I have a a couple of different radio buttons which return an ethnicity and a gender. The script runs inside an internal application so rather than returning "boy", "girl" or "both" I get back 7707330, 7707333, and 7707336. Similar from the ethnicity radio button.

I then need to validate data based on the combonation of ethnicity and gender. This was a pretty simple task but I have ended up with 15 if statements! It all works as it should, but there must be a cleaner solution?

function test(radioResults) {
    var1 = radioResults[0].toString();
    var2 = radioResults[1].toString();

    var roll = parseFloat(parent.roll);

    if (var2 == '7707330') {
        gender = 'boy';
    }
    if (var2 == '7707333') {
        gender = 'girl';
    }
    if (var2 == '7707336') {
        gender = 'both';
    }

    if (var1 == '7707341') {
        maori(gender);
    }
    if (var1 == '7707344') {
        pasifika(gender);
    }
    if (var1 == '7707347') {
        all(gender);
    }
}

function maori(gender) {
    //Maori 
    if (gender == 'boy') {
        ethnicity = parseFloat(parent.getMBoys);
        validation(ethnicity);
    }
    if (gender == 'girl') {
        ethnicity = parseFloat(parent.getMGirls);
        validation(ethnicity);
    }
    if (gender == 'both') {
        ethnicity = parseFloat(parent.getTotalM);
        validation(ethnicity);
    }
}

function pasifika(gender) {
    //Pasifika
    if (gender == 'boy') {
        ethnicity = parseFloat(parent.getPBoys);
        validation(ethnicity);
    }
    if (gender == 'girl') {
        ethnicity = parseFloat(parent.getPGirls);
        validation(ethnicity);
    }
    if (gender == 'both') {
        ethnicity = parseFloat(parent.getTotalP);
        validation(ethnicity);
    }
}

function all(gender) {
    //All
    if (gender == 'boy') {
        ethnicity = parseFloat(parent.getBoys);
        validation(ethnicity);
    }
    if (gender == 'girl') {
        ethnicity = parseFloat(parent.getGirls);
        validation(ethnicity);
    }
    if (gender == 'both') {
        ethnicity = parseFloat(parent.getTotalRoll);
        validation(ethnicity);
    }
}

function validation(ethnicity) {

    percent = ethnicity * 5 / 100;

    if (ethnicity - percent > roll || (ethnicity + percent < roll)) {
        parent.document.getElementById('CF_7784443').value = "FAIL";
    } else {
        parent.document.getElementById('CF_7784443').value = "PASS";
    }
}

How would I go about cleaning this up?

FYI, there is a new Stack Exchange site specifically for Code Reviews... codereview.stackexchange.com Just a heads up :) — jcreamer898
– jcreamer898, Commented Apr 26, 2012 at 3:44

McGarnagle · Accepted Answer · 2012-04-26 04:28:27Z

7

I'm not sure if this is to your taste, but it seems like a cleaner solution to me. Lay out all the mappings separately first. Then resolve the mappings to the correct property of parent. Then invoke validation, just once, for the property you've retrieved. Here's a sketch:

// map var1 and var2 to gender and ethnicity
var genderMap = { "7707330" : "boy", "7707333": "girl", "7707336": "both" };
var ethnicityMap = { "7707341": "maori", "7707344": "pasifica", "7707347": "all" };

// map gender and ethnicity to the correct property of "parent"
var props = 
    { "boy": 
        {
            "maori": "getMBoys", "pacifica": "getPBoys", "all": "getBoys"
        },
        "girl": 
        {   
            "maori": "getMGirls", "pacifica": "getPGirls", "all": "getGirls"
        },
        "all":
        {   
            "maori": "getTotalM", "pacifica": "getTotalP", "all": "getTotalRoll"
        }
    };

// get the correct property    
var prop = props[genderMap[var1]][ethnicityMap[var2]];

// run the validation
validation(parseFloat(parent[prop]));

edited Apr 26, 2012 at 4:28

answered Apr 26, 2012 at 3:56

McGarnagle

103k31 gold badges235 silver badges264 bronze badges

Sign up to request clarification or add additional context in comments.

6 Comments

Chris Over a year ago

Oh nice... That is more what I was looking for. Let me just see if I can get my head around it ;)

Chris Over a year ago

var prop = props[genderMap[var1]][ethnicityMap[var2]]; is giving me an undefined error... I assume that var1 and var2 can stay how they originally were?

McGarnagle Over a year ago

@user1011926 Yes... I just noticed a typo on the second line, I misspelled ethnicityMap ...

Chris Over a year ago

Yeah I also picked up and corrected that, wasn't the issue though sadly 'undefined' is null or not an object

McGarnagle Over a year ago

Ok, there were actually two typos in the same word ("enthicity" or something)... now I'm able to test with var var1 = "7707330", var2 = "7707341";*, which makes prop = getMBoys (I can't test the last line as I don't know what parent is).

|

Pat Wangrungarun · Accepted Answer · 2012-04-26 03:53:10Z

1

Use switch may help

switch (var2) {  
   case '7707330': gender = 'boy'; break;  
   case '7707333': gender = 'girl'; break;  
   case '7707336': gender = 'both'; break;
   default: gender = 'not boy or girl here?';
}

Document here in MDN

Edit: shrink newlines.

edited Apr 26, 2012 at 3:53

answered Apr 26, 2012 at 3:46

Pat Wangrungarun

5364 silver badges12 bronze badges

Comments

Blender · Accepted Answer · 2012-04-26 03:51:54Z

0

I'd code it like this. Some people may suggest switch statements, but I've truthfully never used them in my code, as they increase the line count without impacting readability:

function foo() {
  var func;

  if (var1 == '7707330') {
    func = maori;
  } else if (var1 == '7707344') {
    func = pasifika;
  } else if (var1 == '7707347') {
    func = all;
  }

  if (var2 == '7707330') {
    func('boy');
  } else if (var2 == '7707333') {
    func('girl');
  } else if (var2 == '7707336'):
    func('both');
  }
}

function maori(gender) {
  //Maori 
  if (gender == 'boy') {
    ethnicity = parseFloat(parent.getMBoys);
  } else if (gender == 'girl') {
    ethnicity = parseFloat(parent.getMGirls);
  } else if (gender == 'both') {
    ethnicity = parseFloat(parent.getTotalM);
  }

  validation(ethnicity);
}

function pasifika(gender) {
  //Pasifika
  if (gender == 'boy') {
    ethnicity = parseFloat(parent.getPBoys);
  } else if (gender == 'girl') {
    ethnicity = parseFloat(parent.getPGirls);
  } else if (gender == 'both') {
    ethnicity = parseFloat(parent.getTotalP);
  }

  validation(ethnicity);
}

function all(gender) {
  //All
  if (gender == 'boy') {
    ethnicity = parseFloat(parent.getBoys);
  } else if (gender == 'girl') {
    ethnicity = parseFloat(parent.getGirls);
  } else if (gender == 'both') {
    ethnicity = parseFloat(parent.getTotalRoll);
  }

  validation(ethnicity);
}

function validation(ethnicity) {
  var percent = ethnicity * 5 / 100;

  if (ethnicity - percent > roll || (ethnicity + percent < roll)) {
    parent.document.getElementById('CF_7784443').value = "FAIL";
  } else {
    parent.document.getElementById('CF_7784443').value = "PASS";
  }
}

answered Apr 26, 2012 at 3:51

Blender

300k55 gold badges462 silver badges511 bronze badges

3 Comments

sgowd Over a year ago

Good thing about this, as you said is readability isn't effected.

ajax333221 Over a year ago

just on the border to not use switch, if they would have been 4+, then a switch is undoubtedly better in all ways (including performance, but for 3- if+if elses are better). However, I am not upvoting this

Blender Over a year ago

@ajax333221: Can you justify your performance benefit? I'm pretty sure most JavaScript engines optimize switch statements to if...else blocks in the end.

Ricardo Garza V. · Accepted Answer · 2012-04-26 03:48:52Z

-1

well, in terms of clean code you could exchage your if's for Switch statements, here is a description:

http://www.w3schools.com/js/js_switch.asp

well that is the best substitution you can get. of course your code is also valid.

answered Apr 26, 2012 at 3:48

Ricardo Garza V.

9091 gold badge11 silver badges26 bronze badges

Collectives™ on Stack Overflow

Javascript - Replace lots of if statements

4 Answers 4

6 Comments

Comments

3 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

6 Comments

Comments

3 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related