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

What do you want to improve? The only way I can think of too shorten this is using a Javscript Framework. — jigfox
– jigfox, Commented May 14, 2010 at 10:28
@Jens - there are plenty of ways to shorten this, none of which include adding a library. And improve doesn't necessary mean shorten. — Sean Kinsey
– Sean Kinsey, Commented May 14, 2010 at 11:08
@basit74, please accept the best answer if you are satisfied — Sean Kinsey
– Sean Kinsey, Commented May 14, 2010 at 11:32

Sean Kinsey · Accepted Answer · 2010-05-14 10:37:02Z

5

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

edited May 14, 2010 at 10:37

answered May 14, 2010 at 10:30

Sean Kinsey

38k7 gold badges55 silver badges71 bronze badges

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

2 Comments

user160820 Over a year ago

The problem is I think, on every click I have more and more event listeners. Is it true?

Sean Kinsey Over a year ago

no :) you only add the listeners when calling on(... , and this is only done once right?

Ed Daniel · Accepted Answer · 2010-05-14 10:32:52Z

2

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.

answered May 14, 2010 at 10:32

Ed Daniel

52712 silver badges23 bronze badges

2 Comments

Skilldrick Over a year ago

I hadn't seen this when I left my comment on the question. This is the single biggest improvement to the code, I'd say.

Skilldrick Over a year ago

It's not just to do with edits. In shows the intent better as well. Looking at the code above, it looks like the second if has two elses because of the poor indentation. Adding braces would make it obvious what goes where.

KooiInc · Accepted Answer · 2010-05-14 10:40:28Z

1

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

edited May 14, 2010 at 10:40

answered May 14, 2010 at 10:33

KooiInc

124k32 gold badges145 silver badges181 bronze badges

Comments

bobince · Accepted Answer · 2010-05-14 11:53:52Z

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

polygenelubricants · Accepted Answer · 2010-05-14 10:32:16Z

0

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

answered May 14, 2010 at 10:32

polygenelubricants

385k129 gold badges568 silver badges627 bronze badges

Comments

Dom De Felice · Accepted Answer · 2010-05-14 10:34:25Z

0

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

edited May 14, 2010 at 10:34

answered May 14, 2010 at 10:27

Dom De Felice

4764 silver badges10 bronze badges

3 Comments

Sean Kinsey Over a year ago

I wouldn't necessarily say that jQuery 'cares' about compatibility issues :)

Sean Kinsey Over a year ago

That the developers of jQuery aren't really known for their 'hi-quality' code - with frequent breakage between versions, especially with regards to supporting older browsers.

Dom De Felice Over a year ago

Uhm I didn't know about this. So far I hadn't any problem anyway.

alter · Accepted Answer · 2010-05-14 14:09:09Z

0

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

answered May 14, 2010 at 14:09

alter

4,4548 gold badges34 silver badges38 bronze badges

Collectives™ on Stack Overflow

How to improve this if else structure in JavaScript?

7 Answers 7

2 Comments

2 Comments

Comments

Comments

Comments

3 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

7 Answers 7

2 Comments

2 Comments

Comments

Comments

Comments

3 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related