Reputation: 9285
According to this answer: https://stackoverflow.com/a/1722991/680026 you should only use try-catch if you really do something there besides logging:
Don't catch an exception if you're only going to log the exception and throw it up the stack. It serves no meaning and clutters code.
But what about logging information that are not available at the higher level? eg:
private void AddSomethingToTable(string tablename, string fieldname) {
try {
InsertFieldToTable(tablename, fieldname);
} catch (Exception ex) {
log.ErrorFormat("Could not insert field '{0}' into table '{1}'", fieldname, tablename);
throw;
}
}
private void main() {
try {
AddSomethingToTable("users","firstname");
AddSomethingToTable("users","lastname");
AddSomethingToTable("users","age");
} catch (Exception ex) {
MessageToUser("Sorry. Saving did not work.",ex);
}
}
As you can see: In my (completely made up) example I log the information about the field that did cause the error. This could be some good information to start finding the error.
So even though I only logged the error, this information could be crucial and be lost outside that method.
Is this a valid use of try-catch here or are there other suggested ways to log that? (I don't think that just always logging this info (regardless if an error occurred or not) would be a valid solution)
Upvotes: 1
Views: 8008
Reputation: 82
We use the Try Catch block in a similar way you have suggested and it works well for us. We have implemented ELMAH https://code.google.com/p/elmah/ which is great for logging untrapped errors. With a line of code in the Try Catch block you can also write trapped exceptions into the log.
Catch ex As Exception
Elmah.ErrorSignal.FromCurrentContext.Raise(ex)
Return False
End Try
The error is handled (the relevant function returned false) and user doesn’t get the Yellow Screen of Death, and we can look up full details of any errors in the log.
Upvotes: 0
Reputation: 101130
From your linked quesion:
The basic rule of thumb for catching exceptions is to catch exceptions if and only if you have a meaningful way of handling them.
I put emphasis on "The basic rule of thumb" as it's not a law. It's a best practice. i.e. follow it until you have a good motivation to not do so.
If you catch exceptions to include information you probably should throw a new meaningful exception with more context information. something like:
try
{
//kldfsdölsdöls
}
catch (Exception ex)
{
throw new MoreDetailedException("Text with context data", ex);
}
That way you'll get all context information for each stack level collected into the same exception (since you included the inner exception in it). Thus the log entry in the top level will contain ALL information in the same line.
Upvotes: 2
Reputation: 77846
But what about logging information that are not available at a higher instance?
You can pass those information back to caller while re-throwing the exception
private void AddSomethingToTable(string tablename, string fieldname) {
try {
InsertFieldToTable(tablename, fieldname);
} catch (Exception ex) {
string str = string.Format("Could not insert field '{0}' into table '{1}'", fieldname, tablename);
throw new Exception(str, ex);
}
}
Upvotes: 1
Reputation: 4808
I think you answered your own question with
But what about logging information that are not available at a higher instance
and
this information could be crucial and be lost outside that method
I dislike hard and fast "always do X and never Y", because there are times where it is necessary to go against so-called "best practice" in order to do what is best for your application.
If logging the information is necessary to fix the issue, and you lose this information if you don't log it immediately, then log the information.
Upvotes: 3
Reputation: 10557
There is nothing wrong in what you are trying to do. The idea of the other question/answer was that it is better to log the error at the place where you really handle it. In .NET every exception contains a stack trace. This means that upper layer can report location in the code that has generated this error. Doing that in one place instead of many is way more meaningful. This was their idea.
Upvotes: 2