5

So I am recently writing a relatively complex application written in C# that performs an array of small tasks repeatedly. When I first started the application I realized that a lot of the code I was typing was repetitive and so I began encapsulating the majority of the app's logic into separate helper classes that I could call as needed.

Needless to say the size of my app (and amount of code) was cut in half. But as I was going through I noticed something else in my application that seemed to be repetitive and looked like it could be improved.

Now most of my methods in my helper classes are either making a HttpWebRequest or performing save/delete operations on files. Having said that I need to handle the possibility that eventually the call won't complete, or the file can't save because there isn't enough space, or whatever. The problem I'm running into is that I have to keep writing try/catch statements every time I call one of the methods. On top of that I have to retype the error message (or Eventually a status message. I would like to know when it succeeds as well).

So here's kind of a snippet of what I have to type:

  try
  {
     ItemManager.SaveTextPost(myPostItem);
  }
  // Majority of the time there is more than one catch!
  catch
  {
     //^^^Not to mention that I have to handle multiple types of exceptions 
     //in order to log them correctly(more catches..ugh!)
     MessageBox.Show("There was an error saving the post.");
     //Perform logging here as well
  }

From what I have concluded so far is:

  • To me this is overkill having to write this over 50 times for my app. Sounds like I should be including this in the helper class and include the full set of catches.
  • But how could I know the result? I was maybe thinking of returning a string that contains the error/success message.
  • Really for these types of methods it doesn't require the method from which the helper method is being called from to enclose it in a try/catch block.

Is this approach correct? Is there another way of doing this? Should I be using a try/catch in the calling method at all? Since this kind of my first shot at this I would really like to hear what others who have handled this scenario have to say.

Adam Rackis
  • 82,527
  • 56
  • 270
  • 393
Edward
  • 7,346
  • 8
  • 62
  • 123
  • A similar question was asked recently: http://stackoverflow.com/questions/8156530/exception-handling-in-method-definition-or-call/8156574#8156574 – Tudor Nov 22 '11 at 20:41

7 Answers7

1

AOP libraries like PostSharp are designed to handle cross-cutting concerns exactly like this.

If all these helper methods employ the same boilerplate try catch -> handle multiple types of exceptions, then you can write one aspect that does that, and decorate all of the relevant methods with the appropriate attribute.

Adam Rackis
  • 82,527
  • 56
  • 270
  • 393
1

I think putting the try/catch in the method you are calling is perfectly fine. You can return error/success codes to the calling code in a variety of ways. .NET and c# handle enumerations nicely, so you could have ItemManager.SaveTextPost(myPostItem, out errorCode); where errorCode would be an enumerated value that would let you know if any problems occurred. It could also be as simple as having the method return a bool true if successful, false if otherwise. There are many ways that you could handle that issue, but as far as I'm concerned putting the try/catch in the method is the preferable way of doing things.

Michael Fox
  • 611
  • 3
  • 9
  • +1 for the sending back an enumerated value. I think thats a great idea. Somehow I knew there was a better way than sending a string haha. – Edward Nov 22 '11 at 20:50
  • I wouldn't go with the `SaveTextPost(myPostItem, out errorCode)` way. It could be confusing to people using the code and it's a lot more painful to maintain with enumerates of error values. Besides, aren't exceptions designed to treat errors? Return a boolean is ok though, if `true` and `false` are enough for you. – Otiel Nov 22 '11 at 21:01
  • Exceptions are for errors, but you shouldn't rely on throwing one for every error. Anything that you expect could go wrong should be handled gracefully and anything else that is an exceptional occurrence should throw an exception (perfect example is handling file system IO - you have no idea what state the file is in and what other processes might have handles open on it). Throwing exceptions is expensive and using them in place of defensive programming practices is just bad coding in my opinion. – Michael Fox Nov 23 '11 at 22:56
1

There is no single perfect rule for exception handling, so the answer is: It depends. In general you should only catch exceptions, if you know how to handle them. In your example my first question would be, if the application can continue to run after this error and would still be in a valid state. If not, you should rethrow the exception after logging. Then think about nesting exception: You can catch an exception, add information by nesting it into another and throwing that exception. If you design your exception classes and handlers carefully, your logging and displaying code should become much simpler than you expect it. But the details obviously depend on your application.

Achim
  • 15,415
  • 15
  • 80
  • 144
1

You should throw exceptions in your helper methods by checking with if blocks if every argument is correct, every file is reachable... then if needed, implement some try catch blocks in your helper methods and throw exceptions when catching.

Finally, enclose these helper methods by a try catch block, but at this time, really catch the exceptions:

try
{
   ItemManager.SaveTextPost(myPostItem);
   // SaveTextPost can throw some exceptions (that you know)
}
catch (Exception e)
{
   // You know the exceptions that SaveTextPost can return, so threat them 
   // correctly
   if (e is FileNotFoundException)
       MessageBox.Show("The file was not found when saving the post.");
   else if (e is ArgumentNullException)
       MessageBox.Show("The post can't be null to be saved.");
   else
       MessageBox.Show("There was an error saving the post.");
}

At the end, you need to treat the error and show an error message to the user. You only can decide if the MessageBox.Show should be implemented in the helper method or in the class calling the helper method.
Personally, I think helper methods are destined to be used by any developer that will run your code, so you should let him decide what he wants to do with the error. That means throwing exceptions in the helper method.

Otiel
  • 18,404
  • 16
  • 78
  • 126
  • Yes but would be the purpose of enclosing the method in a try/catch block if the method your calling already handles possible exceptions. To be honest I kind of want to get away from that. I suppose there is a 99.9% chance some how it could fail... – Edward Nov 22 '11 at 20:46
  • +1 I had no idea you could determine the type of exception like this. Something learned. I definetly agree also that the way the developer chooses to communicate the message (sometimes never) should be left up to them. – Edward Nov 22 '11 at 21:01
  • 2
    That type of comparison wont work for what your trying to do.. needs to be something like if( e is FileNotFoundException) {} – iamkrillin Nov 22 '11 at 21:04
  • @loyalpenguin: Not sure about the `e == FileNotFoundException` now that you say it ^^. Maybe you need to do `e.GetType() == FileNotFoundException` or `e is FileNotFoundException`. I will have to test to be sure. – Otiel Nov 22 '11 at 21:05
  • 2
    @Otiel no problem, the syntax for the other way would be if(e.GetType() == typeof(FileNotFoundException) { } – iamkrillin Nov 22 '11 at 21:08
1

Try/Catch is a good solution. One suggestion, I like to use this to stem the flow of errors: exception catching: try { } catch (Exception ex) { ShowExceptionError(ex); } Then I will simply throw all exceptions to a single method, and let the method handle each.

Kind of like this:

private void ShowExceptionError(ex)
{
    string errorMessage = "Type of error:" + ex.GetType().ToString();
    string stackTrace = ex.StackTrace();
    MessageBox.Show(errorMessage + "\n\n" + stackTrace);
}
seeSharper
  • 61
  • 1
  • 2
1

All of these ways are great options. One thing you want not to do is to use a try {} catch {} for flow control. This means something like this (again avoid this)

Void SomeMethod()
{
     try
     {
         while(somecondition)
         {
             try
             {
             }
             catch (Exception ex)
             {

             }
          }
     }
     catch (Exception ex)
     {
          .....
     }

Instead you want to code defensively.

0

My personal take on exception handling is.. add try {} catch {} where it makes sense.

For example always use a try catch when calling "untrusted" code, i.e. modules or plugins. otherwise if you are going to catch an exception make sure you can do something meaningful with it. If not, allow the exception to bubble up to a point where you can.

There are few things worse than trying to debug an app where the developer caught an exception and returns a bool. Just sayin'

iamkrillin
  • 6,798
  • 1
  • 24
  • 51