The add window can now be opened and closed, and its song title is initialized with an empty string. There seems to be nothing left but to make sure the song is added to the song list in the songs window when clicking the ok button. It sounds big and scary, but I don't think I can postpone it.
So I write the following test:
[Test] public void Add_new_song_is_added_to_song_list() { Application .Start(view => { view.ClickAddButton(addView => { addView.ChangeSongTitleTo("Sunday Morning"); addView.ClickOkButton(); }); Assert.That(view.Songs, Is.EqualTo( new [] { new Song("Sunday Morning") })); view.ClickCloseButton(); }); }I open the add song window, enter a song title, click ok, and then verify that the song list contains a song with the given song title.
For java developers, the parameter to the Is.EqualTo() method might look a bit funny. What I do is that I create an array, just like it is done in java, but I don't specify the type of the array. Again, it is the type inference that does its magic. Since the variable inside the initializer is of the type Song, the compiler assumes the array is the type Song[]. This, of course, requires that, if there are more than one item in the list, all are of the same type.
As you notice, the test contains the string "Sunday Morning" twice, once for entering the song title in the text box and once as part of the expected value in the assertion. One might argue that this is duplication and that I should introduce a constant for the two.
I wouldn't complain loudly if you would do that, but I prefer not to. Like before, my argument is readability. For me, having the string stated twice makes the test easier to read. If I introduce a constant I would have to look elsewhere for the definition. Even if I define it as a local constant, that is, in the test method, I would have to move my eyes a few lines up to see what it means.
Also, this kind of duplication is not a dangerous one. Dangerous duplication is when you have something that must or ought to be the same in different types, files and/or projects. In such situations it is not obvious that a change to one of the copies requires you to change some other copy as well. Here, the two copies are so close that it is hard to miss. And even if you accidently change one, the test will quickly remind you to change the other.
One could even argue whether this is actually duplication. Duplication is about concepts, not about code. That is, you could have the exact same string, or code, in two places that actually mean different things. Say, for instance, that the width of your button is 100 pixels, and that the hight of an image in the same dialog also happens to be 100 pixels. Just because you have the number 100 specified in two places doesn't mean you have duplication.
This, I admit, is an extreme example, but I argue that my point is valid. In my test, the situation is a bit less obvious. The first occurence of the string specifies what the user types as the song title and the second what to expect in the songs list. I expect them to be the same, but that doesn't mean they represent the same concept.
After this detour, it is time to make the code compile. So I add the ChangeSongTitleTo() method to the view fake.
public class AddSongViewFake : IAddSongView { // ... public void ChangeSongTitleTo( string songTitle) { } }The question is, what kind of code should I add to the ChangeSongTitleTo() method? Previously, in the methods that triggers a button click, what I did was to raise an event and let a listener handle it. That is, when clicking the close button, for instance, the view doesn't close the view, but passes on that responsibility to the code behind. In the same way, I would prefer if the view doesn't hold on to the song title, but lets the code behind handle that. When the user changes the text, the view should notify the code behind about it and the code behind should update the view accordingly.
However, that's not how the WinForms Textbox control that I'm using works. It keeps track of its own state. When the user types the keyboard, the control updates its internal state and then it passes on an event to listeners saying that it has changed. That is, the control doesn't ask the listeners to make changes to it, it tells the listeners that it has already changed its state.
Should I take advantage of the fact that the view knows its state, and not store the state in the code behind? Whatever choice I make, I want the fake to work just like the real view. And since the real Textbox keeps track of its content, the fake view should also hold on to the song title. So I add that behavior to the fake.
And to keep things simple for now, I don't move the state into the code behind for now, but only keep it in the view. And therefore, I don't even have to notify the code behind that it has changed.
public class AddSongViewFake : IAddSongView { // ... public void ChangeSongTitleTo( string songTitle) { SongTitle = songTitle; } }To make the code compile, I also add a constructor to the Song class.
public class Song { public Song( string songTitle) { } }Now I can finally run the test.
The test fails, as expected, but the failure message is not the best. I understand that there is a Song missing in the songs list, but the information about what is missing is really bad. What happens here is that NUnit outputs the information using the Song.ToString() method, and since I haven't created such method, the default (on the class Object) is used which only outputs the name of the class.
So to improve the error message, I override the ToString() method in the Song class.
public class Song { public string SongTitle { get; private set; } public Song( string songTitle) { SongTitle = songTitle; } public override string ToString() { return string .Format("song title: '{0}'", SongTitle); } }To get a good message, I obviously have to store the song title, and I use an auto property for that.
The string.Format() method requires some explanation. It is like javas printf() methods, but the parameter placeholders are specified with {N} instead of %N (where N is the index of the parameter).
As you can see, I have put the {0} with quotes. This is a nice trick to use for strings since it makes it easy to distinguish empty strings from strings with only whitespace in it.
Now I run the test again.
The failure message is much clearer.
To make the test pass, I need to do something in the Ok button handler. This is what it currently looks like.
public class AddSongShower { // ... public void Show() { View.SongTitle = string .Empty; View.OnOkButtonClick += OnOkButtonClick; View.OnCancelButtonClick += View.CloseView; View.ShowView(); } private void OnOkButtonClick() { View.CloseView(); } }The simplest way I can think of to make this test pass is to pass the SongsShower into the AddSongShower and use that as a mediator to update the song list. So that is what I do.
public class SongsShower { // ... private void OnAddButtonClick() { AddSongShowerFactory.Show( this ); } } public class AddSongShowerFactory { private IAddSongViewFactory AddSongViewFactory { get; set; } public void Show(SongsShower songsShower) { var view = AddSongViewFactory.Create(); new AddSongShower(view, songsShower).Show(); } }I pass the SongsShower into the Show() method of the factory which, in turn, passes it on to the AddSongShower. Unfortunately, I add a dependency from the AddSongShowerFactory to the SongsShower only for it to be able to pass on the parameter. But since it is a factory, it's not that bad.
Now the AddSongShower can add the song.
public class AddSongShower { private IAddSongView View { get; set; } private SongsShower SongsShower { get; set; } public AddSongShower(IAddSongView view, SongsShower songsShower) { View = view; SongsShower = songsShower; } // ... private void OnOkButtonClick() { SongsShower.Add( new Song(View.SongTitle)); View.CloseView(); } }I retrieve the song title from the view and use it to create a new song which I pass on to an Add() method in the SongsShower. And here is my implementation for that method.
public class SongsShower { // ... public void Add(Song song) { View.Songs = new ReadOnlyCollection<Song>( new List<Song>( new [] { song })); } }That was a lot of code. Let's see where it gets me.
What? They look the same, but yet the test fails. And the reason is obviously that I do the comparison using reference equals rather than using value semantics. That is, the Equals() method that the Song class inherits from object is used, so in order to make the test pass I need to implement an Equals() method that checks the song title.
public class Song { // ... public bool Equals(Song other) { if (ReferenceEquals( null , other)) return false; if (ReferenceEquals( this , other)) return true; return Equals(other.SongTitle, SongTitle); } public override bool Equals( object obj) { if (ReferenceEquals( null , obj)) return false; if (ReferenceEquals( this , obj)) return true; if (obj.GetType() != typeof (Song)) return false; return Equals((Song) obj); } public override int GetHashCode() { return SongTitle.GetHashCode(); } }I have two Equals() methods, one for when I compare my instance with an instance that the compiler knows is of the type Song, and one where the type is unknown. Both first check if the instance that I compare against is null and then if the two instances are actually the same instance (that is, they reference the same object). Finally, I compare the song title of the two instances with each other.
Also, I have added a GetHashCode() method. A common way to compare two instances is to calculate the hash code of the two instances and, only if they are equal, use the Equals() method for further comparison. Calculating the hash code is typically much faster than using Equals(), so it is done as an optimization. Most notably, the Dictionary class use it in this way. Therefore, it is important to override GetHashCode() everytime you override Equals() and make sure that two instances that are equal always return the same hash code.
Now when I run the test it is finally green. And running the application, adding a song works great.
There are many things I don't like about the code I just wrote though, so I write a few todo index cards.
- Adding another song won't work since when I set the
Songsproperty on the view I only include the newly added song. - There is a circular dependency between the
SongsShowerand theAddSongShower(through theAddSongShowerFactory). That is, theSongsShowerdepends onAddSongShowerand theAddSongShowerdepend onSongsShower. - I don't think the
Songclass should use value semantics for comparison.
Before I start taking care of my todos, I check in my code.