My next test verifies that the application is closed correctly. This test, I discover, becomes a bit more complicated. I would like the Thingy.ShowView() method to look something like this:
public class Thingy { // ... public void ShowView() { View.OnViewClosed += () => ExitThread(); View.ShowView(); } }That is, Thingy listens to an event in the view that is raised as the view closes. The event handler calls ExitThread().
Java developers will probably raise an eyebrow wondering what kind of strange code that is. And no wonder since there are two language features in action here that don't exist in java.
First, there's the += operator. C# has some built-in language features that make it very easy to implement the observer pattern. Using the += operator I start subscribing to the observable, or event as it is called in C#, OnViewClosed. That is, I will be notified whenever that event is triggered.
On the right hand side of the operator is a lambda expression, a feature currently planned to be included in Java 8. Simplified, you could say that it is an anonymous method. The expression is what will receive the notification, that is, this code is executed when the event is triggered. The empty parentheses state that this lambda expression has no parameters. The => operator states that the body part will follow, and ExitThread() is the body part.
Writing the code like this won't do since I don't want the ExitThread() code to execute in my tests. So in some way I need to make it behave differently in the test. One way would be to use a runtime variable or preprocessor symbol to select what code to execute using if or #if. There's something that tells me to look for a different solution though.
if-clauses are generally quite hard to read. Without them, the code is (or could be) like a table of contents, or like a set of simple instructions that you read one after the other. When you introduce if-clauses you force the reader to keep two flows, or two tables of contents, in the head. The head is quite bad at keeping track of multiple things, so for the sake of readability, which might be the most important thing when programming, you should avoid if-clauses.
Also, if-clauses are often (but not always) a symptom of breaking good design principles. I will revisit this discussion later and demonstrate examples of such violations.
So what can I do if I am not allowed to use if? Well, instead of passing a flag that I use to select a behavior, I can pass in the behavior. That is, instead of passing in a boolean that specifies whether to execute the test code or the production code, I can pass in the code that should be executed: test code from the test and production code when running the real application.
This I do by passing the code that calls ExitThread() as a delegate parameter to the Thingy constructor, like this:
public static class Program { [STAThread] public static void Main() { System.Windows.Forms.Application.EnableVisualStyles(); System.Windows.Forms.Application.SetCompatibleTextRenderingDefault( false ); var thingy = new Thingy( new SongsView(), ac => ac.ExitThread()); thingy.ShowView(); System.Windows.Forms.Application.Run(thingy); } } public class Thingy { private ISongsView View { get; set; } private Action<ApplicationContext> CloseAction { get; set } public Thingy(ISongsView view, Action<ApplicationContext> closeAction) { View = view; CloseAction = closeAction; } public void ShowView() { View.OnViewClosed += () => CloseAction( this ); View.ShowView(); } }This way the tests can pass a different delegate, one that doesn't call ExitThread(). It's not the nicest solution, but I hope to be able to revisit it later.
And so we dig deeper into the world of lamda expressions.
When I create Thingy I pass in ac => ac.ExitThread(). Again, this is a lamda expression. However, instead of the empty parentheses stating that I have no parameters, I have the parameter ac.
I haven't specified the type of the ac parameter though, because I don't have to. The type is specified in the parameter to the Thingy constructor, in another new thing: Action<ApplicationContext>.
The type Action is a type that encapsulates a method. Action is not a class but something called a delegate. Simplified, you could say that a delegate is a reference to a function which means that the method it contains can be executed at a later time, much like when using the command pattern.
Finally, there's the <>. This is something that java indeed has: generics. With the generic parameter I specify that the method that Action encapsulates has a parameter of the type ApplicationContext. And this is the reason why I don't have to specify the type of ac, since the compiler can now figure it out that ac is of the type ApplicationContext.
Now that I know how to write the test I get on with it. First I undo my sketched code and then I update the view fake to raise an OnCloseEvent once it has been shown and validate that the close action is executed. When changing the Thingy constructor I also have to update the other test and pass in a dummy close action.
public class SongsFixture { [Test] public void Songs_window_is_displayed_when_the_application_starts() { var view = new SongsViewFake(); var thingy = new Thingy(view, ac => { }); thingy.ShowView(); Assert.That(view.HasBeenShown, Is.True); } [Test] public void Application_closes_when_songs_window_closes() { var view = new SongsViewFake(); var closeActionHasBeenExecuted = false; var thingy = new Thingy(view, ac => { closeActionHasBeenExecuted = true; }); thingy.ShowView(); Assert.That(closeActionHasBeenExecuted, Is.True); } } public class SongsViewFake : ISongsView { public event Action OnViewClosed; public bool HasBeenShown { get; private set; } public SongsViewFake() { HasBeenShown = false; } public void ShowView() { HasBeenShown = true; ClickWindowCloseButton(); } private void ClickWindowCloseButton() { OnViewClosed(); } }In the ClickWindowCloseButton() method, is the other side of the observer pattern. By calling the OnViewClosed event, as if it were a method, I notify all listeners of the OnViewClosed event.
There is another notable thing in the Application_closes_when_songs_window_closes() test method. The local variable closeActionHasBeenExecuted is being used in the lambda expression that is passed into Thingy. This lambda expression might and will be executed somewhere else, far from where it is declared and far from where the variable is declared. That is not only ok, but also extremely powerful. This language feature is called closure.
If you have prior experience with C# events you might have some objections to my way of raising the OnViewClosed event. If there are no listeners to the event, a NullReferenceException will be thrown. It is therefore often recommended to check the event delegate against null before calling it. Also, it is often recommended that the delegate is assigned to a local variable to get thread-safe code (read more about this here). There is one thing I dislike about the check against null, though, and that is that you create robustness for something that (in this case) cannot happen.
If there are no listeners I have got a bug, and I would like my application to tell me about it by crashing. And this is important! Had I let the application continue executing I would most likely get another error later on, and probably something completely unrelated to the missing event subscription. That is, I would have a hard-to-diagnose bug that would require many hours to find. If I let the application crash early, on the first sign of the bug, the cause would be obvious.
Remember how I mentioned previously that if-clauses are often a symptom of breaking good design principles? Here was another example. Checking for null in this case would either be a sign of that the programmer does not trust his own code, that he doesn't know what it is doing, or a sign of that the programmer does not know how to create robust code.
Regarding the potential threading problem, I don't expect several threads to be executing in this code, so I don't feel the need to worry about that either. Making thread-safe code is difficult and complicated, and in order to handle it correcltly I would have to add a lot of complexity to the code, much more than just assigning an event handler to a local variable. So until you really have a threading problem the best thing is not to pretend you have one.
Back to the code. To make it compile I need to add the parameter to the Thingy constructor in the Main() method.
public static class Program { [STAThread] public static void Main() { System.Windows.Forms.Application.EnableVisualStyles(); System.Windows.Forms.Application.SetCompatibleTextRenderingDefault( false ); var thingy = new Thingy( new SongsView(), ac => ac.ExitThread()); thingy.ShowView(); System.Windows.Forms.Application.Run(thingy); } } public class Thingy : ApplicationContext { public Thingy(ISongsView view, Action<ApplicationContext> closeAction) { view.ShowView(); } // ... }Now the code is compiling, so I run the test.
And I get the NullReferenceException because there are no listeners to the OnViewClosed. Had I not discussed this in the previous paragraph I would probably not have expected this to happen. Rather, I would have been quite puzzled. Yet, it is perfectly valid reason for failure. The failure message, however, could have been better. So before I make the test green I change the code so that I get a better exception.
public class SongsViewFake : ISongsView { // ... private void ClickWindowCloseButton() { var handler = OnViewClosed; if (handler == null ) throw new InvalidOperationException("No handler registered for the event ."); handler(); } }I use InvalidOperationException here since it describes well what went wrong: I tried to perform an operation that is invalid given the current state.
Now when I run the test I get a much more informative message.
Using the stack trace I can figure out which event I tried to raise.
You probably noticed that I just added an if-clause, and it was not long ago that I argued that using if-clauses is a bad practise. Don't worry; I have not changed my mind. There are places where it is perfectly ok to use conditionals. Here I use it to detect an error and to provide a good error message when it occurs, so that's for a good reason. I will talk more about these kinds of checks in a later chapter.
Having good failure messages is really important. Right now the application is small and the failures I get are easy to find. But as the application grows it will be trickier to track down a failure. So it's worth spending a little bit of time on it now when I have to code fresh in my mind.
Next, I do the smallest thing possible to make the test pass, and that is to make sure someone is subscribing to that event. I take the smallest step for several reasons. The more code I add, the more likely that I won't find the best solution. After every step, I get a reflection moment where I discover new things about the code. Taking larger steps, these learning moments happen more seldom. Also, when taking small steps I will see more failure reasons and get the chance to improve failure messages. Perhaps I might even discover failures that I didn't expect.
I add the OnViewClosed event to the ISongsView interface and raise it from the SongsView using the same code as in the fake.
public interface ISongsView { event Action OnViewClosed; void ShowView(); } public partial class SongsView : Form, ISongsView { public event Action OnViewClosed; // ... private void SongsViewFormClosed( object sender, FormClosedEventArgs e) { var handler = OnViewClosed; if (handler == null ) throw new InvalidOperationException("No handler registered for the event ."); handler(); } }Then I subscribe to the event from Thingy with an empty handler.
public class Thingy : ApplicationContext { // ... public void ShowView() { View.OnViewClosed += () => { }; View.ShowView(); } }The test fails and for the expected reason.
Finally, I can make the changes as I sketched it.
public class Thingy : ApplicationContext { private ISongsView View { get; set; } private Action<ApplicationContext> CloseAction { get; set; } public Thingy(ISongsView view, Action<ApplicationContext> closeAction) { View = view; CloseAction = closeAction; } public void ShowView() { View.OnViewClosed += () => CloseAction( this ); View.ShowView(); } }Now that the test passes, the InvalidOperaionException is never thrown. That is, I have written some code that is never executed by a test. There's one occurrence in the view fake and another in the real view. Should I do something about this? Should I try to cover it by a test? That is, should I write a test that tests that the fake view works correctly in case there's an error in the production code?
One could argue that since I have tested it once I know that it's working. And what are the chances that it breaks? And if it breaks, what are the consequences? A somewhat worse failure message. Can I live with that?
I don't think there is a correct answer here. With a dogmatic approach we would certainly write tests for it. With a lazy approach we would not. The pragmatic approach lies somewhere in between.
The interesting thing with the untested code, though, is that there are two copies of it. Both the real view and the fake view contain the exact same code. That duplication bothers me more than not having 100% test coverage. So my next task is to remove this duplication.
Before I do that, I run the application and notice that it works fine to close the window. Then I check in my code.
As you notice, I check in my code often. Typically I check in many times per day and sometimes several times per hour. There are several good reasons for this. It forces me to always have the code in a working state. The build server helps me with feedback saying that this is also the case. Having the code in a working state also ensures that I'm progressing. You never know how long it takes to get non-working code to start working: it could be minutes, it could be days. Furthermore it forces me to integrate continuously with team members, so that I have to handle small code conflicts early rather than big conflicts late. I could go on forever with more reasons…