My other todo note is about that the Application class has a reference to AddSongViewFactory only to pass it on as a parameter to SongsShower. And what's wrong with that? Well, one thing is that it can be quite cumbersome. Currently I have three parameters passed into the Application constructor, making both the caller side and the constructor definition quite long and hard to read. And I'm only starting the development, so I assume the problem will grow worse.
But that's a small problem compared to the real one. The problem with a lot of legacy systems is that they are too tightly coupled: too many things know about too many other things. When you make a change in one place it ripples through the system, affecting things you thought were unrelated. So when it comes to creating a maintainable design, it is desirable to make classes as decoupled as possible. In this case, the Application class has no benefit in having a reference to the AddSongViewFactory class, and therefore I want to try to remove it.
I find it hard to get the whole picture though, now that I have so many classes, and it's difficult to understand what the problem really is. So it's time to draw a few boxes to get a better understanding of the design.
There are a lot of arrows going into the AddSongViewFactory, that is, there are many classes that know about it: ApplicationFake, Application, SongsShower and AddSongShower. By introducing the AddSongShower I seem to have aggravated the situation since there are now two classes, Application and SongsShower, that know about AddSongViewFactory only to pass it on to some other class.
One solution to this problem would be to introduce some kind of global variable that keeps track of the AddSongViewFactory. I could set the variable in the test and in the application start-up respectively, and then read the value in the SongsShower where I need it. There are plenty of software patterns that have to do with keeping track of such a global state, including the Registry Pattern, which resembles the Singleton Pattern.
While this might be a convenient short-term solution, there are some negative sides to it. My biggest objection is that it makes any class that uses the global variable (in this case, the SongsShower) a black box. On the outside, looking at the public interface of the class, there is no way you can see that it is using a global variable. If you want to use it, chances are that you will miss to set the variable, or that someone else has given it the wrong value, and then you will get very surprising results.
Also, the AddSongViewFactory is most likely not the last class that I will want to have different versions of in test and production, and if I use global variables for all such classes I will end up with quite a large and hard to maintain global state.
The good thing is that it is quite easy to avoid these disadvantages. The solution I will use is something that I have used before, but then I used it to solve another problem. The solution is to introduce a factory. Last time I used it to control what version of the view fake is created. Here, I will use it to remove unnecessary dependencies between classes by hiding them inside the factory. Basically, I'm going to create a class, AddSongShowerFactory, that can create AddSongShowers, and that contains all information necessary to do so.
These two types of factories are quite different from each other. The first kind, the one that creates the view, is abstract and also returns an abstract type: the code that uses the factory references an interface (IAddSongViewFactory) and what is created by the factory is also referenced to as an interface (IAddSongView). The kind of factory I will create now is concrete, and it also returns a concrete type. There are no interfaces here.
The normal thing would be to let the factory have a method called Create() that I call to create the shower before showing it, like this:
AddSongShowerFactory.Create().Show();I will however probably always use the Create() and Show() methods together, so therefore I let the factory only have a Show() method that does the creation internally, to make the client code simpler and to reduce duplication.
AddSongShowerFactory.Show();Here is what the factory looks like.
public class AddSongShowerFactory { private IAddSongViewFactory AddSongViewFactory { get; set; } public AddSongShowerFactory(IAddSongViewFactory addSongViewFactory) { AddSongViewFactory = addSongViewFactory; } public void Show() { new AddSongShower(AddSongViewFactory).Show(); } }Next, I replace the references to IAddSongViewFactory in Application and SongsShower with a reference to the AddSongShowerFactory and create the AddSongShowerFactory in the ApplicationFake.
public class ApplicationFake { // ... public void Start(Action<SongsViewFake> showAction) { var asserter = new ViewShownAsserter<SongsViewFake>(showAction); var addSongViewFactory = new AddSongViewFactoryFake(); var addSongShowerFactory = new AddSongShowerFactory(addSongViewFactory); var view = new SongsViewFake(asserter.ShowAction, addSongViewFactory); var application = new Application(view, CloseAction, addSongShowerFactory); application.Start(); asserter.Verify(); } }I do the corresponding change in Program as well. And here is what SongsShower looks like, using the new factory:
public class SongsShower { private IAddSongShowerFactory AddSongShowerFactory { get; set; } // ... private void OnAddButtonClick() { AddSongShowerFactory().Show(); } }Quite elegant, if I may say so. There are no strange details here, nothing that do not belong in the class, but just something that I can delegate the showing of the Add Song Window to. Let's have a look at how this changed the design.
There are three arrows going into the IAddSongViewFactory, one less than before, which is good. But still I wonder if it is not one too many. The AddSongShowerFactory arrow is there only to pass it as a parameter to the AddSongShower constructor, that is, a bad kind of reference. I can't remove it though.
So I'm wondering if I can do something about AddSongShower instead. What if I let AddSongShowerFactory create the view and then pass it into the AddSongShower? That would reduce the number of classes knowing about IAddSongViewFactory, but increase the number of classes knowing about IAddSongView, since AddSongShowerFactory would now create the view only to pass it on. The ones that know about the view would however do it for a good reason, so I'm going to try that solution.
public class AddSongShowerFactory { // ... public void Show() { var view = AddSongViewFactory.Create(); new AddSongShower(view).Show(); } } public class AddSongShower { private IAddSongView View { get; set; } public AddSongShower(IAddSongView view) { View = view; } public void Show() { View.OnCancelButtonClick += View.CloseView; View.ShowView(); } }I think I like it better, but I'm not convinced. I leave it like this for now, though, sure of that I can easily change it later.
Looking that the design, it seems Application now knows about the AddSongShowerFactory, only to pass it as a parameter to SongsShower. I use the same solution here: introduce a SongShowerFactory.
This is what it currently looks like where the view is shown.
public class Application : ApplicationContext { // ... public void Start() { View.OnViewClosed += () => CloseAction( this ); new SongsShower(View, AddSongShowerFactory).Show(); } }To replace this code with a factory, I need the factory to have references to the view and the AddSongShowerFactory. So I create and use the factory.
public class Application : ApplicationContext { // ... public void Start() { View.OnViewClosed += () => CloseAction( this ); new SongsShowerFactory(View, AddSongShowerFactory).Show(); } } public class SongsShowerFactory { private ISongsView View { get; set; } private AddSongShowerFactory AddSongShowerFactory { get; set; } public SongsShowerFactory(ISongsView view, AddSongShowerFactory addSongShowerFactory) { View = view; AddSongShowerFactory = addSongShowerFactory; } public void Show() { new SongsShower(View, AddSongShowerFactory).Show(); } }The tests are green. Next I extract the creation of the SongsShowerFactory to Program and ApplicationFake and pass it in as a parameter into Application.
public class Application : ApplicationContext { private SongsShowerFactory SongsShowerFactory { get; set; } // ... public Application(ISongsView view, Action<ApplicationContext> closeAction, SongsShowerFactory songsShowerFactory) { View = view; CloseAction = closeAction; SongsShowerFactory = songsShowerFactory; } public void Start() { View.OnViewClosed += () => CloseAction( this ); SongsShowerFactory.Show(); } } public class ApplicationFake { // ... public void Start(Action<SongsViewFake> showAction) { var asserter = new ViewShownAsserter<SongsViewFake>(showAction); var addSongViewFactory = new AddSongViewFactoryFake(); var addSongShowerFactory = new AddSongShowerFactory(addSongViewFactory); var view = new SongsViewFake(asserter.ShowAction, addSongViewFactory); var songsShowerFactory = new SongsShowerFactory(view, addSongShowerFactory); var application = new Application.Application(view, CloseAction, songsShowerFactory); application.Start(); asserter.Verify(); } }Let's have another look at the design to see what happened.
Now it looks better. One thing is that SongsShowerFactory has a reference to AddSongShowerFactory only to pass it along to SongsShower, but that's ok since it is a factory and factories typically need extra information in order to create other types.
There is a nice symmetry evolving as well. Both views have a shower and both have factories for the shower. The only big difference is that the view is still created in different ways.
There are a lot of types in action now. For the tiny application that I have so far it is clearly overkill to have that many classes and interfaces. But the plan is not to stop here, but to let the application will grow and have more functionality. In order for me to have a nice design, not only now, but also after five or fifty stories it is important to take care of the trash when it appears.
The tests are still green, and the application works fine, just like before, so I check in the code.