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'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?

Update: 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
add comment

6 Answers

up vote 7 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
add comment

You could associate each condition with a strategy for changing the query:

delegate IQueryable<T> QueryMutator<T>(IQueryable<T> item);

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

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

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

List<SearchFieldMutator> SearchFieldMutators { get; set; }

You would then populate the map as so:

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

Then you simply apply all mutators to a given search:

var users = from user in context.Users ...;

foreach (var searchFieldMutator in SearchFieldMutators)
    users = searchFieldMutator.Apply(search, users);
share|improve this answer
add comment

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
add comment

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
add comment

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
add comment

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
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.