I have preached here once or twice before about the virtues of loose coupling. I will not belabor the point much more here, except to say that I get lots of emails from people asking about specific examples of loose coupling in their corporate projects. It seems everyone is feeling the same pains. I have also received a fair number of emails that go like this: "I am working on a project and was wondering if you could help me understand loose coupling by showing me how to make my code more loosely coupled." Attached to those messages is a zip file containing their Visual Studio project. I apologize if you are the sender of an email that went unanswered or worse, I answered your email and said that I couldn't spend enough time to give you a good answer. The truth is, it is somewhat difficult to look at a large body of unfamiliar code and quickly offer meaningful guidance. I have said before that a good rule of thumb is that vigilant use of good coding practices makes code easier to test - loose coupling is a paramount example of this principle.
Recently, I have come across code that is more tightly coupled than necessary, making unit testing more difficult than need be. Here is one very simple example - consider the code below.
Figure 1: Tight Coupling of Method to Complex Parameter Type
Private Sub DoSomeLogging( _
ByVal p_ApplicationState As ApplicationState)
With p_ApplicationState
'...
End With
End Sub
Nothing looks amiss initially, but try to write a unit test for this method. Do you see it? The DoSomeLogging method is tightly bound to the ApplicationState object. The method isn't useful at all without an ApplicationState object. Also, as you can see, to test this method you must manually construct an ApplicationState object to pass into the DoSomeLogging method. The problem with passing big classes and structures into a method like this is that it can be very time consuming (and potentially dangerous in your production environment) to manually construct such objects.
The boss says write the unit test and ship it, so you do. You build bunches of private methods to manually construct an ApplicationState object. You see that your unit tests pass and everyone's happy. You get to keep your job.
Two weeks later, you make a small change to your code, and start running your unit tests. Many of them are failing. What happened? Someone changed the ApplicationState class and/or the classes that consume it, and now your manual construction of the ApplicationState object is stale. You research the changes that were made, make changes to your ApplicationState construction code, and then deploy your changes. You see that your unit tests pass and everyone's happy.
Two weeks later (yes, it happens again), you make a small change to your code, and start running your unit tests. Many of them are failing. What happened? Someone changed the ApplicationState class and/or the classes that consume it, and now your manual construction of the ApplicationState object is stale. You research the changes that were made, make changes to your ApplicationState construction code, and then deploy your changes. You see that your unit tests pass and everyone's happy.
Two weeks later ... you get the point. The simple feedback I have for this method is that (since the ApplicationState object is difficult to construct and fragile to build manually) it is very likely easier to just pass into DoSomeLogging the data it needs rather than passing in the entire ApplicationState object. It will absolutely lead to longer parameter lists for your methods and some refactoring will be required, but it is worth the effort. Your testing will go more smoothly and your method will be more useful.
The new and improved DoSomeLogging method will look like the code in Figure 1.
Figure 2: Loosely Coupled Version of Method
Private Sub DoSomeLogging( _
ByVal p_Parm1 As String, _
ByVal p_Parm2 As String, _
ByVal p_Parm3 As String, _
ByVal p_Parm4 As String)
'...
End Sub
Not only is this method easier to test, but it is easier to write, it is more likely to be shorter, it is more likely to have a lower cyclomatic complexity, and it is more likely to be reused.
If you read either of my previous articles, you will know that there is more to consider than just loose versus tight coupling. Consider the following.
- What if this method is an overload of another DoSomeLogging method (like the one shown in Figure 2) and its only purpose in life is to make it simple to call the DoSomeLogging method without having to specify all the parameters in every location where the DoSomeLogging method is called. Good call, I think passing ApplicationState as a parameter is a good move.
- What if this method was intended to be used only with ApplicationState objects. My gut tells me that perhaps the DoSomeLogging should have been a method on the ApplicationState class. Even if it isn't, it is still worth considering the pros/cons of decoupling the DoSomeLogging method from the ApplicationState object. You will need to consider how often DoSomeLogging is called (see bullet above), how fool-proof you want to make the DoSomeLogging (you might want to prevent someone from passing in the wrong parameters), and lots of other things before knowing which way is the right way to go: loose versus tight coupling.
- Keep it simple. One look at the code in Figure 1 tells you what the method does and its likely consumers. Readability is a paramount goal of mine and only egregious problems are likely to trump readability.
If you use the difficulty-of-unit-testing method of judging code smell, you would have caught this one right off. Luckily, I think this type of coupling is relatively easy to fix and even easier to avoid.
