An example of wrong port design detection and refinement

1. Introduction.

As part of the Intensive Technical Mentoring Program that we recently did for AIDA, we worked on the Legacy Security Manager kata. This is a great kata to practise dependency-breaking techniques and how to test and refactor legacy code.

2. Introducing tests.

In order to be able to introduce unit tests, we first used the Extract Call & Override dependency-breaking technique, to create two seams, one to spy what was being written on the console, and another one, to control the user input received through the console.

Breaking those dependencies allowed us to write the following unit tests:

3. Separating core logic from infrastructure.

With those tests in place, the pairs started refactoring the code until they were finally able to separate infrastructure from core logic using the “Move Function to Delegate” refactoring^[1] to control the awkward dependencies by inverting them and making them explicit. This is the code after the refactoring:

Notice the two ports, Notifier and InputReader. These ports are the result of directly applying the “Move Function to Delegate” refactoring to the previously existing seams: the methods protected virtual string Read() and protected virtual void Notify(string message) shown in the previous section.

This is how SecurityManagerTest looks after the refactoring:

4. Some test smells pointing to a design problem.

But, if you look closer at the tests, there’s a smell that is pointing to problems in the current design.

Notice inside the helper method VerifyNotifiedMessages how, in order to avoid regressions, we need to enforce the order of the calls to Notifier’s Notify method so that they coincide with the order in which stubbed values are returned by InputReader’s Read method. If we wouldn’t do this, we might change the code in ways that would break the behaviour but not the tests, thus allowing regressions.

Having said that, it shouldn’t be necessary to enforce this order concordance between the stubbed calls to the InputReader and the calls to the Notifier. This is an overspecification that makes the tests more fragile, for instance, changing the order of some of the input requests would break the tests, even though the observable behaviour wouldn’t be changed.

Another signal that something is amiss is the duplication in the verification of the first four calls to the Notifier which we introduced the helper method, VerifyNotifiedMessages, to avoid. This duplication is caused by a leak of implementation details because only the last notification is related to the observable output of the system, and thus, is the only one required to verify the behaviour. We should not need to check the other previous four notifications. The helper method, VerifyNotifiedMessages, is removing this duplication and hinting that we are only interested in the last notification, which is better than leaving the duplication, but this only addresses the symptom but not the cause.

The underlying problem that is causing all this accidental complexity in the tests is that the ports are not well designed.

5. How did we get to this point?

When applying Extract Call & Override it’s advisable to change the code as little as possible because this technique is applied without tests^[2]. Sometimes this may produce seams that are not a good abstraction and are too low level, but that’s ok at that stage, because the goal is to introduce tests assuming as few risks as possible.

The current ports come directly from applying the “Move Function to Delegate” refactoring to the seams that we used when we broke the dependencies that were stopping us from introducing unit tests. Many times this is just fine, and seams directly become ports, but this is not always the case, like in the current example.

The current ports are leaking an implementation detail: that we need to call two methods in sequence in order to get the user input.

6. Fixing the design.

If we think about the purpose or the role of each of the messages written to the console, we may see that they fall in two different categories: some are requests to the user to introduce some specific input, and others are straight away notifications. We may also see that calling two methods in sequence to get the user input is an implementation detail, what the client (SecurityManager) needs is just getting the user input, the two calls in sequence is how we get the user input.

Thinking in those different roles we may refactor the code (applying a parallel change) to use the following new interfaces for the Input and Notifier ports:

This the new code of SecurityManager^[3] using the new ports:

And these are the resulting tests:

Notice how this improved design has made disappear the two smells that were present in the previous version of the tests:

1. We don’t need to coordinate the order of the stubbed values and the order of the corresponding user input requests anymore.
2. We only notify and verify the message that informs about the result of the operation.

7. Conclusions.

We have seen how badly designed ports may lead to smells in how test doubles get used in the tests, in the example shown in this post, duplication and overspecification of the order of the calls. Such overspecification, in particular, makes the tests more fragile.

Those badly designed ports came from blindly applying “Move Function to Delegate” refactoring to every seam directly. In most cases, applying “Move Function to Delegate” to seams it’s perfectly ok, but sometimes, it might lead to ill designed ports.

We saw how thinking about the role or purpose of each interaction led us to better designed ports, and how this new improved design just made the problems in the tests disappear.

8. Acknowledgements.

I’d like to thank Fran Reyes and Alfredo Casado for revising drafts of this post.

Also thanks to the participants in the Intensive Technical Mentoring Program that we recently did for AIDA who worked through the whole kata, and to Audiense’s developers which also worked on a version of the kata that starts with the ill designed ports to practise their detection from test smells and refactoring using parallel change to fix the design. It was a pleasure to work with all of you.

Finally, I’d also like to thank Steve Johnson for the photo.

9. Notes.

[1] Documented by Nicolas Carlo in his book Legacy Code: First Aid Kit.

[2] In fact, dependency-breaking techniques are applied in order to be able to introduce tests.

[3] Keep in mind that SecurityManagers code reflects a snapshot in time while solving the kata. We didn’t want to divert the focus of the post to anything other than the detection of problems in the tests that led to detecting problems in the port interfaces, and how we refined those interfaces. As such SecurityManagers design is still a work in progress. There are still many issues that would be worked later in the kata, such as, for instance:

• Where should the responsibility of deciding if the user input is valid or not be? Maybe UserDataRequester would be a better place.
• Finding a better place for the password encryption responsibility, which now is located in a static method in SecurityManager (being static is a hint that it doesn’t belong there). Not representing the user password with a string would attract that behaviour and, probably, the password length validation.

But those are material for another story…