Refactoring a bunch of and statements

Question

Trying to refactor this code and can't seem to think of a way to make it cleaner. Type is a property in my Person class.

foreach (var item in list)
{
    if (person.Type != PersonType.Employee && person.Type != PersonType.Manager && person.Type != PersonType.Contractor && person.Type != PersonType.Executive)
    {
        DoSomething();
    }
}

public enum PersonType : int {
   Employee = 0,
   Manager = 1,
   Contractor = 2,
   President = 3,
   Executive = 4
}

I should note that there are other types in the PersonType class which I don't want to show.

Answer 1

If the “allowedness” of a PersonType value is specific to the particular algorithm you are writing, I usually write this as an array:

var disallowedTypes = new[] {
    PersonType.Employee,
    PersonType.Manager,
    PersonType.Contractor,
    PersonType.Executive
};

foreach (var item in list.Where(p => !disallowedTypes.Contains(p.Type)))
    DoSomething(item);

If this set of disallowed types is central to your application (or to a particular class), I would declare it as a static field in a static class (or the relevant class). That way your entire program (or that class) has convenient access to it, and you only need to update it in a single place if the business logic changes:

public static class Data
{
    public static readonly PersonType[] DisallowedTypes = {
        PersonType.Employee,
        PersonType.Manager,
        PersonType.Contractor,
        PersonType.Executive
    };
}

// [ ... ]

foreach (var item in list.Where(p => !Data.DisallowedTypes.Contains(p.Type)))
    DoSomething(item);

If the set of disallowed types is inherent in the semantics of the PersonType enum itself, I would make this very explicit by using custom attributes in the enum type itself. Of course you should think up a more descriptive name than IsAllowed and fix the XML comment on the attribute type to explain what it really means:

/// <summary>Specifies that a <see cref="PersonType"/> value is allowed.</summary>
public sealed class IsAllowedAttribute : Attribute { }

public enum PersonType
{
    Employee,
    Manager,
    Contractor,
    Executive,

    [IsAllowed]
    President
}

public static class Data
{
    /// <summary>This array is automatically prefilled with the
    /// PersonType values that do not have an [IsAllowed] custom attribute.
    /// To change this array, change the custom attributes in the
    /// <see cref="PersonType"/> enum instead.</summary>
    public static readonly PersonType[] DisallowedTypes;

    static Data()
    {
        DisallowedTypes = typeof(PersonType)
            .GetFields(BindingFlags.Public | BindingFlags.Static)
            .Where(f => !f.IsDefined(typeof(IsAllowedAttribute), false))
            .Select(f => f.GetValue(null))
            .ToArray();
    }
}

This way the information whether any particular enum value is allowed or not is stored in the enum itself and no-where else. That makes the code very explicit, easy to follow, and easy to modify in obviously-correct ways.

Answer 2

This is such a perfect match for the Replace Conditional with Polymorphism Refactoring that it even looks as if it was specifically designed to demonstrate the Replace Conditional with Polymorphism Refactoring. I mean, the field it is basing its behavior on is even called Type!

interface Person { void DoSomething(); }

class PrivilegedPerson : Person {
    public void DoSomething() {
        // do something
    }
}

class UnprivilegedPerson : Person {
    public void DoSomething() {} // literally do nothing
}

class Employee   : UnprivilegedPerson {}
class Manager    : UnprivilegedPerson {}
class Contractor : UnprivilegedPerson {}
class President  : UnprivilegedPerson {}
class Executive  : UnprivilegedPerson {}

Answer 3

[Edit] Corrected given edit to question

Try something more like this

[Flags]
public enum PersonType
{
    None = 0,
    Employee = 1 << 0,
    Manager = 1 << 1,
    Contractor = 1 << 2,
    President = 1 << 3,
    Executive = 1 << 4
}

foreach (var item in list)
{
    if ((person.Type & PersonType.President) == 0)
    {
        DoSomething();
    }
}

You can now also add combined values such as

[Flags]
public enum PersonType
{
    None = 0,
    Employee = 1 << 0,
    Manager = 1 << 1,
    Contractor = 1 << 2,
    President = 1 << 3,
    Executive = 1 << 4,
    NotPresident = Employee | Manager | Contractor | Executive,
    Boss = Manager | President | Executive
}

Given this enumeration, this NUnit test passes

[Test]
public void TestEnum()
{
    var personType = PersonType.Manager | PersonType.Contractor;
    Assert.That(personType == PersonType.None, Is.Not.True);
    Assert.That((personType & PersonType.Manager) != 0, Is.True);
    Assert.That((personType & PersonType.Boss) != 0, Is.True);
    Assert.That((personType & PersonType.Employee) != 0, Is.Not.True);
}

Answer 4

Assuming you don't want to turn PersonType into an actual polymorphic class, a simple cleanup is:

foreach (var item in list)
{
    switch (person.Type) {
    case PersonType.Employee:
    case PersonType.Manager:
    case PersonType.Contractor:
    case PersonType.Executive:
        // Do nothing.
        break;
    default:
        DoSomething();
    }
}

You have to be careful with enums, though. If a new PersonType is added, is DoSomething() the correct behavior for that type? A safer implementation won't rely on default.

Answer 5

I would probably go with code that looked like this.

list.ForEach(c => 
                {
                    if (DoAction(c.Type))
                    {
                        DoSomething();
                    }
                });


private static bool DoAction(PersonType personType)
        {
            switch (personType)
            {
                case PersonType.Employee:            
                case PersonType.Manager:            
                case PersonType.Contractor:           
                case PersonType.Executive:
                    return false;
                default:
                    return true;
            }
        }

Answer 6

You could use Enum.IsDefined. It has some disadvantages. It can be slow, and it is not strongly typed, but I do like how it reads.

foreach (var item in list)
{
    if (!Enum.IsDefined(typeof(PersonType), person.Type))
    {
        DoSomething();
    }
}

Answer 7

If this is just a one shot test, then I would use something like this. If this was used in multiple places, I would probably use flags like pdr.

PersonType[] ignoredPersonTypes = 
{
    PersonType.Employee,
    PersonType.Manager,
    PersonType.Contractor,
    PersonType.Executive,
}

foreach (var item in list)
{
    if (ignoredPersonTypes.Contains(person.Type) == false)
    {
        DoSomething();
    }
}