December 22, 2009 CodeReview
Sometimes it is hard to convince that catch(…) is not so good!
Today I provided code review to one of my colleague and he was totally sure the he need try{/*code*/}catch(){} in few places.
First looked like:
try
{
int someValueId = Convert.ToInt32(value);
if (someValueId > 0)
{
return new ValidationResult(true, “Valid”);
}
}
catch{}
{
int someValueId = Convert.ToInt32(value);
if (someValueId > 0)
{
return new ValidationResult(true, “Valid”);
}
}
catch{}
I rewrote code to:
int serviceId;
Int32.TryParse(value, out serviceId);
if (serviceId > 0)
{
return new ValidationResult(true, “Valid”);
}
Int32.TryParse(value, out serviceId);
if (serviceId > 0)
{
return new ValidationResult(true, “Valid”);
}
Then I spent few minutes to convince him that this code is ok and that this is not good to have catch for everything.
Another code looked like:
string result = string.Empty;
try
{
result = documentContent.Descendants(“SomeValueCode”).FirstOrDefault().Value;
}
catch (Exception)
{
//He was trying to ensure me that since we are doing validation we should not throw an exception
}
//do some work here
try
{
result = documentContent.Descendants(“SomeValueCode”).FirstOrDefault().Value;
}
catch (Exception)
{
//He was trying to ensure me that since we are doing validation we should not throw an exception
}
//do some work here
Now I tried to explain that FirstOrDefault() returns us either first element of collection OR null, so if we got null we just need to do some another stuff,
but if some exception will occur we should allow it fly out OR write some specific catch for exceptions that could be thrown. Code which I propose looks like:
var resultElement = documentContent.Descendants(“SomeValueCode”).FirstOrDefault();
if(interventionCodeElement != null)
{
var result = resultElement.Value;
//do some work here
}
if(interventionCodeElement != null)
{
var result = resultElement.Value;
//do some work here
}
After conversation we decided that try(/*code*/)catch() is not so good.
2 comments
| Markdown | Result |
|---|---|
| *text* | text |
| **text** | text |
| ***text*** | text |
| `code` | code |
| ~~~ more code ~~~~ |
more code |
| [Link](https://www.example.com) | Link |
| * Listitem |
|
| > Quote | Quote |
Ok, in that samples code easily can be changed. But in some cases there are really many checkings neded. What do you think about this code:
try
{
HtmlElementCollection links = MainBrowser.Document.Window.Frames["mainframe"].Frames["actframe"].Frames["useractions"].Document.GetElementsByTagName("a");
(from link in links where link.InnerHtml.Contains("Enter location") select link).First().InvokeMember("click");
}
catch (NullReferenceException nrex){}
Yea… That is very interesting case. But it looks like code knows exactly how the page is structured.
1. This means that either you are developer of that page and in this case there is no real need to make deep and detailed verification. Read about the Defensive Programming.
2. If code is not yours I see two ways:
– First, purpose of your code is quick solution for some problem needed only in one place and you are not going to extend your project. In only this case I see it reasonable to leave as it is.
– Second, if you DO need and going to work on this project, I would recommend you do not use that "magic" words, but instead use static classes that provide string properties AND create some domain based design, maybe some helper classes that will correspond to your specific Frames, so you will have something like: UserActionsFrame.GetElements(ElementConstants.A).
P.S. Thank you very much for reading my blog. Please share yours.