7
\$\begingroup\$

I have a switch statement used to handle options that a user selected (stored in an array). The array is iterated through, and for each element (enum object) in the array, an appropriate action is performed. However, the enum has over 50 items, so the switch statement is very long.

The items are all necessary and related, so they cannot be moved or removed. They also each call a method or get a property of an object, so no dictionaries. Are there any other options to make this shorter?

public string GetUserAttributes(UserPrincipal user,
    ActiveDirectoryUserAttribute[] attributes)
{
    var userAsDirectoryEntry
        = (DirectoryEntry)user.GetUnderlyingObject();
    var userAttributes = string.Empty;
    foreach (ActiveDirectoryUserAttribute attribute in attributes)
    {
        switch (attribute)
        {
            case ActiveDirectoryUserAttribute.AccountExpirationDate:
                userAttributes += AttributeToString(
                    user.AccountExpirationDate.ToString());
                break;
            case ActiveDirectoryUserAttribute.AccountLockoutTime:
                userAttributes += AttributeToString(
                    user.AccountLockoutTime.ToString());
                break;
            case ActiveDirectoryUserAttribute.Assistant:
                userAttributes += AttributeToString(
                    userAsDirectoryEntry.Properties[AttributeAssistant]
                    .Value.ToString());
                break;
            case ActiveDirectoryUserAttribute.BadLogonCount:
                userAttributes += AttributeToString(
                    user.BadLogonCount.ToString());
                break;
            ...
        }
    }
    return userAttributes;
}
\$\endgroup\$
  • \$\begingroup\$ You can use Reflection: it can be an elegant solution and the whole switch disappears, but one condition is needed: the methods you call... are they all without input parameters (like property getters do)? If yes, I'll post a code snippet with the solution; otherwise, a switch/enum or a dictionary is necessary. \$\endgroup\$ – Massimiliano Kraus May 29 '16 at 14:58
  • \$\begingroup\$ @MK87 Some of the methods do require parameters, and are not just property getters/setters. \$\endgroup\$ – Michael Brandon Morris May 30 '16 at 2:33
  • \$\begingroup\$ Mhm... can you add a snippet of a method with parameters in your code? Where you get the parameters from? \$\endgroup\$ – Massimiliano Kraus May 30 '16 at 9:25
  • \$\begingroup\$ @MK87 The only parameters are the UserPrincipal and the DirectoryEntry. They are in the form GetSomeString(UserPrincipal user, DirectoryEntry userAsDirectoryEntry). \$\endgroup\$ – Michael Brandon Morris May 30 '16 at 10:12
4
\$\begingroup\$

You can put the switch-cases into a dictionary to reduce the boilerplate case and break. The foreach and the string concatenation part can also be reduced with string.Concat(...) or string.Join(delimiter, ...):

public string GetUserAttributes(UserPrincipal user, ActiveDirectoryUserAttribute[] attributes)
{
    var userAsDirectoryEntry = (DirectoryEntry)user.GetUnderlyingObject();

    var getters = new Dictionary<ActiveDirectoryUserAttribute, Func<object>>();
    getters[ActiveDirectoryUserAttribute.AccountExpirationDate] = () => user.AccountExpirationDate;
    getters[ActiveDirectoryUserAttribute.AccountLockoutTime] = () => user.AccountLockoutTime;
    getters[ActiveDirectoryUserAttribute.Assistant] = () => userAsDirectoryEntry.Properties[AttributeAssistant].Value;
    getters[ActiveDirectoryUserAttribute.BadLogonCount] = () => user.BadLogonCount;
    // ...

    // just concat them directly, like you are doing now
    return string.Concat(attributes.Select(attr => getters[attr]()));

    // or, you can separate them with a comma, or anything :
    return string.Join(", ", attributes
        .Select(attr => getters[attr]()));
}

You can also use the collection initializer for dictionary to cut down some letters :

var getters = new Dictionary<ActiveDirectoryUserAttribute, Func<object>>
{
    // C# 6
    [ActiveDirectoryUserAttribute.AccountExpirationDate] = () => user.AccountExpirationDate,
    [ActiveDirectoryUserAttribute.AccountLockoutTime] = () => user.AccountLockoutTime,

    // older version
    { ActiveDirectoryUserAttribute.Assistant, () => userAsDirectoryEntry.Properties[AttributeAssistant].Value },
    { ActiveDirectoryUserAttribute.BadLogonCount, () => user.BadLogonCount },

    // ...
};

Personally I prefer the C# 6 collection initializer the most, second by the method shown in first snippet, and lastly the older initializer.

\$\endgroup\$
  • 1
    \$\begingroup\$ Just a note, the semicolon needs to go after the closing brace of the collection initializer, not after the open/close parentheses. \$\endgroup\$ – Michael Brandon Morris May 27 '16 at 17:20
  • 3
    \$\begingroup\$ You can also omit the parent's empty constructor parentheses in collection/object initializers. \$\endgroup\$ – Dan Lyons May 27 '16 at 18:07
  • \$\begingroup\$ Ahh, Copy-paste programming! The source of all evil! A syntax error and 1 style problem in a single line... \$\endgroup\$ – Xiaoy312 May 27 '16 at 18:24

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.