Optimize JavaScript if else code

Question

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)

Because in the end I want to return true for any of the if statements...and if both have errors, then the 2nd if block would not get executed — copenndthagen
– copenndthagen, Commented Apr 17, 2014 at 7:16
What is the value of check1.messages if check1.isValid === false — xiidea
– xiidea, Commented Apr 17, 2014 at 7:26

T.J. Crowder · Accepted Answer · 2014-04-17 07:23:07Z

1

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.

edited Apr 17, 2014 at 7:23

answered Apr 17, 2014 at 7:17

T.J. Crowder

1.1m201 gold badges2k silver badges2k bronze badges

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

1 Comment

Satpal Over a year ago

I personally will like go with the simple thing here

Barmar · Accepted Answer · 2014-04-17 07:19:37Z

0

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.

answered Apr 17, 2014 at 7:19

Barmar

789k57 gold badges555 silver badges669 bronze badges

Comments

MaxArt · Accepted Answer · 2014-04-17 07:21:02Z

0

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.

answered Apr 17, 2014 at 7:21

MaxArt

22.8k10 gold badges85 silver badges82 bronze badges

Comments

chavakane · Accepted Answer · 2014-04-17 07:22:56Z

0

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);

answered Apr 17, 2014 at 7:22

chavakane

2461 silver badge6 bronze badges

Comments

Werner · Accepted Answer · 2014-04-17 07:40:28Z

0

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.

edited Apr 17, 2014 at 7:40

answered Apr 17, 2014 at 7:33

Werner

2,1743 gold badges24 silver badges34 bronze badges

Collectives™ on Stack Overflow

Optimize JavaScript if else code

5 Answers 5

1 Comment

Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

1 Comment

Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related