Enum string parse and get custom attribute assign to array

Question

I wrote a code which: - parse string to my enum - get description from enum - assign to string[] array

private readonly string[] allowed;

public AuthorizeRoles(params object[] arrayParam)
{
    allowed = (arrayParam.Select(myField => Enum.Parse(typeof (MyEnum), myField.ToString()))
        .Select(temp => temp.GetType().GetField(temp.ToString()))
        .Select(field => GetCustomAttribute(field, typeof (DescriptionAttribute)))).OfType<DescriptionAttribute>()
        .Select(attribute => attribute.Description)
        .ToArray();
}

Could you tell me if it possible to improve this code for better efficiency and clean code.

Answer 1

Overall code seems OK then I highlight just few minor issues (and I propose a slightly bigger change to simplify code).

1.

public AuthorizeRoles(params object[] arrayParam)

arrayParam is not a descriptive name, what is it? List of roles? Make it clear changing name to - for example - roles.

2.

arrayParam.Select(myField => Enum.Parse(typeof(MyEnum), myField.ToString()))

You're converting each arrayParam item to a string however you're not doing any check, if one of them is null then code will fail with NullReferenceException for a long line packed with a lot of code. Not so helpful. You may check in advance with:

if (arrayParam.Any(x => x == null))
    throw new ArgumentException("...");

Otherwise just let Enum.Parse() fail and use Convert.ToString(myField) won't throw for null input. Note that if type is restricted to be string or MyEnum then you may change your function definition accordingly:

public AuthorizeRoles(params string[] arrayParam)
public AuthorizeRoles(params MyEnum[] arrayParam)

Note that a weird user may have this:

object[] pars = null; // It may be a function argument...
AuthorizeRoles(pars);

Then you should also check if arrayParam is null itself:

if (arrayParam == null)
    throw new ArgumentNullException("arrayParam");

3.

.Select(field => GetCustomAttribute(field, typeof(DescriptionAttribute))))
.OfType<DescriptionAttribute>()

In this case you do not need OfType<DescriptionAttribute>() because values are just of one type, it's a minor optimization but I think it's better to don't confuse future readers and Cast<DescriptionAttribute>() or directly (DescriptionAttribute) before GetCustomAttribute().

---

Now if it's allowed to change little bit your code I think it may be simplified. Pick this part:

arrayParam.Select(myField => Enum.Parse(typeof(MyEnum), myField.ToString()))
    .Select(temp => temp.GetType().GetField(temp.ToString()))

First you convert input values to strings then you parse them to have MyEnum and finally you convert them back to string to use them with GetField(). One step should be omitted:

arrayParam.Select(x => Convert.ToString(x))
    .Select(x => typeof(MyEnum).GetField(x))

GetCustomAttribute() is a custom helper function, if you change it to this prototype:

private static T GetCustomAttribute<T>(FieldInfo field)
    where T : Attribute

Then last few lines of your code may be simplified (cast is not required and last .Select() may be merged in previous one). Note that with the generic argument T we don't even need a Type parameter (you can obtain type using typeof(T)).

Then if we rewrite whole function for an overview we have something like this:

public AuthorizeRoles(params object[] roles)
{
    if (roles == null)
        throw new ArgumentNullException("roles");

    if (roles.Any(x => x == null))
        throw new ArgumentException("...");

    allowed = roles.Select(x => Convert.ToString(x))
        .Select(x => typeof(MyEnum).GetField(x))
        .Select(x => GetCustomAttribute<DescriptionAttribute>(x).Description)
        .ToArray();    
}

The two .Select() may be merged but I'd not scarify readability just to keep code shorter and a minor performance gain (after all we're working with Reflection here...) You may also want to add some checks before the second .Select() or inside GetCustomAttribute<T>() to throw a meaningful error message if one of given values is not a valid enum element.