CODEWITHSUNDEEP
c#design-patterns
Chris
Chris

Reputation: 6325

The anti-DRY pattern

I've written this set of code and feel that it's pretty poor in quality. As you can see, in each of the four case statements I'm ending up repeating an awful lot of the same code except for a few variations in each case. Items that vary; session names, gridnames and the ManagerContext group name. Can anyone take this mess of code and show me a better way of doing this?

private void LoadGroup(string option)
{
    switch (option.ToUpper())
    {
        case "ALPHA":
            VList<T> alphaList = FetchInformation(
                                   ManagerContext.Current.Group1);

            if (Session["alphaGroup"] != null)
            {
                List<T> tempList = (List<T>)Session["alphaGroup"];
                alphaList.AddRange(tempList);
            }
            uxAlphaGrid.DataSource = alphaList;
            uxAlphaGrid.DataBind();
            break;
        case "BRAVO":
            VList<T> bravoList = FetchInformation(
                                   ManagerContext.Current.Group2);

            if (Session["bravoGroup"] != null)
            {
                List<T> tempList = (List<T>)Session["bravoGroup"];
                bravoList.AddRange(tempList);
            }
            uxBravoGrid.DataSource = bravoList;
            uxBravoGrid.DataBind();
            break;
        case "CHARLIE":
            VList<T> charlieList = FetchInformation(
                                   ManagerContext.Current.Group3);

            if (Session["charlieGroup"] != null)
            {
                List<T> tempList = (List<T>)Session["charlieGroup"];
                charlieList.AddRange(tempList);
            }
            uxCharlieGrid.DataSource = charlieList;
            uxCharlieGrid.DataBind();
            break;
        case "DELTA":
            VList<T> deltaList = FetchInformation(
                                   ManagerContext.Current.Group4);

            if (Session["deltaGroup"] != null)
            {
                List<T> tempList = (List<T>)Session["deltaGroup"];
                deltaList.AddRange(tempList);
            }
            uxDeltaGrid.DataSource = deltaList;
            uxDeltaGrid.DataBind();
            break;
        default:                
            break;
    }
}

Upvotes: 4

Views: 1126

Answers (8)

jason
jason

Reputation: 241741

I would prefer something like this:

private void LoadGroup(string option) {
    Group group = GetGroup(option);
    string groupName = GetGroupName(option);
    Grid grid = GetGrid(option);

    BindGroup(group, groupName, grid);
}

Group GetGroup(string option) {
    // ideally this should be defined and initialized elsewhere
    var dictionary = new Dictionary<string, Group>() {
        { "ALPHA", ManagerContext.Current.Group1 },
        { "BETA", ManagerContext.Current.Group2 },
        { "CHARLIE", ManagerContext.Current.Group3 },
        { "DELTA", ManagerContext.Current.Group4 }
    };   

    return dictionary[option.ToUpperInvariant()];
}

string GetGroupName(string option) {
    return option.ToLowerInvariant() + "Group";
}

Grid GetGrid(string option) {
    // ideally this should be defined and initialized elsewhere
    var dictionary = new Dictionary<string, Grid>() {
        { "ALPHA", uxAlphaGrid },
        { "BETA", uxBetaGrid },
        { "CHARLIE", uxCharlieGrid },
        { "DELTA", uxDeltaGrid }
    };

    return dictionary[option.ToUpperInvariant()];
}

void BindGroup(Group group, string groupName, Grid grid) {
    VList<T> groupList = FetchInformation(group);
    if (Session[groupName] != null) {
        List<T> tempList = (List<T>)Session[groupName];
        groupList.AddRange(tempList);
    }
    grid.DataSource = groupList;
    grid.DataBind();
}

And now look how nicely we are insulated from changes. GetGroup, for example, can change how it finds groups and we don't have to worry about finding all the places where groups are looked up should those details need to change. Similarly for GetGroupName and GetGrid. What is more, we never repeat ourselves should any of the lookup logic need to be reused anywhere. We are very insulated from change and will never repeat ourselves when factored like this.

Upvotes: 15

Phil
Phil

Reputation: 2725

Here's one just for fun (meaning it's unlikely to be a good idea and it's completely untested):

public class YourClass
{
    private Dictionary<string, Action> m_options;

    public YourClass()
    {
     m_options = new Dictionary<string, Action>
     {
      { "ALPHA",  () => LoadGroup(ManagerContext.Current.Group1, "alphaGroup", uxAlphaGrid) },
      { "BRAVO",  () => LoadGroup(ManagerContext.Current.Group2, "bravoGroup", uxBravoGrid) },
      { "CHARLIE",() => LoadGroup(ManagerContext.Current.Group3, "charlieGroup", uxCharlieGrid) },
      { "DELTA",  () => LoadGroup(ManagerContext.Current.Group4, "deltaGroup", uxDeltaGrid) },
     };
    }

    private void LoadGroup(string option)
    {
     Action optionAction;

     if(m_options.TryGetValue(option, out optionAction))
     {
            optionAction();
     }
    }

    private void LoadGroup(TGroup group, string groupName, TGrid grid)
    {
        VList<T> returnList = FetchInformation(group);

        if (Session[groupName] != null)
        {
                List<T> tempList = (List<T>)Session[groupName];
                returnList.AddRange(tempList);
        }
        grid.DataSource = returnList;
        grid.DataBind();
    }
}

I'd only do something like this if I wanted to be able to dynamically alter (i.e. at runtime) the set of option matches, and I wanted the code executed (the load algorithm) to be completely open ended.

Upvotes: 2

Cade Roux
Cade Roux

Reputation: 89721

You should be able to extract the parts of the case to a parameterized helper function:

function helper(grp, grpname, dg) {
    VList<T> theList = FetchInformation(grp); 
    if (Session[grpname] != null) 
    { 
        List<T> tempList = (List<T>)Session[grpname]; 
        theList.AddRange(tempList); 
    } 
    dg.DataSource = theList; 
    dg.DataBind(); 
}

private void LoadGroup(string option) 
{ 
        switch (option.ToUpper()) 
        { 
                case "ALPHA": 
                        helper(ManagerContext.Current.Group1, "alphaGroup", uxAlphaGrid);
                        break; 
                case "BRAVO": 
                        helper(ManagerContext.Current.Group2, "bravoGroup", uxBravoGrid);
                        break; 
                case "CHARLIE": 
                        helper(ManagerContext.Current.Group3, "charlieGroup", uxCharlieGrid);
                        break; 
                case "DELTA": 
                        helper(ManagerContext.Current.Group4, "deltaGroup", uxDeltaGrid);
                        break; 
                default:                                 
                        break; 
        } 
}

That's one option and there is further refactoring, I'm sure.

For deeper refactorings, I would look at table-driving using a collection of option objects, potentially delegates, or similar. The way this works is that the option would become an object instead of a string and the option would have properties which configure it and methods which invoke the proper delegates. it really depends on the abstraction level you want to maintain. Sometimes it pays to inherit from the regular controls and provide configuration information in the subclass so that they know how to load themselves.

There is not enough space here to really go into that depth of refactoring.

Upvotes: 24

Tomas Aschan
Tomas Aschan

Reputation: 60664

private void LoadGroup(string option)
{
    option = option.ToLower();
    sessionContent = Session[option + "Group"];

    switch(option)
    {
        case "alpha":
            var grp = ManagerContext.Current.Group1;
            var grid = uxAlphaGrid;
            break;
        case "bravo":
            var grp = ManagerContext.Current.Group2;
            var grid = uxBravoGrid;
            break;
        case "charlie":
            var grp = ManagerContext.Current.Group3;
            var grid = uxCharlieGrid;
            break;
        // Add more cases if necessary
        default:
            throw new ArgumentException("option", "Non-allowed value");
    }

    VList<T> groupList = FetchInformation(grp);
    if (sessionContent != null)
    {
        List<T> tempList = (List<T>)sessionContent;
        groupList.AddRange(tempList);
    }

    grid.DataSource = List("alpha";
    grid.DataBind();
}

An alternative to throwing the exception is to re-type the option string into an Enum, with only the values you allow. That way you know that if you get a correct enum as input argument, your option will be handled.

Upvotes: 1

Andy West
Andy West

Reputation: 12509

Something similar to this should work:

private void LoadGroup(string option)
{
        switch (option.ToUpper())
        {
                case "ALPHA":
                        BindGroup(ManagerContext.Current.Group1, "alphaGroup", uxAlphaGrid);
                        break;
                case "BRAVO":
                        BindGroup(ManagerContext.Current.Group2, "bravoGroup", uxBravoGrid);
                        break;
                case "CHARLIE":
                        BindGroup(ManagerContext.Current.Group3, "charlieGroup", uxCharlieGrid);
                        break;
                case "DELTA":
                        BindGroup(ManagerContext.Current.Group4, "deltaGroup", uxDeltaGrid);
                        break;
                default:                                
                        break;
        }
}

private void BindGroup(GroupType group, string groupName, GridView grid)
{
    VList<T> vList = FetchInformation(group);

    if (Session[groupName] != null)
    {
        List<T> tempList = (List<T>)Session[groupName];
        vList.AddRange(tempList);
    }
    grid.DataSource = vList;
    grid.DataBind();
}

Upvotes: 2

Ta01
Ta01

Reputation: 31630

    private void LoadGroup(GridView gv, string groupName, ManagerContext m)
{
    VList<T> list = FetchInformation(m); //not sure if ManagerContext will get what you need
    if (Session[groupName] != null)
    {
       list.AddRange(List<T>Session[groupName]);
       gv.DataSource = list;
       gv.DataBind();
    }   

}

Upvotes: 0

Pavel Radzivilovsky
Pavel Radzivilovsky

Reputation: 19114

Make two functions, GetGroup() returning things like ManagerContext.Current.Group4 and GetGroupName(), returning strings like "deltaGroup" . Then, all code goes away.

Upvotes: 0

Randolpho
Randolpho

Reputation: 56439

Keep in mind, this is only a refactoring of what you've shown. Based on what you've shown, you may want to consider a deeper refactoring of your entire approach. This may not be feasible, however.

And so:

private void LoadGroup(string option)
{
        switch (option.ToUpper())
        {
                case "ALPHA":
                        BindData("alphaGroup", uxAlphaGrid, FetchInformation(ManagerContext.Current.Group1));
                        break;
                case "BRAVO":
                        BindData("bravoGroup", uxBravoGrid, FetchInformation(ManagerContext.Current.Group2));
                        break;
                case "CHARLIE":
                        BindData("charlieGroup", uxCharlieGrid, FetchInformation(ManagerContext.Current.Group3));
                        break;
                case "DELTA":
                        BindData("deltaTeam", uxDeltaGrid, FetchInformation(ManagerContext.Current.Group4));                        
                        break;
                default:                                
                        break;
        }
}

private void BindData(string sessionName, GridView grid, VList<T> data) // I'm assuming GridView here; dunno the type, but it looks like they're shared
{
    if (Session[sessionName] != null)
    {
            List<T> tempList = (List<T>)Session[sessionName];
            data.AddRange(tempList);
    }
    grid.DataSource = data;
    grid.DataBind();

}

Upvotes: 9

Related Questions