Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I'm developing an application using ASP.NET MVC 3 and Entity Framework 4.1. In that application I have a lot of paged lists. Users can filter and sort these lists.

This results in code like the one below. I'm not really happy with this code. Is there a better way to do the filtering and sorting with Entity Framework?

Some of you might suggest putting that code into a service class and not in the controller, but that would just move the ugly code somewhere else. Instead of ugly code in a controller, I'd end up with ugly code in a service.

public UsersController : Controller
{
    private const int PageSize = 25;

    public ActionResult Index(int page = 1, string sort = "", UserSearchViewModel search)
    {
        // Get an IQueryable<UserListItem>
        var users = from user in context.Users
                    select new UserListItem
                    {
                        UserId = user.UserId,
                        Email = user.Email,
                        FirstName = user.FirstName,
                        LastName = user.LastName,
                        UsertypeId = user.UsertypeId,
                        UsertypeDescription = users.Usertype.Description,
                        UsertypeSortingOrder = users.Usertype.SortingOrder
                    };

        // Filter on fields when needed
        if (!String.IsNullOrWhiteSpace(search.Name)) users = users.Where(u => u.FirstName.Contains(search.Name) || u.LastName.Contains(search.Name));
        if (!String.IsNullOrWhiteSpace(search.Email)) users = users.Where(u => u.Email.Contains(search.Email));
        if (search.UsertypeId.HasValue) users = users.Where(u => u.UsertypeId == search.UsertypeId.Value);

        // Calculate the number of pages based on the filtering
        int filteredCount = users.Count();
        int totalPages = Convert.ToInt32(Math.Ceiling((decimal)filteredCount / (decimal)PageSize));

        // Sort the items
        switch(sort.ToLower())
        {
            default:
                users = users.OrderBy(u => u.FirstName).ThenBy(u => u.LastName);
                break;
            case "namedesc":
                users = users.OrderByDescending(u => u.FirstName).ThenByDescending(u => u.LastName);
                break;
            case "emailasc":
                users = users.OrderBy(u => u.Email);
                break;
            case "emaildesc":
                users = users.OrderByDescending(u => u.Email);
                break;
            case "typeasc":
                users = users.OrderBy(u => u.UsertypeSortingOrder);
                break;
            case "typedesc":
                users = users.OrderByDescending(u => u.UsertypeSortingOrder);
                break;
        }

        // Apply the paging
        users = users.Skip(PageSize * (page - 1)).Take(PageSize);

        var viewModel = new UsersIndexViewModel
                        {
                            Users = users.ToList(),
                            TotalPages = totalPages
                        };

        return View(viewModel);
    }
}
share|improve this question
    
I am not sure about what your actual concern is. The code is readable and at least a first pass looks like it should work as expected. It is easy to follow. Sometimes the logic requires this type of complexity but to me its looks pretty clean. I could write the query as a single line but runtime effectively will be the same but it would be much harder to understand. – Chad Jul 21 '11 at 14:16
    
I was hoping there was some cleaner/shorter way to do the filtering (the three if-statements in this case) and the sorting (the switch-statement). In this example, I'm only filtering and sorting on 3 fields, but I also have list where I needs 6 fields or more. That quickly leads to a lot of code and it looks ugly. How would you write it on a single line and where does my logic suck? :-) – Kristof Claes Jul 21 '11 at 14:19
    
I could write the entire line as a single line of code. That would not make it any better just shorter. Code golf is fine for honing skills but making readable, and maintainable code is far more valuable. – Chad Jul 21 '11 at 14:21
    
And I edited my first comment. Your logic doesnt suck. Sometimes the logic gets complex which sucks. As I said I would be quite happy if i was handed this to maintain. By contrast as a single command line would include many compares that replace the if statements. The end result query that gets executed would probably be the same or maybe even worse. But it would be much harder to follow. – Chad Jul 21 '11 at 14:25
    
Oh, OK, I understand. I agree that the code is easy to read and understand, but it's kind of a drag to write it :-) I was hoping for some improvement in that department without sacrificing the readability. – Kristof Claes Jul 21 '11 at 14:33
up vote 13 down vote accepted

I know this is old, but thought it may be helpful for anyone reading this. If you want to clean up the code, you can always refactor it.. something like this is more readable than the original:

public UsersController : Controller
{
    private const int PageSize = 25;

    public ActionResult Index(int page = 1, string sort = "", UserSearchViewModel search)
    {
        var users = GetUsers(search, sort);
        var totalPages = GetTotalPages(users);

        var viewModel = new UsersIndexViewModel
        {
            Users = users.Skip(PageSize * (page - 1)).Take(PageSize).ToList(),
            TotalPages = totalPages
        };

        return View(viewModel);
    }

    private UserListItem GetUsers(UserSearchViewModel search, string sort)
    {
        var users = from user in context.Users
                    select new UserListItem
                    {
                        UserId = user.UserId,
                        Email = user.Email,
                        FirstName = user.FirstName,
                        LastName = user.LastName,
                        UsertypeId = user.UsertypeId,
                        UsertypeDescription = users.Usertype.Description,
                        UsertypeSortingOrder = users.Usertype.SortingOrder
                    };

        users = FilterUsers(users, search);
        users = SortUsers(users, sort);

        return users;
    }

    private UserListItem SortUsers(object users, string sort)
    {
        switch (sort.ToLower())
        {
            default:
                users = users.OrderBy(u => u.FirstName).ThenBy(u => u.LastName);
                break;
            case "namedesc":
                users = users.OrderByDescending(u => u.FirstName).ThenByDescending(u => u.LastName);
                break;
            case "emailasc":
                users = users.OrderBy(u => u.Email);
                break;
            case "emaildesc":
                users = users.OrderByDescending(u => u.Email);
                break;
            case "typeasc":
                users = users.OrderBy(u => u.UsertypeSortingOrder);
                break;
            case "typedesc":
                users = users.OrderByDescending(u => u.UsertypeSortingOrder);
                break;
        }
        return users;
    }

    private UserListItem FilterUsers(object users, UserSearchViewModel search)
    {
        if (!String.IsNullOrWhiteSpace(search.Name)) users = users.Where(u => u.FirstName.Contains(search.Name)
                                                                              || u.LastName.Contains(search.Name));
        if (!String.IsNullOrWhiteSpace(search.Email)) users = users.Where(u => u.Email.Contains(search.Email));
        if (search.UsertypeId.HasValue) users = users.Where(u => u.UsertypeId == search.UsertypeId.Value);
        return users;
    }

    private int GetTotalPages(UserListItem users)
    {
        var filteredCount = users.Count();
        return Convert.ToInt32(Math.Ceiling((decimal)filteredCount / (decimal)PageSize));
    }
}

You can then refactor this further by moving these methods into a service class if you want to.

share|improve this answer

EDIT: I apologize from my earlier sample that didn't quite compile. I've fixed it and added a more complete example.

You could associate each condition with a strategy for changing the query. Each strategy (called SearchFieldMutator in this example) will hold two things:

  1. A way to decide if to apply the strategy.
  2. The strategy itself.

The first part is a delegate (of type Predicate<TSearch>) that returns true or false based on the data in the UserSearchViewModel (or any other type, since it only defines a generic type TSearch). If it return true, the strategy is applied. If it returns false, it's not applied. This is the type of the delegate:

Predicate<TSearch>

(it could also have been written as Func<TSearch, bool>)

The second part is the strategy itself. It is supposed to 'mutate' the query by applying a LINQ operator to it, but really just returns a new query with the added operator, and the caller of it should discard the old query and keep the new one. So it's not really mutation, but it has the same effect. I've created a new delegate type for it, so its usage is clear:

public delegate IQueryable<TItem> QueryMutator<TItem, TSearch>(IQueryable<TItem> items, TSearch search);

NOTE: I've defined both the item type and the search data as generic types (as TItem and TSearch, respectively) so this code is usable in multiple locations in your code. But if this is confusing, you can remove the generics completely, and replace any TItem with UserListItem and any TSearch with UserSearchViewModel.

Now that we have defined the two types of the strategy, we can create a class that holds them both, and also does the (simulated) mutation:

public class SearchFieldMutator<TItem, TSearch>
{
    public Predicate<TSearch> Condition { get; set; }
    public QueryMutator<TItem, TSearch> Mutator { get; set; }

    public SearchFieldMutator(Predicate<TSearch> condition, QueryMutator<TItem, TSearch> mutator)
    {
        Condition = condition;
        Mutator = mutator;
    }

    public IQueryable<TItem> Apply(TSearch search, IQueryable<TItem> query)
    {
        return Condition(search) ? Mutator(query, search) : query;
    }
}

This class holds both the condition and the strategy itself, and by using the Apply() method, we can easily apply it to a query if the condition is met.

We can now go create out list of strategies. We'll define some place to hold them (on one of your classes), since they only have to be created once (they are stateless after all):

List<SearchFieldMutator<UserListItem, UserSearchViewModel>> SearchFieldMutators { get; set; }

We'll then populate the list:

SearchFieldMutators = new List<SearchFieldMutator<UserListItem, UserSearchViewModel>>
{
    new SearchFieldMutator<UserListItem, UserSearchViewModel>(search => !string.IsNullOrWhiteSpace(search.Name), (users, search) => users.Where(u => u.FirstName.Contains(search.Name) || u.LastName.Contains(search.Name))),
    new SearchFieldMutator<UserListItem, UserSearchViewModel>(search => !string.IsNullOrWhiteSpace(search.Email), (users, search) => users.Where(u => u.Email.Contains(search.Email))),
    new SearchFieldMutator<UserListItem, UserSearchViewModel>(search => search.UsertypeId.HasValue, (users, search) => users.Where(u => u.UsertypeId == search.UsertypeId.Value)),
    new SearchFieldMutator<UserListItem, UserSearchViewModel>(search => search.CurrentSort.ToLower() == "namedesc", (users, search) => users.OrderByDescending(u => u.FirstName).ThenByDescending(u => u.LastName)),
    new SearchFieldMutator<UserListItem, UserSearchViewModel>(search => search.CurrentSort.ToLower() == "emailasc", (users, search) => users.OrderBy(u => u.Email)),
    // etc...
};

We can then try to run it on a query. Instead of an actual Entity Framework query, I'm going to use a simply array of UserListItems and add a .ToQueryable() on to it. It will work the same if you replace it with an actual database query. I'm also going to create a simple search, for the sake of the example:

// This is a mock EF query.
var usersQuery = new[]
{
    new UserListItem { FirstName = "Allon", LastName = "Guralnek", Email = null, UsertypeId = 7 },
    new UserListItem { FirstName = "Kristof", LastName = "Claes", Email = "whoknows@example.com", UsertypeId = null },
    new UserListItem { FirstName = "Tugboat", LastName = "Captain", Email = "tugboat@ahoy.yarr", UsertypeId = 12 },
    new UserListItem { FirstName = "kiev", LastName = null, Email = null, UsertypeId = 7 },
}.AsQueryable();

var searchModel = new UserSearchViewModel { UsertypeId = 7, CurrentSort = "NameDESC" };

The following actually does all the work, it changes query inside the usersQuery variable to the one specified by all the search strategies:

foreach (var searchFieldMutator in SearchFieldMutators)
    usersQuery = searchFieldMutator.Apply(searchModel, usersQuery);

That's it! This is the result of the query:

Result of query

You can try running it yourself. Here's a LINQPad query for you to play around with:

http://share.linqpad.net/7bud7o.linq

share|improve this answer
2  
This should be the accepted answer. However the posted code doesn't compile which is probably why it doesn't have more votes and is likely a barrier to many due to the generics involved. – TugboatCaptain Mar 24 '15 at 17:01
    
anyone successfully implement this approach? This line bombs delegate IQueryable<T, in T1> QueryMutator<T>(IQueryable<T> item, T1 condition); CS1960 C# Invalid variance modifier. Only interface and delegate type parameters can be specified as variant. – kiev Aug 31 '15 at 17:36
    
@kiev: I've fixed the code so it complies (plus a working sample). I've also added a more detailed explanation. – Allon Guralnek Oct 11 '15 at 8:04

Sorting functionality may be implemented in more declarative syntax. First declare associative dictionary as private member of the class.

    private Dictionary<string, Func<IQueryable<UserListItem>, IQueryable<UserListItem>>> _sortAssoc = new Dictionary<string, Func<IQueryable<UserListItem>, IQueryable<UserListItem>>>(StringComparer.OrdinalIgnoreCase)
    {
        { default(string),      users => users.OrderBy(u => u.FirstName).ThenBy(u => u.LastName)},
        { "namedesc",           users => users.OrderByDescending(u => u.FirstName).ThenByDescending(u => u.LastName)} ,
        { "emailasc",           users => users.OrderBy(u => u.Email) },
        { "emaildesc",          users => users.OrderByDescending(u => u.Email) },
        //...           
    };

then you can call suitable sorting method this way:

users = _sortAssoc.ContainsKey(sort) ? _sortAssoc[sort](users) : _sortAssoc[default(string)](users);
share|improve this answer

If you used the ADO.NET Entity Framework Generator for EF 4.1, you can write your code like below.

the way is to construct a sort string. " order by personname asc" will be written like below "it.personname asc" - the "it" is used internally by EF.

string sortfield = "personname";
string sortdir= "asc";

IQueryable<vw_MyView> liststore= context.vw_MyView
                            .OrderBy("it." + sortfield  + " " + sortdir)
                            .Where(c => c.IsActive == true
                                && c.FundGroupId == fundGroupId
                                && c.Type == 1
                                );

I've used this only for the ADO.NET EF generator. DBcontext for EF 4.3.1 do not support this feature.

share|improve this answer

I'd consider using EntitySQL as a replace to LINQ to Entities. With EntitySQL you would concatenate the sort field name with a statement. Although there is a big disadvantage of no compile time check.

share|improve this answer

I've written a TON of code like this as well...

I've read about "dynamic LINQ" but that's not going to address the issue when you need to orderby().thenby(). The only things I've done differently is using enums for field names and sort directions which works great in my WebForms apps using ViewState but I'm not sure how I'd maintain the "sort state" in MVC ;)

If your table gets large you may consider server-side paging in stored procedures rather than bringing back the whole database then sorting it locally, but that's another topic :)

share|improve this answer
4  
Oh, the deferred execution of Entity Framework ensures the paging is done server side. In fact a call to the database is only made when I do users.ToList(). (A call is also made for users.Count()) – Kristof Claes Jul 21 '11 at 8:16

Another option:

  • Create a domain object that takes the parameters needed for the filter
  • Create a function on the repository that takes that filter domain object and within that repository function handle the filtering/sorting/paging
  • Call that function from your upper layers (whether it is the business layer/service or MVC controller) passing in the filter domain object

Advantages:

  1. If you ever want to enlarge the filter you need only add to the 'filter' domain object
  2. AND modify the repository to handle that new filter
  3. Everywhere else you want to get that set of data, the caller only needs to populate the filter as needed in that situation

Anyhow for what it's worth, that has proved to be the best way for handling this situation with scalability in mind as well as reducing chance of duplication of code.

share|improve this answer

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.