· Chuck Conway · Programming · 6 min read
Code Refactor
Refactoring a large C# file to be leaner and easier to maintain.
On a recent project, I was tasked with refactoring large parts of a web system. It’s written in C#. Over time some of the code-behind files had grown to 4000 lines. The goal was to get this number down to a more maintainable level.
Over the next few posts, I’ve taken snippets of code that I refactored and will explain my thoughts and how I arrived at the solution.
The first code snippet:
string tmp = Request.QueryString["st"];
_varStartRecNum = tmp;
if ((tmp != null) & (!Page.IsPostBack))
{
_varStartRecNum = tmp;
postBack = true;
}
tmp = Request.QueryString["det"];
if ((tmp != null) & (!Page.IsPostBack))
{
_varDetailsRecNum = tmp;
postBack = true;
}
tmp = Request.QueryString["return"];
if ((tmp != null) & (!Page.IsPostBack))
{
postBack = true;
}
tmp = Request.QueryString["searchnow"];
if ((tmp != null) & (!Page.IsPostBack))
{
Session["selectedTab"] = "mtf";
Session["sessionDSProviders"] = null;
Session["mtfs"] = null;
}
tmp = Request.QueryString["displaywalking"];
if (tmp == "true")
{
dispMtf = false;
postBack = true;
}
tmp = Request.QueryString["sb"];
if ((tmp != null) & (!Page.IsPostBack))
{
_varSortBy = tmp;
postBack = true;
switch (_varSortBy)
{
case "Distance":
case "Drive time":
ddlSortBy.SelectedIndex = 0;
break;
case "Name":
ddlSortBy.SelectedIndex = 1;
break;
case "Gender":
ddlSortBy.SelectedIndex = 2;
break;
case "Clinic":
ddlSortBy.SelectedIndex = 3;
break;
case "City":
ddlSortBy.SelectedIndex = 4;
break;
case "Description":
ddlSortBy.SelectedIndex = 5;
break;
}
}
The above code snippet is a collection of if statements, which are an evaluation and an execution. In my first attempt, I tried to use the same evaluation for all if statements, but then I realize one was different. Not understanding the intent of the code I am forced to preserve the logic in verbatim.
Different if evaluation:
tmp = Request.QueryString["displaywalking"];
if (tmp == "true")
{
dispMtf = false;
postBack = true;
}
The switch statement concerned me. The condition to enter into the switch statement is the same as the others. I decided to proceed and worry about the switch statement later.
The code uses the same variable, the ‘tmp’ variable, to retrieve different query value. The value is overwritten with each query value retrieval. For clarity I create a variable for each query value:
string st = Request.QueryString["st"];
string det = Request.QueryString["det"];
string @return = Request.QueryString["return"];
string searchNow = Request.QueryString["searchnow"];
string displayWaling = Request.QueryString["displaywalking"];
string sb = Request.QueryString["sb"];
The next step was to isolate the evaluation and expression while keeping them associated with each other. If an evaluation is true, I want to execute its corresponding expression. I created a class that represented the association.
private class Evaluate
{
public Func Evaluation { get; set; }
public Action Expression { get; set; }
}
Now I can create an evaluation, and if it’s true, I can execute its expression.
The next problem was how to use the above class with all the if statements. I was worried the expressions might get unwieldy in a collection. The whole purpose was to create a concise scaleable solution. The existing solution was neither.
var eval = new[]
{
new Evaluate {Evaluation = () => (!string.IsNullOrEmpty(st) && !IsPostBack), Expression = () => { _varStartRecNum = st;postBack = true; }},
new Evaluate {Evaluation = () => (!string.IsNullOrEmpty(det) && !IsPostBack), Expression = () => { _varStartRecNum = det;postBack = true; }},
new Evaluate {Evaluation = () => (!string.IsNullOrEmpty(@return) && !IsPostBack), Expression = () => {postBack = true; }},
new Evaluate {Evaluation = () => (!string.IsNullOrEmpty(searchNow) && !IsPostBack), Expression = () => {Session["selectedTab"] = "mtf";Session["sessionDSProviders"] = null; Session["mtfs"] = null;}},
new Evaluate {Evaluation = () => (!string.IsNullOrEmpty(displayWaling)), Expression = () => {dispMtf = false; postBack = true;}},
new Evaluate {Evaluation = () => (!string.IsNullOrEmpty(sb) && !IsPostBack), Expression = () => {_varSortBy = sb;postBack = true; SetSort(_varSortBy);}},
};
It turned out better than I expected. One drawback with my solution is, if you don’t know how to use delegates, you’ll be screwed when it comes to maintaining the above code.
The last stumbling block was the switch statement. It was not going to fit gracefully into my anonymous collection, but then it didn’t need to:
private void SetSort(string sortBy)
{
switch (sortBy)
{
case "Distance":
case "Drive time":
ddlSortBy.SelectedIndex = 0;
break;
case "Name":
ddlSortBy.SelectedIndex = 1;
break;
case "Gender":
ddlSortBy.SelectedIndex = 2;
break;
case "Clinic":
ddlSortBy.SelectedIndex = 3;
break;
case "City":
ddlSortBy.SelectedIndex = 4;
break;
case "Description":
ddlSortBy.SelectedIndex = 5;
break;
}
}
By encapsulating it into a method, I was able reference the method in the expression. It worked every nicely.
new Evaluate {Evaluation = () => (!string.IsNullOrEmpty(sb) && !IsPostBack), Expression = () => {_varSortBy = sb;postBack = true; SetSort(_varSortBy);}
The last component is iterating over the collection:
foreach (var evaluate in eval.Where(evaluate => evaluate.Evaluation()))
{
evaluate.Expression();
}
The complete solution:
private class Evaluate
{
public Func Evaluation { get; set; }
public Action Expression { get; set; }
}
private void SetSort(string sortBy)
{
switch (sortBy)
{
case "Distance":
case "Drive time":
ddlSortBy.SelectedIndex = 0;
break;
case "Name":
ddlSortBy.SelectedIndex = 1;
break;
case "Gender":
ddlSortBy.SelectedIndex = 2;
break;
case "Clinic":
ddlSortBy.SelectedIndex = 3;
break;
case "City":
ddlSortBy.SelectedIndex = 4;
break;
case "Description":
ddlSortBy.SelectedIndex = 5;
break;
}
}
private void EvaluateQueryParameters()
{
string st = Request.QueryString["st"];
string det = Request.QueryString["det"];
string @return = Request.QueryString["return"];
string searchNow = Request.QueryString["searchnow"];
string displayWaling = Request.QueryString["displaywalking"];
string sb = Request.QueryString["sb"];
var eval = new[]
{
new Evaluate {Evaluation = () => (!string.IsNullOrEmpty(st) && !IsPostBack), Expression = () => { _varStartRecNum = st;postBack = true; }},
new Evaluate {Evaluation = () => (!string.IsNullOrEmpty(det) && !IsPostBack), Expression = () => { _varStartRecNum = det;postBack = true; }},
new Evaluate {Evaluation = () => (!string.IsNullOrEmpty(@return) && !IsPostBack), Expression = () => {postBack = true; }},
new Evaluate {Evaluation = () => (!string.IsNullOrEmpty(searchNow) && !IsPostBack), Expression = () => {Session["selectedTab"] = "mtf";Session["sessionDSProviders"] = null; Session["mtfs"] = null;}},
new Evaluate {Evaluation = () => (!string.IsNullOrEmpty(displayWaling)), Expression = () => {dispMtf = false; postBack = true;}},
new Evaluate {Evaluation = () => (!string.IsNullOrEmpty(sb) && !IsPostBack), Expression = () => {_varSortBy = sb;postBack = true; SetSort(_varSortBy);}},
};
foreach (var evaluate in eval.Where(evaluate => evaluate.Evaluation()))
{
evaluate.Expression();
}
}
In the end, I like this solution better than the original. One of the drawbacks is the level it’s written. I wanted to create a simpler solution that any developer could maintain. There isn’t anything difficult about the above code; I’m creating a collection and iterating over it. The confusion comes in with the evaluation and the expressions. It’s not a beginner topic.
