Beware of switch

I am by no means against any of the instructions which exist in C#, including goto (wow, I said that). Each of them can have its place and its normal use in an application. However things can be abused and code gets nasty.

For example let’s consider we are displaying a hierarchy of people in an organization in a tree-like visual component. We will create a class that models a person :

public class Person
{
    public string Name { get; set; }
    public IEnumerable Subordinates { get; set; }
}

All is nice but some requests come in, such as : each type of person should have a distinctive icon at its left, the managers should have salutation type displayed etc.

Then you think “I know, I’ll create a PersonType enum”. That’s when things will begin to get nasty.

public enum PersonType
{
    Undefined,
    Simple,
    Manager
}

public class Person
{
    public IEnumerable Subordinates { get; set; }

    // new members :
    public PersonType Type { get; set; }
    public string Salutation { get; set; }
    private string _name;

    // modified members :
    public string Name
    {
        get
        {
             switch(Type)
             {
                 case PersonType.Simple: return _name;
                 case PersonType.Manager : return Salutation + " " + _name;
                 default : throw new InvalidOperationException();
             }
        }
        set { _name = value; }
    }

    public Image Icon
    {
        get
        {
            switch(Type)
            {
                 case PersonType.Simple: return Icons.SimplePersonIcon;
                 case PersonType.Manager : return Icons.ManagerIcon;
                 default : throw new InvalidOperationException();
            }
        }
    }
}

If you find yourself writing switches in one or more properties or methods of the class, switching on the same thing (PersonType here) then stop it. You are writing hard-to-maintain and error-prone code. Plus its inelegant. This practically is a cry for polymorphism.

The right thing to do™ is to have three classes : an abstract Person class, a SimplePerson class (inheriting the Person class) and a ManagerPerson class (also inheriting the Person class) :


public abstract class Person
{
    public virtual string Name { get; set; }
    public IEnumerable Subordinates { get; set; }
    public abstract Image Icon { get; }
    public string Salutation { get; set; }
}

public class SimplePerson : Person
{
    public override Image Icon { get { Icons.SimplePersonIcon; } }
}

public class ManagerPerson : Person
{
    private string _name;

    public override Image Icon { get { Icons.ManagerIcon; } }
    public override string Name
    {
        get
        {
            return Salutation + " " + _name;
        }
        set
        {
            _name = value;
        }
    }
}

Notice that the code is shorter, simpler to follow and understand. Furthermore although we did introduce two more classes, we also removed the PersonType enum so at a total we introduced only one more class.

Leave a comment ?

21 Comments.

  1. Congrats, mate!
    You’ve just discovered polymorphism :-)

    • Thank you. However only now I managed to write a small article about this so when I need to show this to someone who doesn’t know this OOP thing I’ll just point to the URL. I have discovered polymorphism earlier :)

  2. This is nice in some cases. But what happens when you promote a subordinate to manager ? …

    NOTE: you’ve helped me to writhe this on stackoverflow about 2 years ago :smile:

    http://stackoverflow.com/questions/4254182/inheritance-vs-enum-properties-in-the-domain-model

  3. Jonathan Allen

    And what if you have two factors that you need to switch on? Create NxM subclasses?

    This is a classic example if prematurely using inheritance to solve a non-problem.

    • Well, maybe the example is a bit flawed on the fact that the role of the person should be detached/detachable from the person without requiring complex things.

      However I would not recommend splitting the classes even more as you suggest (i.e.: probably split Person into SmartPerson, StupidPerson, NastyPerson, StupidNastyPerson and so on)

  4. Switch is an extended if/else construct, which is abused in most of the OOP languages. Good application of this refactoring http://sourcemaking.com/refactoring/replace-conditional-with-polymorphism.

  5. +1 Jonathan Allen. I thought the same thing when reading the article.

    Remember that we are generally using classes as abstractions for data in tables in databases. Yeah, I know that most of us think it’s the other way around, but it is not! Most business apps are data driven.

    You will want to store all persons in a persons table and add a column called manager. And then a salutation column. And then a department column. And so on.

    Pretty quick you will find another pattern useful: the Manager Pattern (pun intended) where you have a DepartmentManager class that accepts persons as parameters and does things with them based on their type. And then you have two problems :)

  6. Hi Andrei, nice post. Unfortunately large swaths of programmers will go for enum and argue against your choice. Why? Because this pretty thing called polymorphism doesn’t adhere nicely (as it’s OOP) in some scenarios with ORMs if you do it wrong (the ORM related side). Basically, it’s easier to write in the Person table an EnumID and fill your logic with switch after switch than to devise a proper implementation. Hint: polymorphism over collections in NHibernate; another hint: proxy over the base class, not over the inheritors. Cure: visitor pattern, which is a nice little pattern from GOF. Developers developers developers uhm… patterns patterns patterns.

    I know your opinion regarding ORMs. I value it and I’d follow it for small projects (like less than 15 entities).

    Oh, and I might be wrong. After all, I’m still growing up. Cheers! (From the biutiful lendscheip of semanatoarea)

  7. Nice and simple article.

    Beside the perfect example of inclusion polymorphism, the first code sample(switching on the person type enum) also shows a simple example of rigidity(referred to in the article as “You are writing hard-to-maintain and error-prone code”)

    In fact, I think your first code sample is a more subtle example of Liskov Substituion Principle violation. It is not the typical violation where you have some client code using a reference of an abstraction type but needing to know about all the concrete implementations but rather a single concrete implementation that can have various “flavors” which are represented by an enum instead of using polymorphism. This single concrete implementation then needs to know about all possible flavors in order to determine something in its inner mechanisms (name and icon to display). This is pure rigidity as, needing to add new flavors in the future, one has to change the existing code in class Person.

    And indeed, although the “switch” instruction exists in C# and other object oriented languages for convenience, it is most of the time (if not always) a clear smell of rigidity.

Leave a Comment


NOTE - You can use these HTML tags and attributes:
<a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>

Trackbacks and Pingbacks:

WP Like Button Plugin by Free WordPress Templates