CODEWITHSUNDEEP
javascriptjquery
copenndthagen
copenndthagen

Reputation: 50732

Optimize JavaScript if else code

Is there a better way of writing the below JS code?

var check1 = model1.validateAll();
var check2 = model2.validateAll();

if (check1.isValid === false || check2.isValid === false) {
    self.showValidationErrors(check1.messages);
    self.showValidationErrors(check2.messages);
    return true;
}

My only concern is once inside the if block, either check1.messages OR check2.messages can have some value (depending on check1.isValid OR check2.isValid is false)

Upvotes: 0

Views: 95

Answers (5)

Werner
Werner

Reputation: 2154

I'd do it like this but only if it's not necessary to check both check-objects in order to write a validation message:

var check1 = model1.validateAll();
var check2 = model2.validateAll();

self.showValidationErrors(check1);
self.showValidationErrors(check2);

return !check1.isValid || !check2.isValid;

And to the showValidationError(check)-function adding this to the first row

if (check.isValid) return;

I don't think you have to write both messages if just one validation isn't valid, right? If I'm wrong than this way is of course not the best one.

Upvotes: 0

T.J. Crowder
T.J. Crowder

Reputation: 1074178

First, using === false or === true is very rarely necessary. Typically you just test like this: !check1.isValid There are use cases for the === false form, but they're rare.

The simple thing here is:

if (!check1.isValid && !check2.isValid) {
    if (!check1.isValid) {
        self.showValidationErrors(check1.messages);
    } 
    if (!check2.isValid) {
        self.showValidationErrors(check2.messages);
    } 
    return true;
}

Or if you're in an ES5 environment (and what we have below can be shimmed):

var invalid = false;
[check1, check2].forEach(function(chk) {
    if (!chk.isValid) {
        self.showValidationErrors(chk.messages);
        invalid = true;
    }
});
if (invalid) {
    return true;
}

That may seem inefficient, but as I assume this is in response to a user action, the few milliseconds involved are a non-issue.

Upvotes: 1

chavakane
chavakane

Reputation: 246

I would create a function, that way you won't repeat yourself (DRY).

function validateModel(model, self)
{
   var check = model.validateAll();
   if(check === false){
      self.showValidationErrors(check.messages);
   }
}

Then you would just call it:

validateModel(model1, self);
validateModel(model2, self);

Upvotes: 0

MaxArt
MaxArt

Reputation: 22617

For something totally different:

return [model1, model2].map(function(model) {
    var check = model.validateAll();
    if (!check.isValid) this.showValidationErrors(check.messages);
    return check.isValid;
}, self).indexOf(false) > -1;

Check for the meaning of the second argument of map.

Keep in mind that array methods like map and indexOf aren't supported by IE8 and lower, but they can easily be polyfilled.

Upvotes: 0

Barmar
Barmar

Reputation: 780861

Use separate if blocks. Instead of returning immediately from the block, set a variable and return it when all blocks are done.

returnVal = false;
if (check1.isValid) {
    self.showValidationErrors(check1.messages);
    returnVal = true;
}
if (check2.isValid) {
    self.showValidationErrors(check2.messages);
    returnVal = true;
}
return returnVal;

Also, when you find yourself creating variables with names like check1, check2, etc, and doing the same thing with all of them, you probably should be using an array and looping over them.

Upvotes: 0

Related Questions