For a concrete SQL Repository, should I encapsulate the SQL logic inside or outside the repository methods?

Question

If I have a concrete repository based on SQL data access, should I pass in SQL to the repository methods or encapsulate them within the methods. I feel if I pass them in, then I am coupling my repository with SQL data access, so I am leaning toward putting the SQL in the methods? Here are my two options:

public GetPeople(string sql)
{
 //Query people with passed in sql
}
public GetPeople()
{
 //Query people with sql logic here, possibly conditional logic
}

• RubberChickenLeader: along with the answers, passing in any generic SQL would need lots of checks so you don't get malicious input. (some one somewhere will find it, at least that is how I plan things).

Answer 1

Passing SQL into the repository is definitely not a good way to go about things. It violates the single responsibility principle because the client uses the repository and also tells it how to work instead of it using the repository and the repository knowing how to work itself.

Answer 2

As always, it's useful to think about the pros and cons of each approach:

Pro passing in SQL

• It's flexible. If you have a lot of different queries that are difficult to parameterize you won't end up with a ton of methods, or having to design around it.

Anti passing in SQL

• It's generally a bad idea to present a larger interface than is necessary. The interface may not seem any larger- it's still one method- but it makes a huge promise of "Give me practically ANY query and I'll run it for you". This can be really problematic in, for example, testing, because now instead of testing a class that claims it can answer one specific question, you have to test a class that claims it can answer any question. The same difficulty arises if you're mocking out the repository rather than testing it.

• You end up with absolutely no separation of concerns. SQL concerns will be sprayed throughout your code-base; no matter what a class's responsibility is, it's also going to be implicitly having to deal with the exact implementation and structure of your persistence. It'll be far more difficult to write loosely-coupled or Single Responsibility Principle-compliant code.

• For data which can only be one of a small fixed number of values, it's usually considered good practice to use an enum. This is essentially because the compiler can't peer inside strings- it can't recognise your typos, your IDE can't provide intellisense based on string values and so on. You want to be strongly typed rather than "stringly typed".

  So if that's important for simple data, think how much more important it is for code. The last thing you want is to have behaviour inside strings, with typos causing run-time errors, unit testing completely impossible, no intellisense, no stepping through with a debugger, none of the nice things we get from writing code as code rather than as string values.

  Of course you'll need your SQL to be somewhere, but as part of a repository method it's isolated somewhere that the whole method can be tested as a sensible unit. And outside your repositories, the queries you want to perform will be represented as methods called on objects, rather than as values inside strings.

---

As you can see, this list is a bit one-sided. The kind of "flexibility" you gain is really the worst kind, analogous to adding params object[] extra to every method just in case you need it at some point in the future. Even if there wasn't such a hefty collection of downsides, it'd still be a YAGNI violation. So I would strongly recommend not passing in SQL, certainly not as your starting point for the repository.

If you do, at some point in the future, find yourself in the quite unusual situation where you do need to make many, many different queries, and the differences between them are something hard to simply parameterize, there are still probably better ways to deal with this than passing in SQL. For example, you could create classes which can be used to build up queries by having methods called on them, which then expose the final SQL as a property. But this scenario definitely isn't worth prematurely planning around.