5
\$\begingroup\$

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();
    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";
    }
}
\$\endgroup\$
6
\$\begingroup\$

One option is to have a map from key -> value

for example:

var var2 = radioResults[1].toString();
var gender = {'7707330': 'boy',
               '7707333': 'girl',
               '7707336': 'both'}[var2];

You could even put functions as the values. For example:

var var1 = radioResults[0].toString()
{'7707341': maori,
 '7707344': pasifika,
 '7707347': all}[var1](gender);
\$\endgroup\$
3
\$\begingroup\$

As Michael Deardeuff has already pointed out, in this particular case, maps are a good answer.

But the generic answer to "I have a lot of if (var == value) statements" is "use switch (var)." A switch reads better - it lets you read a single path and ignore the rest, knowing they won't be used for this value. For example:

function test(radioResults) {

    roll = parseFloat(parent.roll);   // This is unused
    switch (radioResults[1].toString())
    {
        case '7707330':
            gender = 'boy';
            break;
        case '7707333':
            gender = 'girl';
            break;
        case '7707336':
            gender = 'both';
            break;
        default:
            /* Your code ignores the possibility of an unexpected value */
    }

    switch (radioResults[0].toString())
    {
        case '7707341':
            maori(gender);
            break;
        case '7707344':
            pasifika(gender);
            break;
        case  '7707347':
            all(gender);
            break;
        default:
            /* Your code ignores the possibility of an unexpected value */
    }
}

Or, in this simple case, even:

function test(radioResults) {

    switch (radioResults[1].toString())
    {
        case '7707330': gender = 'boy'; break;
        case '7707333': gender = 'girl'; break;
        case '7707336': gender = 'both'; break;
    }

    switch (radioResults[0].toString())
    {
        case '7707341': maori(gender); break;
        case '7707344': pasifika(gender); break;
        case '7707347': all(gender); break;
    }
}
\$\endgroup\$
2
\$\begingroup\$

As the other two commenters have noted, there are simpler ways of expressing those if-statements. However, I think there is something more important that needs to be addressed in your code: organisation. You've already made some good progress at separating out logic into small, contained functions. However there are two issues I immediately notice:

  1. A function should try to follow the Single Responsibility Principle – it should only focus on one thing.
  2. Your functions rely on a the global state of several variables, this can cause many unexpected bugs and make them hard to track down – pass your functions everything they need to know.

So, with those two things in mind, this is a better way to write your code using a similar style:

function validate(parent, radioResults) {
  var gen = gender(radioResults[0].toString()),
      eth = ethnicity(radioResults[1].toString()),
      mult = multiplier(parent, gen, eth),
      roll = parseFloat(parent.roll, 10),
      element = parent.document.getElementById('CF_7784443');

  if (isValidPercentage(mult, roll))
    element.value = 'PASS';
  else
    element.value = 'FAIL';
}

function gender(val) {
  switch(val) {
    case '7707330':
      return 'boy';

    case '7707333':
      return 'girl';

    case '7707336':
      return 'both';

    default:
      // Nadda
  }
}

function ethnicity(val) {
  switch(val) {
    case '7707341':
      return 'maori';

    case '7707344':
      return 'pasifika';

    case '7707347':
      return 'all';

    default:
      // Nadda
  }
}

function multiplier(parent, gender, ethnicity) {
  switch(ethnicity) {
    // All
    case 'all':
      if (gender == 'boy')
        return parseFloat(parent.getBoys, 10);
      else if (gender == 'girl')
        return parseFloat(parent.getGirls, 10);
      else
        return parseFloat(parent.getTotalRoll, 10);

    // Maori
    case 'maori':
      if (gender == 'boy')
        return parseFloat(parent.getMBoys, 10);
      else if (gender == 'girl')
        return parseFloat(parent.getMGirls, 10);
      else
        return parseFloat(parent.getTotalM, 10);

    // Pasifika
    case 'pasifika':
      if (gender == 'boy')
        return parseFloat(parent.getPBoys, 10);
      else if (gender == 'girl')
        return parseFloat(parent.getPGirls, 10);
      else
        return parseFloat(parent.getTotalP, 10);

    default:
      // Nadda
  }
}

function isValidPercentage(multiplier, roll) {
  var percent = multiplier * 5 / 100;

  return ! (multiplier - percent > roll || (multiplier + percent < roll));
}

As you can see I have renamed some of the functions to more accurately represent their purpose, I have ensured that I pass everything I need into my functions as arguments instead of using global variables, and finally I have made each function responsible for a single task.

\$\endgroup\$
0
\$\begingroup\$

Tip 1, don't do something with every if if it isn't necessary:

if (gender == 'boy') {
    ethnicity = parseFloat(parent.getMBoys);
    //why run...      validation(ethnicity);
}
if (gender == 'girl') {
    ethnicity = parseFloat(parent.getMGirls);
    //this line...    validation(ethnicity);
}
if (gender == 'both') {
    ethnicity = parseFloat(parent.getTotalM);
    //on every if?     validation(ethnicity);
}
validation(ethnicity); //move it outside here :)

Tip 2, when you have multiple ifs of the same thing that can't be true for more than 1, use if...else ifs, why would you continue checking them if they already matched something?

if (gender == 'boy') {}
//in case it is 'boy', why run this -> if (gender == 'girl') {}
else if (gender == 'girl') {}
//else if (...) {...}

Tip 3, merge similar functions when possible, maori(), pasifika() and all() are almost identical, so why not use a single function that accept a parameter for every one of them?, example 1->maori, 2->pasifika and 3->all, so you just need to check if it is 1,2 or 3 and do stuff with them accordingly

Tip 4, if you are going to apply something to all of them, you don't need to apply them on every if, you can (and should) wait until they are finished, and then apply the change

if (...) {
    //why use...   ethnicity = parseFloat(parent.getBoys);
   ethnicity = parent.getBoys;
}
if (...) {
    //parseFloat...   ethnicity = parseFloat(parent.getGirls);
    ethnicity = parent.getGirls;
}
if (...) {
    //on each one?   ethnicity = parseFloat(parent.getTotalRoll);
    ethnicity = parent.getTotalRoll;
}
ethnicity = parseFloat(ethnicity); //do the change

I numbered the tips in case you need further details on a specific tip

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.