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, thenArea
is being assigned instead ofCountry
.else if (!string.IsNullOrWhiteSpace(param1)) { SearchType = SearchStateType.Address; Area = param1.ToUnfriendlyUrl(); }
-
When 3 or 4(with Map/Kort) arguments are passed,
SubArea
is assigned beforeArea
.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