Next, I'm going to get rid of the logic that hides in the Thingy constructor. Here is the Thingy code as it looks before the change:
public class Thingy { public Thingy(ISongsView view) { view.ShowView(); } }The plan is to move the call to ISongsView.ShowView() from the constructor into a method that is called after Thingy has been created.
When refactoring, I always try to take as small steps as possible. When making changes, I prefer to have the code not compiling for no more than a few seconds. After each small change, I run the unit tests to make sure I haven't broken anything. If the tests are red, I revert my last change to get back on safe ground before I make further changes.
With modern IDE:s, many refactorings can be made as atomic operations. That is, each change is made in a way so that the code is always compiling and working, and the code is not broken for even a single second. While these refactoring tools are thoroughly tested, it does happen that something does get broken though, so I still make sure to run the tests after each refactoring step.
I will show detailed steps for this refactoring, but for most future changes I will be much briefer in my explanations.
First, I would like to introduce a property where I store the reference to ISongsView so that I can use it in the method. There are many ways of doing this, but with my refactoring tool (ReSharper) I do the following:
- Introduce field: convert view into a private field.
- Encapsulate field: convert the field into a private auto property.
Here is the result:
public class Thingy { private ISongsView View { get; set; } public Thingy(ISongsView view) { View = view; View.ShowView(); } }I run the test, and it is still green. Next, I extract the ShowView() call into a public method that I call ShowView().
public class Thingy { private ISongsView View { get; set; } public Thingy(ISongsView view) { View = view; ShowView(); } public void ShowView() { View.ShowView(); } }The test is still green. Finally, I want to move the ShowView() call from the constructor to the code that instantiates Thingy. I don't know how to do this in an atomic operation using my refactoring tool, so I have to do it manually. I remove the call from the constructor:
public class Thingy { private ISongsView View { get; set; } public Thingy(ISongsView view) { View = view; } // ... }And I add it to the test:
[TestFixture] public class SongsFixture { [Test] public void Songs_window_is_displayed_when_the_application_starts() { var view = new SongsViewFake(); var thingy = new Thingy(view); thingy.ShowView(); Assert.That(view.HasBeenShown, Is.True); } }I run the test and it's green.
But I'm not done yet. The dangerous thing with doing these manual refactorings is that it's easy to forget something. What I forgot this time is to call ShowView() in the Main() method. The mistake was easy to spot and quick to fix, but as the application grows in size, these things will be easier to miss. This time, the mistake was in untested code, and the lesson to be learned is that we should try to reduce the amount of untested logic.
Here is the changed 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()); thingy.ShowView(); System.Windows.Forms.Application.Run(thingy); } }I run the application and it works fine. However, when closing it (using the X in the upper right corner) I notice a problem: the application hangs and won't close properly. After some investigation I find out that in order to close the application I must call the ApplicationContext.ExitThread() method, and I must do it after the message loop has started. That is, I must listen to the SongsView.FormClosed event and call ExitThread() when it's raised.
I check in my code into the version control system and start my next test.