Method that checks whether to disable or enable the buttons depending on whether data is null or not

Question

I have this bit of code that needs refactoring/simplification. The below grabs the planning data for view A and B. Then it joins the planning and matching image for view details. So then it determines if it should set the buttons to be disabled or not based on whether or not, if the data is empty/null or not.

I think the major refactoring might be needed for adding the elements to the dictionaries for each view.

[OutputCache(NoStore = true, Duration = 0, VaryByParam = "*")]
public ActionResult CheckButtons()
{
 int planId = (int)UserSession.GetValue(StateNameEnum.ID, "PlanID");
 planning resultSetA = db.planning.Where(t => t.PlanId == planId).Join(db.matching_image, p => p.MatchingImageId, i => i.Id,
 (p, i) => new
 {
 planning = p,
 i.View
 }).Where(j => j.View == "A").Select(j => j.planning).FirstOrDefault();
 structure sA = db.structure.FirstOrDefault(s => s.PlanId == planId && s.Type == "A");
 planning resultSetB = db.planning.Where(t => t.PlanId == planId).Join(db.matching_image, p => p.MatchingImageId, i => i.Id,
 (p, i) => new
 {
 planning = p,
 i.View
 }).Where(j => j.View == "B).Select(j => j.planning).FirstOrDefault();
 structure sB = db.structure.FirstOrDefault(s => s.PlanId == planId && s.Type == "B");
 Dictionary<string, string> buttonADictionary = new Dictionary<string, string>();
 Dictionary<string, string> buttonBDictionary = new Dictionary<string, string>();
 buttonADictionary.Add("C", resultSetA.CorrespondingId != null ? "disabled" : "enabled");
 buttonADictionary.Add("R", resultSetA.ResponseId != null ? "disabled" : "enabled");
 buttonADictionary.Add("S", (sA != null && sA.Point_x != null) ? "disabled" : "enabled");
 buttonADictionary.Add("D", resultSetA.DistalId != null ? "disabled" : "enabled");
 buttonADictionary.Add("P", resultSetA.ProximalId != null ? "disabled" : "enabled");
 buttonBDictionary.Add("C", resultSetB.CorrespondingId != null ? "disabled" : "enabled");
 buttonBDictionary.Add("R", resultSetB.ResponseId != null ? "disabled" : "enabled");
 buttonBDictionary.Add("S", (sB != null && sB.Point_x != null) ? "disabled" : "enabled");
 buttonBDictionary.Add("D", resultSetB.DistalId != null ? "disabled" : "enabled");
 buttonBDictionary.Add("P", resultSetB.ProximalId != null ? "disabled" : "enabled");
 bool aHasData = buttonADictionary.ContainsValue("disabled");
 bool bHasData = buttonBDictionary.ContainsValue("disabled");
 return Json(new { buttonADictionary, buttonBDictionary, aHasData, bHasData }, JsonRequestBehavior.AllowGet);
}

Answer 1

In looking at your method, you have a lot of repetition in building your two dictionaries before composing your final result. If you were to simply regroup your code to separate the building of your first and second dictionaries, and the data that supports the construction, you would produce the following method:

[OutputCache(NoStore = true, Duration = 0, VaryByParam = "*")]
public ActionResult CheckButtons()
{
 int planId = (int)UserSession.GetValue(StateNameEnum.ID, "PlanID");
 planning resultSetA = db.planning.Where(t => t.PlanId == planId).Join(db.matching_image, p => p.MatchingImageId, i => i.Id,
 (p, i) => new
 {
 planning = p,
 i.View
 }).Where(j => j.View == "A").Select(j => j.planning).FirstOrDefault();
 structure sA = db.structure.FirstOrDefault(s => s.PlanId == planId && s.Type == "A");
 Dictionary<string, string> buttonADictionary = new Dictionary<string, string>();
 buttonADictionary.Add("C", resultSetA.CorrespondingId != null ? "disabled" : "enabled");
 buttonADictionary.Add("R", resultSetA.ResponseId != null ? "disabled" : "enabled");
 buttonADictionary.Add("S", (sA != null && sA.Point_x != null) ? "disabled" : "enabled");
 buttonADictionary.Add("D", resultSetA.DistalId != null ? "disabled" : "enabled");
 buttonADictionary.Add("P", resultSetA.ProximalId != null ? "disabled" : "enabled");
 bool aHasData = buttonADictionary.ContainsValue("disabled");
 planning resultSetB = db.planning.Where(t => t.PlanId == planId).Join(db.matching_image, p => p.MatchingImageId, i => i.Id,
 (p, i) => new
 {
 planning = p,
 i.View
 }).Where(j => j.View == "B").Select(j => j.planning).FirstOrDefault();
 structure sB = db.structure.FirstOrDefault(s => s.PlanId == planId && s.Type == "B");
 Dictionary<string, string> buttonBDictionary = new Dictionary<string, string>();
 buttonBDictionary.Add("C", resultSetB.CorrespondingId != null ? "disabled" : "enabled");
 buttonBDictionary.Add("R", resultSetB.ResponseId != null ? "disabled" : "enabled");
 buttonBDictionary.Add("S", (sB != null && sB.Point_x != null) ? "disabled" : "enabled");
 buttonBDictionary.Add("D", resultSetB.DistalId != null ? "disabled" : "enabled");
 buttonBDictionary.Add("P", resultSetB.ProximalId != null ? "disabled" : "enabled");
 bool bHasData = buttonBDictionary.ContainsValue("disabled");
 return Json(new { buttonADictionary, buttonBDictionary, aHasData, bHasData }, JsonRequestBehavior.AllowGet);
}

It's evident at this point that your code at the top and at the bottom is effectively identical, only differing on a value passed into two queries and then on variable names. Inside this single method, you are violating the DRY principle: don't repeat yourself. So let's make the first extraction and create the method here:

private Dictionary<string, string> GetButtonDictionary(int planId, string value)
{
 planning resultSet = db.planning.Where(t => t.PlanId == planId).Join(db.matching_image, p => p.MatchingImageId, i => i.Id,
 (p, i) => new
 {
 planning = p,
 i.View
 }).Where(j => j.View == value).Select(j => j.planning).FirstOrDefault();
 structure st = db.structure.FirstOrDefault(s => s.PlanId == planId && s.Type == value);
 Dictionary<string, string> buttonDictionary = new Dictionary<string, string>();
 buttonDictionary.Add("C", resultSet.CorrespondingId != null ? "disabled" : "enabled");
 buttonDictionary.Add("R", resultSet.ResponseId != null ? "disabled" : "enabled");
 buttonDictionary.Add("S", (st != null && st.Point_x != null) ? "disabled" : "enabled");
 buttonDictionary.Add("D", resultSet.DistalId != null ? "disabled" : "enabled");
 buttonDictionary.Add("P", resultSet.ProximalId != null ? "disabled" : "enabled");
 return buttonDictionary;
}

The method above takes two parameters: a planId that you have obtained, as well as a value. This second variable probably isn't a great name, but you have overloaded the meaning of your string in your original method (it's used to compare against a View property and a Type property) such that I couldn't immediately think of a better name. If you can think of one, just sub it in.

With this extraction, it cleans up your original method nicely, because it can be cleaned up to the following:

[OutputCache(NoStore = true, Duration = 0, VaryByParam = "*")]
public ActionResult CheckButtons()
{
 int planId = (int)UserSession.GetValue(StateNameEnum.ID, "PlanID");
 Dictionary<string, string> buttonADictionary = GetButtonDictionary(planId, "A");
 Dictionary<string, string> buttonBDictionary = GetButtonDictionary(planId, "B");
 bool aHasData = buttonADictionary.ContainsValue("disabled");
 bool bHasData = buttonBDictionary.ContainsValue("disabled");
 return Json(new { buttonADictionary, buttonBDictionary, aHasData, bHasData }, JsonRequestBehavior.AllowGet);
}

Could we clean this up a little more? Possibly, but it's simplified enough for now so let's go back to the method we extracted. It has a couple of database queries that should probably individually be their own methods (SRP - Single Responsibility Principle), so let's go ahead and do that.

private Dictionary<string, string> GetButtonDictionary(int planId, string value)
{
 planning resultSet = GetPlanningResultSet(planId, value);
 structure st = GetStructure(planId, value);
 Dictionary<string, string> buttonDictionary = new Dictionary<string, string>();
 buttonDictionary.Add("C", resultSet.CorrespondingId != null ? "disabled" : "enabled");
 buttonDictionary.Add("R", resultSet.ResponseId != null ? "disabled" : "enabled");
 buttonDictionary.Add("S", (st != null && st.Point_x != null) ? "disabled" : "enabled");
 buttonDictionary.Add("D", resultSet.DistalId != null ? "disabled" : "enabled");
 buttonDictionary.Add("P", resultSet.ProximalId != null ? "disabled" : "enabled");
 return buttonDictionary;
}
private planning GetPlanningResultSet(int planId, string value)
{
 planning resultSet = db.planning.Where(t => t.PlanId == planId).Join(db.matching_image, p => p.MatchingImageId, i => i.Id,
 (p, i) => new
 {
 planning = p,
 i.View
 }).Where(j => j.View == value).Select(j => j.planning).FirstOrDefault();
 return resultSet;
}
private structure GetStructure(int planId, string value)
{
 return db.structure.FirstOrDefault(s => s.PlanId == planId && s.Type == value);
}

After doing this, you have the obvious bit of repetition left where you get your enabled settings where you repeat your null checks. So let's extract a method to deal with that.

private Dictionary<string, string> GetButtonDictionary(int planId, string value)
{
 planning resultSet = GetPlanningResultSet(planId, value);
 structure st = GetStructure(planId, value);
 Dictionary<string, string> buttonDictionary = new Dictionary<string, string>();
 buttonDictionary.Add("C", GetEnabledSetting(resultSet.CorrespondingId));
 buttonDictionary.Add("R", GetEnabledSetting(resultSet.ResponseId));
 buttonDictionary.Add("S", GetEnabledSetting(st != null ? st.Point_x : null));
 buttonDictionary.Add("D", GetEnabledSetting(resultSet.DistalId));
 buttonDictionary.Add("P", GetEnabledSetting(resultSet.ProximalId));
 return buttonDictionary;
}
private string GetEnabledSetting(object value)
{
 return value != null ? "disabled" : "enabled";
}

After we have completed this extraction, the full code now looks like the following:

[OutputCache(NoStore = true, Duration = 0, VaryByParam = "*")]
public ActionResult CheckButtons()
{
 int planId = (int)UserSession.GetValue(StateNameEnum.ID, "PlanID");
 Dictionary<string, string> buttonADictionary = GetButtonDictionary(planId, "A");
 Dictionary<string, string> buttonBDictionary = GetButtonDictionary(planId, "B");
 bool aHasData = buttonADictionary.ContainsValue("disabled");
 bool bHasData = buttonBDictionary.ContainsValue("disabled");
 return Json(new { buttonADictionary, buttonBDictionary, aHasData, bHasData }, JsonRequestBehavior.AllowGet);
}
private Dictionary<string, string> GetButtonDictionary(int planId, string value)
{
 planning resultSet = GetPlanningResultSet(planId, value);
 structure st = GetStructure(planId, value);
 Dictionary<string, string> buttonDictionary = new Dictionary<string, string>();
 buttonDictionary.Add("C", GetEnabledSetting(resultSet.CorrespondingId));
 buttonDictionary.Add("R", GetEnabledSetting(resultSet.ResponseId));
 buttonDictionary.Add("S", GetEnabledSetting(st != null ? st.Point_x : null));
 buttonDictionary.Add("D", GetEnabledSetting(resultSet.DistalId));
 buttonDictionary.Add("P", GetEnabledSetting(resultSet.ProximalId));
 return buttonDictionary;
}
private planning GetPlanningResultSet(int planId, string value)
{
 planning resultSet = db.planning.Where(t => t.PlanId == planId).Join(db.matching_image, p => p.MatchingImageId, i => i.Id,
 (p, i) => new
 {
 planning = p,
 i.View
 }).Where(j => j.View == value).Select(j => j.planning).FirstOrDefault();
 return resultSet;
}
private structure GetStructure(int planId, string value)
{
 return db.structure.FirstOrDefault(s => s.PlanId == planId && s.Type == value);
}
private string GetEnabledSetting(object value)
{
 return value != null ? "disabled" : "enabled";
}

This is enough contribution for me. I'll leave it up to you to possibly create better variable names and get the code into a bit better compliance with C# standards. For example, you seem to have classes starting in lowercase (planning, structure) and those should start with capital letters. You seem to have DbSets that should be plural (and capitalized). Finally, this seems to be a lot of work to be in a controller, perhaps it could be be further extracted to a pure business layer. Those modifications would, of course, be up to you.

• Kala J: Thank you for your contributions! I work with others and the model names just stuck like that from the previous person. So I didn't want to change it to a proper naming convention it too late into the project but this definitely helps me spot it for future projects. Bw, I am curious about when you say, some of the logic should be put into the business layer? Do you mean to put the queries in my viewmodel? Thank you so much again. I'll take a look at your suggestions again and start working on the improvements :)
• Anthony Pegram: @KalaJ, when I say business layer, I essentially mean that if it were me, I'd have the method that produces the ActionResult in the controller, and nothing else. Even that method would probably get slimmed down further. The queries, the construction of the dictionaries, etc., that would live somewhere else. As such, all those private methods we extracted would exist in one or more classes, and the controller would have an extremely small method for the ActionResult that simply delegates. But that's me, and it might not fit the scale of your project.
• BCdotWEB: I'd switch the logic of return value != null ? "disabled" : "enabled"; to return value == null ? "enabled" : "disabled";, because that makes it simpler to understand, IMHO.
• Kala J: @BCdotWEB, wait why?
• BCdotWEB: @KalaJ Because that's easier to read. In the case of return value != null ? "disabled" : "enabled"; the logic translates to "if value is NOT null then button is NOT enabled". It is far simpler to say "if value is null then button is enabled", i.e. return value == null ? "enabled" : "disabled"; I try to avoid "negative conditions".