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
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
Reset to default 4If 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!
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.
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).
On a related note, in IE, even though the specs say you can, you can not use a try/finally bination. 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();
}
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);
}
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.