The anti-DRY pattern

Question

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;
 }
}

Answer 1

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.

Answer 2

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();

}

Answer 3

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

Answer 4

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();
}

Answer 5

    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();
    }   

}

Answer 6

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.

Answer 7

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.

Answer 8

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.