patterncsharpModerate
Refactoring Methods With Conditionals
Viewed 0 times
refactoringmethodswithconditionals
Problem
I have always suffered with too many conditionals in some of my methods. The following is a pseudocode/skeleton of one of my method like that:
Actual Code which returns an object filled with Controller and corresponding Action names:
As you can see the same
Is there an
private List DoSth(string name = null)
{
List names = new List();
for (int i = 0; i < somearray.Length; i++)
{
if (somecondition)
{
if (name == null)
{
//'Add' to list
}
else
{
if (someothercondition)
{
//'Add' to list
}
}
}
}
return names;
}Actual Code which returns an object filled with Controller and corresponding Action names:
private List GetControllerActions(string ControllerName = null)
{
Type[] typelist = Assembly.GetExecutingAssembly().GetTypes().Where(t => String.Equals(t.Namespace, "Project.Web.Controllers", StringComparison.Ordinal)).ToArray();
List names = new List();
for (int i = 0; i w.ReturnType.BaseType != null && (w.ReturnType.BaseType.Name == "ActionResult" || w.ReturnType.Name == "ActionResult"))
.Select(s => s.Name).ToList()
});
}
else
{
if (typelist[i].Name == ControllerName)
{
names.Add(new AppController
{
ControllerName = typelist[i].Name,
ActionName = typelist[i].GetMethods()
.Where(w => w.ReturnType.BaseType != null && (w.ReturnType.BaseType.Name == "ActionResult" || w.ReturnType.Name == "ActionResult"))
.Select(s => s.Name).ToList()
});
}
}
}
}
return names;
}As you can see the same
Add code is being repeated twice and the 'Add' code is not an external method and I would prefer not making it one also. I can also provide the actual code if required.Is there an
Solution
You could just combine the logic into one statement, assuming there is not other things occurring during your if-else blocks and stuff.
If there is, then this won't necessarily work for you, and you should post the real code, because a lot of times these problems are subjective to the code.
I guess one lesson to possibly learn from this is just to realize the end result, and build a condition that matches the necessary conditions to do something.
Edit: To expand upon the OPs full code being posted.
My original answer is still valid you would write it like this:
However, now that you have actual code to review, I will also point out that there is no reason to be using a
At this point I am recognizing that we are iterating through a list, and conditionally doing one thing. This appears to be the perfect candidate for using some linq.
using Where and some lambda we can make this statement to return an
On our newly created
Now if this was a time where names was an existing list we only needed to append to we could use
we can just ToList() our results and save them as the list.
At this point you could have a few different variables, or you could make your code look awesome and confusing and one-line it all. because I see now that typelist is only being referenced once. (Btw it should probably named typeList, with a capital L, but we're getting rid of it anyway).
So in the very end you can have this beautiful chunk of code. Note: I am also going to use
If there is, then this won't necessarily work for you, and you should post the real code, because a lot of times these problems are subjective to the code.
I guess one lesson to possibly learn from this is just to realize the end result, and build a condition that matches the necessary conditions to do something.
if (somecondition && (name == null || someothercondition))
{
//'Add' to list
}Edit: To expand upon the OPs full code being posted.
My original answer is still valid you would write it like this:
for (int i = 0; i w.ReturnType.BaseType != null && (w.ReturnType.BaseType.Name == "ActionResult" || w.ReturnType.Name == "ActionResult"))
.Select(s => s.Name).ToList()
});
}However, now that you have actual code to review, I will also point out that there is no reason to be using a
for loop vs a foreach loop. I would write that as follows.foreach (var t in typelist)
{
if (t.BaseType.Name == "Controller" && (ControllerName == null || t.Name == ControllerName))
names.Add(new AppController
{
ControllerName = t.Name,
ActionName = t.GetMethods()
.Where(w => w.ReturnType.BaseType != null && (w.ReturnType.BaseType.Name == "ActionResult" || w.ReturnType.Name == "ActionResult"))
.Select(s => s.Name).ToList()
});
}At this point I am recognizing that we are iterating through a list, and conditionally doing one thing. This appears to be the perfect candidate for using some linq.
using Where and some lambda we can make this statement to return an
IEnumerable which only contains Types we need to add.typelist.Where(t => t.BaseType.Name == "Controller" && (ControllerName == null || t.Name == ControllerName))On our newly created
IEnumerable we can Select what we want to add to the names list..Select(t => new AppController
{
ControllerName = t.Name,
ActionName = t.GetMethods()
.Where(w => w.ReturnType.BaseType != null && (w.ReturnType.BaseType.Name == "ActionResult" || w.ReturnType.Name == "ActionResult"))
.Select(s => s.Name).ToList()
});Now if this was a time where names was an existing list we only needed to append to we could use
names.AddRange() but because this list was created here for this purpose (I think).we can just ToList() our results and save them as the list.
At this point you could have a few different variables, or you could make your code look awesome and confusing and one-line it all. because I see now that typelist is only being referenced once. (Btw it should probably named typeList, with a capital L, but we're getting rid of it anyway).
So in the very end you can have this beautiful chunk of code. Note: I am also going to use
string.IsNullOrWhiteSpace instead of just checking if it is null.private List GetControllerActions(string ControllerName = null)
{
return
System.Reflection.Assembly.GetExecutingAssembly().GetTypes().Where(t => String.Equals(t.Namespace, "Project.Web.Controllers", StringComparison.Ordinal))
.Where(t => t.BaseType.Name == "Controller" && (string.IsNullOrWhiteSpace(ControllerName) || t.Name == ControllerName))
.Select(t => new AppController
{
ControllerName = t.Name,
ActionName = t.GetMethods()
.Where(w => w.ReturnType.BaseType != null && (w.ReturnType.BaseType.Name == "ActionResult" || w.ReturnType.Name == "ActionResult"))
.Select(s => s.Name).ToList()
})
.ToList();
}Code Snippets
if (somecondition && (name == null || someothercondition))
{
//'Add' to list
}for (int i = 0; i < typelist.Length; i++)
{
if (typelist[i].BaseType.Name == "Controller" && (ControllerName == null || typelist[i].Name == ControllerName))
names.Add(new AppController
{
ControllerName = typelist[i].Name,
ActionName = typelist[i].GetMethods()
.Where(w => w.ReturnType.BaseType != null && (w.ReturnType.BaseType.Name == "ActionResult" || w.ReturnType.Name == "ActionResult"))
.Select(s => s.Name).ToList()
});
}foreach (var t in typelist)
{
if (t.BaseType.Name == "Controller" && (ControllerName == null || t.Name == ControllerName))
names.Add(new AppController
{
ControllerName = t.Name,
ActionName = t.GetMethods()
.Where(w => w.ReturnType.BaseType != null && (w.ReturnType.BaseType.Name == "ActionResult" || w.ReturnType.Name == "ActionResult"))
.Select(s => s.Name).ToList()
});
}typelist.Where(t => t.BaseType.Name == "Controller" && (ControllerName == null || t.Name == ControllerName)).Select(t => new AppController
{
ControllerName = t.Name,
ActionName = t.GetMethods()
.Where(w => w.ReturnType.BaseType != null && (w.ReturnType.BaseType.Name == "ActionResult" || w.ReturnType.Name == "ActionResult"))
.Select(s => s.Name).ToList()
});Context
StackExchange Code Review Q#45492, answer score: 11
Revisions (0)
No revisions yet.