Should I Refactor This Code?

Question

I am working on modifying a control on a existing site. All controls from the site inherit form a base class. I have a requirement to hide several links on the master page so I wrote this method on my control:

private void HideCartLink (bool visible)
{
  Control control1 = Page.Master.FindControl( "link1" );
  control1.Visible = visible;

  Control control2 = Page.Master.FindControl( "link2" );
  control2.Visible = visible;
}

I then moved on to another control and I had to do the same thing. So I refactored my code and modified my base class with this:

public void HideMasterPageControl (string controlName, bool visible)
{
    Control control = Page.Master.FindControl( controlName );
    control.Visible = visible;
}

and added this method on my controls:

void CartLinkVisible(bool visible)
{
    ////hide cart link
    HideMasterPageControl("link1", visible);
    HideMasterPageControl("link2", visible);
}

Now I moved on to a third control and realized I have to do the same thing.

Should I refactor my code one more time so that my base class has a method that knows specifically which links to hide? Or should I leave my base class generic and let my controls decided what to hide?

I am not sure what the rule is here...

Answer 1

If I'm honest, I'd be more inclined to turn this upside-down. I'd have something that added the buttons/links as necessary, based on the criteria of the page. This would then give you something more like a toolbar, and in fact you could make it configurable (or based on user privileges) which buttons/links appear on each page.

This saves you from the awkward position of working out on what basis to hide things that may or may not even be there.

Of course, assuming you can't do this, you're probably making the best of a bad job to be honest. Don't worry too much further about the about the final implementation. With only a few controls you're okay and would be making it worse by over-complicating it.

You might want to consider the Visitor pattern, though. This would mean you implement a class that "visits" all the links on a given control and goes to configuration to look up whether they should be hidden or not. This would save you hardcoding the control names for the links, and allow the method to be in a base class that has no idea what actual controls there are.

Answer 2

I'd have a generic base method that would call a virtual method to get a list of links to hide and cycle through them. The base implementation of the second method would contain the list of most commonly hidden links. Each control I'd just overwrite the method to add or modify that list.

Though, I have to agree with Neil, it seems better if you flip the logic and decide which links need to be included vs. which need to be excluded.