Is it best practice for each call to a SQL Server to be in its own class, even when using dependency injection?

Question

I often see/write classes that contain every part of the application where the application will talk to a SQL server. For example, you may have a class like this

public class Data
{
    private Func<IDbConnection> Factory { get; }

    public Data(Func<IDbConnection> factory)
    {
        Factory = factory;
    }

    public IList<Customer> FindAll()
    {
        using (var connection = Factory.Invoke())
        {
            const string sql = "SELECT Id, Name FROM Customer";
            using (var command = new SqlCommand(sql, (SqlConnection) connection))
            {
                command.Connection.Open();
                // Then execute command and return the results
            }
        }
    }

    public Customer FindOne(int CustomerID)
    {
        using (var connection = Factory.Invoke())
        {
            string sql = $"SELECT Id, Name FROM Customer WHERE ID = {CustomerID}";
            using (var command = new SqlCommand(sql, (SqlConnection) connection))
            {
                command.Connection.Open();
                // Then execute command and return the results
            }
        }
    }

    public IList<Job> FindAll()
    {
        using (var connection = Factory.Invoke())
        {
            const string sql = "SELECT Id, Name FROM Jobs";
            using (var command = new SqlCommand(sql, (SqlConnection) connection))
            {
                command.Connection.Open();
                // Then execute command and return the results
            }
        }
    }
}

and you would expect to add a new method in this class whenever you need your application to talk to SQL in a different way (e.g. when you need to add a new query).

Clearly, this is a terrible idea. Before you know it, you will have 100 methods in the same file. Above all else, it violates the Open-Closed principle because it will have to change whenever we want to add a new SQL query to our application.

At face value, a more sensible idea would be to put the Customer methods in their own class and the Job methods in their own separate class. Let's call them CustomerSql and JobSql This is much nicer, but it violates the Open-Closed principle for the same reason that the original design did: If I want to add a new SQL query for Customers, then I will have to change CustomerSql.

This leave us only with the idea that each call to SQL should be in its own class. For example, the FindAll() method for Customers should be in a different class to the FindOne() method for Customers. Is this considered best practice? And if so, how can I square it with dependency injection? Whenever I try to imagine implementing it, I find myself in dependency injection Hell. With one class per SQL call, I cannot imagine how ugly my Composition Root would be. Even when using a DI framework such as Microsoft.Extensions.DependencyInjection (it's the one I'm stuck with), I can't imagine it turning out well if I'm writing a CRUD app that has Form1 depend on 30 different SQL queries. I suspect that an example showing that this is actually very simple would instantly address my concerns.

Answer 1

This looks like the repository pattern, which is generally considered to be a good thing, separation of concerns between your business and data layer.

The reason to put all these methods together in a single class rather that in a class per table, which would be a "generic repository pattern" or as you suggest a class per method, which i dont think has a name, Is because in your database the Customer and Job objects may be related. ie, if I try and save a job without a linked customer the database might throw a foreign key constraint error and you want to be able to enforce this relationship in your datalayer. Which you would find harder to do with the other patterns.

Or, you may want to select all the Jobs related to a Customer. Again, separating the Job and Customer repositories complicates things although it's not impossible.

Does the repository pattern violate the open/closed principle? I don't think so. For one thing sql isn't an object so its going to fall outside of a strict definition of the principle. Secondly the object is clearly open to extension, you can inherit from it and add more methods.

Ps. I should note that putting the Save/load methods on the object itself. ie Customer.LoadFromDb(id) would be the active record pattern and is generally considered a bad thing. As it tightly couples the object to the unrelated concern of how to persist the object

Answer 2

You would expect to add a new method in this class whenever you need your application to talk to SQL differently (e.g. when you need to add a new query).

In my experience, the rule of 80/20 (Pareto's principle) has been quite consistent from project to project. Usually, we can cover 80% of the reads and writes with only 4-6 methods.

public long count(Criteria[])
public Entity load(ID)
public List find(Criteria[], Sort)
public ID save(Entity)
public long delete(Criteria[])

The remaining 20% are functions with special needs. For example, pagination or call.

public List page(Criteria[], Sort, Pagination)
public Result call(Procedure)

Or they are simply too complex to model

public Result execute(Query)

At face value, a more sensible idea would be to put the Customer within their class and the Job within their separate class. Let's call them CustomerSql and JobSql This is much nicer, but it violates the Open-Closed principle

But it keeps complexity at bay and, at the same time, fulfils other practical principles like single responsibility.

No principle is better. Their value is contextual and often principles pull in opposite directions. When this happens, it's ok giving up on those not aligned with our immediate needs. Code easy to read and test, and cheap to maintain, along with DRY, KISS and YAGNI can be (and are) good arguments.

Anyways, it is not your case. Simply, OCP doesn't apply because it's not clear why Data is (or should be closed) closed. Thought work: Is not our app's source code always open to change? After all, we are the only clients.

Is this considered best practice?

No, I don't think so.

Over time, the complexity (tens or hundreds of small classes) can be so high, that doesn't make it suitable for all developments. Its sustainability is arguable too.

I think the best practice for you (from the OOP perspective) would be: to review code, look for patterns or duplicated code and implement abstractions (classes or functions) oriented to enable the 80/20 comments earlier in this answer.

I will have to think more about how the stored procedures should be put inside classes (e.g. how many classes?)

It's not a matter of how many lines of code; it's a matter of abstractions. All stored procedures have something in common: from the app's perspective they are RPC (remote procedure calls); start working on a single to create a SqlCommand, add input arguments, add output arguments (if any), execute the command, fetch output arguments' values (if any), return a response (if needed).

Answer 3

If the system is small, the current approach is fine. Only when you have lots of tables and different query patterns would you need some sort of abstraction so you are not repeating yourself all the time. One could have one class to handle all the database calls so that you don't repeat.

Consider this:

public IList<Customer> FindAll()
public Customer FindOne(int CustomerID)
public IList<Job> FindAll()

To this:

public IList<T> FindAll()
public IList<T> Find()
public T FindOne(int Id)

This could be in some sort of interface called IRepository.

Now we can find Customers, Jobs, Widgets, Foos, Bars, and so forth.

Using the type T we can build a query to find what we are looking for. Consider some sort of query factory/query builder to handle building the actual query to the database. This could be segregated so that the building of the query is separated from the execution.

Then in your IRepository implementation open the connection, set up the command and execute the query using an underlying data provider (kind of what you are doing now. You will need a mapping layer as well to map the result to the appropriate object type.

There are already several existing ORMs that do this in some shape or form, each with their own pros and cons, or you write a simple one yourself.

It's up to you to determine when it's time to refactor to keep the code base tidy and neat.

From a dependency injection standpoint, you only need to inject one inteface...IRepository for classes that need to interact with the database.

Answer 4

First of all, adding a method to a class does not necessarily violate the Open-Closed-Principle in my opinion, or at least a sane interpretation of this principle. For such a violation there has to be a behavioral modification of the class. Your class resembles DAO or Repository class (see for e.g. this question for a discussion of these patterns). Such a class usually only contains a set of distinct functional methods and should not manage any inner state, that might be affected by adding methods. Thus, I think, adding methods is a reasonable way to extend the code in this case.

Second, the way you wrote your Data class is perfectly fine. If I interpret the syntax correctly ("using", I guess, is some kind of try-with statement), then it is actually the way to go: it does not look very nice, but closing resources as soon as possible is always a good idea - needless to say closing them at all. If you mind the boilerplate code, then you have different ways to avoid it. For example, you could use an ORM library that might help you to avoid writing a lot of code like this. An example for such a library is Hibernate in Java. However, using such an ORM framework has its heavy downsides. ORMs are quite complex by nature and might lead to completely different types of problems. Furthermore, looking at the actual SQL statements like in your implementation is much simpler and clearer than an obscure framework that some way or other assembles the statement magically. This is why many people avoid ORMs in actual production code. If you mind the boilerplate anyway and it is possible in your language, you could avoid some of it like this:

protected <T> T runSql(string sql, Function<SqlCommand, T> function) {
    T value;
    
    using (var connection = Factory.Invoke())
    {
        using (var command = new SqlCommand(sql, (SqlConnection) connection))
        {
            value = function.apply(command);
        }
    }
    
    return value;
}

T is a generic type and Function is a functional interface that accepts one argument, in this case SqlCommand, and returns the generic type T. Then you could use it like this:

public Customer FindOne(int CustomerID)
{
    return runSql($"SELECT Id, Name FROM Customer WHERE ID = {CustomerID}", command -> {
        // Then execute command and return the results 
        // ....
    });
}

()->{} is a lambda/anonymous function. Anyway, this is just one way how you could avoid some of the boilerplate code, if you feel the need to.

Answer 5

Having a class for each query method is not best practice.

A core OO design principle is: Classes should have low coupling and high cohesion. Unfortunately, the high cohesion side is often forgotten, so developers spit classes into atoms in pursuit of low coupling. But creating a separate class for every method just creates an organization problem at a higher level, since some of these classes are closer related than others.

The guiding principle should not be the number of methods - neither 1 or 100 is inherently better. The question is which queries belong together and which don't.

Unfortunately, there is no universal answer to how to organize queries, since it depends on the conceptual data model of your application. It is tempting to create a class per database table, but this creates too tight a coupling to the database schema. Domain driven design has the concept of aggregates, which is a set of entities usually treated as a whole. For example, an "order" would contain related entities (like order lines, shipping address etc.) but would be handled as a unit in application code. It makes sense to have all queries related to orders in a single class, but a separate class for queries related to say customers.

(Btw. I disagree with your interpretation of the open/closed principle. Adding methods is not a problem, the problem is modifying the behavior of existing methods which clients depend on.)

Answer 6

There are some tangential things to address about your persistence approaches, which I've already left a comment on your question for. This answer focuses on your final question, i.e.

Is this considered best practice? And if so, how can I square it with dependency injection? Whenever I try to imagine implementing it, I find myself in dependency injection Hell.

The solution to that injection hell is the mediator pattern. The easiest way to think of a mediator's responsibility is to think of those switchboard operators from half a century ago:

Whoever called the company, and whoever this person needed to speak to, there was only one switchboard. A phone call would come in, the operator would consider what the phone call's purpose was (i.e. who they were trying to reach), and the operator would use the switchboard to route the caller to their intended destination.

Instead of someone needing to know all of the company's phone numbers in order to reach every person in the company; they only needed to know the switchboard's number. It's a lot easier to remember one number instead of needing to be up to date with who currently works for the company and what their number is.

The above example should sound very similar to the problem you are describing.

In essence, the mediator pattern gives you a single injectable dependency (let's call it "the mediator" to keep it simple). The mediator takes in a particular input object, and based on that input object, it routes your input to the correct handler.

Suppose you have two database queries like this (code heavily simplified)

public class MyFancyQuery1
{
     public MyFancyQuery1Result Run(MyFancyQuery1Input input)
     {
         return database.Execute("my fancy query 1", input);
     }
}

public class MyFancyQuery2
{
     public MyFancyQuery2Result Run(MyFancyQuery2Input input)
     {
         return database.Execute("my fancy query 2", input);
     }
}

Based on your question, you would need to inject specific queries into your code, e.g.:

public class MyConsumer
{
    private readonly MyFancyQuery1 myFancyQuery1;
    private readonly MyFancyQuery2 myFancyQuery2;

    public MyConsumer(MyFancyQuery1 query1, MyFancyQuery2 query2)
    {
        myFancyQuery1 = query1;
        myFancyQuery2 = query2;
    }

    public Foo GetFoo()
    {
        MyFancyQuery1Result result1 = myFancyQuery1.Run(new MyFancyQuery1Input());
        MyFancyQuery2Result result2 = myFancyQuery2.Run(new MyFancyQuery2Input());

        return Map(result1, result2);
    }
}

And this can get out of hand when your consumer needs several database queries/commands.

The mediator acts as a single access point for all queries and commands. This reduces the dependency injection hell as you always just inject the mediator, you don't have to go and look for the specific types to inject.

public class MyConsumer
{
    private readonly IMediator mediator;

    public MyConsumer(IMediator mediator)
    {
        this.mediator = mediator;
    }

    public Foo GetFoo()
    {
        MyFancyQuery1Result result1 = mediator.Run(new MyFancyQuery1Input());
        MyFancyQuery2Result result2 = mediator.Run(new MyFancyQuery2Input());

        return Map(result1, result2);
    }
}

Notice how the two code examples of MyConsumer are the exact same behaviorally, but the dependency injection itself has been simplified. Of course the benefits of doing so increase when there would be more than just two dependencies to inject - this is just an example.

I'll let you read up on the mediator pattern yourself as there is much more to be said than what can be captured in an answer here.