Is it good practice to have linq query in Controllers?

Question

I'm not very familiar with the MVC pattern. Could you tell me which one of the following three controller actions is better? Thanks :)

(1) Have query in action:

public ActionResult List()
{
   var query = repository.Query().Where(it => it.IsHandled).OrderBy(it => it.Id);
   // ...
}

(2) Have query in service:

public ActionResult List() 
{
    var items = service.GetHandledItemsOrderById();
    // ...
}

(3) Have order by in action:

public ActionResult List()
{
    var items = service.GetHandledItems().OrderBy(it => it.Id);
    // ...
}

If we choose (1), then we have too much business logic in controller?

If we choose (2), there might be lots of service methods like GetXXXByYYY().

If we choose (3), why we encapsulate Where(it => it.IsHandled) but not
OrderBy(it => it.Id.

Any ideas?

score 5 · Answer 1 · answered Aug 22 '11 at 02:14

5

I'm sure opinions may vary, but I've learned to try to keep as much business logic in the service as you can. 3 would be my choice. With 1, you've already spotted the issue. With 2, you are introducing display precedence in a service. With 3, you handle display preferences where necessary. If you were to introduce another interface to your business layer, you are requiring potentially, unnecessary code iterations by choosing 2.

answered Aug 22 '11 at 02:14

malckier

1,052
2
14
22

The page needs to show items that are handled order by Id. Why "display already handled(processed) items" is business logic and should be in service, while "display items order by id" is display preference? – Mouhong Lin Aug 22 '11 at 04:05
I think "display handled items" and "display items order by id" are both display preference? So we should choose 1? – Mouhong Lin Aug 22 '11 at 04:07
1

I think my question should be: (1) Is "Where(it => it.IsHandled)" business logic? (2) Is "OrderBy(it => it.Id)" business logic? – Mouhong Lin Aug 22 '11 at 04:08

score 3 · Accepted Answer · edited Dec 16 '18 at 11:31

3

It depends. :)

My opinion:

I like to keep my service loose, to minimize duplicate code. I'm also a fan of pipes and filters.

Here's what i'd do (and DO do).

Service

public ICollection<Item> GetHandledItems<TKey>(OrderingOptions<Item, TKey> orderingOptions) 
{
   return repository
      .Query()
      .WhereHandled()
      .WithOrdering(orderingOptions)
      .ToList();     
}

ItemFilters.cs

public static IQueryable<Item> WhereHandled(this IQueryable<Item> source)
{
   return source.Where(it => it.IsHandled);
}

public static IOrderedQueryable<T> WithOrdering<T, TKey>(
   this IQueryable<T> source,
   OrderingOptions<T, TKey> orderingOptions)
{
   return orderingOptions.SortDescending 
      ? source.OrderByDescending(orderingOptions.OrderingKey) :                                                    
        source.OrderBy(orderingOptions.OrderingKey);
}

OrderingOptions.cs

 public class OrderingOptions<T,TKey>
 {
    public OrderingOptions(Expression<Func<T,TKey>> orderingKey, bool sortDescending = false)
    {
       OrderingKey = orderingKey;
       SortDescending = sortDescending;
    }

    public Expression<Func<T,TKey>> OrderingKey { get; private set; }
    public bool SortDescending { get; private set; }
 }

This way, you can specify the ordering in the Controller:

var items = service.GetHandledItems(new OrderingOptions(it => it.Id));

Differences between the above and options 3:

Above materializes sequence before returning to Controller. Option 3 does not, which is dangerous (you could end up returning query to View and break MVC pattern).
Generic "Ordering" POCO, can be used anywhere and keeps your queries D-R-Y.
Service becomes dumb, and simply a mitigator between the Repository and the Controller (which is all it should do, IMO). Logic (e.g filters) abstracted to one place.

edited Dec 16 '18 at 11:31

halfer

19,824
17
99
186

answered Aug 22 '11 at 02:24

RPM1984

72,246
58
225
350

Thanks. Bu if "GetHandledItems()" returns a collection (not IQueryable), we cannot perform projections on the query. It will affect performance. – Mouhong Lin Aug 22 '11 at 04:11
@Dylan - So do your projections in the service before you execute the query, either inline or via another pipe method, e.g: `return repo.Query().WhereHandled().WithOrdering().AsSomeProjectedType()` – RPM1984 Aug 22 '11 at 04:17
Also watch this **brilliant** RobCon vid on MVC Pipes & Filters: http://www.asp.net/mvc/videos/aspnet-mvc-storefront-part-3-pipes-and-filters – RPM1984 Aug 22 '11 at 04:48
If we do projections in service, we need to write "view model" or "data transfer object" in the service layer, I think the VO and DTO should be in the Controller layer? And, difference pages need different proejections, for example, page 1 needs Property1 and Property2, but pqge 2 needs Property1, Property2 and Property3. How to solve this kind of problems? – Mouhong Lin Aug 22 '11 at 06:01
@Dylan Lin. All valid questions, and here are the answers. 1) DTO projections should *not* go in the controller. Where are you transferring to?? DTO's should be either between tiers or between repository and controller. 2) ViewModel projection should be done *after* materialization. *Shaping* can be done on the query. They are two different things. 3) Use AutoMapper for ViewModel creation to simplify the above. 4) If two pages need different properties, shape a service query to fit both pages, and project differently in the controller, or if they are vastly different, use diff service methods. – RPM1984 Aug 22 '11 at 06:08
I think I've been confused by "ViewModel" and "Shaping". The service layer should not be aware of the ViewModels, correct? If service layer returns collections of domain models (not IQueryable), we are not able to make the ORM generate sqls like "select field1_I_need, field2_I_need from ..". This will result in bad performance. – Mouhong Lin Aug 22 '11 at 12:36
@RPM1984 let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/2729/discussion-between-dylan-lin-and-rpm1984) – Mouhong Lin Aug 22 '11 at 12:37
What's the point of `WhereHandled` filter when you can just do a generic `Where` filter? The same as you did with order filter – Martin Dawson Jun 19 '16 at 18:18

Is it good practice to have linq query in Controllers?

2 Answers2