Refactoring a razor legacy code.

I will discuss about my generic approach in refactoring in a specific post, but a few weeks ago I found an old code and I remembered what was happened that specific time.

Like in every nightmare, everything started with a simple request
“Could we re-arrange few lists of categories in a different layout?”

More in detail, the task was to rearrange in a different rows-columns layout four lists of categories sorted and grouped by macro category.
In theory pretty easy so the answer was “of course!”

When I do estimates I have to consider development (TDD on my code, the best that I can on the legacy code), a bit of refactoring (boy scout rule) and the deploy in three environments (dev, staging, production) and verify that is ok in all of them.

So, that time, not being the owner the file, I estimated 2 hours (ish) just to be sure.

Then I opened the file responsible of the view.
(I changed the context and put a “category” instead of the real subject, so I renamed specific macro categories in “animals”, “vegetable”, “minerals” and “Alien”)

Take your time to read the file.

@using Project.DATA.Categories.Models

@functions{

    Core.Data.PagedResult<Category> model;
    int catCount = 0;
    int count = 1;
    String catName = "";

    string GetMacroCategory(string catg)
    {
        string catFullName = string.Empty;

        switch (catg)
        {
            case "VEGETABLE":
                catFullName = "Amazing Vegetable"; break;
            case "ANIMAL":
                catFullName = "Amazing Animals"; break;
            case "MINERAL":
                catFullName = "Amazing Minerals"; break;
            default:
                catFullName = "Amazing Alien things"; break;
        }

        return catFullName;
    }
    
    
}

    <div class="catsearch_container columns">
        <p style="margin-top: 1.25rem;font-size:1.4rem;margin-bottom:2rem;"><span style="font-size:2rem;">Search</span> for a category or specific objects</p>
    <div class="small-12 large-12 columns" style="padding:0px;">
        <div class="small-12 medium-8 large-5 column" style="padding:0px;">
        <div class="tabs-container row">
            <div class="row tabs-menu">
                <div class="small-12 large-12 medium-12 columns search-header">
                    <dl class="tabs search-tab-header" data-tab>
                        <dd class="active" style="width:50%;" data-value="1"><a href="#tab-content1">Objects</a></dd>
                        <dd class="" style="width:50%;" data-value="2"><a href="#tab-content2">Category</a></dd> 
                    </dl>
                </div>
            </div>
            <div class="tabs-content" style="margin-bottom:0px;">
                <div class="content active" id="tab-content1">
                     <div class="">
                        <input type="text" placeholder="Enter a Object" id="SearchObjects" name="SearchObjects" maxlength="30" value="" style="height: 2.8125rem;" />
                         @Html.Hidden("SearchObjectsSeoName")
                    </div>
              </div>
                 <div class="content" id="tab-content2">
                     <div class="">
                        <input type="text" placeholder="Enter a Category" id="SearchCategory" name="SearchCategory" maxlength="30" value="" style="height: 2.8125rem;" />
                         @Html.Hidden("SearchcategorieseoName")
                    </div>
              </div>
            </div>
        </div>
    </div>
    </div>
</div>
    <div class="row Category_container" style="margin-top:25px;">
        <p style="margin-top: 1.25rem;margin-left: 0.9375rem;margin-right: 0.9375rem;font-size:1.4rem;"><span style="font-size:2rem;">Select</span> from the Category list below</p>
    <div class="small-12 large-12 columns">
        <ul class="small-block-grid-1 medium-block-grid-2 large-block-grid-2">
             @{foreach (Project.DATA.Categories.Models.Category catlist in Model.Items)
               {
                   

                   if(catName!=catlist.CategoryType)
                   {
                       catCount++;
                       
                       if (catCount != 1) 
                           {
                                @Html.Raw("</ul>");
                                @Html.Raw("<a href=\"javascript:;\" class=\"seeall_button\" data-ul =\"cat_list" + (catCount-1) + "\">See All</a>");
                                @Html.Raw("<a href=\"javascript:;\" class=\"seeless_button\" data-ul =\"cat_list" + (catCount - 1) + "\">See Less</a>");
                                @Html.Raw("</li>");
                           }
                           @Html.Raw("<li class=\"cat_list_li\">");
                           @Html.Raw("<ul id=\"cat_list" + catCount + "\" class=\"cat_list\"><li class=\"header\">" + GetMacroCategory(catlist.CategoryType) + "<div class=\"show-for-small-only category_small_expend\" style=\"float:right; padding-right:20px;font-size:25px; line-height:1;cursor:pointer;\" data-ul =\"cat_list" + (catCount) + "\">-</div></li>");
                           @Html.Raw("<li><a href=\"/categories/categories/" + catlist.categorieseoName + "\">" +  catlist.CategoryName + "</a></li>")
                    
                    }
                   else
                   {
                       if (catlist.categorieseoName!="a-specific-category")
                       {
                            @Html.Raw("<li><a href=\"/categories/categories/" + catlist.categorieseoName + "\">" + catlist.CategoryName + "</a></li>");
                       }
                    }

                   if (count == Model.Items.Length)
                   {
                       @Html.Raw("</ul>");
                       @Html.Raw("<a href=\"javascript:;\" class=\"seeall_button\" data-ul =\"cat_list" + (catCount) + "\">See All</a>");
                       @Html.Raw("<a href=\"javascript:;\" class=\"seeless_button\" data-ul =\"cat_list" + (catCount) + "\">See Less</a>");
                       @Html.Raw("</li>");
                   }
                   count++;
                   catName = catlist.CategoryType;
               }
             }
            
        </ul>
    </div>
</div>

Read it again.
If you feel confused don’t be worried: to understand it (roughly) it took me one hour moving things around and seeing what happened.
Putting aside for a moment the stylesheets inline (sigh), I went straight to the foreach where the categories were generated.

Here the questions in my mind:

  • Why to use Html.Raw and not to write directly the HTML?
  • What is the point in catName!=catlist.CategoryType ? I suppose that a Name should be always different from a Type. So I have to suppose that both are the same thing even if the names suggest something different. So (at least) one of the name is misleading
  • What is the point in
    catCount++;
    if (catCount != 1)
    At the top there is int catCount= 0;
    Why the dev didn’t initialize it as 1 directly or didn’t just check for 0?
    And what is the point in that if?
  • What is the point in catName=catlist.CategoryType;? So the name is the type?

The first thing that I tried to do was to remove the Html.Raw and put html tags.
The view broke badly.

If you use the tags in razor the output must produce valid markup, so for instance if you open a tag and close it in a if, it breaks.

Even if you try something like this:


<div>
    @if (true)
    {
    </div>
    }

The first Html.Raw contained a </ul> and that made invalid everything.
At that moment I begun to understand.

catCount != 1 meant: if it’s not the first iteration there is an <ul> to close.
catName!=catlist.CategoryType meant: (inverting the if statement) if the object is part of the current category, doesn’t open/close anything and if it is not grouped in a category that I don’t want to render (catlist.categorieseoName!=”a-specific-category”) just render the object in the category.

Then the cycle close with an “if” that verifies if it is the last loop: if it is true close the last list.

To understand all these things I spent (wasted) one hour.

03-psycho-screen

But why?
Obviously the view model is not the best for this view, but why?
I went backwards in the application’s layers and I discovered that the model wasn’t a view model.
It was the entity of the repository passed down to the view.
The previous developer may have thought it was a easier or faster approach.

03-psycho-screen

Ok, I commented the code and kept it there as a reference for the markup then I wrote from scratch.
The first point was to split this flat object into the collections that I needed.
I started with something simple just to be sure of the logic

var vegetable = Model.Items.Where(c => c.CategoryType == "VEGETABLE").ToList();
...
...
<ul>
@foreach (var object in vegetable)
            {
                <li><a href="~/categories/category/@object.CategorySeoName">@object.CategoryName</a></li>
            }
</ul>

It worked.
It was still missing the name of the category as “header” of the list, but I decided to add it later.
At that moment I was able to render the lists everywhere in the page, so I did something like this

var vegetable = Model.Items.Where(c => c.CategoryType == "VEGETABLE").ToList();
var animals = Model.Items.Where(c => c.CategoryType == "ANIMALS").ToList();
var minerals = Model.Items.Where(c => c.CategoryType == "MINERALS").ToList();
var aliens = Model.Items.Where(c => c.CategoryType != "VEGETABLE" && c.CategoryType != "ANIMALS" && c.CategoryType != "MINERALS").ToList();
...
...
<ul>
@foreach (var object in vegetable)
            {
                <li><a href="~/categories/category/@object.CategorySeoName">@object.CategoryName</a></li>
            }
</ul>
...
...
<ul>
@foreach (var object in animals )
            {
                <li><a href="~/categories/category/@object.CategorySeoName">@object.CategoryName</a></li>
            }
</ul>
...
...
<ul>
@foreach (var object in minerals )
            {
                <li><a href="~/categories/category/@object.CategorySeoName">@object.CategoryName</a></li>
            }
</ul>
...
...
<ul>
@foreach (var object in aliens)
            {
                <li><a href="~/categories/category/@object.CategorySeoName">@object.CategoryName</a></li>
            }
</ul>
...
...

It worked, but it was there a lot of duplication, so I extracted the linq queries in two private method and I moved the functions block at the bottom of the page.

var vegetable = GetCategoriesByCode("VEGETABLE");
var animals = GetCategoriesByCode("ANIMALS");
var minerals = GetCategoriesByCode("MINERALS");
var aliens = GetAliensCategories();
...
...
...
...
@functions{

        private List<Category> GetCategoriesByCode(string CategoryCode)
        {
            return Model.Items.Where(c => c.CategoryType == CategoryCode).ToList();
        }

        private List<Category> GetAliensCategories()
        {
            return Model.Items.Where(c => c.CategoryType != "VEGETABLE" && c.CategoryType != "ANIMALS" && c.CategoryType != "MINERALS").ToList();
        }
}

The foreach cycles were a duplication too, so I copied the snippet in an another file and i called for times the @Html.RenderPartial.

var vegetable = GetCategoriesByCode("VEGETABLE");
var animals = GetCategoriesByCode("ANIMALS");
var minerals = GetCategoriesByCode("MINERALS");
var aliens = GetAliensCategories();
...
...
...
Html.RenderPartial("~/Views/Shared/categories/_Category.cshtml", vegetable);
...
Html.RenderPartial("~/Views/Shared/categories/_Category.cshtml", animals);
...
Html.RenderPartial("~/Views/Shared/categories/_Category.cshtml", minerals);
...
Html.RenderPartial("~/Views/Shared/categories/_Category.cshtml", aliens);

Because all the includes were using the same view just passing a different list, I extracted that call in another private method, so the code become

var vegetable = GetCategoriesByCode("VEGETABLE");
var animals = GetCategoriesByCode("ANIMALS");
var minerals = GetCategoriesByCode("MINERALS");
var aliens = GetAliensCategories();
...
...
...
RenderCategory(vegetable);
...
RenderCategory(animals);
...
RenderCategory(minerals);
...
RenderCategory(aliens);
...
...
private void RenderCategory(List<Category> Category)
    {
        Html.RenderPartial("~/Views/Shared/categories/_Category.cshtml", Category);
    }

Then I cleaned up the inline stylesheets and in the new view I added the logic for the header.
Because the search functionality was something not directly related to the view I extracted in another partial.
Here the final result.

The Categories file

@using Project.DATA.Categories.Models
@model Core.Data.PagedResult<Category>

    @{
        var vegetable = GetCategoriesByCode("VEGETABLE");
        var animals = GetCategoriesByCode("ANIMALS");
        var minerals = GetCategoriesByCode("MINERALS");
        var aliens = GetAliensCategories();
    }
    <div class="categoriessearch_container columns">
        @{ Html.RenderPartial("~/Views/Shared/categories/_search.cshtml");}
    </div>
    <div class="row categories_container">
        <p class="sub-header"><span class="enhance-text">Select</span>
 from the category list below</p>
        <div class="small-12 large-12 columns">
            <div class="row">
                <div class="column large-6 small-12 medium-6 noPadding">
                    @{
                        RenderCategory(vegetable);
                        RenderCategory(minerals);
                    } 
                </div>

                <div class="column large-6 small-12 medium-6 noPadding">
                    @{
                        RenderCategory(animals);
                        RenderCategory(aliens);
                    }
                </div>
            </div>
        </div>
    </div>

@functions{

        private List<Category> GetCategoriesByCode(string CategoryCode)
        {
            return Model.Items.Where(c => c.CategoryType == CategoryCode).ToList();
        }

        private List<Category> GetAliensCategories()
        {
            return Model.Items.Where(c => c.CategoryType != "VEGETABLE" && c.CategoryType != "ANIMALS" && c.CategoryType != "MINERALS").ToList();
        }

    private void RenderCategory(List<Category> Category)
    {
        Html.RenderPartial("~/Views/Shared/categories/_Category.cshtml", Category);
    }

}

The Category partial

@using Project.DATA.Categories.Models
@model List<Category>

    <div class="category">
        <ul class="cat_list" id="@Model.First().CategoryType">
            @{
                var count = 0;
            }
            @foreach (Category category in Model)
            {

                if (count == 0)
                {
                    <li class="header">@GetMacroCategoryFullName(category.CategoryType)</li>
                    count++;
                }
                <li><a href="~/categories/categories/@category.CategorySeoName">@category.CategoryName</a></li>
            }
        </ul>
    </div>

    @functions {
        private string GetMacroCategoryFullName(string categoryType)
        {

            switch (categoryType)
            {
                case "VEGETABLE":
                    return "Amazing Vegetable";
                case "ANIMALS":
                    return "Amazing Animals";
                case "MINERALS":
                    return "Amazing Minerals";
                default:
                    return "Amazing Alien things";
            }

        }

    }

Refactoring the partial I understood the meaning of the GetMacroCategory method that, of course, doesn’t return any Macro Category, but, given a type, it returns a full name version of the categoryType field.
So I renamed it in GetMacroCategoryFullName

I decided it was enough even though the code could be optimized even more.
Total time to solve the task, refactoring, moving in all environments: 4.5 hours

4.5 hours to solve a task that in theory was just a re-arrangement of four lists in a different row/column set.
To solve the task I had to go backwards in all the application layers till the DB to understand and verify my assumptions, I had to remove inline stylesheet and rewrite the logic to extract and render the view.

03-psycho-screen

Oh! About the if (catlist.categorieseoName!=”a-specific-category”)
It simply couldn’t happen, so I got rid of it.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google photo

You are commenting using your Google account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s