I have function with data and callback parameters. The data is an object with many attributes and I have to validate these. I wrote multiple if, else if statements to do it, but it seems so disgusting to me.

function(data, callback) {

 if (data.a != 'x') {
   logger.log(...);
   return callback({status: false, code: 'x'});
 } else if (data.b != 'y') {
   logger.log(...);
   return callback({status: false, code: 'y'});
 } else if (data.c != 'z') {
   logger.log(...);
   return callback({status: false, code: 'z'});
 } else if (data.d != 'w') {
   logger.log(...);
   return callback({status: false, code: 'w'});
 }

 //... logic ...
 return callback({status: true});
}

I think It's not the appropriate way to do it.

I have function with data and callback parameters. The data is an object with many attributes and I have to validate these. I wrote multiple if, else if statements to do it, but it seems so disgusting to me.

function(data, callback) {

 if (data.a != 'x') {
   logger.log(...);
   return callback({status: false, code: 'x'});
 } else if (data.b != 'y') {
   logger.log(...);
   return callback({status: false, code: 'y'});
 } else if (data.c != 'z') {
   logger.log(...);
   return callback({status: false, code: 'z'});
 } else if (data.d != 'w') {
   logger.log(...);
   return callback({status: false, code: 'w'});
 }

 //... logic ...
 return callback({status: true});
}

I think It's not the appropriate way to do it.

• javascript
• node.js

• 1 Maybe use a switch? – zozo Commented Feb 20, 2017 at 14:26
• 5 @zozo He's paring different properties in each one, so switch is out. – Useless Code Commented Feb 20, 2017 at 14:27
• 6 It seems disgusting, but actually looks quite fine to me to be fair. – roberrrt-s Commented Feb 20, 2017 at 14:28
• 4 Also the final callback is called no matter which if gets executed. Maybe there should be an else before it – Soubhik Mondal Commented Feb 20, 2017 at 14:31
• 1 I'm not a huge fan of it but you could do something like this, just giving you options but I prefer georg's or BlazeSahlzen's answer better, for no other reason than they look 'cleaner' – George Commented Feb 20, 2017 at 14:40

5 Answers 5

One way would be to factor the validation into a separate function:

function failureCode(data) {
    if (data.a != 'x')
        return 'x';
    if (data.b != 'y')
        return 'y';
    if (data.c != 'z')
        return 'z';
}

function (data, callback) {
    var code = failureCode(data);
    if (code) {
        logger.log(...);
        return callback({status: false, code: code});
    }

    //... logic ...
}

Also, don't forget to return from the "failed" branch.

How about this? Since you are actually doing a validation and a "submission" you can just throw the codes and catch the result.

function(data, callback) {
    try {
         if (data.a != 'x') {
             throw 'x';
         }

         if (data.b != 'y') {
             throw 'y';
         }

         // etc

         // logic

         callback({status: true});
    }
    catch(code) {
        callback({status: false, code: code});
    }
}

Since it seems the code is correct but needs to be pleasing to the eyes, here's something you can try:

function(data, callback) {

  var code;

  if (data.a != 'x') code = 'x';
  else if (data.b != 'y') code = 'y';
  else if (data.c != 'z') code = 'z';
  else if (data.d != 'w') code = 'w';

  if (code) {
    callback({ status: false, code: code });
  } else {
    callback({ status: true });
  }
}

If the parisons are known beforehand, then:

function(data, callback) {

  var code,

    parisons = [{
      key: 'a',
      val: 'x'
    }, {
      key: 'b',
      val: 'y'
    }, {
      key: 'c',
      val: 'z'
    }, {
      key: 'd',
      val: 'w'
    }];

  for (each of parisons) {

    if (data[each.key] != each.val) {
      code = each.val;
      break;
    }
  }

  if (code) {
    callback({ status: false, code: code });
  } else {
    callback({ status: true });
  }
}

There is no easy way to do this because you're reading from different fields on data, and using inequalities. If you were reading from the same key on data (call it code), you could do the following in idiomatic javascript:

function (data, callback) {
  switch(data.code) {
      case 'x':
          logger.log(...);
          callback({status: false, code: 'x'});
          break;
      case 'y':
          logger.log(...);
          callback({status: false, code: 'y'});
          break;
      case 'z':
          logger.log(...);
          callback({status: false, code: 'z'});
          break;
      case 'w':
          logger.log(...);
          callback({status: false, code: 'w'});
          break;
      default:
          callback({status: true})
   }
}

Note that the switch statement is actually fairly distinct from your original functionality. First of all it is a truthy assertion on value, which results in only one branch every running. E.g. in your original code if data were an object like so:

const data = {a: 'a', b: 'b', c: 'c', d: 'd'} every branch would run, while in the switch implementation only the default branch would run. In either case, I would encourage you to rearchitect your solution for ease of reasoning into a shape that fits the switch.

If you are ok with using ES6 syntax/object destructuring, you could hook up your project with a transpiler like babel, and still use multiple keys like your original implementation, to do the following

function ({a, b, c, d}, callback) {
  if (a != 'x') {
    logger.log(...);
    callback({status: false, code: 'x'});
  } else if (b != 'y') {
    logger.log(...);
    callback({status: false, code: 'y'});
  } else if (c != 'z') {
    logger.log(...);
    callback({status: false, code: 'z'});
  } else if (d != 'w') {
    logger.log(...);
    callback({status: false, code: 'w'});
  }

  //... logic ...
  callback({status: true});
}

With a little bit more preconfiguration you could do something like:

const keys = ['a', 'b', 'c', 'd']
const codes = {'a': 'x', 'b': 'y', 'c': 'z', 'd': 'w'}

Your function could now be:

function(data, callback) {
  keys.map(function(key) {
    if (data[key] != codes[key]) {
      logger.log(...);
      callback({status: false, code: codes[key]});
    }})
    //logic
    callback({status: true})
    }

note you could pass in the codemap / keylist as a function argument if you so wish

I would include a set of expected values in my validation routine:

function(data, expected, callback) {
  var checkProp = function(prop) {
    return (data[prop] === expected[prop]);
  };

  for (var p in data) {
    if (data.hasOwnProperty(p)) {
      if (!checkProp(p)) {
        callback({status: false, code: expected[p]});
        return;
      }
    }
  }
  callback({status: true});
}

So, for example, if I had an object:

{ a: 1, b: 2}

I might include an expected parameter like this:

{ a: 3, b: 4 }

Here's a working demo:

function validate(data, expected, callback) {
  var checkProp = function(prop) {
    return (data[prop] === expected[prop]);
  };

  for (var p in data) {
    if (data.hasOwnProperty(p)) {
      if (!checkProp(p)) {
        callback({status: false, code: expected[p]});
        return;
      }
    }
  }
  callback({status: true});
}

var obj = { a: 1, b: 2 },
  expected = { a: 1, b: 4 };
validate(obj, expected, function(res) {
  if (!res.status) {
    console.log('failed with value ' + res.code);
  } else {
    console.log('success');
  }
});