5
\$\begingroup\$

I have an ASP.NET MVC3 web application that displays a list of parts (a part is a simple entity with a number and a description).

I have updated the action method to support filtering and paging:

[HttpGet]
public ViewResult Index(int page = 1, int pageSize = 30, string filter = "All")
{
    IEnumerable<Part> parts;
    int totalParts;

    var acceptableFilters = new string[] { 
        "All", "123", "A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", 
        "M", "N", "O", "P", "Q", "R", "S", "T", "U", "V", "W", "X", "Y", "Z" };

    // This checks that the filter value received by the method is an acceptable value.
    // E.G. If a user types in a value in the query string that is not contained in the
    // array above, then the filter is reset to 'All' and all records are queried.
    if (!acceptableFilters.Contains(filter))
    {
        filter = "All";
    }

    if (filter == "All")
    {
        parts = Database.Parts
            .OrderBy(p => p.Number)
            .Skip((page - 1) * pageSize)
            .Take(pageSize);

        totalParts = Database.Parts.Count();
    }
    else if (filter == "123")
    {
        var numbers = new string[]{"1","2","3","4","5","6","7","8","9","0"};

        parts = Database.Parts
            .OrderBy(p => p.Number)
            .Where(p=> numbers.Contains(p.Number.Substring(0,1)))
            .Skip((page - 1) * pageSize)
            .Take(pageSize);

        totalParts = Database.Parts
            .Count(p => numbers.Contains(p.Number.Substring(0, 1)));
    }
    else
    {
        parts = Database.Parts
            .OrderBy(p => p.Number)
            .Where(p => p.Number.StartsWith(filter))
            .Skip((page - 1) * pageSize)
            .Take(pageSize);

        totalParts = Database.Parts.Count(p => p.Number.StartsWith(filter));
    }

    PartsListViewModel viewModel = new PartsListViewModel()
    {
        Filter = filter,
        PageInfo = new PageInfo(page, pageSize, totalParts),
        Parts = parts,
    };

    return View(viewModel);
}

The idea is this:

  • If the filter is equal to 'All' then query all records.
  • If the filter is equal to '123' then query all records that start with a number.
  • If the filter is equal to a letter (A, B, C) then query all records that begin with said letter.

Once the required records have been queried I then need to do some calculations to determine how many pages I have and how many items to display on each page.

This works perfectly but I do not like the code I currently have, specifically the if statement that determines the Linq query to be used as the majority of the code is identical except for the where clause (or lack of where clause if all records are being pulled down). I also don't like the fact that I have to run each query twice: once to get the records and a second time to determine the total record set size.

So, is there a better way of achieving the same result? can the Linq queries be restructured in a more elegant way to reduce the redundant code or is this the best way?

\$\endgroup\$
11
\$\begingroup\$

You can construct LINQ queries step by step instead of writing them all at once

parts = Database.Parts;
if (filter == "123") {
    parts = parts.Where(p => p.Number[0].IsDigit())
} else if (filter != "All") { // Alphabetic filter
    parts = parts.Where(p => p.Number.StartsWith(filter))
}
int totalParts = parts.Count();
parts = parts
    .OrderBy(p => p.Number)
    .Skip((page - 1) * pageSize)
    .Take(pageSize);

I followed the DRY software design principle here. DRY = Don't Repeat Yourself. If you repeat the same (or almost the same) code over and over, the code is less maintainable and is more susceptible to mistake. The fact that you have to write more, if you repeat yourself, is rather secondary.

\$\endgroup\$
  • \$\begingroup\$ + 1 - I especially like this answer as it does not make the code any more complex (the original intent is kept intact). Thank you very much. \$\endgroup\$ – Benjamin Aug 27 '12 at 20:37
  • \$\begingroup\$ I simplified the code further (I un-nested the ifs) in response to Jeff Vanzella’s post and placed a totals count before the paging part. \$\endgroup\$ – Olivier Jacot-Descombes Aug 28 '12 at 16:50
3
\$\begingroup\$

Oliver's looks simple, but you are nesting if statments, which gets really messy.

I also don't like having the numbers array created everytime this method is called. The numbers array should be move to a class readonly Static, saving memory and processing time to create it.

the but IF...else...else statement should be moved to a switch, keeping the code much cleaner.

You don't have to, but I like the pattern of using a dictionary (again, static readonly) with and enum to determine which branch to use.

Instead of getting the count from the source list, get the count from the items returned. This will eliminate one call to the source every time this method is called. I added the .ToList() to the end of the query to make sure the list is stored in memory, so the .Count will not query the data source again.

I also created a method to return your list, this way your declaration and assignment are much closer together, making the method much easier to read.

I can also see a problem with using All, there might be circumstances where it conflicts with a general string query. I would suggest moving the matching patterns in my dictionary into some kind of parsing routine (Regex maybe?) to make sure you are not stepping on toes so to speak. This will also shorten the dictionary to 3 entries.

Anyways, my solution follows:

private enum FilterType
{
    All,
    PartNumber,
    StringQuery
}

private static readonly IDictionary<string, FilterType> AcceptableFilters =
    new Dictionary<string, FilterType>
        {
            {"All", FilterType.All},
            {"123", FilterType.PartNumber},
            {"A", FilterType.StringQuery},
            {"B", FilterType.StringQuery},
            {"C", FilterType.StringQuery},
            {"D", FilterType.StringQuery},
            {"E", FilterType.StringQuery},
            {"F", FilterType.StringQuery},
            {"G", FilterType.StringQuery},
            {"H", FilterType.StringQuery},
            {"I", FilterType.StringQuery},
            {"J", FilterType.StringQuery},
            {"K", FilterType.StringQuery},
            {"L", FilterType.StringQuery},
            {"M", FilterType.StringQuery},
            {"N", FilterType.StringQuery},
            {"O", FilterType.StringQuery},
            {"P", FilterType.StringQuery},
            {"Q", FilterType.StringQuery},
            {"R", FilterType.StringQuery},
            {"S", FilterType.StringQuery},
            {"T", FilterType.StringQuery},
            {"U", FilterType.StringQuery},
            {"V", FilterType.StringQuery},
            {"W", FilterType.StringQuery},
            {"X", FilterType.StringQuery},
            {"Y", FilterType.StringQuery},
            {"Z", FilterType.StringQuery},
        };

private static readonly IList<string> Numbers = new List<string>
                                                    {"1", "2", "3", "4", "5", "6", "7", "8", "9", "0"};

[HttpGet]
public ViewResult Index(int page = 1, int pageSize = 30, string filter = "All")
{
    // This checks that the filter value received by the method is an acceptable value.
    // E.G. If a user types in a value in the query string that is not contained in the
    // array above, then the filter is reset to 'All' and all records are queried.
    if (!AcceptableFilters.Keys.Contains(filter))
    {
        filter = "All";
    }

    var parts = RetrievePartsAccordingToFilter(filter);
    var totalParts = parts.Count;

    var viewModel = new PartsListViewModel
                        {
                            Filter = filter,
                            PageInfo = new PageInfo(page, pageSize, totalParts),
                            Parts = parts,
                        };

    return View(viewModel);
}

private IList<Part> RetrievePartsAccordingToFilter(string filter)
{
    switch (AcceptableFilters[filter])
    {
        case FilterType.All:
            return (Database.Parts
                .OrderBy(p => p.Number)
                .Skip((page - 1)*pageSize)
                .Take(pageSize)).ToList();
        case FilterType.StringQuery:
            return (Database.Parts
                .OrderBy(p => p.Number)
                .Where(p => Numbers.Contains(p.Number.Substring(0, 1)))
                .Skip((page - 1)*pageSize)
                .Take(pageSize)).ToList();
        default:
            return (Database.Parts
                .OrderBy(p => p.Number)
                .Where(p => p.Number.StartsWith(filter))
                .Skip((page - 1)*pageSize)
                .Take(pageSize)).ToList();
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for your comments. I had already moved the arrays to a static class to improve performance. The count needs to be performed separately because I need to know the total number of items rather than the amount per page (E.G. if I have 300 items matching my criteria but am only showing 30 per page then your method would only return 10). Olivers method actually makes the count very easy - I simply perform the count after all the If's have been evaluated (so I have selected all my items) but before the paging occurs. This only needs to be performed once. \$\endgroup\$ – Benjamin Aug 27 '12 at 21:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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