2
\$\begingroup\$

Using a Select2, what's the best way to refactor multiple if statements.

This code returns Json to a select2 based on what information it gets in the query. The action result is used in many places so it has accommodate different parameter being passed.

public ActionResult *********(string query = "", int? id = null, int? accountId = null)
    {
        var empty = new object();

        if (query == "" && accountId == null)
        {
            return Json(empty, JsonRequestBehavior.AllowGet);
        }

        if (query == "" && accountId != null)
        {
            var search = Db.Entities.Find(accountId);
            var suggestions = Db.Entities.Where(
                x => x.Name.Contains(search.Name) && x.EntityId != accountId && !String.IsNullOrEmpty(x.Group.Label))
                .ToList()
                .Select(item => new { id = item.GroupId, text = item.Group.Label });

            return Json(suggestions, JsonRequestBehavior.AllowGet);

        }

        if (id != null)
        {
            var accountGroup = Db.Groups.Find(id);
            if (accountGroup != null)
                return Json(new { id = accountGroup.GroupId, text = accountGroup.Label },
                    JsonRequestBehavior.AllowGet);
            return Json(null, JsonRequestBehavior.AllowGet);
        }
        var list = Db.Groups.Where(
            r => !String.IsNullOrEmpty(query) && query.Length > 2 && (r.StatusId == GroupStatus.Both || r.StatusId == GroupStatus.Customer) && r.Label.ToLower().Contains(query.ToLower())).OrderBy(r => r.Label).Select(item => new { id = item.GroupId, text = item.Label }).ToList();
        if (!list.Any())
        {
            return Json(new List<object>() { new { id = query, text = query } }, JsonRequestBehavior.AllowGet);
        }
        return Json(list, JsonRequestBehavior.AllowGet);
    }
\$\endgroup\$
6
  • 2
    \$\begingroup\$ You've "censored" a lot of things, and I fear that will cause this question to be closed. \$\endgroup\$
    – BCdotWEB
    May 13, 2015 at 10:06
  • \$\begingroup\$ @BCdotWEB Fixed \$\endgroup\$ May 13, 2015 at 10:13
  • 1
    \$\begingroup\$ Besides the redactions, the code is unreviewable because you haven't explained what the code is supposed to accomplish. Please add a description and retitle the post accordingly. See How to Ask \$\endgroup\$ May 13, 2015 at 10:14
  • \$\begingroup\$ @200_success Fixed \$\endgroup\$ May 13, 2015 at 10:19
  • \$\begingroup\$ Some examples of how you call this method. E.g do you call the method like *****(); or *****(someStringVariable); \$\endgroup\$
    – Heslacher
    May 13, 2015 at 10:19

3 Answers 3

3
\$\begingroup\$

In following query

var suggestions = Db.Entities.Where(x => x.Name.Contains(search.Name)
                              && x.EntityId != accountId
                              && !String.IsNullOrEmpty(x.Group.Label))
                 .ToList()
                 .Select(item => new { id = item.GroupId, text = item.Group.Label });

You shouldn't call ToList(). Read this answer from a question on StackOverflow. This is the essential part:

For example, in an Entity Framework query we can only use a restricted number of methods. So if, for example, we need to use one of our own methods in a query we would typically write something like

   var query = context.Observations.Select(o => o.Id)
               .AsEnumerable().Select(x => MySuperSmartMethod(x))

ToList – which converts an IEnumerable to a List – is often used for this purpose as well. The advantage of using AsEnumerable vs. ToList is that AsEnumerable does not execute the query. AsEnumerable preserves deferred execution and does not build an often useless intermediate list.

On the other hand, when forced execution of a LINQ query is desired, ToList can be a way to do that.


No need to check twice for the query to see if it is empty. Your code would be better like this:

if (query == "")
{
    if (accountId == null)
    {
        //return Json
    }
    else
    {
        //return some other Json
    }
}

You can improve the performance of your LinQ query by building it correctly. First check for the EntityId != accountId, then !String.IsNullOrEmpty(x.Group.Label) and last for x.Name.Contains(search.Name):

var suggestions = Db.Entities.Where(x => x.EntityId != accountId
                              && !String.IsNullOrEmpty(x.Group.Label)
                              && x.Name.Contains(search.Name))
                 .AsEnumerable()
                 .Select(item => new { id = item.GroupId, text = item.Group.Label });

Notice that I changed ToList to AsEnumerable.


Variable names like list and search are not meaningful. Give clear names to your variables that kind of represent what they are holding.

var list = Db.Groups.Where(...);

would become:

var groups = Db.Groups.Where(...);
\$\endgroup\$
7
  • \$\begingroup\$ WRT if (query == "") I'd recommend using string.IsNullOrEmpty() instead. \$\endgroup\$
    – BCdotWEB
    May 13, 2015 at 11:49
  • \$\begingroup\$ @BCdotWEB What is WRT? :) You're absolutely right, I was intending to add this too but I forgot. Thanks for mentioning! \$\endgroup\$
    – Abbas
    May 13, 2015 at 11:53
  • 1
    \$\begingroup\$ This will change the behaviour of the method. If query == null the original method would return return Json(new List<object>() { new { id = query, text = query } }, JsonRequestBehavior.AllowGet);. Whereas this will return return Json(new object(), JsonRequestBehavior.AllowGet); \$\endgroup\$
    – Heslacher
    May 13, 2015 at 11:57
  • 1
    \$\begingroup\$ using string.IsNullOrEmpty() instead of either query == "" or query == String.Empty in the first if condition. \$\endgroup\$
    – Heslacher
    May 13, 2015 at 12:01
  • 1
    \$\begingroup\$ Ooooh, now I see, I changed it back to query == ""! :D \$\endgroup\$
    – Abbas
    May 13, 2015 at 12:03
2
\$\begingroup\$

By splitting this method into multiple smaller methods you can reduce the complexity of this method.

if (query == "" && accountId == null)
{
    return Json(empty, JsonRequestBehavior.AllowGet);
}

if (query == "" && accountId != null)
{
    var search = Db.Entities.Find(accountId);
    var suggestions = Db.Entities.Where(
        x => x.Name.Contains(search.Name) && x.EntityId != accountId && !String.IsNullOrEmpty(x.Group.Label))
        .ToList()
        .Select(item => new { id = item.GroupId, text = item.Group.Label });

    return Json(suggestions, JsonRequestBehavior.AllowGet);

}  

here you have one common condition which is query == "" so let us extract this to a separate method.

private ActionResult *********1(int? accountId = null)
{ 
    if (accountId == null)
    {
        return Json(new object(), JsonRequestBehavior.AllowGet);
    }  

    var search = Db.Entities.Find(accountId);
    var suggestions = Db.Entities.Where(
        x => x.Name.Contains(search.Name) && x.EntityId != accountId && !String.IsNullOrEmpty(x.Group.Label))
        .ToList()
        .Select(item => new { id = item.GroupId, text = item.Group.Label });

    return Json(suggestions, JsonRequestBehavior.AllowGet);
}

leaving the original like

public ActionResult *********(string query = "", int? id = null, int? accountId = null)
{
    if (query == "")
    {     
         return *********1(accountId);
    }

    if (id != null)
    {
        var accountGroup = Db.Groups.Find(id);
        if (accountGroup != null)
            return Json(new { id = accountGroup.GroupId, text = accountGroup.Label },
                JsonRequestBehavior.AllowGet);
        return Json(null, JsonRequestBehavior.AllowGet);
    }
    var list = Db.Groups.Where(
        r => !String.IsNullOrEmpty(query) && query.Length > 2 && (r.StatusId == GroupStatus.Both || r.StatusId == GroupStatus.Customer) && r.Label.ToLower().Contains(query.ToLower())).OrderBy(r => r.Label).Select(item => new { id = item.GroupId, text = item.Label }).ToList();
    if (!list.Any())
    {
        return Json(new List<object>() { new { id = query, text = query } }, JsonRequestBehavior.AllowGet);
    }
    return Json(list, JsonRequestBehavior.AllowGet);
}  

Next I would extract the part where (id != null).

private ActionResult *********2(int? id = null)
{
    var accountGroup = Db.Groups.Find(id);
    if (accountGroup != null)
    {
        return Json(new { id = accountGroup.GroupId, text = accountGroup.Label },
            JsonRequestBehavior.AllowGet);
    }
    return Json(null, JsonRequestBehavior.AllowGet);
}  

which results in the original looking like

public ActionResult *********(string query = "", int? id = null, int? accountId = null)
{
    if (query == "")
    {     
         return *********1(accountId);
    }

    if (id != null)
    {
         return *********2(id);
    }

    var list = Db.Groups.Where(
        r => !String.IsNullOrEmpty(query) && query.Length > 2 && (r.StatusId == GroupStatus.Both || r.StatusId == GroupStatus.Customer) && r.Label.ToLower().Contains(query.ToLower())).OrderBy(r => r.Label).Select(item => new { id = item.GroupId, text = item.Label }).ToList();
    if (!list.Any())
    {
        return Json(new List<object>() { new { id = query, text = query } }, JsonRequestBehavior.AllowGet);
    }
    return Json(list, JsonRequestBehavior.AllowGet);
}  

Next I would refactor this monster linq query like

public ActionResult *********(string query = "", int? id = null, int? accountId = null)
{
    if (query == "")
    {     
         return *********1(accountId);
    }

    if (id != null)
    {
         return *********2(id);
    }

    if (query != null && query.Length > 2)
    {
        string lowerQuery = query.ToLower();

        var list = Db.Groups.Where(r => (r.StatusId == GroupStatus.Both || r.StatusId == GroupStatus.Customer) && r.Label.ToLower().Contains(lowerQuery))
                            .OrderBy(r => r.Label)
                            .Select(item => new { id = item.GroupId, text = item.Label });

// removed the call of ToList() because you only need to test if at least one item is in the list. 
// This results in 1 additional trip to the database if at least one item is found.  

        if (list.Any())
        {
            return Json(list.ToList(), JsonRequestBehavior.AllowGet);
        }  

    }

    return Json(new List<object>() { new { id = query, text = query } }, JsonRequestBehavior.AllowGet);

}

By changing the order of conditions you can speed up the queries too.

var suggestions = Db.Entities.Where(
            x => x.Name.Contains(search.Name) && x.EntityId != accountId && !String.IsNullOrEmpty(x.Group.Label))....  

the fastest check would be x.EntityId != accountId followed by !String.IsNullOrEmpty(x.Group.Label) and last x.Name.Contains(search.Name).


You should be consistent with the coding style you use. Some times you are using braces {} for single instrauction if or else and sometimes not. Although there is no explicit rule about using them, I would like to encourage you to always use them to make your code less error prone.


I have no idea about your domain model, but I would like to suggest to rename some of your variables to a better more meaningful name.

var search = Db.Entities.Find(accountId); // looks more like a searchResult  
var list = Db.Groups.Where()              // list is not the best name either
\$\endgroup\$
1
  • \$\begingroup\$ Always nice to read your thorough reviews! \$\endgroup\$
    – Abbas
    May 13, 2015 at 11:22
0
\$\begingroup\$

I'm a bit worried about how this one method seems to be dealing with distinct requests. The value of id for instance is only relevant for a small part that involves neither query nor accountId, so why then clutter this method by inserting that logic here?

Why not instead have:

  • a separate public ActionResult SearchById(int? id = null)
  • and a public ActionResult *********(string query = "", int? accountId = null)?

I also feel query is too generic a name, since it is only used here: r.Label.ToLower().Contains(query.ToLower())). Why not call it groupLabel?

I don't think !String.IsNullOrEmpty(query) && query.Length > 2 should be part of your Where clause, IMHO this is input validation that should happen before you compile the Where clause.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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