Chuck Conway

Chuck Conway

Building Inspiring Software

Menu
  • Home
  • Projects
  • Notes
  • About
Menu

Code Refactor

Posted on October 22, 2010 by Chuck Conway

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.

Leave a Reply Cancel reply

Your email address will not be published. Required fields are marked *

    Archives

    • March 2022
    • November 2021
    • October 2021
    • May 2021
    • April 2021
    • March 2021
    • December 2020
    • November 2020
    • October 2020
    • September 2020
    • August 2020
    • July 2020
    • November 2019
    • October 2019
    • September 2019
    • August 2019
    • July 2019
    • June 2019
    • June 2018
    • October 2017
    • December 2015
    • November 2015
    • August 2015
    • May 2015
    • April 2015
    • March 2015
    • February 2015
    • January 2015
    • November 2014
    • October 2014
    • March 2014
    • February 2014
    • December 2013
    • March 2013
    • October 2012
    • August 2012
    • May 2012
    • January 2012
    • December 2011
    • June 2011
    • May 2011
    • December 2010
    • November 2010
    • October 2010

    Categories

    • Architecture
    • Article
    • Code
    • Conceptual
    • Design
    • General
    • Influence
    • Notes
    • Process
    • Satire
    ©2023 Chuck Conway