A TDD Story

My first todo note has do to with the hasBeenShown variable. Let's look at the test I just added.

It looks like a wart on an otherwise nice test. There are several reasons I can think of why it looks like a wart. One is that the variable and the rest of the method are on different abstraction levels. I will explain what I mean with that later in this chapter, using another example. The other reason, and the one I will discuss now, is the fact that hasBeenShown is a variable whose value changes within the method. It is a mutable local variable.

This requires some explanation. One of the most difficult things in programming, and a thing that, if not handled correctly, adds most complexity to the code, is program state. The program state is everywhere, and is essentially the reason why the application exists. It hides in fields and variables, in the user interface, in the database and so on. Some states are there to handle the problem domain, such as the information encapsulated in the Song class in my application, while other states are about implementation details, such as the hasBeenShown variable.

State is difficult, really difficult. And there are many things you can do to simplify the problem. I will talk here about state represented in local variables, since that is the problem I have right now. I will get the chance to revisit the state discussion though, and then I will talk about classes.

The ugly thing with hasBeenShown is, as I mentioned, that its value changes. It is initialized with the value false and might later change to true. It is not the worst kind of variable since the value actually always makes sense. The worst kind of variable is probably the one that lie. Here's an example:

On the first line, the variable lies. It says that it is the width including the border width when it is not. A much better version of the same code goes like this:

Now there are no more lies. And the magic change was to introduce another variable with its own descriptive name for the value representing the width excluding the border. These lies are often the result from reusing the same variable for more than one purpose.

There is an attribute that goes hand in hand with single-purpose variables, and that is that once initialized it never changes. The variable is initialized on the same line as it is declared, and it stays with that value for the rest of its life-time. It represents a state that never changes, and that is a state that is simple to keep track of.

The hasBeenShown variable doesn't lie, though. Rather, it represents a state that needs to change. There are two ways to handle such a state, and both are applications of the Single Responsibility Principle. One way is to create a method whose only purpose is to keep track of the state, and the other is to create a class whose only purpose is to keep track of the state.

A method that keeps track of a state is typically a method that contains some kind of algorithm. Here is an example:

This code is a simple algorithm that calculates the width of something as the sum of all its containing parts. There is no way to implement this without, somewhere, having a state that changes. And the best way to handle this changing state is to extract it into a method whose only purpose is to calculate the width. That is, place all lines from the code above, and nothing but those lines, in a method that returns the result. That method will then have one sole purpose, and that is to calculate the width.

The hasBeenChanged variable is not part of any algorithm, though, so I have to resort to the second alternative, and that is to create a class whose only purpose is to keep track if it.

I create a ViewShownAsserter class and move the state handling into it.

The state that was handled by the hasBeenShown variable is now handled by the property WasShown in the ViewShownAsserter. And keeping track of the WasShown property is the sole purpose of the class, which means it adheres to the Single Responsibility Principle.

After this change, I need to verify that things are still working. Running the tests results in no errors, as expected. However, I want to verify that I still get a failure message when the view is not shown, so I remove the ShowView() call in the SongsShower which gives me the following message.

That looks ok.

While the test looks a little bit better, it still doesn't look good. The ViewShownAsserter is too much of an implementation detail to be in a test. This makes me think that perhaps it should be handled as an implementation detail.

Say that this test, the test that checks that the view was shown, would fail, what would then happen to all other tests? If this test fails, it is due to that the showAction was never executed. Many of the other tests have their asserts inside the showAction, which means that they would be green. And they would be green even when they have some other bug inside in them. Tnat is, it would be a false negative.

This is clearly not good, so I'm thinking that verifying that the view has been shown should not be done in only one test, but should actually be done by all tests. This I can do either by duplicating the code in all tests, or I can move the use of ViewShownAsserter to the ClickAddButton() method.

Obviously, I choose the second option, and at the same time I modify the ViewShownAsserter so that it can append itself to an existing showAction.

Now the check is hidden in the test infrastructure which means I don't have to have a separate test that that explicitly verifies that the showAction is executed, that is, I can remove the Click_add_button_opens_add_song_window() test.

I like this change very much, and I want to do the same thing for when showing the SongsView, that is, replace with Songs_window_is_displayed_when_the_application_starts() test with an infrastructure check in the ApplicationFake.Start() method.

If I change the ViewShownAsserter to be generic I can reuse that class for this check as well, so I do it.

And now I can start using it for checking the SongsView as well.

I check that the asserter is working by changing the production code so that the view is never shown. This gives me the following failure message for one of the tests.

That's not the best message. From this, I would have guessed that there is something wrong with the population of the song list, not that the view was never shown. So I add a failure message to the assertion.

Running the same test I now get a better failure message.

Restoring the call to ShowView() gives me green tests again. I can throw away the todo note about the hasBeenShown ugliness and check in the code.

As you might remember, I have said previously that assertions should be stated clearly in the test. Now I have added an assertion that is hidden inside the ViewShownAsserter. But this one is different. I see this assertion as a sanity check: it doesn't make sense to continue executing the code if showing the view is not working.

So there are (at least) two kinds of assertions. There are those that exist to verify the thing that the test is created to verify, and those should be stated clearly in the test method. And then there are those that verify assumptions about the code and that stop the test unless it makes sense to continue executing it. These are implementation details and should not be inside the test method.

While moving the view shown assertion into the infrastructure, I also removed the only test I had that opens the Add Song Window. That is, I now have some production code that is not covered by tests, so my safety net is broken. That was a bit of a mistake, and I probably should have waited with deleting the test. So I restore the test before continuing.

With the safety net back in place, I could take care of another todo note. But before I do so I would like to add a little bit more of functionality, to have more behavior to work with when doing my refactorings.