How to improve this if else structure in JavaScript?

Question

How to improve the following if-else structure in JavaScript?

if(isIE())
    if(termin != "")
        TargetElement.onclick = function() {merkzettelRemove(this, id, termin)};
    else
        TargetElement.onclick = function() {merkzettelRemove(this, id)};
    else
        if(termin != "")
            TargetElement.setAttribute("onclick","merkzettelRemove(this, " + id + ",
               '" + termin + "')");
        else
            TargetElement.setAttribute("onclick","merkzettelRemove(this, " + id + ")");

Answer 1

I know this is not an answer for your question, but if you don't have any reason for not doing so you should definitely use a library that cares about compatibility issues for you, such as jQuery. Then you could write something like this:

var el = $(TargetElement);

if (termin != "")
    el.click(function() {merkzettelRemove(this, id, termin)});
else
    el.click(function() {merkzettelRemove(this, id)});

Answer 2

// get a cross-browser function for adding events
var on = (function(){
    if (window.addEventListener) {
        return function(target, type, listener){
            target.addEventListener(type, listener, false);
        };
    }
    else {
        return function(object, sEvent, fpNotify){
            object.attachEvent("on" + sEvent, fpNotify);
        };
    }
}());

// add the event listener
on(TargetElement, "click", function(){
    // if termin is empty we pass undefined, this is the same as not passing it at all
    merkzettelRemove(this, id, (termin) ? termin : undefined);
});

Answer 3

I think using a Javascript framework is the best solution, but you can try something like this:

var src="merkzettelRemove(this, " + id + (termin && (",'" + termin + "'")) + ")";
if (isIE()) {
   TargetElement.onclick = new Function(src);
} else {
   TargetElement.setAttribute("onclick", src);
}

Answer 4

First things first, put some more braces in. It'll make it clearer what's going on, as well as saving untold heartache when you come to edit this in future.

Yes, you can get away without the braces if they wrap a single statement. When you come along three months from now and add something else into one of those blocks, your code will break unless you remember to wrap the whole block in braces. Get into the habit of doing it from the beginning.

Answer 5

First, use { } everywhere, it will make your loops more readable.

Second: I think this does the same

if(isIE()) {
   TargetElement.onclick = function() {
            merkzettelRemove(this, id, termin || null); 
          };
 } else {
   TargetElement.setAttribute("onclick",
          "merkzettelRemove(this, " + id + ",'" + termin || null + "')");
}

Third, but you could try using unobtrusive javascript to add handlers to TargetElement

Answer 6

Firstly, lose the browser-sniffing. onclick= function... works everywhere; setAttribute is not only broken in IE, but also ugly. JavaScript-in-a-string is a big code smell; avoid. setAttribute on HTML attributes has IE problems and is less readable than simple DOM Level 1 HTML properties anyway; avoid.

Secondly, it would be ideal to make merkzettelRemove accept an out-of-band value (null, undefined, or even '' itself) as well as an omitted argument. It is possible it already allows undefined, depending on what mechanism it is using to support optional arguments. If so you could say simply:

TargetElement.onclick= function() {
    merkzettelRemove(this, id, termin || undefined);
};

If you must completely omit the argument and you don't want to use an if...else, there's another way around although IMO the clarity is worse:

TargetElement.onclick= function() {
    merkzettelRemove.apply(null, termin==''? [this, id] : [this, id, termin]);
};

Answer 7

how about using short circuiting operators. So the following code

if(A) {

if(B) { C; }
else{ D;} else{ if(E)
F; else G; } }

Will become

A && ((B&&C) || D || ((E&&F) || G));