c# 4.0 - best way to refactor a block of "If (something is Type) {}" statements?

Question

I've got some code that looks like this,

public void ResetControls(Control controlOnPage)
{
    if (controlOnPage is TextBox)
    {
        ResetTextBoxControl(controlOnPage);
    }

    if (controlOnPage is MediaPicker)
    {
        ((MediaPicker)controlOnPage).Media = null;
    }

    if (controlOnPage is RelatedContentPicker)
    {
        ((RelatedContentPicker)controlOnPage).RelatedContentCollection = null;
    }
    ...
    ...

    foreach (Control child in controlOnPage.Controls)
    {
        ResetControls(child);
    }
}

The idea behind it is that I can pass a page to the method and it'll recursively reset all the controls on it to their default states - in the case of MediaPicker and RelatedContentPicker, these are user controls that I've created.

FXCop warns me "Do Not Cast Unnecessarily" for this code - but I'm unsure how to rewrite it to make it better. Any ideas?

Answer 1

To avoid the duplicated cast, you could transform this:

if (controlOnPage is MediaPicker)
{
    ((MediaPicker)controlOnPage).Media = null;
}

to be:

MediaPicker mediaPickerControl = controlOnPage as MediaPicker;
if (mediaPickerControl != null)
{
    mediaPickerControl.Media = null;
}

Repeat for your other casts.

You could also combine this with some nested cases so that you don't have to try to do the cast if you know it was already something else, such as a Textbox

Answer 2

The most obvious thing to do is to throw in an else in front of the if's:

if (controlOnPage is TextBox)
{
    ResetTextBoxControl(controlOnPage);
}

else if (controlOnPage is MediaPicker)
{
    ((MediaPicker)controlOnPage).Media = null;
}

else if (controlOnPage is RelatedContentPicker)
{
    ((RelatedContentPicker)controlOnPage).RelatedContentCollection = null;
}

There is no need to try to cast to MediaPicker or RelatedContentPicker if the control is a TextBox. I'd also order them so that more common cases comes before less common cases.

Answer 3

I think that FXCop doesn't like the code, because a nice object-oriented solution would be to add virtual method ResetControl to the Control class (which you, of course, cannot do).

If you wanted a clear object-oriented solution, you could create an interface IResetableControl and create a derived class for each of the control that would implement the interface.

If you just want to make the existing code syntactically nicer, you can use the following helper method:

public void IfCast<T>(object obj, Action<T> f) {
  if (obj is T) f((T)obj);
}

Then you could just write:

IfCast(controlOnPage, (TextBox t) =>
    ResetTextBoxControl(t));

IfCast(controlOnPage, (MediaPicker mp) => {
    mp.Media = null; });

This has exactly the same semantics as your original code, but it is a bit nicer (and I think that FxCop will accept it). Note that the generic type parameter is infered from the type specified in the lambda expression e.g. (TextBox t). This also removes the need to do additional casting inside the code that handles the case, because you already get a value of the right type (e.g. mp).