views:

1196

answers:

5

I'd love some other opinions on what's more efficient in this code. Basically in the following code, there's a setInterval loop and I need 4 requirements to be true before the code runs in the loop. So in v.1 I wrote an if statement checking all 4. Worked fine.

Then I switched to just using try/catch, with the code I want to execute sitting in try{}. The logic was that during each loop, an exception would be generated but suppressed for each invalid condition. On the final loop where all conditions are true, the code executes and clears the interval.

Either works. I like the try/catch method because there's less conditional code that I need to write and worry about breaking. But I'm worried that try/catch is really inefficient, especially in a setInterval() loop hitting at 100ms. What are some opinions of other bright minds here on SO?

Try/Catch

var intvar = setInterval(function(){
try{    
 clearInterval(intvar);

 jQuery('#'+nav[pageid].t1+'>a').replaceWith(jQuery('<span>'+jQuery('#'+nav[pageid].t1+'>a').text()+'</span>'));

 //set display classes for nav
 jQuery('#'+nav[pageid].t1).addClass('selected').find('#'+nav[pageid].t2).addClass('subselect'); //topnav
 jQuery('#'+nav[pageid].t3).addClass('selected').find('#'+nav[pageid].t4).addClass('subselect'); //leftnav
}catch(err){}
},100);

IF Block

var intvar = setInterval(function(){

if(typeof jQuery == 'function' && typeof nav == 'object' && typeof pageid != 'undefined' && typeof document.getElementById('leftnav') == 'object'){
 clearInterval(intvar);
 jQuery('#'+nav[pageid].t1+'>a').replaceWith(jQuery('<span>'+jQuery('#'+nav[pageid].t1+'>a').text()+'</span>'));

 //set display classes for nav
 jQuery('#'+nav[pageid].t1).addClass('selected').find('#'+nav[pageid].t2).addClass('subselect'); //topnav
 jQuery('#'+nav[pageid].t3).addClass('selected').find('#'+nav[pageid].t4).addClass('subselect'); //leftnav
}

},100);
+11  A: 

Exceptions should be used for exceptional circumstances (i.e. things that you don't expect to happen normally). You should not, in general, use exceptions to catch something that you can test for with an if statement.

Also, from what I understand, exceptions are much more expensive than if statements.

cdmckay
+1  A: 

I would write the following code:

var startTime = (new Date()).getTime();
for (var i=0; i < 1000; ++i) intvar();
var endTime = (new Date()).getTime();
alert("Took " + ((endTime - startTime) / 1000.0) " seconds");

Then I would try both versions of intvar and see which runs more quickly. I'd do this myself but I don't have the page layout you do so my code wouldn't work.

Some stylistic comments - it doesn't seem necessary to test that jQuery is a function. If it isn't, your webpage is likely messed up such that not running the intvar code will not help you. If you rarely expect the exceptions to be thrown, I'd use the try/catch.

Claudiu
+4  A: 

Use the if statement. I don't know what the overhead is for a TRY/CATCH, but I suspect it's far greater than evaluating a boolean expression. To hit the TRY/CATCH you will have to: execute a statement, generate an error [with that associated overhead], log the error (presumably), made a stacktrace(presumably), and moved back into the code. Additionally, if you have to debug code near those lines the real error could get obfuscated with what you are TRY/CATCHing.

Furthermore, it's a misuse of TRY/CATCH and can make your code that much harder to read. Suppose you do this for longer or more obfuscated cases? Where might your catch end up?

This is referred to as Expection handling

EDIT: As commented below, you only take the runtime performance hit if you actually cause an exception.

Will
Just to add to this, the cost of the try/catch is when an exception is thrown, otherwise it's insignificant. Hence if you can reliably test for the condition (and avoid the exception) with an if it's more performant.
Esteban Brenes
I don't know about JavaScript, but as far as I know a try block pushes some things on the stack. A try block inside a tight loop would make it much slower than a try block outside the loop. Is it anyhow different in JavaScript?
Hosam Aly
+1  A: 

For the provided example where you are wrapping a try/catch around a block of code that should always run anyway (unless if something horrible happens), it is good form to use try/catch. An analogy for you: do you always test "Is the sky Blue?" in your if statement, or would you wrap it in a try/catch that is triggered only when the sky happens to turn Green.

Use the If statement method if you are dealing with user provided input or if the chances of a function not existing is much higher due to something else happening in the code.

Keep in mind that if you don't trigger the exception you don't have any unwinding or backtracking across code. In the example the catch will only execute if something is wrong (jQuery is missing or some such thing), however the if statement method has an evaluation happening on EVERY SINGLE CALL to that function - you shouldn't do more work than you have to.

Redbeard 0x0A
A: 

The other answers are correct, try/catch is for exceptional circumstances and error handling. if conditions are for program logic. "Which is faster?" is the wrong question.

A good rule of thumb, if you're doing nothing with the exception, it's probably not an exception!

To figure out which to use, let's break down your if condition.

  1. typeof jQuery == 'function' Is the jQuery() function defined?
  2. typeof nav == 'object' Does the nav global variable contain an object?
  3. typeof pageid != 'undefined' Is the pageid global variable defined?
  4. typeof document.getElementById('leftnav') == 'object' Does the document contain a leftnav element?

The first is clearly an exception. You ain't getting far without a jQuery() function.

The second is also an exception. You're not going anywhere without a nav object.

The third is an exception. You need a pageid to do anything.

The fourth is probably logic. "Only run this code if there is a leftnav element". It's hard to tell because the rest of the code doesn't reference a leftnav element! Only the comments do, a red flag. So it's probably a programming mistake.

So I'd probably do this (with apologies if I'm butchering jQuery):

var intvar = setInterval(function() {
    // If there's no leftnav element, don't do anything.
    if( typeof document.getElementById('leftnav') != 'object') {
        return;
    }

    try {
        clearInterval(intvar);
        jQuery('#'+nav[pageid].t1+'>a')
            .replaceWith(jQuery('<span>'+jQuery('#'+nav[pageid].t1+'>a').text()+'</span>'));

        //set display classes for nav
        jQuery('#'+nav[pageid].t1)
            .addClass('selected')
            .find('#'+nav[pageid].t2)
            .addClass('subselect');     //topnav
        jQuery('#'+nav[pageid].t3)
            .addClass('selected')
            .find('#'+nav[pageid].t4)
            .addClass('subselect');     //leftnav
    }
    catch(err) {
        ...do something with the error...
    }
},100);

...but I'd really examine if the leftnav element check is applicable.

Finally, I can't help but comment that this "function" is working with global variables. You should instead be passing nav and pageid into the function in order to maintain encapsulation and your sanity.

Schwern