I undertook an interesting refactoring at work. We have a legacy app that feels like it was written in VS2005 and C# 2.0 and then was at some point upgraded to VS2010 and C# 4.0. The app is of web forms and had no tests prior to my touch. It has the sort of fat classes that I hate as they are impossible to roll tests into. I once saw Eric Anderson speak on how to spruce up an legacy code base and make it maintainable and he suggested trying to start by raising an island out of the ocean of bad. That which is above water is that which is under test. Once you have an island, hopefully you can grow it. I started by pulling all of the getsetters that governed user permissions out of the User class which was unfortunately more than a POCO into a class called UserPermissions which then was a POCO and easy to test.
namespace Whatever.Core
{
public class UserPermissions
{
public bool ActiveUser { get; set; }
public bool ManagementReportViewer { get; set; }
public bool CanSelectTimeEntry { get; set; }
public bool CanSelectActivity { get; set; }
public bool CanAddContractingAction { get; set; }
public bool CanSubmitContractingAction { get; set; }
public bool IsDivisionPoc { get; set; }
public bool IsDivisionManager { get; set; }
public bool IsDivisionAdmin { get; set; }
public bool IsAdmin { get; set; }
public bool IsTTUser { get; set; }
public bool IsTTUserForProjectEntry { get; set; }
public bool IsTTUserForSummaryEntry { get; set; }
public bool IsProjectEntryUser { get; set; }
public bool IsABCAdmin { get; set; }
public bool IsABCPOC { get; set; }
public bool IsABCViewer { get; set; }
public bool IsTech { get; set; }
public bool IsContact { get; set; }
public bool IsContributor { get; set; }
public bool IsProd { get; set; }
public bool IsIso { get; set; }
public bool IsContractingOfficer { get; set; }
public UserPermissions()
{
ActiveUser = false;
ManagementReportViewer = false;
CanSelectTimeEntry = false;
CanSelectActivity = false;
CanAddContractingAction = false;
CanSubmitContractingAction = false;
IsDivisionPoc = false;
IsDivisionManager = false;
IsDivisionAdmin = false;
IsAdmin = false;
IsTTUser = false;
IsTTUserForProjectEntry = false;
IsTTUserForSummaryEntry = false;
IsProjectEntryUser = false;
IsABCAdmin = false;
IsABCPOC = false;
IsABCViewer = false;
IsTech = false;
IsContact = false;
IsContributor = false;
IsProd = false;
IsIso = false;
IsContractingOfficer = false;
}
}
}
I got UserPermissions under test. I used reflection, as seen below, to confirm that 23 getsetters existed and nothing more. There will be no way to fatten up this object without the test breaking and that might just be a good thing.
using System;
using System.Reflection;
using Microsoft.VisualStudio.TestTools.UnitTesting;
namespace Whatever.Core.Tests
{
[TestClass]
public class UserPermissionsTests
{
public UserPermissionsTests()
{
}
[TestMethod]
public void permissions_are_all_false_by_default()
{
UserPermissions permissions = new UserPermissions();
Assert.AreEqual(permissions.ActiveUser, false);
Assert.AreEqual(permissions.ManagementReportViewer, false);
Assert.AreEqual(permissions.CanSelectTimeEntry, false);
Assert.AreEqual(permissions.CanSelectActivity, false);
Assert.AreEqual(permissions.CanAddContractingAction, false);
Assert.AreEqual(permissions.CanSubmitContractingAction, false);
Assert.AreEqual(permissions.IsDivisionPoc, false);
Assert.AreEqual(permissions.IsDivisionManager, false);
Assert.AreEqual(permissions.IsDivisionAdmin, false);
Assert.AreEqual(permissions.IsAdmin, false);
Assert.AreEqual(permissions.IsTTUser, false);
Assert.AreEqual(permissions.IsTTUserForProjectEntry, false);
Assert.AreEqual(permissions.IsTTUserForSummaryEntry, false);
Assert.AreEqual(permissions.IsProjectEntryUser, false);
Assert.AreEqual(permissions.IsABCAdmin, false);
Assert.AreEqual(permissions.IsABCPOC, false);
Assert.AreEqual(permissions.IsABCViewer, false);
Assert.AreEqual(permissions.IsTech, false);
Assert.AreEqual(permissions.IsContact, false);
Assert.AreEqual(permissions.IsContributor, false);
Assert.AreEqual(permissions.IsProd, false);
Assert.AreEqual(permissions.IsIso, false);
Assert.AreEqual(permissions.IsContractingOfficer, false);
}
[TestMethod]
public void UserPermissions_has_46_reflected_methods_for_23_getsetters()
{
Type type = typeof(UserPermissions);
var methods = type.GetMethods();
Assert.AreEqual(methods.Length, 50);
MethodInfo info = methods[0];
Assert.AreEqual(info.Name, "get_ActiveUser");
info = methods[1];
Assert.AreEqual(info.Name, "set_ActiveUser");
info = methods[2];
Assert.AreEqual(info.Name, "get_ManagementReportViewer");
info = methods[3];
Assert.AreEqual(info.Name, "set_ManagementReportViewer");
info = methods[4];
Assert.AreEqual(info.Name, "get_CanSelectTimeEntry");
info = methods[5];
Assert.AreEqual(info.Name, "set_CanSelectTimeEntry");
info = methods[6];
Assert.AreEqual(info.Name, "get_CanSelectActivity");
info = methods[7];
Assert.AreEqual(info.Name, "set_CanSelectActivity");
info = methods[8];
Assert.AreEqual(info.Name, "get_CanAddContractingAction");
info = methods[9];
Assert.AreEqual(info.Name, "set_CanAddContractingAction");
info = methods[10];
Assert.AreEqual(info.Name, "get_CanSubmitContractingAction");
info = methods[11];
Assert.AreEqual(info.Name, "set_CanSubmitContractingAction");
info = methods[12];
Assert.AreEqual(info.Name, "get_IsDivisionPoc");
info = methods[13];
Assert.AreEqual(info.Name, "set_IsDivisionPoc");
info = methods[14];
Assert.AreEqual(info.Name, "get_IsDivisionManager");
info = methods[15];
Assert.AreEqual(info.Name, "set_IsDivisionManager");
info = methods[16];
Assert.AreEqual(info.Name, "get_IsDivisionAdmin");
info = methods[17];
Assert.AreEqual(info.Name, "set_IsDivisionAdmin");
info = methods[18];
Assert.AreEqual(info.Name, "get_IsAdmin");
info = methods[19];
Assert.AreEqual(info.Name, "set_IsAdmin");
info = methods[20];
Assert.AreEqual(info.Name, "get_IsTTUser");
info = methods[21];
Assert.AreEqual(info.Name, "set_IsTTUser");
info = methods[22];
Assert.AreEqual(info.Name, "get_IsTTUserForProjectEntry");
info = methods[23];
Assert.AreEqual(info.Name, "set_IsTTUserForProjectEntry");
info = methods[24];
Assert.AreEqual(info.Name, "get_IsTTUserForSummaryEntry");
info = methods[25];
Assert.AreEqual(info.Name, "set_IsTTUserForSummaryEntry");
info = methods[26];
Assert.AreEqual(info.Name, "get_IsProjectEntryUser");
info = methods[27];
Assert.AreEqual(info.Name, "set_IsProjectEntryUser");
info = methods[28];
Assert.AreEqual(info.Name, "get_IsABCAdmin");
info = methods[29];
Assert.AreEqual(info.Name, "set_IsABCAdmin");
info = methods[30];
Assert.AreEqual(info.Name, "get_IsABCPOC");
info = methods[31];
Assert.AreEqual(info.Name, "set_IsABCPOC");
info = methods[32];
Assert.AreEqual(info.Name, "get_IsABCViewer");
info = methods[33];
Assert.AreEqual(info.Name, "set_IsABCViewer");
info = methods[34];
Assert.AreEqual(info.Name, "get_IsTech");
info = methods[35];
Assert.AreEqual(info.Name, "set_IsTech");
info = methods[36];
Assert.AreEqual(info.Name, "get_IsContact");
info = methods[37];
Assert.AreEqual(info.Name, "set_IsContact");
info = methods[38];
Assert.AreEqual(info.Name, "get_IsContributor");
info = methods[39];
Assert.AreEqual(info.Name, "set_IsContributor");
info = methods[40];
Assert.AreEqual(info.Name, "get_IsProd");
info = methods[41];
Assert.AreEqual(info.Name, "set_IsProd");
info = methods[42];
Assert.AreEqual(info.Name, "get_IsIso");
info = methods[43];
Assert.AreEqual(info.Name, "set_IsIso");
info = methods[44];
Assert.AreEqual(info.Name, "get_IsContractingOfficer");
info = methods[45];
Assert.AreEqual(info.Name, "set_IsContractingOfficer");
info = methods[46];
Assert.AreEqual(info.Name, "ToString");
info = methods[47];
Assert.AreEqual(info.Name, "Equals");
info = methods[48];
Assert.AreEqual(info.Name, "GetHashCode");
info = methods[49];
Assert.AreEqual(info.Name, "GetType");
}
}
}
Here is why I picked this to-do. The application generates a menu based upon a Web.sitemap file. A blob of C# (in a class a master page code behind inherits from) hid menu items based upon user permissions with some if/then logic. If one removed a line item from the XML in Web.sitemap one had to know to look to remove counterpart logic in the C# class. I did not know at first and I broke something. This seemed like a good thing to get under test in such a manner that one could not wreck the site by removing a menu item. My solution was to move the permissions logic into the XML, making line items now look like this:
<siteMapNode url="~/OpenProjectForm.aspx?d=1" title="New Ongoing Project" description="New Ongoing Project" criteria="IsAdmin|IsDivisionAdmin|IsDivisionManager" />
The criteria parameter is new. I wrote a static helper to loop through all of the nodes and return a blacklist of nodes a user should not see.
using System;
using System.Collections.Generic;
using System.Reflection;
using System.Xml;
namespace Whatever.Core
{
public static class DesignationsOfInaccessibleMenuItemsHelper
{
public static List<string> Retrieve(XmlDocument menu, UserPermissions keys)
{
List<string> blacklist = new List<string>() {};
List<string> whitelist = new List<string>() {};
IEnumerable<string> getters = GetListOfGetters();
foreach(XmlNode tierI in menu)
{
blacklist = BuildBlacklist(tierI, blacklist);
whitelist = Judge(tierI, whitelist, keys, getters);
foreach (XmlNode tierII in tierI)
{
blacklist = BuildBlacklist(tierII, blacklist);
whitelist = Judge(tierII, whitelist, keys, getters);
foreach (XmlNode tierIII in tierII)
{
blacklist = BuildBlacklist(tierIII, blacklist);
whitelist = Judge(tierIII, whitelist, keys, getters);
foreach (XmlNode tierIV in tierIII)
{
blacklist = BuildBlacklist(tierIV, blacklist);
whitelist = Judge(tierIV, whitelist, keys, getters);
foreach (XmlNode tierV in tierIV)
{
blacklist = BuildBlacklist(tierV, blacklist);
whitelist = Judge(tierV, whitelist, keys, getters);
foreach (XmlNode tierVI in tierV)
{
blacklist = BuildBlacklist(tierVI, blacklist);
whitelist = Judge(tierVI, whitelist, keys, getters);
foreach (XmlNode tierVII in tierVI)
{
blacklist = BuildBlacklist(tierVII, blacklist);
whitelist = Judge(tierVII, whitelist, keys, getters);
foreach (XmlNode tierVIII in tierVII)
{
blacklist = BuildBlacklist(tierVIII, blacklist);
whitelist = Judge(tierVIII, whitelist, keys, getters);
}
}
}
}
}
}
}
}
foreach (string whiteitem in whitelist) blacklist.Remove(whiteitem);
return blacklist;
}
private static List<string> Judge(XmlNode node, List<string> whitelist,
UserPermissions keys, IEnumerable<string> getters)
{
Type type = typeof(UserPermissions);
if (node.Attributes != null)
{
foreach(XmlAttribute attribute in node.Attributes)
{
if (attribute.Name == "criteria")
{
string[] criteria = attribute.Value.Split('|');
string description = node.Attributes["description"].Value;
bool isUserAllowed = false;
foreach (string permission in criteria)
{
foreach (string getter in getters)
{
if (permission == getter)
{
MethodInfo info = type.GetMethod(Prefix() + getter);
if ((bool)info.Invoke(keys, null))
{
isUserAllowed = true;
}
}
}
}
if (isUserAllowed) whitelist.Add(description);
}
}
}
return whitelist;
}
private static List<string> BuildBlacklist(XmlNode node, List<string> blacklist)
{
Type type = typeof(UserPermissions);
if (node.Attributes != null)
{
foreach (XmlAttribute attribute in node.Attributes)
{
if (attribute.Name == "criteria")
{
string description = node.Attributes["description"].Value;
blacklist.Add(description);
}
}
}
return blacklist;
}
private static IEnumerable<string> GetListOfGetters()
{
List<string> getters = new List<string>() {};
Type type = typeof(UserPermissions);
var methods = type.GetMethods();
foreach(MethodInfo info in methods)
{
if (info.Name.StartsWith(Prefix()))
{
getters.Add(info.Name.Replace(Prefix(), ""));
}
}
return getters;
}
private static string Prefix()
{
return "get_";
}
}
}
I got the helper under test like so. This is cool code. It shows off manipulating the XmlDocument type.
using System.Xml;
using System.Collections.Generic;
using Microsoft.VisualStudio.TestTools.UnitTesting;
namespace Whatever.Core.Tests
{
[TestClass]
public class DesignationsOfInaccessibleMenuItemsHelperTests
{
public DesignationsOfInaccessibleMenuItemsHelperTests()
{
}
[TestMethod]
public void happy_pass_returns_an_empty_list()
{
XmlDocument menu = new XmlDocument();
UserPermissions keys = new UserPermissions();
List<string> designations =
DesignationsOfInaccessibleMenuItemsHelper.Retrieve(menu, keys);
Assert.AreEqual(menu.InnerXml, "");
Assert.AreEqual(designations.Count, 0);
}
[TestMethod]
public void simple_example_for_returning_one_record()
{
XmlDocument menu = SiteMapFactory();
UserPermissions keys = new UserPermissions();
keys.IsAdmin = true;
List<string> designations =
DesignationsOfInaccessibleMenuItemsHelper.Retrieve(menu, keys);
Assert.AreEqual(designations.Count, 5);
Assert.AreEqual(designations[0], "IsABCAdminExample");
Assert.AreEqual(designations[1], "IsDivisionManagerExample");
Assert.AreEqual(designations[2], "ShouldAlwaysBeHidden");
Assert.AreEqual(designations[3], "CanSelectTimeEntryExample");
Assert.AreEqual(designations[4], "IsContributorExample");
}
[TestMethod]
public void match_multiple_permissions()
{
XmlDocument menu = SiteMapFactory();
UserPermissions keys = new UserPermissions();
keys.IsContributor = true;
keys.IsDivisionManager = true;
keys.CanSelectTimeEntry = true;
List<string> designations =
DesignationsOfInaccessibleMenuItemsHelper.Retrieve(menu, keys);
Assert.AreEqual(designations.Count, 3);
Assert.AreEqual(designations[0], "IsAdminExample");
Assert.AreEqual(designations[1], "IsABCAdminExample");
Assert.AreEqual(designations[2], "ShouldAlwaysBeHidden");
}
[TestMethod]
public void will_hide_everything_in_the_absence_of_permissions()
{
XmlDocument menu = SiteMapFactory();
UserPermissions keys = new UserPermissions();
List<string> designations =
DesignationsOfInaccessibleMenuItemsHelper.Retrieve(menu, keys);
Assert.AreEqual(designations.Count, 6);
Assert.AreEqual(designations[0], "IsAdminExample");
Assert.AreEqual(designations[1], "IsABCAdminExample");
Assert.AreEqual(designations[2], "IsDivisionManagerExample");
Assert.AreEqual(designations[3], "ShouldAlwaysBeHidden");
Assert.AreEqual(designations[4], "CanSelectTimeEntryExample");
Assert.AreEqual(designations[5], "IsContributorExample");
}
private XmlDocument SiteMapFactory()
{
string description = "description";
string criteria = "criteria";
XmlDocument menu = new XmlDocument();
XmlElement tierI = (XmlElement)menu.AppendChild(menu.CreateElement("Node"));
tierI.SetAttribute(description, "IsAdminExample");
tierI.SetAttribute(criteria, "IsAdmin");
XmlElement tierIIa = (XmlElement)tierI.AppendChild(menu.CreateElement("Node"));
tierIIa.SetAttribute(description, "IsABCAdminExample");
tierIIa.SetAttribute(criteria, "IsABCAdmin");
XmlElement tierIIb = (XmlElement)tierI.AppendChild(menu.CreateElement("Node"));
tierIIb.SetAttribute(description, "IsContributorExample");
tierIIb.SetAttribute(criteria, "IsContributor");
XmlElement tierIIIa =
(XmlElement)tierIIa.AppendChild(menu.CreateElement("Node"));
tierIIIa.SetAttribute(description, "IsDivisionManagerExample");
tierIIIa.SetAttribute(criteria, "IsDivisionManager");
XmlElement tierIIIb =
(XmlElement)tierIIa.AppendChild(menu.CreateElement("Node"));
tierIIIb.SetAttribute(description, "CanSelectTimeEntryExample");
tierIIIb.SetAttribute(criteria, "CanSelectTimeEntry");
XmlElement tierIVa =
(XmlElement)tierIIIa.AppendChild(menu.CreateElement("Node"));
tierIVa.SetAttribute(description, "ShouldAlwaysBeHidden");
tierIVa.SetAttribute(criteria, "NeverMatchThis");
XmlElement tierIVb =
(XmlElement)tierIIIa.AppendChild(menu.CreateElement("Node"));
tierIVb.SetAttribute(description, "ShouldNeverBeHidden");
return menu;
}
}
}
There is a lot of my code that begs for further refactoring. I'm not crazy about the nested foreach statements in my helper. I had to do something comparable below in the name of using the generated list of unacceptable nodes to hide said nodes. I wonder if there was a way I could have "flattened" the XML hierarchy to beat this problem. What follows is the beginning of a method. You can see how I remove nodes from XML and use a double minus approach to walking a loop so that I never increment past something that was removed. (Honestly, the double minus stuff was already in the code base. I can't take credit.)
public void HideMenuItems(List<string> itemsToHide)
{
for (int i = AppMenu.Items.Count - 1; i >= 0; i--)
{
MenuItem itemI = AppMenu.Items[i];
string nameOfItemI = itemI.Text;
if (itemsToHide.Contains(nameOfItemI))
{
AppMenu.Items.Remove(itemI);
} else {
for (int ii = AppMenu.Items[i].ChildItems.Count - 1; ii >= 0; ii--)
{
MenuItem itemII = AppMenu.Items[i].ChildItems[ii];
string nameOfItemII = itemII.Text;
if (itemsToHide.Contains(nameOfItemII))
{
AppMenu.Items[i].ChildItems.Remove(itemII);
} else {
for (int iii = AppMenu.Items[i].ChildItems[ii].ChildItems.Count - 1; iii >= 0; iii--)
{
No comments:
Post a Comment