Should my code embrace more of the "Single Responsibility Principle"?

[SparklySpartan]

Hi everyone,

I just wrote some code to properly delete one of my data types from the database I'm working on. I originally came up with the need for it when I realized that when a record from this particular table (the "Links" table) gets deleted, the code should also check for records associated with it in another table ("Settings") and delete those as well. So I wrote a subroutine with the section in the middle (the SQL_Delete sub is a part of the codebase I'm working with, I know one could use CurrentDB.Execute, but I'm trying to follow the way things were written before I was handed the codebase).

After writing the middle part, I thought it would be good to include an "Are you sure?" message because that's what I typically do before anything gets deleted in the application. So I added the message box providing the option to bail out of the deletion. After doing that, however, I realized that one could argue I had put a limitation on the subroutine. What if I wanted to delete a link independent of the message? Perhaps somewhere else in the future I'll want to loop through a recordset of Links and delete them in bulk. I built in the ability to not show the message via an optional parameter.

Lastly, there is a form in my application that remains open most of the time called "Favorites", and what is displays is largely based on the links and the records associated to them in the Settings table. So I added one final clause to call two subs on the Favorites form that refresh what it's displaying, if the Favorites form happens to be open.

' Properly deletes a link from the database as well as all of the favorites assignments associated with it
Sub Delete_Link(ID As Long, Optional confirmation_message As Boolean = True)
    If confirmation_message Then
        If MsgBox("Are you sure you want to delete this link and all of it's button / dropdown assignments?", vbYesNo) = vbNo Then Exit Sub
    End If
    
    Call SQL_Delete("Links", "[ID]=" & ID)
    Call SQL_Delete("Settings", "(([Key] LIKE ""Favorites_Button_*"") OR ([Key] LIKE ""Favorites_Dropdown_*"")) AND [Value_Long]=" & ID)
    
    If IsFormLoaded("Favorites_f") Then
        Call Form_Favorites_f.Refresh_Captions
        Call Form_Favorites_f.Refresh_Dropdowns
    End If
End Sub

It took me less than five minutes to write all of this, but then I realized that I had just written a sub that appears to be handling 3 different responsibilities: potentially showing a confirmation message, deleting the data I want to delete, and refreshing a form. From what I've studied and been taught about programming in general, this isn't considered the best way to design a subroutine.

To be honest though, I'm not sure if this breaks SRP or not. I do agree that it does three different things, but it's not as if those things aren't tightly related and supposed to happen together most of the time anyways. Still, I'm open to your thoughts on other ways to design it, or to hear if others would also choose to design it this way.

What do you think about the conditional confirmation message? If you wanted a standard confirmation message to show in all of the 3-4 places this sub would be run from, would you opt for what I did, or use another function to handle that "UI" responsibility and let the Delete_Link exclusively handle the "Data manipulation" responsibility?

And what are your thoughts on refreshing the form? I have a gut feeling there are better places to put this code but I don't know where else I would besides repeating it in the 3-4 places which doesn't feel good to do either.

I might be obsessing over this too much. I know I shouldn't force my code to follow something like SRP just for the sake of it if it doesn't make sense within the scope and scale of the application I'm making. That said, I like to remain on the lookout for things I could be doing better at all times, because hopefully I'll learn something in the process of seeking it out. So I'd love to hear your thoughts. Thanks in advance!

 

[The_Doc_Man]

It took me less than five minutes to write all of this, but then I realized that I had just written a sub that appears to be handling 3 different responsibilities: potentially showing a confirmation message, deleting the data I want to delete, and refreshing a form. From what I've studied and been taught about programming in general, this isn't considered the best way to design a subroutine.

From a viewpoint of maintainability, "purity of purpose" is actually a derivative result of the "divide and conquer" principle that repeatedly carves out smaller chunks from your work list until you can stop subdividing and just build your actual code. The importance of this approach cannot be downplayed, and you are absolutely right to ask the question.

As always, there are theoretical trade-offs regarding "purity of purpose." The right questions to ask regarding those three steps you listed would be (a) do - or should - those three actions ever NOT appear/occur together and (b) from a business operation viewpoint, are they directly related to each other as part of some specific process?

From a viewpoint of current or future functionality, some things need to be kept together because they are intrinsically related - sort of like macaroni and cheese or ham and eggs. The problem here is that we cannot make the required decision because we don't know the business rules or environment intimately well... if at all.

The question about refreshing a form leads to another issue. OK, you refresh the form. If ALL you do is a Me.Refresh, and it has no logging or other functionality, then isolating the Refresh operation to a subroutine is overkill because it is ALREADY down to a one-line action. Adding another tightly purposed function or sub adds run-time overhead and probably doesn't add much value to the code.

When adding a subroutine of only a few lines, the question would have to be asked, "Is that all you were planning to do?" Sometimes, repeating an elementary action in code just happens. Note that if your form Refresh operation included changing other things on the form before or after the Refresh, or if it performed a test before continuing an action, then isolation to a subroutine might make sense. But in general, the less you do in the routine, the harder it is to justify MAKING it a routine. Where is the dividing line on that decision? My friend, if you could answer that question definitively, you would have people banging on your door for advice.

 

[MarkK]

You are performing business logic AND user interface logic in the same routine. Generally you should isolate both. The hazard of not isolating them are...
• If you refactor SQL_Delete() or the refresh methods of the Form, this routine will break without warning.
• If your system continues to grow with these kinds of built-in dependencies, it becomes almost impossible to make significant improvements to it, because every edit causes wide-spread breakage.
• If you sprinkle the assertion of business logic--and the construction of the where clause in this routine is business logic--then updating your business logic gets hard because it could be anywhere (and has likely been repeated in various places).

A more flexible and reliable pattern is to encapsulate all business logic for a single information domain in a single service class. Write a class called cSettingsService and make it solely responsible for managing logic related to settings. It should not store any state, so it should exist apart from any particular setting. If a setting needs to be deleted, prompt the user in the UI. If confirmed, call SettingsService.Delete(SettingID as long). Then that method can call other methods to update the database, and make changes to any other dependent services. If SettingsService.Delete succeeds, it can raise a global notification event. UI elements can subscribe to that event, and refresh/requery themselves completely independently.

Advantages of this approach:
• You know exactly where to put, and where to find back, business logic.
• The increased isolation of each component of your system means that changes to other components are far less likely to cause unforseen breakage.

Disadvantages:
• You have to discipline yourself as to when and where you do what in your code, so its harder.
• It takes more than 5 minutes to write a delete method.

 

[Pat Hartman]

Like everything else we do, structuring code requires common sense. Even sanity checks if you think that dividing code into single line procedures makes for "better", "more reusable" code. Procedures should do one and only one thing and they should do as much of that one thing within their boundaries as possible. You may open the same form from 10 different places. Does that mean you need to write a procedure to open formA and call it 10 times? Absolutely not. We would need to call a psychiatrist for a sanity check if you thought that was a good idea.

@MarkK makes some excellent points on applying logic in determining how to split code and why.

 

[SparklySpartan]

Advantages of this approach:
• You know exactly where to put, and where to find back, business logic.
• The increased isolation of each component of your system means that changes to other components are far less likely to cause unforseen breakage.

You make some excellent points for why I'd want to do it that way. I'll definitely have to look into doing that. It would be the first service class implemented in this codebase so far, but there are probably a lot of other times it would've helped. If it goes well it might just cause a refactoring avalanche.

Appreciate the responses everyone!

 

[SparklySpartan]

Update on the promise to try out what MarkK has suggested, I created my first event system in VBA today on a blank database. Created a class module, learned to use a function to create a "Singleton Instance" of said class that can be accessed throughout my code by running said function. Then, I made a bunch of forms holding "WithEvents" references to that instance of the class. I created a button to raise a custom event in my class called "SomethingHappened" and defined various different behaviors inside the events for the various forms.

Works fantastic! None of the forms know about each other, and they don't have to, which is absolutely wonderful, and I can see how that would decrease the amount of points the code could break. One form does not have to reference another, it simply raises an event and each other form can handle it however it needs to. This is the kind of code I've been feeling like I should be writing, so I'm glad I finally learned this technique.

If you were curious to see my silly first custom event system, I thought I'd share it here. These were just silly examples of course, but I'm excited to take this further in my actual application. Thanks again for the insights!

(Also, yes my background is the blue screen of death. Apologies if I've inadvertently brought back unpleasant memories to some! )