8
\$\begingroup\$

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.

\$\endgroup\$
13
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks to you and PDR for your comments. Both seem like logical choices, but as you mentioned, this is central to my app, so your solution works better in my case. \$\endgroup\$ – Dan Feb 23 '11 at 20:17
14
\$\begingroup\$

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 {}
\$\endgroup\$
  • 2
    \$\begingroup\$ This may not be the accepted answer, but it's the right one in my opinion. \$\endgroup\$ – RubberDuck Dec 20 '14 at 3:07
5
\$\begingroup\$

[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);
}
\$\endgroup\$
  • \$\begingroup\$ I would consider this bad design. You are using a Flags enum just to make the comparisons more convenient, but this goes against the semantics of the enum. Your design suggests that it is possible for someone to be a Manager and a Contractor at the same time, while explicitly not being an Employee... \$\endgroup\$ – Timwi Feb 23 '11 at 17:29
  • \$\begingroup\$ @Timwi - FWIW, a Contractor probably isn't going to be an Employee. However, I do see your point. The original question had only 0, 1, 2, 4 so I was thrown in the direction of Flags. If the question had been written as it is now, I might have given the same answer you did \$\endgroup\$ – pdr Feb 23 '11 at 17:57
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ +1 for a clean switch statement. I personally like the look of those. I also agree that the safer implementation would not rely on default, especially with an enum, I tend to use the default case to throw an invalid value exception. \$\endgroup\$ – pstrjds Feb 24 '11 at 14:12
2
\$\begingroup\$

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;
            }
        }
\$\endgroup\$
  • \$\begingroup\$ That won't DoSomething if c.Type is 3. \$\endgroup\$ – sepp2k Feb 23 '11 at 15:26
  • \$\begingroup\$ Fixed a glaring error. \$\endgroup\$ – Erin Feb 23 '11 at 15:31
2
\$\begingroup\$

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();
    }
}
\$\endgroup\$
  • \$\begingroup\$ Sorry for the confusion. You're right that this would work, but there are types that I don't want to show (updated question). \$\endgroup\$ – Dan Feb 23 '11 at 15:50
0
\$\begingroup\$

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();
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.