2

I think that I've taken the Open-Closed and Single Responsibility principles too far. Previously, I had one huge static class containing every method that has C# talk to stored procedures on my SQL Server. Here is one such method.

public static void DeletePerson(int PersonID)
{
   CallCommandAndHandleErrors((Connection) =>
   {
        var Command = BuildCommand("[Schema].[DeletePerson]", Connection);
        Command.Parameters.AddWithValue("PersonID", PersonID)
        return Command;
   });
}

My huge class contains about 30 of these methods, along with their shared helpers. This violates the Open-Closed and Single Responsibility principles. I therefore moved the helper methods to an abstract class

public abstract class SqlProcedureHandler
{
    private SqlConnection _connection;
    protected SqlCommand Command;

    // Doesn't need arguments any more, because they're now fields.
    private CallCommandAndHandleErrors(); 
    {
         // Same implementation as in original, but using fields.
    }

    protected void BuildCommand(string ProcedureName)
    {
         // Same implementation as in original, but setting/using fields.
         // Now has an overload, not shown, allowing dependency injection of different SqlConnections.
         // I expect that overload to be ambrosia for unit tests.
    }

    public void Call() => CallCommandAndHandleErrors();
}

and I used this abstract class as the parent class of new classes that each hold exactly one of my old methods. For example, DeletePerson(int PersonID) above is now

public class DeletePerson : SqlProcedureHandler
{
    public DeletePerson(int PersonID)
    {
        BuildCommand("[Schema].[DeletePerson]");
        Command.Parameters.AddWithValue("PersonID", PersonID)
    }
}

and where calls to it were once HugeDataClass.DeletePerson(PersonIDHere), they are now the much uglier (new DeletePerson(PersonIDHere)).Call().

My question is this: Is the change that I've made bad practice? My derived class's only feature is its constructor. It seems very clear that my new class should just be a function and that they're stupid classes. Indeed, it originally was a function and every single usage of it is just (new DeletePerson(PersonIDHere)).Call()! But I just can't fathom how to do that and satisfy the Open-Closed and Single Responsibility principles. The dependency injection of my SqlConnection seems to make matters even more complicated.

18
  • 3
    I've done something similar to this at some point but in a whole different context (not sql related). It all depends on your own context. I personally try to find a middle ground between patterns and over engineering. Given the info in the question you might be over engineering. But it might make sense in the broader context of the code base.. SRP doesn't mean a class must contain a single method or something literal like that. The R must make sense from a business /requirements perspective, otherwise complex software wouldn't be possible. The original class doesn't seem to break SRP.
    – Ccm
    May 15, 2023 at 22:52
  • 1
    E. G. A typical example of a factory might seem to break SRP if it creates objects from wildly different things (files, mouse cursors, sockets etc). But it in fact doesn't break SRP. it's R is to create objects.
    – Ccm
    May 15, 2023 at 22:56
  • 7
    So you are modifying something eventually. SRP is easy to test. Ask the question "what does this class do?" if your answer is "it does sql stuff" then it is compliant. If the answer is "it does sql stuff AND drawing dogs on the screen" then it is not compliant. As for open close, the idea is to prevent modification of code that you trust. It doesn't mean you should never modify a class. Use common sense based on your context. The way I see it, the original class was fine. So is the new one. Pick which is easier for you to maintain.
    – Ccm
    May 15, 2023 at 23:15
  • 2
    This looks like a standard implementation of a Command pattern, probably in a context where it makes sense. So why should this simple standard case be a "bad practice"?
    – Doc Brown
    May 16, 2023 at 5:50
  • 2
    Does this make the program better? If not, don't do it.
    – user253751
    May 16, 2023 at 16:33

4 Answers 4

4

Seeing this code:

(new DeletePerson(PersonIDHere)).Call()

Makes me cringe. Not because this clearly should be a static method. But because calling it this way destroys what the design provided.

This design allows you to decouple code that knows what should be deleted from code that knows when it should be deleted. If you construct the object and call it from the same place you don't get any decoupling. Now if you don't need decoupling that's fine. But if you never separate construction and use with this you're making a simple call ugly for no reason.

But none of that means static method calls are the only answer.

Method calls provide two features. Polymorphism and state. So a redesign like (new DeletePerson()).Call(PersonIDHere) makes the object stateless. But thanks to polymorphism it is still not useless because it could be rewritten as deletePerson.Call(PersonIDHere) where deletePerson was passed in from somewhere else that had constructed it as a new DeletePersonFromFileSystem().

It used to be you couldn't do that with statics at all. The calling code had to know, statically, what code it was calling. But lambdas, method references, and other functional tricks have also been added. Regardless, it still comes down to if you want force the calling code to statically know exactly what it's calling. As it's always been, statically knowing is faster, but less flexible.

Use static methods when you know you will never need this flexibility, or if you're that desperate for the speed.


My huge class contains about 30 of these methods, along with their shared helpers. This violates the Open-Closed and Single Responsibility principles.

Well no. Having 30 methods is bad. But 30 is bad even if OCP and SRP are satisfied. 30 is bad because humans are bad at holding that many things in their head. We're better at numbers around 5-9.

OCP says to choose a coding style that lets you adapt to change by adding new files while leaving old ones untouched. SRP says to keep the number of things/people/orgs that can force this file to change at 1. But that doesn't limit the number of methods. That limits what you can put in the methods. If those principles are calling out a problem here I don't see it yet.

When you get that many methods hanging around together in whatever form it's worth reconsidering how you're organizing things. The number of methods can explode for many reasons:

  • You've encoded in the method names what should be the values of a parameter
  • You've lumped together many groups of things that should be in separate files
  • You've created a junk drawer full of random ideas without trying to find their proper home

If things like that are your problem the SOLID principles are only distracting you. When you're architecting software you have a lot of freedom. A large part of the job is managing that freedom. It's OK to go through phases where you have it working but it's ugly. Add tests that prove it's still working while you make it pretty. If you're willing to spend time doing that it's OK to put things where they don't really belong to get things working. Gives you a chance to stand back later and see patterns that were not visible before.


One suggestion. What if SqlProcedureHandler was an API that DeletePerson called rather than lived underneath as a child? I've never been a fan of inheritance hierarchies that expand the number of methods. See prefer composition of over inheritance.

13
  • 2
    I think I'm slightly misunderstanding. If I don't need polymorphism, decoupling what from when, or state, then are you saying that I should stick to the original design?
    – J. Mini
    May 16, 2023 at 16:32
  • @J.Mini I'm saying if you're sure you will never need polymorphism, decoupling, or state and can't stop people from calling the design this way then it's just making your code ugly for no good reason. This happens whenever designs are too clever for those who use them. Either teach people how to tie knots or stop giving them enough rope to hang themselves. May 16, 2023 at 16:41
  • That all sounds very sensible, but what of my concerns about the original design? That is, that I think it violates the Open-Closed and Single Responsibility principles?
    – J. Mini
    May 16, 2023 at 16:42
  • @J.Mini OK better now? May 16, 2023 at 21:03
  • 2
    @J.Mini it doesn't get to deep into solutions but if you're just starting with DI Wikipedia has a decent article. Walks you through doing it yourself. What Mark Seemann called Pure DI. Don't think using DI containers is a must. More of a fad. DI is actually an old idea. We used to call it reference passing. May 16, 2023 at 22:39
10

Lets first check your goals. The OCP is for making classes extensible without having to change their code, because their code lives in a library which you cannot change easily for organizational reasons.

So is SqlProcedureHandler part of a reusable lib, and are the derivations living in a different library, maybe maintained by someone else? Maybe there are different derivations in different libraries? Then applying the OCP here makes sense. If not, applying the OCP has a certain risk of overengineering. Don't get me wrong, your new design might be perfectly justified, but probably the OCP isn't as important as you think for your case.

Next, let's check your doubts about classes having just "one constructor". That is nothing special, quite the opposite, you find this, for example, in the standard GoF command pattern. Of course, your code looks like it is just an implementation of this pattern, and it makes most sense if you need to pass cour sql commands as first-class objects around.

Let us assume the OCP is of minor importance here since the whole code is maintained by you or your team, and maybe just inside one software system. Then, the real question you need to ask yourself to judge about your new design is:

  1. Does it help you to reduce code repetition, so certain kind of logic is kept in one place? (DRY principle)

  2. Does it help you to keep the extra requirements like "dependency injection of the connection" orthogonal from the main parts?

  3. Is there a simpler solution which achieves the same goals with less and easier to understand code (maybe using functional means)?

I think those questions can only be answered seriously by someone who knows the code in full. From what you have shown us, I guess the answer to #1 is "maybe a little bit", to #2 "yes", and to #3 "there may be a shorter functional solution, but it is probably debatable if it is really more readable." So in the end, it is a judgement call, if you will stick to this design, you are not doing something inherently wrong.

4
  • You're right that this looks a lot like the command pattern. That puts my mind at ease and tempts me to accept this answer immediately. As for answering the questions you've listed: 1) Not really. The lines saved by changing the old arguments to fields are lost to the class foo : bar { [...] } boilerplate. 2) I can imagine how to get this same benefit in the original design. I'd have to make the original static class in to a normal class that takes Func<SqlConnection> in its constructor and keep an instance of that class in its users. Not great but it would work 3) I agree with you.
    – J. Mini
    May 16, 2023 at 14:48
  • 1
    1) I did not ask for reducing number of code lines. I asked for less duplicate logic, even if the boilerplate increases. In the end, I think it stays to be a judgement call. If you are unsure now, I recommend to let the design currently as it is, and wait until you get a new requirement which could benefit from the new design. Then you should refactor.
    – Doc Brown
    May 16, 2023 at 15:27
  • Very sensible. I'll probably do that.
    – J. Mini
    May 16, 2023 at 15:41
  • Sometimes, I've wondered if object-oriented systems should partition classes into those which are extendable and to which references can exist, and those which can be instantiated, but which must be referred to using the parent (extensible) class type. This wouldn't really fit with generic new() constraints, which are otherwise quite useful, but code that says something like T thing = new T() wouldn't care whether thing received a reference to a T, or something extended from that type.
    – supercat
    May 16, 2023 at 17:31
2

Is this bad practice?

It depends.

For instance, if you have a single class for all such operations, then to use it one would need to know field names in all relevant tables, like "PersonID" in your example.

If the table name is "person" and the field name is "PersonID", this can be easy. But if you have to deal with a database that you cannot change, and table name for persons is "T_527", and PersonID is stored in the field "F_239", then you can still define a class DeletePerson and can give readable name PersonID to the constructor parameter, and the technical details around naming you hide in the constructor.

Then any developer in your team who needs to delete a person will just use this class, without needing to know implementation details, like table name or field name.

Thus, having a separate class, even if it only defines a constructor, can be an advantage.

my new class should just be a function and that they're stupid classes

No necessarily. It depends on what are you going to do.

For instance, may be according to business logic you need to implement a scenario, a sequence of commands. Suppose, there are foreign keys in your database and, when deleting a person, you decided not to use database cascading deletes, but use fine grained control of every step. Then you may create a scenario that consists of several commands that need to be executed in particular order:

Scenario.Add( new DeletePersonOrders( PersonID ) );
Scenario.Add( new DeletePersonPayments( PersonID ) );
Scenario.Add( new DeletePersonFavorites( PersonID ) );
Scenario.Add( new DeletePerson( PersonID ) );

Then pass this scenario to some place where it needs to be executed. There it can be called via Scenario.Call().

Or you can put such scenario into a new command like DeletePersonCascading that will include other commands.

Also, when you have classes, this gives you the possibility to create commands in one place in the application and to execute them in some other, as follows:

Command = new DeletePerson( PersonID );
ExecutionResult = Executor.execute( Command );

And this executor can do additional logic, like performance measurement, or logging, or checking permissions, etc. The code of executor may look as follows:

protected ExecutionResult execute(Command Command)
{
    PerformanceMeasurement = new PerformanceMeasurement( Command.getName() );
    PerformanceMeasurement.start();
    Command.call();
    PerformanceMeasurement.stop();
}

Again, only you can say, if this is an advantage or not in your particular case.

3
  • 1
    Sorry, but I don't find this convincing. I can't see anything in here that I can't achieve with the original design. In the original design, a well-named static method with well-named arguments would give the same benefit that you're saying a class's constructor with well-named arguments would give. Similarly, your Scenario example can just be a list of methods.
    – J. Mini
    May 16, 2023 at 14:54
  • @J.Mini: The static function in your code is void. But with classes you can create objects and pass them to some other place for further processing. With functions you cannot do that. For instance, you want to measure performance. You can add measurements to the executor that calls each command in the list subsequently. Or you can add logic for permissions checks - to check if particular user is allowed to call particular command. And this can work independently on the command. You can pass the command as parameter to executor. But with functions you cannot do that.
    – mentallurg
    May 16, 2023 at 15:37
  • @J.Mini: I have updated the answer. Again, you decide if this has an advantage in your case.
    – mentallurg
    May 16, 2023 at 15:48
0

My main problem with the code is that it works from a solution, and tries to create objects around that. That may seem like it is OO, but it really isn't. What, after all, is an SqlProcedureHandler? You want to delete a person with a certain ID from some kind of restristry - already an abstract notion by itself. If that is implemented using an SQL database is an implementation detail.

This is even further exacerbated by calls like (new DeletePerson()).Call(PersonIDHere) or deletePerson.Call(PersonIDHere). To me, DeletePerson is not an entity. Even as a command I would expect instances of DeletePerson already to contain information on which person to delete. I'd expect something like PersonRegister.deletePerson(int personID) or something similar instead.

This all probably started with HugeDataClass. You should not represent all of your DB with a single class. You've clearly started with a DB design, and then implemented everything on top of that using this class. However, the data in the DB already should consist of many entities. You should redesign your application in such a way that an object is representing data in one or two tables within your DB.


That doesn't mean that the command design pattern by itself is wrong. You can and probably should use it if the calls to the underlying DB require too much boilerplate (which, remember, is a different reason than that you quoted). It should not worry you if you only have a constructor for most implementation calls; it usually doesn't take long before additional command-specific functionality is added. I've used a similar approach for commands that needed to be executed on a smart card (those commands required a help, a usage string, specific ways of handling errors etc.).

As the command building for the calls to the DB is - as mentioned earlier - an implementation detail from the OO perspective, I would put all the commands and the interface in a separate namespace, so that the other classes can be easily identified. As already mentioned in another answer, you should however keep to the design pattern and perform cmd = new DeletePersonCommand(id) followed by response = executor.execute(deletePersonCommand). The executor should contain & manage the connection details.

To me a private CallCommandAndHandleErrors doesn't make much sense. That's the responsibility of the executor. A command could indicate specific ways of handling errors to the executor. Only errors in the response should be handled by the command itself, otherwise it cannot return the response to the calling procedure. This is conveniently forgotten within the design pattern and for deletePerson you would only expect a response that indicates that the person could be found and yes, the person was deleted. This is however different when retrieving data from a person.


I'll show a way that is slightly different from the design pattern and shows the way to use a method instead of a constructor for most of the work:

public class SQLCommand
{
    protected abstract SQLResponse Execute(Connection con);
    // helper methods to use
    protected void BuildCommand(String commandString, Parameters params);
}

// this class can be extended so that additional information can be retrieved from the response (requiring a cast later on)
public class SQLResponse
{
    private boolean success;

    SQLResponse(boolean success)
    {
        this.success = success;
    }

    public boolean Success()
    {
        return success;
    }
}


public class DeletePersonCommand: SQLCommand
{
    // could also be grouped somewhere else
    const String SCHEMA_COMMAND = "[Schema].[DeletePerson]";
    const String PARAM_PERSON_ID = "PersonID";

    private int id;

    public DeletePersonCommand(int id)
    {
        // some fast-fail error conditions may be checked here, e.g. some specific id's may not be allowed
        this.id = id;
    }
    
    // called from executor
    public SQLResponse Execute(Connection con)
    {
        String command = BuildCommand(SCHEMA_COMMAND, new Parameters(PARAM_PERSON_ID, id));
        // execute the command over con, leave connection exceptions for executor
        return new SQLResponse(True);
    }
}

This hasn't been compiled (I will on a later stage). It is probably very possible to use templates to make sure that at least the response is of the correct type for the specific command.

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.