August 21, 2010 CodePuzzle, CodeReview 1 comment
Code Review – Way too interesting exception handling
Today, I’ve been reviewing some piece of source code, and found way too interesting exception handling approach.
In following code snippet we are trying to wrap existing exception with new one having more descriptive message and saving the stack trace of the original exeption. The way how it is done is just amazing:
private void OnProcessingException(Exception exception)
{
if (ExceptionOccured != null)
{
try
{
throw new Exception(string.Format(“Very descriptive message here.” + “{1}StackTrace of InnerException: {0} {1}”, exception.StackTrace, Environment.NewLine), exception);
}
catch (Exception outerException)
{
ExceptionOccured(this, new ProcessorExceptionEventArgs(outerException));
}
}
}
I’m not trying to blame someone. But sometimes people see so much more complicated and sophisticated solutions to problems that could be solved with one-two lines, that it makes me think, what we also do such things but on another – higher level.
Developer who wrote this code is trainee, so doesn’t mean low quality of our code, simple he needs more experience. My point is that we are also trainee, but on another level, and we might not see simple solutions as well.
So who will write me one-or-few lines of code that can do the same? Please do not hesitate :)
Code Review: try – catch
December 22, 2009 CodeReview 2 comments
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.
