Shorten code to perform search based on four slash-delimited parameters

Posted on

Problem

How can I make this code more compact?

It is a ASP.net WebForms project

It is a SinglePageApplication for searching in list mode and map mode.

I have many paths for a page.

  • /Denmark
  • /Denmark/Copenhagen
  • /Denmark/Copenhagen/Christiansborg
  • Map/Denmark
  • Map/Denmark/Copenhagen
  • Map/Denmark/Copenhagen/Christiansborg

in which that control the UI and parameteres passed to a JavaScript function

string parameters = (string)Page.RouteData.Values["params"],
    param1 = string.Empty,
        param2 = string.Empty,
            param3 = string.Empty,
                param4 = string.Empty;

if (!string.IsNullOrWhiteSpace(parameters))
{
    var parameterArray = parameters.Split('/');
    var i = 0;
    while (i < parameterArray.Length)
    {
        if (i == 3)
            param4 = parameterArray[3];

        if (i == 2)
            param3 = parameterArray[2];

        if (i == 1)
            param2 = parameterArray[1];

        if (i == 0)
            param1 = parameterArray[0];

        i++;
    }
}


SearchState = new RoomSearchState(param1, param2, param3, param4);

And the RoomStateSearch

public RoomSearchState(string param1, string param2, string param3, string param4)
{
    if (!string.IsNullOrWhiteSpace(param1))
    {
        if (param1.Equals("Kort", StringComparison.InvariantCultureIgnoreCase))
        {
            ViewType = "Map";

            if (!string.IsNullOrWhiteSpace(param2) && !string.IsNullOrWhiteSpace(param3) && !string.IsNullOrWhiteSpace(param4))
            {
                Country = param2.ToUnfriendlyUrl();
                Area = param3.ToUnfriendlyUrl();
                SubArea = param4.ToUnfriendlyUrl();
                SearchType = SearchStateType.Address;
            }
            else if (!string.IsNullOrWhiteSpace(param2) && !string.IsNullOrWhiteSpace(param3))
            {
                SearchType = param2.StartsWith("@") ? SearchStateType.Coordinates : SearchStateType.Address;
                Area = param3.ToUnfriendlyUrl();
                Country = param2.ToUnfriendlyUrl();
            }
            else if (!string.IsNullOrWhiteSpace(param2) && param2.StartsWith("@"))
            {
                SearchType = SearchStateType.Coordinates;
                Area = param2;
            }
            else if (!string.IsNullOrWhiteSpace(param2))
            {
                SearchType = SearchStateType.Address;
                Area = param2.ToUnfriendlyUrl();
            }
            else
            {
                SearchType = SearchStateType.Empty;
            }
        }
        else
        {
            ViewType = "Default";

            if (!string.IsNullOrWhiteSpace(param1) && !string.IsNullOrWhiteSpace(param2) && !string.IsNullOrWhiteSpace(param3))
            {
                Country = param1.ToUnfriendlyUrl();
                Area = param3.ToUnfriendlyUrl();
                SearchType = SearchStateType.Address;
                SubArea = param2.ToUnfriendlyUrl();
            }
            else if (!string.IsNullOrWhiteSpace(param1) && !string.IsNullOrWhiteSpace(param2))
            {
                Area = param2.ToUnfriendlyUrl();
                SearchType = SearchStateType.Address;
                Country = param1.ToUnfriendlyUrl();
            }
            else if (!string.IsNullOrWhiteSpace(param1) && param1.StartsWith("@"))
            {
                SearchType = SearchStateType.Coordinates;
                Area = param1;
            }
            else if (!string.IsNullOrWhiteSpace(param1))
            {
                SearchType = SearchStateType.Address;
                Area = param1.ToUnfriendlyUrl();
            }
            else
            {
                SearchType = SearchStateType.Empty;
            }
        }
    }
    else
    {
        SearchType = SearchStateType.Empty;
    }
}

Solution

This code doesn’t look like much fun at all

if (!string.IsNullOrWhiteSpace(parameters))
{
    var parameterArray = parameters.Split('/');
    var i = 0;
    while (i < parameterArray.Length)
    {
        if (i == 3)
            param4 = parameterArray[3];

        if (i == 2)
            param3 = parameterArray[2];

        if (i == 1)
            param2 = parameterArray[1];

        if (i == 0)
            param1 = parameterArray[0];

        i++;
    }
}

you should have a string array called param and fill it with the parameters you need.

var parameterArray = parameters.Split('/');
var param = new string[4];

for (int i = 0; i < parameterArray.Length; i++) {
    param[i] = parameterArray[i];
}

SearchState = new RoomSearchState(param[0], param[1], param[2], param[3]);

This is much simpler than what you were doing and makes a good use of a string array


in RoomStateSearch your nested if statements are horribly redundant.

You should look at the logic and see what you can combine in the first set you come to you repeat !string.IsNullOrWhiteSpace(param2) in the conditional statement when it should be it’s own if statement that houses everything restricted by this condition.

Same thing with the second (big) if statement block only the condition is !string.IsNullOrWhiteSpace(param1)

Code Structure

  • It is quite tedious what you trying to do in the first piece of code. You might as well just pass the string array directly. As accessing an indexed element is not much different than variable named like paramx.

  • The second piece of code is mirrored from the middle. The lower section and upper section are essentially doing the same job with only 1 offset in parameter, and ViewType being different.

  • Never repeatedly make the same test, if possibly. Try to process by reduction and elimination, or change the flow of code.

Logic Flaws

  • When only one argument or two arguments (that begins with "Map" or "Kort") are passed, then Area is being assigned instead of Country.

    else if (!string.IsNullOrWhiteSpace(param1))
    {
        SearchType = SearchStateType.Address;
        Area = param1.ToUnfriendlyUrl();
    }
    
  • When 3 or 4(with Map/Kort) arguments are passed, SubArea is assigned before Area.

    if (!string.IsNullOrWhiteSpace(param1) && !string.IsNullOrWhiteSpace(param2) && !string.IsNullOrWhiteSpace(param3))
    {
        Country = param1.ToUnfriendlyUrl();
        Area = param3.ToUnfriendlyUrl();
        SearchType = SearchStateType.Address;
        SubArea = param2.ToUnfriendlyUrl();
    }
    

Polished Code

SearchState = new RoomSearchState(parameters.Split('/'))
public RoomSearchState()
{
    SearchType = SearchStateType.Empty;
    ViewType = "Default";
}

public RoomSearchState(string[] args) : this()
{
    if (!args.Any()) return;

    if (args[0] == "Kort" || args[0] == "Map")
    {
        ViewType = "Map";
        args = args.Skip(1).ToArray();
    }

    switch(args.Length)
    {
        case 1:
            if (args[0].StartsWith("@"))
            {
                Country = args[0];
                SearchType = SearchStateType.Coordinates;
            }
            else
            {
                Country = args[0].ToUnfriendlyUrl();
                SearchType = SearchStateType.Address;
            }
            break;
        case 2:
            Country = args[0].ToUnfriendlyUrl();
            Area = args[1].ToUnfriendlyUrl();
            SearchType = SearchStateType.Address;
            break;
        case 3:
            Country = args[0].ToUnfriendlyUrl();
            Area = args[1].ToUnfriendlyUrl();
            SubArea = args[2].ToUnfriendlyUrl();
            SearchType = SearchStateType.Address;
            break;
        default: throw new ArgumentException("args");
    }
}

I can’t believe nobody has suggested using regular expressions?
I’m not too familiar with ASP.NET but I’ve done development in JavaScript.

MatchCollection matches = Regex.Matches( "/?(w+)/?", parameters );

Then you can use matches as the parameter array.

* Note I don’t have the ability to test this

Leave a Reply

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