Using Repository Pattern with .NET Entity Framework with a single Get method with optional parameters for each table include

Question

We have a very messy data repository component, with dozens of methods to interface a DbContext (entire database) in Entity Framework.

It was (and is) coded in a way that adds a new repo method for each table combination a query needs to run on/return.

For example:

1. GetEmployees(), returning something like DbContext.Employees.Where(...)
2. GetEmployeesWithDepartments, returning DbContext.Employees.Include(x => x.Deparments).Where(...)
3. GetEmployeesWithDeparmentsWithManagers, returning DbContext.Employees.Include(x => x.Departments).Include(x => x.Managers).Where(...)

something like that.

I am now working on refactoring that, and proposed to unify all such repo methods - into one around the same main table. In the example above, those 3 methods would give place to a single one looking like

GetEmployees(bool include includeDepartments = false, bool include includeManager = false) { var query = dbContext .Employees(); if (includeDepartments) query.Include(x => x.Departments); if (includeManagers) query.Include(x => x.Managers); //apply query predicate and return... } 

Someone said this conflicts with S in SOLID. I don't see how (the responsibility is one - querying a specific db main table - with the convenience of allowing to return additional stuff). The proposed above has the obvious advantage that, when refactoring in the future - we will only need to touch a single place.

Am I wrong in principle ? Are there other motifs my suggestion is not a good idea ?

Looks like MSDN does encourage the behavior I am proposing, in their Repository best practices (see Implement a Generic Repository and a Unit of Work Class).

Answer 1

I am now working on refactoring that, and proposed to unify all such repo methods

I agree with the feedback you received that this isn't what I'd do. However, ...

Someone said this conflicts with S in SOLID.

I disagree. It violates the O.

The proposed above has the obvious advantage that, when refactoring in the future - we will only need to touch a single place.

Needing to touch the same place for future extensions is precisely what you don't want, as per the OCP. If you add a new table, it forces you to have to change this method to add a parameter (and thus break the contract) for the existing method.

More importantly, and this is not a letter of SOLID but in my opinion an even more pertinent pain point: whatever code handles the result that is returned from this method, it has to run a gauntlet of potential null reference exception for each and any dependent dataset that you did not include in your query.

What I think is happening here is that you're falling into the honeytrap that catches so many (driven and dedicated) developers: eagerly wanting to maximize reusability.

Taking a bit of a detour here, what is your response when you see this?

There's two ways to respond to this:

• Wow, that's an amazing achievement to have these cars interleave without collisions. I want to be able to do that.
• Holy hell that's a huge accident waiting to happen. I'm not going near this intersection.

I suspect a lot of developers are drawn to the former, because we are drawn to the elegance of a well designed machine. To some degree, that is good. This is exactly the kind of attitude that built the kinds of complex manufacturing machines we have nowadays.
However, there's a point of overdoing it, where we are drawn like moths to a flame to a complex machine, when it would be significantly easier to create several simple machines and avoid any collisions or ambiguities.

In my opinion, this is one of those cases. The complexity and risk of merging these repository methods seems to outweigh the benefit of doing so. I suspect your interest in building this is biasing your decision on whether it is the right thing to do.

If you stick with the repository pattern, separate methods seems better. Note also that there's nothing wrong with having these public methods depend on some shared private logic for the parts that are in fact reusable across these public methods (e.g. applying the same employee filter in all cases).

I'm personally a fan of the query object approach, where each public repo method is split off into a class of its own, effectively creating "one method repos". This prevents the needless categorization exercise of "what repo should this query live in?", which is IMHO a futile exercise. Instead, I think it's more productive to focus on each query on its own, so that you can build each individual query to fit with its own individual specifications.

Whether you use repositories or query objects is not the main focus here, but I do suggest that you keep the queries separate, whether as public repo methods or individual query objects.

Answer 2

Not sure which letter it comes under but yes, having parameters that change the function of a method is considered bad practice.

Generally speaking I blame your problem on entity framework. Essentially you are forced to add child objects to your models, but optionally populating them is a bad idea. They should always be populated.

In order to make this work in a sensible way you will have to look at your models and decide on your "aggregate roots" ie where you want child objects and where you are going to just include the ids and make a separate call.

eg, you could have

Employee { Name DepartmentId ManagerId } 

and make a separate endpoint:

GetAllDepartments() 

Now the calling code can get the employees and be sure that all the data is always included. If they want to show the department data they can make a second call and join the data together on their side with map reduce.

If they want the list of departments for a drop down selection, the method is already there etc.

Answer 3

The SRP is usually concerned with the responsibility of classes.

When I got this right, your repository originally was a class with multiple public methods, and you just changed the API, so it is now a class with one method and some boolean parameters instead. This did not change any responsibility of the class. Hence, if the SRP wasn't violated beforehand, it cannot be afterwards - your refactoring did not change any responsibility of the repo.

Moreover, having one repo bundling all kind of queries for a single entity is pretty standard design and unlikely to violate the SRP in the first place. If it is a good idea to allow partial loading of entities or not (which can be sometimes justified for performance reasons), or to have public methods with boolean flags in a repo's API (which may be a code smell, but nothing more) is a completely different question, unrelated to yours.