FxCop rookie question

[hansiman]

Just beginning using FxCop in my asp.net projects...

I get a lot of error messages under the header: AvoidUnusedParameters

for funtions and routines lik:

Sub isUserAuthenticated(ByVal blnLoggedIn As Boolean)
If Not (blnLoggedIn) Then
System.Web.HttpContext.Current.Response.Redirect(pcPageLogin)
End If
End Sub

Where the suggested resolution is:
"The 'this' parameter (or 'Me' in VB) of UtilsAdmin.
isUserAuthenticated(Boolean):Void is never used. Mark
the method as static (or Shared in VB) or use 'this'/Me
in the method body."

I don't get it? How do I use 'this'?

 

[Tor Bådshaug]

FxCop suggests that your method should be static,
as it does not use or depend upon any instance members (non-static state or
methods)
(What is the scope of pcPageLogin?).

How do I use 'this'?

'this' is a keyword (or in this context, as far as FxCop is concerned, an
implicit parameter) that may be used to access
the current instance in your code. Each time you reference instance data or
method FxCop considers that to be "using 'this'".

You should consider making the method static.
I don't think this FxCop rule should be taken literally, in my opinion it
should only be used as a tool for not forgetting to consider whether a
method should be static or not (my experience tells me that it is quite
common just to use objects and instance methods just by habit).
So, there will definetely be cases where you will want to break this
particular FxCop rule (sometimes you HAVE to).
As a rule of thumb, make members static when it doesn't make logical sense
to associate the member with a particular object instance.

Hope this helps.

Tor Bådshaug
tor.badshaug [//at\\] bekk.no.

 

[John Saunders]

Just beginning using FxCop in my asp.net projects...

I get a lot of error messages under the header: AvoidUnusedParameters

for funtions and routines lik:

Sub isUserAuthenticated(ByVal blnLoggedIn As Boolean)
If Not (blnLoggedIn) Then
System.Web.HttpContext.Current.Response.Redirect(pcPageLogin)
End If
End Sub

Where the suggested resolution is:
"The 'this' parameter (or 'Me' in VB) of UtilsAdmin.
isUserAuthenticated(Boolean):Void is never used. Mark
the method as static (or Shared in VB) or use 'this'/Me
in the method body."

I don't get it? How do I use 'this'?

FxCop is complaining that you do _not_ use Me.

For instance, if your Sub had said

If Not (blnLoggedIn) Then
Response.Redirect(pcPageLogin)
End If

Then it would not have complained, because "Response" is actually
"Me.Response". By Using HttpContext.Current.Response, you're going out of
your way to avoid using Me.Response.

(Why, by the way? Was that line transported from elsewhere?)

At any rate, the other way to meet FxCop's suggestion would be to declare
the sub as

Shared Sub isUserAuthenticated(ByVal blnLoggedIn As Boolean)


John Saunders

P.S. I don't know if FxCop will complain about it, but as long as we're
nitpicking, I'm going to complain about your choice of method name. A method
named Is<something> should be a Function returning Boolean, and should test
to see if the current instance is <something>, or if the supplied parameter
is <something>. An example would be IsBlank or IsNull or IsNumeric. From the
looks of things, you already know whether your user is authenticated, and
you're just looking to _do_ something with that knowledge.

I'd call the method MaybeRedirect or RedirectIfAuthenticated.

 

[Tor Bådshaug]

I generally agree John. I think you got the method name wrong, though.
What about "EnsureAuthenticated" or something?

Tor Bådshaug
tor.badshaug [//at\\] bekk.no

 

[John Saunders]

I generally agree John. I think you got the method name wrong, though.
What about "EnsureAuthenticated" or something?

Not really, since it's being informed of whether or not the user is
authenticated. In fact, if it weren't for the names of the methods and
parameters, we would have no reason to suspect that this method had anything
to do with authentication. Take a look at what you get if you rename:

Sub Something(a As Boolean)
If a Then
Response.Redirect(wherever)
End If
End Sub

It looks like this method conditionally redirects, or rather it's more like
"redirect if I tell you to".

I'd say some refactoring was in order. I wonder if that sub is really called
from multiple places with different expressions for "a", or do all callers
use the same expression? If that's the case, then the expression should be
moved inside of the Sub:

Sub RedirectIfNotAuthenticated
If Not Request.IsAuthenticated Then
Response.Redirect(pcPageLogin)
End If
End Sub


John Saunders
"There's nothing worse than a developer learning refactoring"

 

[Karl Seguin]

The message is saying that the isUserAuthenticated function doesn't appear
to be an instance method (in that it doesn't work against a specific
instance of a class), but instead seems to be more of a utility method. It
recommends (and from what I can tell, it's right), to declare the sub as
shared:

shared sub isUserAuthenticated()
...
end sub

If this "shared" word is new to you, you might wanna do some reading:
http://www.amazon.com/exec/obidos/ASIN/0321169514/panopticoncen-20/103-5479544-3615055

Karl

 

[Tor Bådshaug]

Yes, of course, I was a little quick there (although RedirectIfAuthenticated
actually was wrong - RedirectIfNotAuthenticated is fine), imagining the
method in question actually looked like
the RedirectIfNotAuthenticated method you suggest in your last post (which
is exactly the same
form of the method I have used for my EnsureAuthenticated-method in one of
my projects)....
Sorry.

Tor Bådshaug
tor.badshaug [//at\\] bekk.no

 

[John Saunders]

Yes, of course, I was a little quick there (although
RedirectIfAuthenticated
actually was wrong - RedirectIfNotAuthenticated is fine), imagining the
method in question actually looked like
the RedirectIfNotAuthenticated method you suggest in your last post (which
is exactly the same
form of the method I have used for my EnsureAuthenticated-method in one of
my projects)....
Sorry.

No problem. I understood the use of the term "Ensure" (perhaps you use
EnsureChildControls at times?).

As you point out, it's a matter of perspective. In my Forms Authentication
applications, I have been able to allow ASP.NET to do my ensuring for me,
and haven't found a need to call such a method. On the other hand, I _have_
had the need to do conditional redirects on my Forms Authentication login
page. This page can also be considered as the "unauthorized access handler
page", and a few such login pages of mine have had to redirect based on the
_reason_ the user was bounced back to the login page.

So, to me, this was a conditional redirect.

Different beholders, different eyes, same code.

John Saunders

Tor Bådshaug
tor.badshaug [//at\\] bekk.no

Sub RedirectIfNotAuthenticated
If Not Request.IsAuthenticated Then
Response.Redirect(pcPageLogin)
End If
End Sub


John Saunders
"There's nothing worse than a developer learning refactoring"

 

[hansiman]

Thanks for your insights on both FxCop and how to improve the code...
I use the sub redirect an unauthenticated user. I think I will
reconsider the security structure! Hadn't heard of
Request.IsAuthenticated! I'll dig into this.