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

Answer 1

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

Answer 2

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