1

I have a method that checks the exception passed in and returns a bool value.

Currently my implementation is like this

private bool ExceptionShouldNotify(Exception e)
{
    return
    e is FirstCustomException  ||
    e is SecondCustomException ||
    e is ThirdCustomException  ||
    e is FourthCustomException ||
    e is FifthCustomException  ||
    e is SixthCustomException  ||
    e is SeventhCustomException;
}

However is it better performance-wise to use a dictionary lookup rather than several OR statements and the is check?

Something like this:

private bool ExceptionShouldNotify(Exception e)
{
    var dict = new Dictionary<String, int> {
        { "FirstCustomException",   1 },
        { "SecondCustomException",  1 },
        { "ThirdCustomException",   1 },
        { "FourthCustomException",  1 },
        { "FifthCustomException",   1 },
        { "SixthCustomException",   1 },
        { "SeventhCustomException", 1 }
    };

    return dict.ContainsKey(e.GetType().Name);
}
sjc_w
  • 179
  • 14

3 Answers3

5

Hardcoding (1st solution) is a bad practice, that's why I vote for the dictionary (2nd solution), but I suggest different implementation of the idea:

   // HashSet - you don't use Value in the Dictionary, but Key
   // Type - we compare types, not their names
   private static HashSet<Type> s_ExceptionsToNotify = new HashSet<Type>() {
     typeof(FirstCustomException),
     typeof(SecondCustomException),
     ...
   };   

   // static: you don't use "this" in the method
   private static bool ExceptionShouldNotify(Exception e) {
     return s_ExceptionsToNotify.Contains(e.GetType());
   }

Having an exception caught (which include stack tracing) you've already had a big overhead; that's why performace (7 simple comparisons versus computing a hash) is not the main issue in the context

Dmitry Bychenko
  • 180,369
  • 20
  • 160
  • 215
  • I tried something like this Type y = ex.GetType(); switch (y) { case typeof(FirstCustomException): case typeof(SecondCustomException): case typeof(ThirdCustomException): case typeof(FourthCustomException): case typeof(FifthCustomException): case typeof(SixthCustomException): case typeof(SeventhCustomException): return true; break; default: return false; } But it says error as "A switch expression or case label must be a bool, char, string, integral, enum, or corresponding nullable type". Could you please explain me why switch doesn't work with "Type". – Sethu Bala Dec 14 '16 at 11:18
  • @Sethu Bala: please, do not *hardcode*: separate *data* (i.e. exceptions' types) and the *source code* (which should not depend on the data) – Dmitry Bychenko Dec 14 '16 at 11:22
  • I got that point, but my doubt is like why do switch case do not work with Type. – Sethu Bala Dec 14 '16 at 11:26
  • 1
    @Sethu Bala: http://stackoverflow.com/questions/298976/is-there-a-better-alternative-than-this-to-switch-on-type; seems that we have to wait for C# 7.0 – Dmitry Bychenko Dec 14 '16 at 11:30
1

It makes sense to look up the dictionary if you have considerable number of exception types to check. However, I would avoid using strings in this context. You can rely on the Type type instead:

var dict = new Dictionary<Type, int>()
{
    [typeof(FirstCustomerException)] = 1,
    [typeof(SecondCustomException)] = 1,
    ...
};

You can then look up the dictionary by the type of the exception object:

try
{
    ...
}
catch (Exception ex)
{
    int mapTo;
    if (dict.TryGetValue(ex.GetType(), out mapTo))
    {
        // Use mapTo value here
    }
}

You can even use the latest feature from C# 6 called exception filters to only catch those exceptions that exist in the dictionary:

try
{
    ...
}
catch (Exception ex) when (dict.ContainsKey(ex.GetType())
{
    int mapTo = dict[ex.GetType()];
    // Use mapTo value here
}

This implementation is one tiny bit slower because you have to call GetType twice, and also to perform dictionary mapping two times. But it comes with the benefit that it guarantees that your catch block will indeed handle the exception because it will be entered only for those exceptions that are in the map and not for any other type of exception.

Zoran Horvat
  • 10,924
  • 3
  • 31
  • 43
0

Yes it should. According to the Dictionary documentation the Contains method approaches O(1) while the chain of or statements is O(N).

https://msdn.microsoft.com/en-us/library/kw5aaea4(v=vs.110).aspx#Remarks

If you use the Dictionary, I would change the keys to use nameof(Exception) to make it bit more save against typos.

Mats391
  • 1,199
  • 7
  • 12