Code Refactor : Part 1

On a recent project, I was tasked with refactoring large parts of a web system. It’s written in C#. Overtime 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 gracefully fit 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 refernce 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.

Getting the Job Done

There is a fine balance between doing it right and delivering.

I manage a team of 8 developers. Each hour the team is blocked is 8 lost development hours.

A co-worker was tasked with getting me a database. After two days of waiting, I inquired the status. He said we was having problems with the data import. When he scripted out the data it was almost 1 gig. 1 gig was a bit much to check into subversion. He was looking for an alternative method.

Another day goes by. I asked him how it’s coming. He says it’s almost done. Ok cool, I’m 3 days down, 192 hours of lost development time. I’m itching to get started. My team is bleeding hours.

On the fourth day he’s ready. Finally! He sends me the database project and the database import scripts. He’s using powershell and bcp to import the data. I send them off to the team with detailed instructions.

The team is 12 hours ahead of me, in India. The feedback loop is 12 hours. It takes 24 hours to get anything started.

As Murphy Law states: “Anything that can go wrong, will go wrong”. The team ran the data import scripts and were met with failure. The Powershell scripts failed, a security issue prevented the import of data.

I’m 5 days behind. 320 hours have been lost. The deadline is looming, we need to get started.

At this point, I need to get the database up and running for the team. On my machine, I detach the database, zip it up and send it to the team. Every developer knows to reattach a database. With in an hour of getting the database all eight developers have a functioning database. Success!

Creating a clever process is good, but sometimes just getting the job done is more important than clever.

Business Layer Value

I had a great discussion with my supervisor about application architecture.

The question at hand was what’s the value of the Business Layer? Most applications I’ve worked on are CRUD applications. Is there any value in a thin veneer over the data layer?

In my experience most business layers consist of pass through methods.

If there isn’t any value, call the data layer directly. Handle the business logic on a case-by-case basis. In most cases this will entail creating a service class to encapsulate the business logic.

In the end, having a business layer that provides nothing but pass through methods is pre-optimization. It’s the “it will save me down the road” mentality. 95% of the time, it’s a waste, it creates multiple of points of change and it increases maintainability.

Weighted Random Distribution

This is genius. I could have used this a couple years ago. I’m posting it here for safe keeping. Note that I am NOT using the random class. The random class is not truly random. It’s based on time. Time is predictable.

RNGCryptoServiceProvider rng = new RNGCryptoServiceProvider();

byte[] result = new byte[8];
rng.GetBytes(result);
double rand = (double)BitConverter.ToUInt64(result, 0) / ulong.MaxValue;
 
//40 percent chance of being selected.
if (rand < 0.40d )
{
 ...
}