CODEWITHSUNDEEP
c#refactoringcode-duplication
Pete McPhearson
Pete McPhearson

Reputation: 469

Refactoring c# : How to remove duplicated code when properties to update differ in each case

After hacking about for a little while on a prototype I've ended up with number of methods that update boolean flags on an object and then update interface and do some processing based on the new value. These are pretty much all the same - but the value they update is different

for example - imagine we have a bunch of colored boxes to update - I might have some methods that look like this:

        protected void SetBlueBoxVisibility(bool blueBoxVisibility)
    {
        Project project = LoadProject();
        project.ShowBlueBox = blueBoxVisibility
        ReDrawSomeThings();
        CalcualteSomeStuff();
        Project.UpdateBoxStatus();
        SaveProject(project);
        ShowBlueBoxPanel(blueBoxVisibility);
        RaiseStatusUpdated();
    }

    protected void SetRedBoxVisibility(bool redBoxVisibility)
    {
        Project project = LoadProject();
        project.ShowRedBox = redBoxVisibility
        ReDrawSomeThings();
        CalcualteSomeStuff();
        Project.UpdateBoxStatus();
        SaveProject(project);
        ShowRedBoxPanel(redBoxVisibility);
        RaiseStatusUpdated();

    }

Now, obviously - most of that stuff is repeated - which is a pain when I come to change anything. Particularly if I end up with twenty different box colors rather than just two!

I was thinking that there must be a way to strip out the code that changes and gather the stuff that's the same in a more generic method - but I'm having trouble getting to quite how to do that.

I've heard of closures - but I haven't gotten my head around them enough to know if they would help here.

I was thinking possible the following might be on the right line - but I'm I don't know how to tell the generic method which property to operate on - [Project Variable To Update]

        protected void SetRedBoxVisibility(bool redBoxVisibility)
    {
        SetGenericBoxVisibility([Project Variable To Update],redBoxVisibility)
        ShowRedBoxPanel(redBoxVisibility);
        RaiseStatusUpdated();   
    }

    protected void SetBlueBoxVisibility(bool blueBoxVisibility)
    {
        SetGenericBoxVisibility([Project Variable To Update],blueBoxVisibility)
        ShowBlueBoxPanel(blueBoxVisibility);
        RaiseStatusUpdated();   
    }

    protected void SetGenericBoxVisibility([Project Variable To Update], boxVisibility)
    {
        Project project = LoadProject();
        project.**[Project Variable To Update]** = boxVisibility
        ReDrawSomeThings();
        CalcualteSomeStuff();
        Project.UpdateBoxStatus();
        SaveProject(project);
    }

Any pointers as to how to handle this kind of thing would be useful :)

Upvotes: 2

Views: 1626

Answers (3)

Doug Chamberlain
Doug Chamberlain

Reputation: 11351

I think to really refactor this you need to make an IBox interface. Basically, that interface acts as a contract defining what methods and properties all Box objects must have at the very least.

interface IBox {
 //your generic properties and method stubs
 Bool visibility;
}

Now, implement the interface for each of your 'boxes' 

class blueBox : IBox
{
 //here you will have your concrete implementations of the above methods and properties
public Bool visibility   {get; set;} // this doesn't make sense with auto getter setters. you would need to write your bluebox specific getter and setters

}

class redBox : IBox
{
//more concrete implementation

}


public myMethod_To_Do_Stuff(IBox myBox) { // see how I am passing the interface not the conrete classes

myBox.visibility = true;

}

Upvotes: 1

Jon Skeet
Jon Skeet

Reputation: 1503489

Well, you could extract it like this:

protected void SetGenericBoxVisibility(Action<Project> propertySetter,
                                       Action<bool> panelShower,
                                       bool boxVisibility)
{
    Project project = LoadProject();
    propertySetter(project);
    ReDrawSomeThings();
    CalculateSomeStuff();
    Project.UpdateBoxStatus();
    SaveProject(project);
    panelShower();
    RaiseStatusUpdated();
}

Then:

protected void SetBlueBoxVisibility(bool blueBoxVisibility)
{
    SetGenericBoxVisibility(project => project.ShowBlueBox = blueBoxVisibility,
                            () => ShowBlueBoxPanel(blueBoxVisibility));
}

protected void SetRedBoxVisibility(bool redBoxVisibility)
{
    SetGenericBoxVisibility(project => project.ShowRedBox = redBoxVisibility,
                            () => ShowRedBoxPanel(redBoxVisibility));
}

It's not terribly nice, admittedly...

Upvotes: 3

Kieren Johnstone
Kieren Johnstone

Reputation: 42013

I think you may have bigger problems - one method per box to update simply doesn't fit. You have SetGenericBoxVisibility but then undo any good work by continuing to have Set*BoxVisibility methods. I don't know which technology you're using - if it's WPF look into MVVM then you can simply update your ViewModels. If it's WinForms you should create some kind of dictionary perhaps - Dictionary<BoxType, Box> _boxLookup where BoxType is an enum of the types you define. Then to set box visibility, you do _boxLookup[BoxType.Red].Property = value; or you could have methods to manipulate a box take a BoxType parameter.

However, some more context would be very useful, since even that solution isn't ideal. Moving some other code or getting some insight into the bigger picture about the problem you are solving with multiple boxes should lead to a few more 'aha' moments..

Upvotes: 1

Related Questions