Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
    }
}
share|improve this question
    
you should post the updated code as a new question if you are looking for more review on the new code –  Malachi Jun 4 at 14:25
    
Argh, okay.. This results is okay for me right now. –  TryingToImprove Jun 4 at 14:26
    
you are still using the redundant !string.IsNullOrWhiteSpace(param1) in your Map Method, you will likely have a deeply nested if statement there if you use if statements and can't find a different logical path to accomplish what you are doing –  Malachi Jun 4 at 14:28
1  
As was mentioned, you should post a new question if you'd like further review. New code should not be appended and reviewed in the same post. –  Jamal Jun 4 at 14:56

3 Answers 3

up vote 3 down vote accepted

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)

share|improve this answer
2  
I got it working with: var param = new string[4]; –  TryingToImprove Jun 4 at 14:13
    
@Marc-Andre, OP already assigns an Empty String value to each index of the Parameter array by assigning it in the parameters string. –  Malachi Jun 4 at 14:21
    
I have updated my answer.. :) –  TryingToImprove Jun 4 at 14:24
    
@Malachi You can just use the Array.Resize to expand it. The empty area will be filled with null which will fail the string.IsNullOrWhiteSpace test. –  Xiaoy312 Jun 4 at 14:48
    
@Marc-Andre the OP starts out with a 4 element array merged into a string, then takes the string apart into a 4 element array, there will always be a 4 element array the way that the OP has it written. –  Malachi Jun 4 at 23:30

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");
    }
}
share|improve this answer

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

share|improve this answer
1  
I did use regular expressions on the client-side to handle pushState, but went away from it because it was not very easy to mange changes to –  TryingToImprove Jun 5 at 7:41
    
@TryingToImprove I'm not sure what you mean by "not very easy to manage changes", care to elaborate? :) –  h4unt3r Jun 5 at 19:03
    
@h4unt3r, elaborating that Regex is Regex. now you have 2 problems the original and the Regex. there is no reason for the regex the code is able to separate the parameters based on a single delimiter / and the inputted parameters are already in the format they need to be, Regex would make this code less readable in my opinion. –  Malachi Jun 5 at 19:58
    
@h4unt3r What Malachi says.. aaaand I am not very good to regex.. ;) But I like the idea of using regex –  TryingToImprove Jun 6 at 7:53

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.