Anti dry model

Question

I wrote this set of code and feel that it is rather poor in quality. As you can see, in each of the four statements of the case, I finish repeating a lot of the same code, except for a few variations in each case. Items that are changing; Session names, network names, and the name of the ManagerContext group. Can someone take this mess of code and show me the best way to do 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 retrieve the housing parts in a parameterized auxiliary 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; } } 

This is one of the options, and there is another refactoring, I'm sure.

For deeper refactoring, I would look at managing tables using a collection of option objects that are potentially delegated or similar. The way this works is that this parameter will become an object instead of a string, and the parameter will have properties that configure it, and methods that the corresponding delegates call. it really depends on the level of abstraction you want to keep. Sometimes it pays to inherit from regular controls and provide configuration information in a subclass so they know how to load themselves.

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

Answer 2

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

Now let's see how pleasantly we are isolated from the changes. GetGroup , for example, can change how it finds groups, and we don’t have to worry about finding all the places to search for groups if these details need to change. Similarly for GetGroupName and GetGrid . Moreover, we never repeat if any search logic should be reused anywhere. We are very isolated from change and will never repeat ourselves when this happens.

Answer 3

Keep in mind that this is just a refactoring of what you showed. Based on what you have shown, you can consider a deeper refactoring of your entire approach. However, this may not be possible.

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

This is just fun (this is unlikely to be a good idea and 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 would only do something like this if I wanted to dynamically change (i.e., at runtime) the set of options, and I would like the executable code (loading algorithm) to completely close.

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 an exception is reusing the option string in Enum, only with valid values. This way, you know that if you get the correct enumeration as an input argument, your parameter will be processed.

Answer 7

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

Answer 8

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