0

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.

9
  • The problem you’re facing with the alternative approach is clear, but what actual problem do you have with the implementation that violates the open/closed principle?
    – Rik D
    May 19, 2023 at 15:38
  • @RikD Just that it violates the principle and therefore inherits the issues that you get from that. Nothing more.
    – J. Mini
    May 19, 2023 at 15:58
  • What issues would that be? The way I deal with the principle is that one doesn’t break it as long as existing code doesn’t have to be modified to add the new behavior. And sometimes it’s just necessary to change existing code.
    – Rik D
    May 19, 2023 at 18:22
  • @RikD To add a new call to SQL to my system, the class holding all of the SQL calls need to change. This means that extending my system requires changing a class that already exists. That's a violation of the OCP.
    – J. Mini
    May 19, 2023 at 18:28
  • 4
    The basis of this question highlights that you have little to no experience with common patterns used in persistence. I highly suggest you read up on them (specifically: repository pattern, unit of work, CQRS, query objects) because it will significantly streamline any further conversation you wish to have on the topic. As it stands, it is prohibitively complex to answer your questions as it entails having to explain well-documented pattern in order for the answer to make sense.
    – Flater
    May 22, 2023 at 3:58

6 Answers 6

1
+50

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

7
  • "Secondly the object is clearly open to extension, you can inherit from it and add more methods." - Wouldn't that be a terrible way of extending it? I suppose it wouldn't violate the OCP, but I can't imagine why it would be considered a good idea.
    – J. Mini
    May 19, 2023 at 18:15
  • i wouldn't recommend it over just adding a method to the object, but from a pure object design point of view I don't see how you can consider it a violation of open/closed
    – Ewan
    May 19, 2023 at 18:20
  • 1
    To add a new call to SQL to my system, the class holding all of the SQL calls need to change. This means that extending my system requires changing a class that already exists. That's a violation of the OCP.
    – J. Mini
    May 19, 2023 at 18:28
  • it doesn't though. you can inherit and extend.
    – Ewan
    May 19, 2023 at 19:05
  • 2
    @J.Mini you are correct about inheritance, extension is absolutely not a relevant approach here. However, you are incorrect about OCP. If you define a repository as a collection of all database methods, and then you need that repository to have more features, then it inherently warrants changes made to the repository - this is not an OCP violation. OCP only exists to avoid things like evergrowing if A else if B else if C code that would need to be changed when you create D, then E, then ... OCP does not exist to tell you that you cannot ever change a class' code once it has been written.
    – Flater
    May 22, 2023 at 4:17
1

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).

3
  • Excellent points on how the SQL ought to be, but I'm unfortunately stuck with the stored procedures that I've got. I think you're right about how the OCP does(n't) apply here, but I will have to think more about how the stored procedures should be put inside classes (e.g. how many classes?).
    – J. Mini
    May 22, 2023 at 14:23
  • Don't know how stored procedures are implemented in SQL Server, but Oracle's SP can be grouped in packages. Packages are mere namespaces that can be easily translated into classes on the server side. So, we often have 1 class per package. We do it this way because when the number of stored procedures is small and we don't expect new ones that often. Think in corner cases. If we have a lot of them (this is a problem per se) and we can't get rid, worth thinking in abstractions that balance complexity, maintainability and capacity to change and extend over time.
    – Laiv
    May 22, 2023 at 15:25
  • Given two solutions, the "best" one is often in the middle, because it has the better tradeoff balance. For example, you might have a single function to cover up all possible sp calls. For example public void execute(storedPName, inputs[], output) (yes, SP often use output arguments). Both inputs and outputs are optional. It's not super sophisticated (no new classes and abstractions, only parameters), not the most OOed, but ey! quite easy to maintain and test and it's likely to cover 80% of the use cases. It's ugly, but it's functional. It's simple, but it's easy to test and mock
    – Laiv
    May 22, 2023 at 15:33
0

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.

0

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.

0

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.)

-1

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:

enter image description here

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.

2
  • 1
    It took me many reads before I got your point, so forgive me if I've still missed it. It sounds to me as if your mediator object is just the original huge Data class that I've suggested. I truly see no difference between the two.
    – J. Mini
    May 25, 2023 at 23:38
  • @J.Mini Hardcoding the mediator's handlers is the most barebones way of doing a mediator. Look into the Mediatr library for an example on how a mediator can work without requiring you to collect every handler in a god class. Essentially, you develop loose components and Mediatr will connect the dots for you at runtime.
    – Flater
    May 25, 2023 at 23:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.