8

In JavaScript is it wrong to use a try-catch block and ignore the error rather than test many attributes in the block for null?

try{ 
   if(myInfo.person.name == newInfo.person.name
      && myInfo.person.address.street == newInfo.person.address.street
      && myInfo.person.address.zip == newInfo.person.address.zip) {
         this.setAddress(newInfo);
    } 
} catch(e) {} // ignore missing args

6 Answers 6

4

If you expect a particular condition, your code will be easier to maintain if you explicitly test for it. I would write the above as something like

if(   myInfo && newInfo 
      && myInfo.person && newInfo.person
      && myInfo.person.address && newInfo.person.address
      && ( myInfo.person.name == newInfo.person.name
           && myInfo.person.address.street == newInfo.person.address.street
           && myInfo.person.address.zip == newInfo.person.address.zip
         )
) 
{
     this.setAddress(newInfo);
} 

This makes the effect much clearer - for instance, suppose newInfo is all filled out, but parts of myInfo are missing? Perhaps you actually want setAddress() to be called in that case? If so, you'll need to change that logic!

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

Comments

2

Yes. For one, an exception could be thrown for any number of reasons besides missing arguments. The catch-all would hide those cases which probably isn't desired.

Comments

1

I would think that if you're going to catch the exception then do something with it. Otherwise, let it bubble up so a higher level can handle it in some way (even if it's just the browser reporting the error to you).

Comments

1

On a related note, in IE, even though the specs say you can, you can not use a try/finally combination. In order for your "finally" to execute, you must have a catch block defined, even if it is empty.

//this will [NOT] do the reset in Internet Explorer
try{
  doErrorProneAction();
} finally {
  //clean up
  this.reset();
}

//this [WILL] do the reset in Internet Explorer
try{
  doErrorProneAction();
} catch(ex){
  //do nothing
} finally {
  //clean up
  this.reset();
}

Comments

0

You could always write a helper function to do the checking for you:

function pathEquals(obj1, obj2, path)
{
    var properties = path.split(".");
    for (var i = 0, l = properties.length; i < l; i++)
    {
        var property = properties[i];
        if (obj1 === null || typeof obj1[property] == "undefined" ||
            obj2 === null || typeof obj2[property] == "undefined")
        {
            return false;
        }

        obj1 = obj1[property];
        obj2 = obj2[property];
    }

    return (obj1 === obj2);
}

if (pathEquals(myInfo, newInfo, "person.name") &&
    pathEquals(myInfo, newInfo, "person.address.street") &&
    pathEquals(myInfo, newInfo, "person.address.zip"))
{
    this.setAddress(newInfo);
}

Comments

-1

For the example given I would say it was bad practice. There are instances however where it may be more efficient to simply trap for an expected error. Validating the format of a string before casting it as a GUID would be a good example.

1 Comment

Only applies to strongly typed languages. Not JavaScript.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.