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, it's 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.