Recently, at one of our sprint retrospectives, somebody submitted the following comment:
There are too many suppressions (of FxCop warnings) in the code.
We should either clean those up or remove unneeded rules from the rule set.
Fair enough. But where to start?
First of all, out of curiosity, I checked how many suppressions there actually are in our code.
In a little over 1000 files we had over 1600 suppressions. Is that a lot?
Not sure, don't have much to compare this with.
But still, it doesn't mean that we should just suppress warnings and not try to fix them.
One of the warnings that I stumbled upon was the
CA1800:DoNoCastUnnecessarily warning.
(BTW, those MSDN documents on FxCop warnings are pretty good. And it's usually enough to just enter the warning ID (CA1800) into Google.)
Not too many suppressions in our code - about 20 - but I figured the fix is easy enough that I'll start with this one.
One of the standard constructs in our code that this warning is reported for goes something like this:
If TypeOf item Is Button Then
Dim button As Button = CType(item, Button)
. . .
End If
(DirectCast can be substituted for CType, but this still causes a warning.)
This is very easy to change:
Dim button As Button = TryCast(item, Button)
If button IsNot Nothing Then
. . .
End If
Is the change worth it? I find that the second version is usually easier to read (one check for type instead of two).
And shows the logic of the whole thing a bit better (IMO).
So, how about this?
If TypeOf item Is Button Then
BuildButton(CType(item, Button))
ElseIf TypeOf item Is Panel Then
BuildPanel(CType(item, Panel))
End If
One option is to just follow the pattern described above:
Dim button As Button = CType(item, Button)
If button IsNot Nothing Then
BuildButton(button)
Else
Dim panel As Panel = CType(item, Panel)
If panel IsNot Nothing Then
BuildPanel(panel)
End If
End If
But this is getting a bit too verbose (especially compared to the original version).
So, maybe this:
Dim button As Button = CType(item, Button)
Dim panel As Panel = CType(item, Panel)
If button IsNot Nothing Then
BuildButton(button)
ElseIf panel IsNot Nothing Then
BuildPanel(panel)
End If
But, funnily enough, CA1800 is a warning about performance; we're not gaining too much by always casting to two different objects...
And what about code like this (and longer)?
(And don't tell me about design issues here - this is legacy code and I'm not about to rework a core, and working, class just for the fun of it.)
If TypeOf item Is Button Then
BuildButton(CType(item, Button))
ElseIf TypeOf item Is Panel Then
BuildPanel(CType(item, Panel))
ElseIf TypeOf item Is X Then
BuildX(CType(item, X))
ElseIf TypeOf item Is Y Then
BuildY(CType(item, Y))
End If
Well, with just two conditions, I decided to resolve the warning; with more than two conditions - keep the suppression
and add a justification to the warning - all in all, this is a bit more readable (by being concise and by following the same pattern);
the multi-level nesting of Ifs that would be required wouldn't be that easy to parse...
A second case where we had this warning was in Linq queries; something like this:
Public Function FindViewModel() As IViewModel
Return
(From viewModel As IViewModel In DocumentService.OpenDocuments
Where (TypeOf viewModel Is IQueryViewModel)
AndAlso (CType(viewModel, IQueryViewModel).ProjectID = _projectID)
AndAlso (CType(viewModel, IQueryViewModel).IsDirty)
.FirstOrDefault
End Function
Whatever the meaning of IViewModel, IQueryViewModel and other properties above,
the point is that viewModel is cast to IQueryViewModel multiple times.
(And, I think, that we tend to overuse Linq queries - but that's a topic for another post, maybe.)
I
found these two nice suggestions on how Linq queries can be reworked:
1. Use the Let instruction to introduce a variable of the correct type, inner to the Linq query:
Public Function FindViewModel() As IViewModel
Return
(From viewModel As IViewModel In DocumentService.OpenDocuments
Let queryViewModel As IQueryViewModel = TryCast(viewModel, IQueryViewModel)
Where (queryViewModel IsNot Nothing)
AndAlso (queryViewModel.ProjectID = _projectID)
AndAlso (queryViewModel.IsDirty)
.FirstOrDefault
End Function
2. Use the OfType extension function (System.Linq) to filter objects of an IEnumerable:
Public Function FindViewModel() As IViewModel
Return
(From queryViewModel As IQueryViewModel
In DocumentService.OpenDocuments.OfType(Of IQueryViewModel)
Where (queryViewModel.ProjectID = _projectID)
AndAlso (queryViewModel.IsDirty)
.FirstOrDefault
End Function
I find the second approach a bit more elegant - we're straight away iterating over IQueryViewModel's only
and there's no need to test queryViewModel against Nothing.
Anyway, our developers (the author as well as reviewers) will need to keep an eye on
any new CA1800 suppressions from now on... ;-)
Top
|
Texts
