1
\$\begingroup\$

This is a query to return search results, but I feel it could be cleaned up a little. I just don't know how.

var queryResult = (from r in dc.Retailers
                    where
                        (string.IsNullOrEmpty(firstName) ||
                        SqlFunctions.PatIndex(firstName.Trim(), r.FirstName.Trim()) > 0) &&
                        (string.IsNullOrEmpty(lastName) ||
                        SqlFunctions.PatIndex(lastName.Trim(), r.LastName.Trim()) > 0) &&
                        (string.IsNullOrEmpty(companyName) ||
                        SqlFunctions.PatIndex(companyName.Trim(), r.CompanyName.Trim()) > 0) &&
                        (string.IsNullOrEmpty(phone) ||
                        SqlFunctions.PatIndex(phone.Trim(), r.Phone.Trim()) > 0) &&
                        (string.IsNullOrEmpty(email) ||
                        SqlFunctions.PatIndex(email.Trim(), r.Email.Trim()) > 0) &&
                        (string.IsNullOrEmpty(city) ||
                        SqlFunctions.PatIndex(city.Trim(), r.City.Trim()) > 0) &&
                        (string.IsNullOrEmpty(zip) ||
                        SqlFunctions.PatIndex(zip.Trim(), r.Zip.Trim()) > 0) &&
                        (string.IsNullOrEmpty(state) ||
                        SqlFunctions.PatIndex(state.Trim(), r.State.Trim()) > 0) &&
                        (string.IsNullOrEmpty(country) ||
                        SqlFunctions.PatIndex(country.Trim(), r.Country.Trim()) > 0)
                    select r
    );

Update: For those confused by the query. Here is another version, which is probably better because it only builds the where parts when necessary. But I would still like to reduce the repetition.

var queryResult = (from r in dc.Retailers select r);
if (!string.IsNullOrEmpty(firstName))
    queryResult = queryResult.Where(ex => SqlFunctions.PatIndex(firstName.Trim(), ex.FirstName.Trim()) > 0);
if (!string.IsNullOrEmpty(lastName))
    queryResult = queryResult.Where(ex => SqlFunctions.PatIndex(lastName.Trim(), ex.LastName.Trim()) > 0);
if (!string.IsNullOrEmpty(companyName))
    queryResult = queryResult.Where(ex => SqlFunctions.PatIndex(companyName.Trim(), ex.CompanyName.Trim()) > 0);
if (!string.IsNullOrEmpty(phone))
    queryResult = queryResult.Where(ex => SqlFunctions.PatIndex(phone.Trim(), ex.Phone.Trim()) > 0);
if (!string.IsNullOrEmpty(email))
    queryResult = queryResult.Where(ex => SqlFunctions.PatIndex(email.Trim(), ex.Email.Trim()) > 0);
if (!string.IsNullOrEmpty(city))
    queryResult = queryResult.Where(ex => SqlFunctions.PatIndex(city.Trim(), ex.City.Trim()) > 0);
if (!string.IsNullOrEmpty(zip))
    queryResult = queryResult.Where(ex => SqlFunctions.PatIndex(zip.Trim(), ex.Zip.Trim()) > 0);
if (!string.IsNullOrEmpty(country))
    queryResult = queryResult.Where(ex => SqlFunctions.PatIndex(country.Trim(), ex.Country.Trim()) > 0);
if (!string.IsNullOrEmpty(state))
    queryResult = queryResult.Where(ex => SqlFunctions.PatIndex(state.Trim(), ex.State.Trim()) > 0);
\$\endgroup\$
  • 1
    \$\begingroup\$ What is the point of the query? It looks like, for each field, you're searching for Retailers where the field is null or empty OR is not null or empty... which doesn't make sense. \$\endgroup\$ – Alex Dresko Nov 1 '13 at 16:44
  • 1
    \$\begingroup\$ It only filters by a search criteria if a value exists. For example. If you type in a state, it will only filter by state. If you type in a state and a zip, it will filter by both. \$\endgroup\$ – Javalsu Nov 1 '13 at 16:58
  • \$\begingroup\$ I think I'd exploit the fact that IQueryable<T> can be "built" from multiple chained calls, ie make a function that takes and returns an IQueryable for each search criteria. \$\endgroup\$ – Mathieu Guindon Nov 1 '13 at 18:02
  • \$\begingroup\$ You want to be careful while trying to refactor this code. You may end up writing code which does filtering on the client side instead of doing it in the database. Because you have all these local variables, it will be hard to make it more generic. Look in to PredicateBuilder -albahari.com/nutshell/predicatebuilder.aspx class to help you build this more dynamically if you were to get rid of the local variables. \$\endgroup\$ – SolutionYogi Nov 1 '13 at 18:11
3
\$\begingroup\$

Apply the common refactoring used for repeated code: extracting a method. In your case, creating the method won't be trivial, because you want to create a different Expression each time. That is, unless you use LINQKit:

public static IQueryable<T> WhereEqualsByPatIndex<T>(
    this IQueryable<T> query, string value, Expression<Func<T, string>> propertyExpression)
{
    if (string.IsNullOrEmpty(value))
        return query;

   return query.Where(
       ex => SqlFunctions.PatIndex(
           value.Trim(), propertyExpression.Invoke(ex, propertyExpression).Trim()) > 0);
}

Usage would be like this (that call to AsExpandable() is important):

dc.Retailers
  .AsExpandable()
  .WhereEqualsByPatIndex(firstName, ex => ex.FirstName)
  .WhereEqualsByPatIndex(lastName, ex => ex.LastName)
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Great answer. I did not know about AsExpandable. \$\endgroup\$ – SolutionYogi Nov 4 '13 at 19:53
  • 1
    \$\begingroup\$ @SolutionYogi Regarding your suggested edit: without the call to AsExpandable(), the code would throw an exception, saying that EF doesn't understand the expression. There are some cases where doing things incorrectly results in performing the filtering on the client, but this is not one of them. \$\endgroup\$ – svick Nov 4 '13 at 20:16

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.