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.