Correct JavaScript IF Else Syntax

Question

I am updating an existing JavaScript application. Part of this update involves formatting things better and cleaning up code.

Right now this is one of the Functions which uses if() and else statements without all the correct brackets in place... I personbally hate it when people do this shorthand method and even more so when they mix it and use it sometimes and not others. THat is the case in this example code below.

this.setFlags = function() {
    if (document.documentElement)
        this.dataCode = 3;
    else
    if (document.body && typeof document.body.scrollTop != 'undefined')
        this.dataCode = 2;
    else
    if (this.e && this.e.pageX != 'undefined')
        this.dataCode = 1;

    this.initialised = true;
}

I honestly do not know how to correctly add the brackets to this function, below is what I have but I am thinking it might not be correct. Can a JavaScript expert let me know?

this.setFlags = function() {
    if (document.documentElement){
        this.dataCode = 3;
    }else{
        if (document.body && typeof document.body.scrollTop != 'undefined'){
            this.dataCode = 2;
        }else{
            if (this.e && this.e.pageX != 'undefined'){
                this.dataCode = 1;
            }
        }
    }

    this.initialised = true;
}

It's correct. Congrats on adding the brackets correctly. Is that your only question? — Dave Chen, Oct 05 '14 at 04:44
Both are fine, when you have to write just one line in `if` statement, there is no need to put `{}`. — Mritunjay, Oct 05 '14 at 04:45
not using {} on single line if cases/conditions is often considered a bad practice, because errors can hide in plain sight if you have poor indentation. http://stackoverflow.com/questions/4797286/are-curly-braces-necessary-in-one-line-statements-in-javascript — Allan Nienhuis, Oct 05 '14 at 05:00

Ted Hopp · Accepted Answer · 2014-10-05T04:59:38.520

8

I share your dislike of unbracketed if/else statements. Your bracketing seems correct to me. However, there's a simpler way that's equivalent:

this.setFlags = function() {
    if (document.documentElement) {
        this.dataCode = 3;
    } else if (document.body && typeof document.body.scrollTop != 'undefined') {
        this.dataCode = 2;
    } else if (this.e && this.e.pageX != 'undefined') {
        this.dataCode = 1;
    }

    this.initialised = true;
}

As Dave Chen points out in a comment, because of the simplicity and parallelism of the if/else branches—they all assign a value to this.dataCode—you could also use nested ternary operators:

this.setFlags = function() {
    this.dataCode = document.documentElement                           ? 3
                  : document.body
                      && typeof document.body.scrollTop != 'undefined' ? 2
                  : this.e && this.e.pageX != 'undefined'              ? 1
                  :                                                      this.dataCode;

    this.initialised = true;
}

The advantage of this is that it is clearer that all the conditions are simply determining which value to assign to this.dataCode. The disadvantage (a big one, in my opinion) is that unless the nested ternary operators are formatted in a careful way (such as what I did here), it's basically impossible to get any sense of what's going on without carefully studying the expression. Unfortunately, most JavaScript code formatters that I'm familiar with do a very poor job of formatting such expressions (or preserving such formatting). (Interestingly, several Perl formatters that I've used do a wonderful job of this. But this isn't Perl.)

edited Oct 05 '14 at 04:59

answered Oct 05 '14 at 04:45

Ted Hopp

232,168
48
399
521

1

You could also use ternary, but I don't think that's what the OP is asking. The question isn't, "How can I make this code better?". – Dave Chen Oct 05 '14 at 04:47
@DaveChen - Yes, that's another possibility. I'll add that to my answer. Thanks. – Ted Hopp Oct 05 '14 at 04:47
It's great, I know it was a super basic question but the way it was formatted it wasn't all that clear to me, thanks and yes improvements are always welcome! – JasonDavis Oct 05 '14 at 04:48
@DaveChen - Well, I take that back. There's no unconditional `else` case so there is no value to use as the last operand in a compound ternary expression (when all three `if` tests fail). – Ted Hopp Oct 05 '14 at 04:50
1

No, because you could just set it to itself. Ternary is ugly sometimes -- but always works. `ex. this.dataCode == whatever ? set value : this.dataCode` – Dave Chen Oct 05 '14 at 04:52
Sadly if I would of realized the `else if` was an actual `else if` I would of never posted this question but you can see how the `else if` in the original is all on separate lines so it just looked funky to me I guess. Now that i'm looking at them as `else if` it actually makes more sense i feel silly just not use to seeing an `if else` without brackets – JasonDavis Oct 05 '14 at 04:58
@DaveChen - Right you are again. It's too late at night for me to be posting on SO. I've now added your suggestion. – Ted Hopp Oct 05 '14 at 05:00
@TedHopp Can that be my excuse as well ;) – JasonDavis Oct 05 '14 at 05:05
Thanks for the updates, it's nice to see it done different ways – JasonDavis Oct 05 '14 at 05:06

score 1 · Answer 2 · answered Oct 05 '14 at 04:49

It's correct but you can make it neater:

this.setFlags = function() {
  if (document.documentElement) {
    this.dataCode = 3;
  } else if (document.body && typeof document.body.scrollTop != 'undefined') {
    this.dataCode = 2;
  } else if (this.e && this.e.pageX != 'undefined') {
    this.dataCode = 1;
  }

 this.initialized = true;
};

oh this is already posted

Correct JavaScript IF Else Syntax

2 Answers2