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.

This code works fine, but I just don't like all the if else. If I keep adding field to filter, it's going to get messy.

I am not using EF and cannot on this.

Controller code:

var books = bookRepository.GetDogs();    
if (!String.IsNullOrEmpty(searchString))
{

    if (!String.IsNullOrWhiteSpace(searchString) && searchGender != String.Empty && searchHand != String.Empty)
    {
        books = books.Where(b => b.Name.ToUpper().Contains(searchString.ToUpper())
                                 && b.Gender == searchGender && b.Handedness == searchHand);
    }
    else if (!String.IsNullOrWhiteSpace(searchGender) && searchString == String.Empty && searchHand == String.Empty)
    {
        books = books.Where(b => b.Gender == searchGender);
    }
    else if (String.IsNullOrWhiteSpace(searchGender) && searchString != String.Empty && searchHand == String.Empty)
    {
        books = books.Where(b => b.Name.ToUpper().Contains(searchString.ToUpper()));
    }
}

return View(books);

My view:

<p>Find by Name : @Html.TextBox("searchString", ViewBag.CurrentFilter as string)


@Html.DropDownList("searchGender", new SelectList(ViewBag.BookNames), "-- Select All --")
@Html.DropDownList("searchHand", new SelectList(ViewBag.HandNames), "---Select All--")
share|improve this question

migrated from stackoverflow.com Oct 6 '12 at 20:28

This question came from our site for professional and enthusiast programmers.

    
I only ever use the lambda syntax so I might be wrong but isn't var books = from b in bookRepository.GetDogs() select b; equivalent to bookRepository.GetDogs()? –  George Mauer Oct 6 '12 at 20:38
    
I think your right. I am new to the MVC and linq. I am going to make that change. –  Stone Temple Aviators Oct 8 '12 at 17:24
    
If you're new to LINQ then you should definitely use only the lambda syntax. It is far more powerful and makes it much more clear what is actually going on. There are really very very few instances where I would recommend using the sql-style syntax. –  George Mauer Oct 8 '12 at 22:40

2 Answers 2

Just put if, not else if so you can have a cumulative multi criteria search, and one condition in every if.

So every condition is easy to read, and if more than one criteria is filled, they will cumulate fine.

if (!String.IsNullOrWhiteSpace(searchString))
   books = books.Where(b => b.Name.ToUpper().Contains(searchString.ToUpper());       

if (!String.IsNullOrWhiteSpace(searchGender))
   books = books.Where(b => b.Gender == searchGender);

if (String.IsNullOrWhiteSpace(searchHand))
   books = books.Where(b => b.Handedness == searchHand);
share|improve this answer

Ok, so I just recently arrived at a way of doing this sort of thing that I actually like so let me share.

In my opinion, the if statements aren't really the problem, its more that they are not really pertinent to controller code. Actions should strive to contain code only about major decisions of what code path will be executed.

Solution: You take advantage of the fact that a Linq query returns an IQueryable to store the query logic along with the model and an extension method to give it a nice DSL like interface. Similar to the pipes and filters pattern.

This is what the action is going to look like:

public ViewResult Index(SearchModel searchQuery) 
{
  var books = entities.Get<Book>()
                .Where(b=>b.IsInPrint)
                .FilterBy(searchQuery);
  return View(books);
}

And this is what my model looks like:

public class SearchModel 
{
   public string Text { get; set; }
   public bool InStockOnly { get; set; }
}
public static class SearchModelFilterQueryEx
{
  public static IQueryable<Book> FilterBy(this IQueryable<Book> query,  SearchModel search)
  {
    if(search == null)
      return query;
    if(!String.IsNullOrWhiteSpace(search.Text)) 
      query = query.Where(b=>b.Name.Contains(search.Text));
    if(String.InStockOnly)
      query = query.Where(b=>b.NumberInStock > 0);
    return query;
  }
}
share|improve this answer
    
I'm not a huge fan of using IQueryable in views. I think it would be best to at least ToList() the books before passing into the view itself. However in saying that I still like this idea. –  dreza Mar 19 at 19:44
    
Fine point @dreza –  George Mauer Mar 20 at 14:30

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.