views:

368

answers:

7

I have a method of an object which is something like a factory. You give it a type, it creates an instance and does a few other things. An elegant way to do it (in my opinion) is like this:

public T MagicMethod<T>() where T: SomeBaseClass
{
    // Magic goes here
}

But this upsets FxCop who says that this is a bad style - I get a "CA1004: Generic methods should provide type parameter" warning. Something about not being to use inference and stuff. So, the only other way I can think of is something like this:

public SomeBaseClass MagicMethod(Type T)
{
    // Same magic goes here
}

I believe this is inferior to the first method on many accounts, but the style rule... The MSDN article on the warning even says that there is no reason for suppressing it.

Am I doing it right by suppressing this warning after all?

A: 

The second approach is not even equivalent to the first. In the second one, you are literally given a type but you can't instantiate an object of that type (unless you use Reflection --- eeek!) and you have to explicitly declare the return type (which defeats the purpose of generics to begin with).

See this note about suppressing it. It looks like it is okay to suppress.

EDIT: Now here is another idea. What if you changed it to an 'out' parameter and didn't return it through the return variable? Would it remove the warning then?

public void MagicMethod<T>( out T retVar ) where T: SomeBaseClass
{
    // Magic goes here
}
j0rd4n
Probably, but that wouldn't be very comfortable to use. :)
Vilx-
True, can't fault you at that.
j0rd4n
A: 

Personally I would bother with most warning in Fxcop.

You seem to know what you are doing, why would some automated piece of software know better?

Well, it cant, it's a guess.

leppie
Well, I figured that a lot more talented and experienced developers than me have written that tool, and if they say I shouldn't do it, maybe I shouldn't. :)
Vilx-
+8  A: 
Adam Robinson
Well... actually, it's not exactly a constructor... it calls into another 3rd party library where I give the type as a simple Type parameter, and that then constructs the object... But those are unnecessary details anyway. :)
Vilx-
+3  A: 

I would have no issue with suppressing this warning. For starters the equivalent in MS's own code is Activator.CreateInstance<T>()

public static T CreateInstance<T>()

It implies that the analysis rule should consider whether the return type of the method is covered by the generic parameter...

This has been mentioned in many places before:

And there have been previous bugs in the rule for example:

public static void GenericMethod<T>(List<T> arg);

previously would trigger it (fixed in 2005 SP1).

I suggest filing a connect bug for your specific example

ShuggyCoUk
Phew. So it's not just me! :)
Vilx-
+2  A: 

FXCop warnings are just that - warnings. Just like implicit cast warnings, they serve to let you know that something you're doing may have behavior you're not anticipating, or may not be what you intended.

An implicit cast warning is dealt with by looking at the code, determinining if you really did intend to do that, and if so, adding an explicit cast.

Same thing with FXCop. Look at the warning, look at your code, and determine if the warning is valid. If it is, fix it. If not, suppress it. A suppression is the equivalent of an explicit cast - "Yes, FXCop, I'm sure I want to do this."

If it was really truly an error, it would probably be a compiler error.

kyoryu
Yes, but the MSDN page with the details on this warning explicitly states that "you should not suppress this warning". Honestly, am I smarter than the collective wisdom of the FXCop team?
Vilx-
A: 

First of all that warning is just to make sure that callers do everything knowingly. It is possible to call your method without passing any type parameter because the compiler knows the type of the object before hand. FxCop is telling you to let it be implicit so that syntax for using generic and non-generic overloads looks identical (I disagree with that principle but that is personal and not relevant here).

Secondly, your second method will do more damage than you can think of right now. There is no compile time type checking there, so be warned of runtime invalid cast exceptions if you use it.

Ak
+1  A: 

FxCop will fire that warning even if you do use the generic type parameter in one or more of the arguments, if it is not "stripped":

public void LinkedList<T> Slice<T>(LinkedList<T> collection, Predicate<T> match)
{
    ...
}

At least rule 'CA1004' fired "in error" here the other day on a method with this signature.

For being smarter than the FxCop team, I'm not sure that the rules are able to correctly determine code in all cases, that's what the confidence level is for :)

Cecil Has a Name