0

After reading this Stackoverflow question I have stumbled upon an MSDN article Implementing the Repository and Unit of Work Patterns in an ASP.NET MVC Application.

There is a proposed implementation of a repository:

public virtual IEnumerable<TEntity> Get( Expression<Func<TEntity, bool>> filter = null, Func<IQueryable<TEntity>, IOrderedQueryable<TEntity>> orderBy = null, string includeProperties = "") { IQueryable<TEntity> query = dbSet; if (filter != null) { query = query.Where(filter); } foreach (var includeProperty in includeProperties.Split (new char[] { ',' }, StringSplitOptions.RemoveEmptyEntries)) { query = query.Include(includeProperty); } if (orderBy != null) { return orderBy(query).ToList(); } else { return query.ToList(); } }

I recently blamed people for the similar code, since in my opinion that code was bad, and now I am confused.
My arguments:

  • Whoever uses this method must know and enumerate all included properties of a class
  • Every time someone uses this method in a new code he must recall which required properties he needs to include, so he will probably copy-paste the previous usage case
  • If someone decides to change property name, then he should Ctrl+Shift+F through a project and change all textual usages
  • If someone adds a property which should be included everywhere, then he should find all usages and add ",NewProperty" to every string

For me it sounds like a violation of SRP, DRY and other rules of OOP.

I cannot believe that someone like tdykstra can write bad code, so there should be something I just can't understand and it makes me feel sad. Could you please explain where I made a mistake?

Improve this question

1 Answer 1

Reset to default
4

There are really two separate concerns:

1) You will have to decide from case to case which properties you need eagerly loaded in a query. Eagerly loading properties which you don't use cause overhead, and if you just loaded everything eagerly, the query would load the whole database. On the other hand, if you do need the properties then lazy loading would cause the n+1 problem which is also a performance killer.

2) Specifying the properties as strings. This do indeed have the problems you mention. There is a strongly typed alternative - see: https://msdn.microsoft.com/en-us/library/gg671236(VS.103).aspx

That said, the method seem over-designed to my eye. It would be simpler if it just exposed IQueryable and let the client add filtering and ordering in linq. Chopping filtering and ordering into separate parameters does not seem to provide any benefit. But some people are scared of leaking IQueryable, so YMMV.

Improve this answer

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.