Why does FxCop insist on this?

  • Thread starter Thread starter Karsten Schramm
  • Start date Start date
K

Karsten Schramm

I would write

BaseClass var;
.....
if(var is DerivedClass)
{
DerivedClass derivedVar = (DerivedClass)var;
DoSomething(derivedVar);
}



Unfortunately company policies requires FxCop and FxCop complains about
the above. FxCop wants it this way:

BaseClass var;
DerivedClass derivedVar;
....
derivedVar = var as DerivedClass;
if(derivedVar != null)
{
DoSomething(derivedClass);
}


I think it's kinda pointless to define a variable in a scope where it
isn't really needed just to comfort FxCop. :-/
 
Karsten,
FxCop is merely an intelligent rules robot. If your organization doesn't
like a particular rule, you can either change it, or just turn it off.
Peter
 
Karsten said:
I would write

BaseClass var;
....
if(var is DerivedClass)
{
DerivedClass derivedVar = (DerivedClass)var;
DoSomething(derivedVar);
}



Unfortunately company policies requires FxCop and FxCop complains about
the above. FxCop wants it this way:

BaseClass var;
DerivedClass derivedVar;
...
derivedVar = var as DerivedClass;
if(derivedVar != null)
{
DoSomething(derivedClass);
}


I think it's kinda pointless to define a variable in a scope where it
isn't really needed just to comfort FxCop. :-/

Well, it's a matter of taste, isn't it? I mean, it's annoying to have
to change the variable's scope, but then it's inefficient to do the
cast twice (as it was in the original code).

So... choose your poison: inappropriate variable scope or doing double
the work to no good purpose.
 
Karsten Schramm said:
I would write

I think it's kinda pointless to define a variable in a scope where it
isn't really needed just to comfort FxCop. :-/

Well, you can always add an extra level of scoping if you want,
although that's pretty ugly. I suspect the reason it complains is
because you're effectively doing the same work twice. The difference in
performance is likely to be negligible, but that's likely to be the
reason...
 
Hi,

Well rules are rules :)

and in this case there is a little pointless, you have in one side:
1- declare an extra variable
2- do a cast once.

and in the other side:
1- do not declare an extra variable
2- do the cast twice

by the way did you try :

if(var is DerivedClass)
{
DoSomething((DerivedClass)var);
}

You may get the code you like and not a complain :)
 
Well rules are rules :)

I wonder who made these rules.
I like the .net guidelines, but a lot of rules marked as "Error" or
"CriticalError" in FxCop are really nonsense or impossible to follow in some
situations.
 
* Ignacio Machin ( .NET/ C# MVP ) wrote, On 26-7-2006 19:51:
Hi,

Well rules are rules :)

and in this case there is a little pointless, you have in one side:
1- declare an extra variable
2- do a cast once.

and in the other side:
1- do not declare an extra variable
2- do the cast twice

by the way did you try :

if(var is DerivedClass)
{
DoSomething((DerivedClass)var);
}

You may get the code you like and not a complain :)


You can always try this:

DoSomething(var as DerivedClass);

As you're not checking against null anyways the additional if statement
isn't needed. And with the if gone, no variable assignment is needed
either. And with the as, an invalidcastexception won't even happen...

But: You did create a possible nullreferenceexception...

Jesse
 
Jesse Houwing said:
You can always try this:

DoSomething(var as DerivedClass);

As you're not checking against null anyways the additional if statement
isn't needed.

The behaviour is then completely different.

With the original code, DoSomething() isn't called if I pass in a non-
DerivedClass. With your code, it is.
 
Alvin Bruney said:
the rules are configured in an xml file. find it and remove it or just
ignore it.

Yes, you're right.
I only think that a ruler marked as "Error" should be a followed (standard)
guideline.
If that rule is stupid should I consider that the guideline is stupid or
that the rule souldn't be marked as Error?
 
Hi,

You can always try this:

DoSomething(var as DerivedClass);

As you're not checking against null anyways the additional if statement
isn't needed. And with the if gone, no variable assignment is needed
either. And with the as, an invalidcastexception won't even happen...

The OP code execute DoSomething ONLY if it's an instance of DerivedClass ,
I'm pretty sure it will not handle a null.

You subtlely changed the behavior
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Back
Top