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.

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:

private List<ReturnType> DoSth(string name = null)
{
    List<ReturnType> names = new List<ReturnType>();

    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<AppController> GetControllerActions(string ControllerName = null)
{
    Type[] typelist = Assembly.GetExecutingAssembly().GetTypes().Where(t => String.Equals(t.Namespace, "Project.Web.Controllers", StringComparison.Ordinal)).ToArray();

    List<AppController> names = new List<AppController>();

    for (int i = 0; i < typelist.Length; i++)
    {
        if (typelist[i].BaseType.Name == "Controller")
        {
            if (ControllerName == null)
            {
                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()
                }); 
            }
            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 any scope of making this code more concise and compact like you can do with ternary operators for example?

share|improve this question
add comment

2 Answers 2

up vote 11 down vote accepted

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.

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 < 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()
    }); 
}

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<Type> 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<Type> 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<AppController> 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();
}
share|improve this answer
    
Thanks for your comment. I have posted the actual code above. –  Md.lbrahim Mar 27 at 13:04
    
@Md.lbrahim with your updated code it looks like my answer still stands as valid. –  BenVlodgi Mar 27 at 13:06
1  
@Md.lbrahim I have updated my post again to use linq and one-line the entire method –  BenVlodgi Mar 27 at 13:46
1  
Epic. Thanks very much! –  Md.lbrahim Mar 27 at 14:08
add comment

As far as I can guess you are trying to test whether some method returns a subclass of System.Web.Mvc.ActionResult repeated snippet here:

w => w.ReturnType.BaseType != null && (w.ReturnType.BaseType.Name == "ActionResult" || w.ReturnType.Name == "ActionResult")

But this snippet fails in three cases:

  1. If a class inherits from a ActionResult indirectly. e.g.

    class Y: System.Web.Mvc.ActionResult {}
    class X: Y {}
    typeof(X).BaseType.Name != "ActionResult"
    
  2. If a class is not a subclass of System.Web.Mvc.ActionResult but happens to be named ActionResult e.g. Some.Unrelated.Name.Space.ActionResult. e.g.

    var t1 = typeof(Some.Unrelated.Name.Space.ActionResult);
    var t2 = typeof(System.Web.Mvc.ActionResult);
    t1.Name == t2.Name;
    t1 != t2;
    
  3. If a class is not a subclass of System.Web.Mvc.ActionResult but happens to directly inherit from a class named ActionResult e.g.

    class X:Some.Unrelated.Name.Space.ActionResult {}
    typeof(X).BaseType.Name == "ActionResult"
    // but 
    typeof(X).BaseType != typeof(System.Web.Mvc.ActionResult)
    

What you actually want is Type.IsAssignable. So:

.Where(w => w.ReturnType.BaseType != null && (w.ReturnType.BaseType.Name == "ActionResult" || w.ReturnType.Name == "ActionResult")) 

should become:

.Where(method => typeof(System.Web.Mvc.ActionResult).IsAssignable(method.ReturnType)))
share|improve this answer
    
Thanks for your insight. Actually I was trying to detect only methods with return type as ActionResult or any type directly inheriting from ActionResult. Your point no. 2 and 3 is definitely correct in my case though. –  Md.lbrahim Mar 28 at 6:20
add comment

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.