4
\$\begingroup\$

I designed my repository class and I need some advice from you. Now I have a method GetByID that returns a generic type:

 public T GetById(object id)
 {
    return this.Entities.Find(id);
 }

I'm going to use OData to filter data, and for that (when I'm using $expand for single row) I need to do something like this:

SingleResult.Create(repository.GetByID(id));

But of course I received an error because SingleResult needs a type IQueryable<T>.

So, I decided to change my GetByID to:

public IQueryable<T> GetById(object id)
{
    return this.Entities.FirstOrDefault(p => p.ID == (int)id);
}

Could you please tell me if is it the right way? What type should be returned from GetByID?

I prefer to use T GetById because it is more correct in my opinion, but I don't know.

\$\endgroup\$
  • \$\begingroup\$ GetById returns one result or the default value for T. Why then I you trying to pass this to SingleResult.Create? Why create a SIngleResult<T> when you already have T? \$\endgroup\$ – David Arno Sep 23 '15 at 9:17
  • \$\begingroup\$ Yes, but without SingleResult my OData like http://localhost:3761/api/Street/1?$expand=StreetType not work \$\endgroup\$ – Roman Marusyk Sep 23 '15 at 9:20
  • 2
    \$\begingroup\$ Ah, I missed the bit about OData. Your question effectively highlights the problem with that standard. Returning IQueryable<T> feels wrong and that's for very good reasons: a queryable abstraction is a leaky one. You may like to read roysvork.wordpress.com/2013/06/24/… and blog.ploeh.dk/2012/03/26/IQueryableTisTightCoupling before committing yourself too much to this solution. \$\endgroup\$ – David Arno Sep 23 '15 at 11:01
  • \$\begingroup\$ There are some issues with this code. Doesn't FirstOrDefault only return a single object not a collection? Also, what type is Entities? DbSet? What's the point of calling a DbSet method in a generic repository? Doesn't that couple it to the entity type? \$\endgroup\$ – moarboilerplate Sep 27 '15 at 2:19
2
\$\begingroup\$

If you're only going to make the minimal required change

Accommodate the IQueryable requirement in the layer where it is needed, not in your repository layer. That will probably be some object that takes in an entity, wraps it in an IEnumerable, and calls AsQueryable() on it. (Disclaimer: This may or may not work. I haven't tested it.) If you were to do otherwise and change your repo signature to use IQueryable, that would be coupling the repository layer to downstream requirements, which you are correct in wanting to avoid.

A side pet peeve

Please avoid using object in signatures if you're going to use generics in the same class and you don't have a reason to box (I believe DbSet has to use reflection to find the key so it has a boxing requirement, but your code does not require boxing). Use strong typing all the way until you absolutely must cast to object. You'll thank me later. And make the parameter names expressive:

public TEntity GetById<TKey>(TKey id)
{
    return this.Entities.Find((object)id);
}

Digging deeper

It seems a bit wonky and counterintuitive to have to pass an IQueryable to SingleResult. But come! Join me on a journey in pursuit of greater understanding of this "IQueryable".

An IQueryable is a bit more than just a collection. It functions as a sort of gateway to the data you're after, letting you query that data without having to use its unique language (SQL, or OData queries, what have you). This is accomplished through an associated query provider, which translates the lambda expressions you provide it to the language of your data store.

More importantly, that query provider decides how to compose and manage queries, and that includes utilizing deferred evaluation. The fact that you can pass it a query from your EF context is its way of telling you "I got this bro. This IQueryable has all I need to pull the data from here when I need it." It will then pull the data it needs for the OData query it's building on demand.

If you instead return it an entity, you have eagerly loaded that entity from the DB yourself, when you may not have needed to. Or at the very least, before you needed to.

A Catch-22

Using the generic repository here directly pits the benefits of IQueryable (deferred evaluation, the need for less "intermediary" code) against the reusability of a library that abstracts data access away from queries (is this even being reused?). In order for these two to coexist, some compromises are going to have to be made, compromises that may result in an extremely mediocre "solution."

A simpler appeal

If you look at the way the OData provider functionality is designed, it's evident that it's meant to directly consume EF context queries including in your web app layer. If the DbContext was meant to be shut away inside a library, they would have designed things a lot differently.

To sum up

I would recommend reconsidering the purpose of your generic repository, why you need it, and what it's buying you.

\$\endgroup\$
  • \$\begingroup\$ This is a great answer, but I'm not sold on the concept that it's meant to be used directly in the WebApp layer. A useful abstraction on top of it, in between, can be... useful. \$\endgroup\$ – RubberDuck Sep 27 '15 at 0:22
  • \$\begingroup\$ @RubberDuck tautologies aside, it depends on what utility the abstraction is providing. The improved mockability of DbSet and DbContext in tandem with an injected factory to create the context is sufficient to completely decouple from the database and test your code in isolation. In my opinion that's good enough for a large majority of enterprise code. Beyond that is the world of DDD and abstracting everything behind a layer, which is a completely different beast. \$\endgroup\$ – moarboilerplate Sep 27 '15 at 1:14
  • \$\begingroup\$ @RubberDuck Plus, check out the tutorial...static context instantiated with the controller, disposed when the controller's disposed. All that's needed to make it testable is to inject a context factory. \$\endgroup\$ – moarboilerplate Sep 27 '15 at 1:16
  • \$\begingroup\$ It's not about testability, it's about useful abstractions. Real world databases often aren't well structured. Adding a layer of misdirection allows us to compensate for it and further decouple from the persistence layer. \$\endgroup\$ – RubberDuck Sep 27 '15 at 1:31
  • 1
    \$\begingroup\$ @RubberDuck Right on. Let me be clear: The EF DbContext should be sufficient for all scenarios in which you do not need a domain model. If you don't have a domain, but you're building walls around EF, it's probably going to become a big ouchie. \$\endgroup\$ – moarboilerplate Sep 27 '15 at 2:06

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.