25

Is there a better way to catch exceptions? I seem to be duplicating a lot of code. Basically in every controller I have a catch statement which does this:

try
{
     Do  something that might throw exceptions.
}
catch (exception ex)
{
     Open database connection
     Save exception details.
     If connection cannot be made to the database save exception in a text file.
}

I have 4 controllers and around 5-6 actions methods in each controller which is a lot of code duplication. How can I trim down on the amount of line in the try catch statement above?

Stormenet
  • 25,926
  • 9
  • 53
  • 65
bren bree
  • 253
  • 3
  • 5
  • 4
    Look into "Aspect Oriented Programming in ASP .NET MVC4" – Ahmed KRAIEM Oct 16 '13 at 08:01
  • 2
    @AhmedKRAIEM or just drop down to C++ and use macros for everything. AOP is a hack. -1 if i could. – Gusdor Oct 16 '13 at 08:09
  • 4
    @Gusdor "AOP is a hack." That's your opinion. But nonetheless, AOP isn't the best solution here, [maybe a custom `ActionFilter` or the `Application_Error` event?](http://prideparrot.com/blog/archive/2012/5/exception_handling_in_asp_net_mvc) – Ahmed KRAIEM Oct 16 '13 at 08:21
  • @AhmedKRAIEM You are crowbaring macros into c#. If you build this solutions without PostSharper or w/e installed, the logging won't function. Tell me, how does one debug this if the database connection breaks in the logger? – Gusdor Oct 16 '13 at 08:58
  • @Gusdor So you would rather duplicate code everywhere instead of centralizing logging to one location so you can more easily troubleshoot it? – Wouter de Kort Oct 16 '13 at 08:59
  • 1
    @WouterdeKort Duplicate? No sir. Wrap the whole lot in an extension/helper method in a common code base and just call that? One line in each method call (like a postsharper attribute!) with no IL butchering and third party tools required to build? Yes please! – Gusdor Oct 16 '13 at 09:02
  • 2
    @gusdor a custom action filter as mentioned by Ahmed is doesn't require IL butchering. I understand AOP is not supported in a nice way in .NET yet but attributes are a way to implement cross cutting concerns without having to manually add code to each method – Wouter de Kort Oct 16 '13 at 09:05
  • 1
    @Gusdor `or just drop down to C++ and use macros for everything.` LOL. Great comment on a C# ASP.Net question. Well done. "-1 if I could." – Reinstate Monica Cellio Oct 16 '13 at 12:17
  • @Archer Neither AOP or C++ are acceptable solutions here - the juxtaposition with intended to reinforce that point but it seems i failed. – Gusdor Oct 16 '13 at 12:44
  • 2
    @Gusdor - I see. You should learn to veil your sarcasm more thinly :p – Reinstate Monica Cellio Oct 16 '13 at 13:28

8 Answers8

42

You could make use of Extension methods here.

Create an extension method in a new class.

public static class ExtensionMethods
{
    public static void Log(this Exception obj)
    {
        // log your Exception here.
    }
}

And use it like:

try
{
}
catch (Exception obj)
{
    obj.Log();
}
Xaruth
  • 4,034
  • 3
  • 19
  • 26
Harminder
  • 2,209
  • 23
  • 20
  • 1
    So simple, so clever, once pointed out! Thanks for that. Definitely worthy of an upvote. – David Arno Oct 16 '13 at 08:14
  • 9
    +1 for the hidden answer "Just put all your exception handling code in a static method and call that method". Whether or not to use an extension method is a matter of taste, `MyErrorHandler.Log(ex)` would be just as fine. – Heinzi Oct 16 '13 at 12:32
  • 2
    I refrained to give such answer because it cannot be that programs are not aware of functions that are means of factoring out the duplicate pieces of code. Programmers cannot ask such questions. I wonder who constitutes the most part of stackoverflow. – Val Oct 16 '13 at 16:46
  • 3
    @Val: I believe SE/SO is a place for all ppl w/ any knowledge level to ask any question. I strongly believe SE/SO is a place to help each other; not to judge each other. – kaptan Oct 17 '13 at 18:38
13

You don't need to put try/catch blocks on every method. That's tedious and painful! Instead you can use the Application_Error event of Global.asax for logging the exceptions. The code below is the sample implementation which can be used to catch exceptions that occur in your web application.

protected void Application_Error(object sender, EventArgs e)
{
    var error = Server.GetLastError();
    if (!string.IsNullOrWhiteSpace(error.Message))
    {
        //do whatever you want if exception occurs
        Context.ClearError();
    }
}

I would like also to stress that "Handled exception" especially trying to put try/catch blocks on most methods is one of the "Top 3 silent performance killers for IIS / ASP.NET apps" as explained in this blog http://mvolo.com/fix-the-3-high-cpu-performance-problems-for-iis-aspnet-apps/

Jobert Enamno
  • 4,403
  • 8
  • 41
  • 63
  • 3
    In ASP.NET MVC you can also use filters as a nicer way of catching errors in controllers. – Wouter de Kort Oct 16 '13 at 09:01
  • 2
    You might have misunderstood the article: Using `try/catch` does *not* (or only negligibly) affect performance. Exceptions *that are actually* thrown affect performance. – Heinzi Oct 16 '13 at 12:57
11

What you are trying to do is called a cross-cutting concern. You are trying to log any error that happens anywhere in your code.

In ASP.NET MVC cross-cutting concerns can be achieved by using Filters. Filters are attributes that can be applied globally, to a controller or to a method. They run before an action method executes or after it.

You have several types of filters:

  • Authorization filters, they run to check if the user is allowed to access a resource.
  • Action filters, these run before and after an action method executes.
  • Result filters, these can be used to change the result of an action method (for example, add some extra HTMl to the output)
  • Exception filters run whenever an exception is thrown.

In your case, you are looking for exception filters. Those filters only run when an exception happens in in an action method. You could apply the filter globally so it will automatically run for all exceptions in any controller. You can also use it specifically on certain controllers or methods.

Here in the MSDN documentation you can find how to implement your own filters.

Wouter de Kort
  • 39,090
  • 12
  • 84
  • 103
4

Personally, since I greatly dislike try/catch blocks, I use a static Try class that contains methods that wrap actions in reusable try/catch blocks. Ex:

public static class Try {
   bool TryAction(Action pAction) {
      try {
         pAction();
         return true;
      } catch (Exception exception) {
         PostException(exception);
         return false;
      }
   }

   bool TryQuietly(Action pAction) {
      try {
         pAction();
         return true;
      } catch (Exception exception) {
         PostExceptionQuietly(exception);
         return false;
      }
   }

   bool TrySilently(Action pAction) {
      try {
         pAction();
         return true;
      } catch { return false; }
   }

   // etc... (lots of possibilities depending on your needs)
}
Dave Cousineau
  • 12,154
  • 8
  • 64
  • 80
4

I have used a special class in my applications that is called ExceptionHandler, in the class that is static I have some methods to handle application's exceptions. It gives me an opportunity to centralize exception handling.

public static class ExceptionHandler
{
    public static void Handle(Exception ex, bool rethrow = false) {...}
    ....   
}

In the method you can log the exception, rethrow it, replace it with another kind of exception, etc.

I use it in a try/catch like this

try
{
    //Do something that might throw exceptions.
}
catch (exception ex)
{
    ExceptionHandler.Handle(ex);
}

As Wouter de Kort has rightly stated in his answer, it is cross-cutting concern, so I've put the class in my Application Layer and have used it as a Service. If you defined the class as an interface you would be able to have different implementations of it in different scenarios.

Abbas Amiri
  • 3,074
  • 1
  • 23
  • 23
3

Also you can use Singleton pattern:

sealed class Logger
{
    public static readonly Logger Instance = new Logger();

    some overloaded methods to log difference type of objects like exceptions
    public void Log(Exception ex) {}
    ...
}

And

Try
{
}
Catch(Exception ex)
{
    Logger.Instance.Log(ex);
}

Edit Some peoples don't like Singleton for reasonable grounds.instead of singleton we can use some DI:

class Controller
{
    private ILogger logger;

    public Controller(ILogger logger)
    {
        this.logger = logger;
    }
}

And use some DI library that will inject one instance of ILogger into your controllers.

MRB
  • 3,752
  • 4
  • 30
  • 44
  • 1
    Please, don't use singletons. The singleton is an anti-pattern that makes testing hard, hard, hard and should be avoided. – David Arno Oct 16 '13 at 08:13
  • @DavidArno I use 'Singleton' classes in my c++ codes. what is your suggestion for 'Factory' classes or some Manager classes(for example 'ThreadManager' or 'ThreadPool') instead of Singleton? – MRB Oct 16 '13 at 08:58
  • @MohammadRB Deendency injection as you stated. Singletons make your code harder to read because of unknown side effects (the code executes a method on some globally available object that you don't pass in). DI makes it clear what your code depends on. – Wouter de Kort Oct 16 '13 at 09:00
  • @MohammadRB, as Wouter points out, you answer your own question: use DI instead of singletons. Downvote removed though as you updated your question to talk about DI too. – David Arno Oct 16 '13 at 09:05
  • 1
    @DavidArno: Beware of stereotypes! A static extension method (which you upvoted) is just as hard to test as a logging method contained in a singleton. – Heinzi Oct 16 '13 at 12:34
  • @Heinzi, The difficulty of testing extension methods is a factor of how well written they are. If written well, they are easy to test. A singleton is inherently non-unit-testable though and requires bodges (eg by compromising the singleton pattern to make it re-creatable) to enable singleton solutions to be properly unit tested. – David Arno Oct 16 '13 at 13:01
  • 1
    @DavidArno: I'm afraid don't get your point. Compare `public static SomeMethodA() { ... }` with `public static MyClass Instance = new MyClass(); private MyClass() {} public SomeMethodB() { ... }`. Both `SomeMethodA` and `SomeMethodB` are *exactly* equally hard to test! The only difference is that the first one is executed using `MyClass.SomeMethodA()` and the second one using `MyClass.Instance.SomeMethodB()`. – Heinzi Oct 16 '13 at 13:05
  • @Heinzi, I don't dispute that. However as SomeMethodB isn't a singleton, it's a somewhat moot point. – David Arno Oct 16 '13 at 13:09
3

I like the answers suggesting general solutions, however I would like to point out another one which works for MVC. If you have a common controller base (wich you should anyways, it's a Best Practice IMO). You can simply override the OnException method:

public class MyControllerBase : Controller
{
    protected override void OnException(ExceptionContext filterContext)
    {
        DoSomeSmartStuffWithException(filterContext.Exception);
        base.OnException(filterContext);
    }
}

Then simply inherit your normal controllers from your common base instead of Controller

public class MyNormalController : MyControllerBase 
{
    ...

If you like this you can check out the Controller class for other handy virtual methods, it has many.

Hari
  • 4,514
  • 2
  • 31
  • 33
  • I disagree about a common controller base being a best practice, in fact, I consider it a bad practice, unless there is good reason for it. The reason why it's a bad practice is that it encourages the base class to become a junk collector of common and semi-common data and methods, which bloats the controller. I prefer to use extension methods over common base classes in most situations. If you do create a common base class, then it should be as slim as possible and it should ONLY contain things which apply to all derived classes. – Erik Funkenbusch Oct 21 '13 at 04:27
  • In every MVC project I was part of there were logic common for all controllers. In a project where there isn't any this is not a best practice indeed, however I don't think there are many projects where this is true. On the other hand if I had developers who put wrong code at the wrong place all the time I would either educate or fire them. Using extension methods, filters or whatsoever instead of the pure OOP construct just to limit places where junk can be placed feels like sweeping dust under the carpet. – Hari Oct 24 '13 at 12:24
  • I have developed over 50 MVC apps, some of them quite large, and I have never needed a common base class. Yes, that was always what most developers wanted to jump to first as a solution, but there was always a better way to do it. For instance, recently, some developers wanted to have a common base class for data stored in the header of the master page. Instead, We used a Child Action with its own view model, which worked out much better and followed SRP. Common base classes tend to violate SRP and are simply the lazy way to do things, In my opinion. – Erik Funkenbusch Oct 24 '13 at 20:02
  • I have checked a couple of past projects having a common controller base to see what's in there. Well not much, usually some logging and extensions of the framework's Controller (eg. json methods using Json.net etc.) So I accept that this is not a best practice, but it is not a bad practice either. "Lazy way" is a pro not a contra. Well maybe I would consider a common base evil too if I had to struggle with developers putting wrong code to the wrong place all the time, but that won't make it a bad practice. – Hari Oct 30 '13 at 09:39
1

In ASP .NET MVC you can implement your own HandleErrorAttribute to catch all the exceptions that occur in all controllers:

public class CustomHandleErrorAttribute : HandleErrorAttribute
{
    public override void OnException(ExceptionContext filterContext)
    {
      var ex = filterContext.Exception;

      //     Open database connection
      //     Save exception details.
      //     If connection cannot be made to the database save exception in a text file.
    }
 }

Then register this filter:

public class FilterConfig
{
    public static void RegisterGlobalFilters(GlobalFilterCollection filters)
    {
       filters.Add(new CustomHandleErrorAttribute());
    }
 }

And of-course call the register method on application start-up:

public class MvcApplication : HttpApplication
{
    protected override void OnApplicationStarted()
    {
       // ...
       FilterConfig.RegisterGlobalFilters(GlobalFilters.Filters);
       // ...
    }
}

Wouter de Kort has already explained the concept behind this in his answer.

Community
  • 1
  • 1
kaptan
  • 3,060
  • 5
  • 34
  • 46