current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 12 down vote favorite

I have the following enum extension method:

public static IList<SelectListItem> SelectListFor<T>()
{
  Type enumType = typeof(T);

  if (enumType.IsEnum)
  {
    return Enum.GetValues(enumType)
    .Cast<int>()
    .Where(i => !i.Equals(0))
    .Select(e => new SelectListItem()
            {
            Value = e.ToString(),
      Text = GetDisplayName(enumType, Enum.GetName(enumType, e))
    })
    .ToList();
  }

  return null;
}

I can call this by doing

EnumExtensions.SelectListFor<BrochureTypes>()

And it will allow me to create a drop down list for the values in my BrochureTypes enum

However, when I compile the code, I get the following warning:

CA1004 Consider a design where 'EnumExtensions.SelectListFor()' doesn't require explicit type parameter 'T' in any call to it.

Can I somehow change the above code to overcome this warning? Or should I just suppress the warning?

share|improve this question
    
Did you read the MSDN article on this? You need to provide a type as a parameter when using generic methods. – Quill Jan 13 at 14:27
2  
This question is being discussed on meta. – Dan Pantry Jan 13 at 15:16
up vote 5 down vote accepted

As written in the relative MSDN page, the cause of the warning is

The parameter signature of an externally visible generic method does not contain types that correspond to all the type parameters of the method.

Looking at your code, you get the type of the generic param into the method body (which can be seen also as a violation of the Separation of Concerns principle). In order to solve the issue, you can pass the type as a parameter. The resulting code should be something like the following:

public static IList<SelectListItem> SelectListFor(Type enumType)
{
    if (enumType.IsEnum)
    {
        return Enum.GetValues(enumType)
            .Cast<int>()
            .Where(i => !i.Equals(0))
            .Select(e => new SelectListItem()
            {
                Value = e.ToString(),
                Text = GetDisplayName(enumType, Enum.GetName(enumType, e))
            })
            .ToList();
    }

    return null;
}

and can be called in the following way:

EnumExtensions.SelectListFor(typeof(BrochureTypes))

As an alternative, you can make it an extension method of the Type class, and I'd suggest a rename of the method in this case:

public static IList<SelectListItem> GetSelectList(this Type enumType)
{
    if (enumType.IsEnum)
    {
        return Enum.GetValues(enumType)
            .Cast<int>()
            .Where(i => !i.Equals(0))
            .Select(e => new SelectListItem()
            {
                Value = e.ToString(),
                Text = GetDisplayName(enumType, Enum.GetName(enumType, e))
            })
            .ToList();
    }

    return null;
}

and call it like this:

(typeof(BrochureTypes)).GetSelectList();

or:

enumInstance.GetType().GetSelectList();
share|improve this answer
1  
This gets around the warning but it just doesn't seem as nice having to pass typeof(BrochureTypes) into a function rather than just calling SelectListFor<BrochureTypes>() – Pete Jan 13 at 14:54
2  
@Pete I can agree with you, but from a design point-of-view you are working with a Type object. If a method is working with an object it makes perfect sense to pass it as a parameter. – Gentian Kasa Jan 13 at 15:00
    
Ah, nice edit I think I will go with the extension method, but put it onto the actual enum and make it use all but the selected one so that the 0 in !i.Equals(0) doesn't have to be a hard coded value – Pete Jan 13 at 15:12
up vote 6 down vote

Naming

Type enumType = typeof(T);

this name is misleading because one would assume that its always an enum which you later check by reading the IsEnum property. So just name it type or pay the price and call typeof(T) twice.

General

I usualy tend to return a empty List<T> instead of null because that makes any null check redundant.

I would suggest reverting the if condition to return early. In its current form its not quite obvious (maybe because of the indentation) how/where the method returns.

Warning

I somehow couldn't reproduce the warning here on my side, but as a hacky workaround you can pass a optional parameter to that method.

Implementing the mentioned points will lead to 

public static IList<SelectListItem> SelectListFor<T>(T dummy = default(T))
{
    // using the dummy parameter to omit the CA1801 warning
    if (!dummy?.GetType().IsEnum) // C# 6.0 feature
    {
        return new List<SelectListItem>();
    }

    Type enumType = typeof(T);

    return Enum.GetValues(enumType)
        .Cast<int>()
        .Where(i => !i.Equals(0))
        .Select(e => new SelectListItem()
            {
                Value = e.ToString(),
                Text = GetDisplayName(enumType, Enum.GetName(enumType, e))
            })
        .ToList();

}
share|improve this answer
    
Nice, but unfortunately I wouldn't be able to use this as the above would fail on CA1026, meaning I would have to provide an overload with no parameters calling the the above, which would bring us back to the original problem – Pete Jan 13 at 15:37

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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