0

I'm trying to make a function that can take in a parameter of class Parent that is actually of either type Child1, Child2, or Child3. I want to do something different based on which child type it is and I can't think of a way to do it other than what I am showing below. This feels wrong to me so any suggestions for a better way to do this would be much appreciated.

public static bool DoStuff(Parent parent)
{
    try
    {
        Child1 child = parent as Child1;
        DoStuffChild1(child);
    }
    catch (Exception)
    {
        try
        {
            Child2 child = parent as Child2;
            DoStuffChild2(child);
        }
        catch(Exception)
        {
            try
            {
                Child3 child = parent as Child3;
                DoStuffChild3(child);
            }
            catch (Exception)
            {
                HandleError();
            }
        }
    }
}
cpcolella
  • 247
  • 4
  • 10
  • Is C# 7 an option? The pattern matching feature is certainly a better solution than this. – Jonathon Chase Feb 14 '18 at 19:10
  • 3
    Even excluding that, why do you need the exception handling? If the cast fails then child should be `null`. – Evan Trimboli Feb 14 '18 at 19:12
  • You're absolutely right, this is wrong. Not only would I reject this in a code review I'd take you out back and shoot you ;) Look up `typeof` and `.GetType()` - if you want, I'll write an answer covering those options. – Jamie - Fenrir Digital Ltd Feb 14 '18 at 19:14
  • Yeah, like @EvanTrimboli said, this won't work the way you're expecting it to as-is, since the cast failure will just result in an assignment of null rather than an exception. – pushasha Feb 14 '18 at 19:14
  • I assume it's catching the NullReferenceException that ends up occurring. So, exceptions as control flow. – Jonathon Chase Feb 14 '18 at 19:15
  • @JonathonChase Nothing in the code shown would throw a `NullReferenceException` – Rufus L Feb 14 '18 at 19:23
  • @RufusL - It certainly could depending on how `DoStuffChild1` etc are implemented. – Lee Feb 14 '18 at 19:26
  • @Lee that doesn't make it right ;) – Jamie - Fenrir Digital Ltd Feb 14 '18 at 19:27
  • @Lee I meant the explicit code shown. Yes, it could throw that or just as likely an `ArgumentNullException` depending on how those methods were implemented. – Rufus L Feb 14 '18 at 19:32
  • 1
    I know this would work because Visual Studio required a try-catch around the "as" cast here for the case where it cannot cast it to the child class. However, it is so disgusting that I could never submit for any sort of review. @EvilGeniusJamie I almost wanted to take myself out back and soot me for thinking of this. Thanks for all the answers. – cpcolella Feb 14 '18 at 19:59

3 Answers3

3

You can simply use is keyword instead:

public static bool DoStuff(Parent parent)
{
            try
            {
                if (parent is Child1)
                {
                    Child1 child = parent;
                    DoStuffChild1(child);
                }
                else if (parent is Child2)
                {
                    Child2 child = parent;
                    DoStuffChild2(child);
                }
                else if (parent is Child3)
                {
                    Child3 child = parent;
                    DoStuffChild3(child);
                }
            }
            catch (Exception)
            {
                HandleError();
            }
}

is returns true if an instance is in the inheritance tree.

Additionally, following is a very good answer on type checking: https://stackoverflow.com/a/983061/4222487

FaizanHussainRabbani
  • 3,256
  • 3
  • 26
  • 46
2

If you're using C# 7.0 or later, you can achieve what you're looking for with the below code:

if (parent is Child1 child1)
{
    DoStuffChild1(child1);
}
else if (parent is Child2 child2)
{
    DoStuffChild2(child2);
}

Hope this helps.

Artak
  • 2,819
  • 20
  • 31
0

This method will work on older versions of C# if C# 7 is not an option.

Don't use try/catch as control flow. Ever.

If you need to do something different depending on the class of the object, you can use typeof and .GetType(). In your example

public static bool DoStuff(Parent parent)
{
    if (parent.GetType() == typeof(Child1))
    {
        Child1 child = parent as Child1;
        DoStuffChild1(child);
    }
    else if (parent.GetType() == typeof(Child2))
    {
        Child2 child = parent as Child2;
        DoStuffChild2(child);
    }
    else if (parent.GetType() == typeof(Child3))
    {
        Child3 child = parent as Child3;
        DoStuffChild3(child);
    }
    else
    {
        HandleError();
    }
}

is a simple way to handle it.

The reason you would use this over the is keyword in FaizanRabbani's answer is that this method is a strict type check, ignoring super-classes - for example:

if (parent is Parent)

would evaluate true, but

if (parent.GetType() == typeof(Parent))

would evaluate false, if it was Child1, Child2 or Child3, for example.

Jamie - Fenrir Digital Ltd
  • 1,337
  • 8
  • 20