How can I refactor my Controller Action to not be so complex?

I'm building a website that is used to search our system. The users can search on a lot of critera at once. As I try to do this though, I can't figure out how to simplify this horrible action.

```
public ActionResult Search(SearchViewModel model)
{
Expression> filterexpression = PredicateBuilder.True();

this.TryUpdateModel(model);

IQueryable query = service.GetAllParcels().AsQueryable();

if (model == null)
{
model = new SearchViewModel();
}

// TODO: add conditions to only include expressions that have values
if (model.ParcelSearch.Strap != null)
{
filterexpression = filterexpression.And(service.StrapStartsWith(model.ParcelSearch.Strap.AsStrap()));
}

if (model.ParcelSearch.OwnerKeywords != null)
{
filterexpression = filterexpression.And(service.ContainsInOwnerName(model.ParcelSearch.OwnerKeywords));
}

if (model.ParcelSearch.AddressKeywords != null)
{
filterexpression = filterexpression.And(service.ContainsInAddress(model.ParcelSearch.AddressKeywords));
}

if (model.ParcelSearch.ZipCode != null)
{
filterexpression = filterexpression.And(service.LocatedWithinZipCode(model.ParcelSearch.ZipCode));
}

if (model.ParcelSearch.Subdivision != null)
{
filterexpression = filterexpression.And(service.IsInSubdivision(model.ParcelSearch.Subdivision));
}

// TODO: add boolean conditions. no need to check these for nulls ATM.
filterexpression =
filterexpression.Expand()
.And(service.HasDock(model.PropertyFeatureSearch.Dock))
.And(service.HasPool(model.PropertyFeatureSearch.Pool))
.And(service.HasSeaWall(model.PropertyFeatureSearch.SeaWall))
.And(service.HasTennisCourt(model.PropertyFeatureSearch.Tennis))
.And(service.IsVacant(model.PropertyFeatureSearch.VacantLand))
.And(service.IsNew(model.PropertyFeatureSearch.StatusNew))
.Expand();

if

I imagine you want to get rid of repetitive if/And blocks. To do that, you may start by generalizing the following pattern (note: I'm using object since you haven't specified the precise types you use; in your case, object will be replaced by something more specific).

var includeExpression = new Action, object>(
    (predicate, obj) =>
    {
        if (predicate)
        {
            filterexpression = filterexpression.And(obj);
        }
    });

This makes it possible to rewrite all those repetitive blocks in a more compact way:

includeExpression(
    model.ParcelSearch.Strap != null,
    service.StrapStartsWith(model.ParcelSearch.Strap.AsStrap()));

You can push it a bit more for the first five blocks, by creating an additional function, based on the first one:

var includeIfNotNull = new Action>(
    (eval, transform) =>
    {
        includeExpression(eval != null, transform(service, model.ParcelSearch)));
    });

Now, you can simply write:

includeIfNotNull(model.ParcelSearch.Strap, (s, p) => s.StrapStartsWith(p.Strap.AsStrap()));

given that this last optimization would be available only for the first five blocks.

An additional rewrite would be to make an IEnumerable containing the predicates and the objects, and call includeIfNotNull inside the first foreach for the first five entries, then includeExpression inside the second foreach for the last five ones. However, I'm not convinced that it would improve readability.

A few other suggestions:

-
Putting the first five and the last five blocks in separate methods would make things clearer, IMO. This also applies to the block:

filterexpression =
    filterexpression.Expand()
                    .And(service.HasDock(model.PropertyFeatureSearch.Dock))
                    .And(service.HasPool(model.PropertyFeatureSearch.Pool))
                    .And(service.HasSeaWall(model.PropertyFeatureSearch.SeaWall))
                    .And(service.HasTennisCourt(model.PropertyFeatureSearch.Tennis))
                    .And(service.IsVacant(model.PropertyFeatureSearch.VacantLand))
                    .And(service.IsNew(model.PropertyFeatureSearch.StatusNew))
                    .Expand();

-
The code:

var timer = new Stopwatch();
timer.Start();

should be:

var timer = Stopwatch.StartNew();

-
What about extracting:

IQueryable output;

if (model.SortOption == "strap")
{
    output = query.OrderBy(x => x.STRAP).Take(500);
}
else if (model.SortOption == "PropertyUse")
{
    output = query.OrderBy(x => x.PROPERTY_USE).Take(500);
}
else
{
    output = query.OrderByDescending(x => x.LastUpdated).Take(500);
}

into a separate method which returns IQueryable?

-
timer.Elapsed.TotalSeconds.CompareTo(10) 10 instead.

-
Put the measurement of time into a method which takes an Action as a parameter, like:

private static long MeasureTime(Action action)
{
    var timer = Stopwatch.StartNew();
    action();
    return timer.Milliseconds();
}

so instead of having:

var timer = Stopwatch.StartNew();
Run();
Some();
Code();
timer.Stop();
LogElapsedTime(timer.ElapsedMilliseconds);

you can simply write:

var elapsedMilliseconds = MyClass.MeasureTime(
    () =>
    {
        Run();
        Some();
        Code();
    });

LogElapsedTime(elapsedMilliseconds);

or instead of:

var timer = Stopwatch.StartNew();
RunSomeCode();
timer.Stop();
LogElapsedTime(timer.ElapsedMilliseconds);

you can have:

var elapsedMilliseconds = MyClass.MeasureTime(RunSomeCode);
LogElapsedTime(elapsedMilliseconds);

StackExchange Code Review Q#26850, answer score: 5