2
\$\begingroup\$

As a source for a jquery-ui autocomplete field, I've written this action:

public ActionResult Autocomplete(string term)
{
    List<Relation> relations =
        db.Relation
            .Where(l => l.AdministrationId == Helpers.Session.AdministrationId &&
                        l.IsSupplier == true)
            .ToList();

    term = term.ToLowerInvariant();

    List<string> results = new List<string>();

    foreach (Relation r in relations) {
        string name = r.Name.ToLowerInvariant();

        if (name.StartsWith(term) || name.Contains(term)) {
            results.Add(name);
        }
    }

    results = results.OrderBy(l => l).Take(5).ToList();

    return Json(results, JsonRequestBehavior.AllowGet);
}

Since this is very verbose and probably far from optimized, I'd like to know what I can improve.

I've tried to put it all in a single LINQ query, like the code below. However, LINQ will try to interpret everything as extension methods and throw an error (ToLowerInvariant() and StartsWith()).

term = term.ToLowerInvariant();

IEnumerable<string> suppliers =
    db.Relations
        .Where(l => l.AdministrationId == Helpers.Session.AdministrationId &&
                    l.IsSupplier== true &&
                    (l.Name.ToLowerInvariant().StartsWith(term) ||
                     l.Name.ToLowerInvariant().Contains(term)))
        .Select(l => l.Name)
        .OrderBy(l => l)
        .Take(5);

What I'm looking for is a way to improve the first code snippet. It looks very verbose now and I'm pretty sure it's far from optimized when handling a lot of requests and when I have lots of rows in my database.

\$\endgroup\$
4
\$\begingroup\$

Naming
Instead of term maybe searchTerm would be better. As in your second example you name the IEnumerable<> variable suppliers, your method would be better named as AutocompleteSuppliers.

Review

A Boolean variable can be evaluated just by using the variable, so && l.IsSupplier == true can be replaced by && l.IsSupplier. As an evaluation of an boolean is faster as the comparing of an int (assuming AdministrationId is an int) we change the order of the where condition like

List<Relation> relations =
    db.Relation
        .Where(l => l.IsSupplier &&
               l.AdministrationId == Helpers.Session.AdministrationId)  

Inside the foreach loop you are first checking if the Relation.Name is starting with term and if not, you are checking if term is contained inside Relation.Name. This will be faster if term is often the starting of the name, but will be slower if it is contained as of the double check.
So let us just remove the StartsWith().

Your combination of the Linq statement and the loop does look ok, but will produce different results than in the Autocomplete() method, as you are selecting Relation.Name instead of Relation.Name.ToLowerInvariant().

Refactoring

As this refactoring is based on the original method we use the Select() with ToLowerInvariant() first and are checking afterwards if it contains the searchTerm

public ActionResult AutocompleteSuppliers(string searchTerm)
{
    searchTerm = searchTerm.ToLowerInvariant();

    IEnumerable<string> suppliers = db.Relations
        .Where(l => l.IsSupplier && l.AdministrationId == Helpers.Session.AdministrationId)
        .Select(l => l.Name.ToLowerInvariant())
        .Where(s => s.Contains(searchTerm))
        .OrderBy(s => s)
        .Take(5);

    return Json(suppliers, JsonRequestBehavior.AllowGet);

}
\$\endgroup\$
  • \$\begingroup\$ I had to tweak it a little to get it to work, but these are some nice suggestions. For some reason, LINQ won't recognize ToLowerInvariant() (it will try to execute it as an extension method), but it does know ToLower(). \$\endgroup\$ – Ivo Coumans Sep 17 '14 at 12:11
  • \$\begingroup\$ For me with .NET 4.0 it is working flawless \$\endgroup\$ – Heslacher Sep 17 '14 at 12:12

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.