Is there a better way to write this mutiple or conditional?

Question

I have the following IF statement in javascript:

if ( !(cmd === 'JustifyLeft' || cmd === 'JustifyRight' || cmd === 'JustifyCenter' || cmd === 'JustifyFull') )

Any suggestions on how it could be written in a cleaner way?

Thanks

Better how? To make it . . . Faster? Clearer? Less code? More Generic? More Specific? What do you think is wrong with it? — Binary Worrier, Jun 09 '09 at 07:35
The if-statement looks just fine to me. Perhaps some line-break formatting would improve readability. — cllpse, Jun 09 '09 at 08:04
I think this question is more appropriate for codereview.stackexchange.com — Phil C, Mar 12 '11 at 10:57

scragar · Answer 1 · 2009-06-09T07:41:35.113

17

if(!cmd.match(/^Justify(Left|Right|Center|Full)$/))

In response to a few comments you can also mimic your strict comparison with a small edit:

if( typeof cmd != 'String' || !cmd.match(/^Justify(Left|Right|Center|Full)$/))

This will react in the exact same way as your current code, ignoring anything that's not a string.

Personally I think it is highly unlikely that you will need it.

edited Jun 09 '09 at 07:41

answered Jun 09 '09 at 07:32

scragar

6,764
28
36

1

Exactly the kind of answers we need here :) +1 from me – Rob Cooper Jun 09 '09 at 07:34
I was thinking regex as well, but in the sample above the strict equality operator (===) is used and I guess that the regex does not make that kind of strict comparison. In this case I can't see how it would matter though. +1 – Fredrik Mörk Jun 09 '09 at 07:35
The === operator is for type-strict comparison. The above would fail if cmd was not a string. One could write this to avoid that though: if(typeof(cmd) == 'string' && !cmd.match(/^Justify(Left|Right|Center|Full)$/)) – Blixt Jun 09 '09 at 07:42
'Some people, when confronted with a problem, think "I know, I'll use regular expressions." Now they have two problems.' [Jamie Zawinski] – Will Jun 09 '09 at 07:45
Sorry, I meant if (typeof(cmd) != 'string' || ...) – Blixt Jun 09 '09 at 07:48
Voted down: Depending on how often the if-statement is run, this is a potential performance drain. – cllpse Jun 09 '09 at 08:02
Personally I wouldn't consider regex for a 'simple' if statement. Now I'm waiting for someone to provide an XML based alternative. – Onots Jun 09 '09 at 08:48
I agree that this is not an efficient way, but I didn't vote it down simply for its elegance. Personally I'd still use the switch-statement however. – Blixt Jun 09 '09 at 09:09
I'd recommend using a non-capturing group in the regex: /^Justify(?:Left|Right|Center|Full)$/ – Helen Jun 09 '09 at 12:24

score 4 · Answer 2 · answered Jun 09 '09 at 07:36

4

This sounds like a good situation to use a switch. Just be aware that switches only do equality checking (==) not identity checking (===), though this should be fine.

switch (cmd) {
    case "JustifyLeft" :
    case "JustifyRight" :
    case "JustifyCenter" :
    case "JustifyFull" :
        // do something
    break;
    case "somethingElse" :
    default:
        // do something else
    break;
}

answered Jun 09 '09 at 07:36

nickf

537,072
198
649
721

This is a more efficient way than using regular expressions so +1 for that. But what is the case "somethingElse" doing there? No matter what, it will never alter the behavior of the switch. – Blixt Jun 09 '09 at 07:47
In case it isn't obvious to the reader, the case "somethingElse" isn't actually needed here. – Miles Jun 09 '09 at 07:48
1

yeah - i was just showing how you'd do the equivalent of an else / else if. – nickf Jun 09 '09 at 07:51

score 1 · Answer 3 · answered Jun 09 '09 at 12:09

I would create a IsJustifyCommand(s) method or create a command abstract class that has a IsJustifyCommand() method on it. Then the code will read like a description of what it is trying to do.

Using regex may be neat, but will lead to maintenance problems if someone that is not a hard-core JavaScript programmer has to work on the code. However if you have lots of cases when regex is a good solution, then use it, as anyone looking at the code will soon pick it up.

(However I am a C# programmer not a JavaScript programmer, but like most programmer I have to look at / edit JavaScript code sometimes. I think most JavaScript is maintained by none JavaScript programmers.)

score 0 · Answer 4 · answered Jun 10 '09 at 15:21

I hate when something is written like that. First I look at the code and think "if cmd is equal to JustifyLeft or JustifyRight... then invert that and... if that's true do the thing.. so that means if it's JustifyLeft...". For me it takes alot of time and I have to re-read the line to be sure I got it right.

I think it's better to write.

if ((cmd !== 'JustifyLeft') && (cmd !== 'JustifyRight') && (cmd !== 'JustifyCenter') && (cmd !== 'JustifyFull'))

It might be a little more verbose but I find it easier to follow. I read it as "cmd can't be any of the Justify-strings". Checking a long boolean expression and then inverting the whole answer is irritating.

I like the solution from scragar, just wanted to give my thoughts about inverting a long boolean expression.

Is there a better way to write this mutiple or conditional?

4 Answers4