Open HelgeStenstrom opened 5 years ago
HelgeStenstrom
commented
5 years ago Some books that have helped me, and which have inspired my proposed changes: https://www.goodreads.com/book/show/3735293-clean-code https://www.goodreads.com/book/show/18043011-clean-architecture https://www.goodreads.com/book/show/37646821-effective-java
HelgeStenstrom
commented
5 years ago My changes were a step in the wrong direction. All fields that were moved are static, and represent global state. It’s still global state after the move. Instead the fields should be made member variables of a class does not exist yet.
goxr3plus
commented
5 years ago @HelgeStenstrom THank you for the book links :)
HelgeStenstrom
commented
5 years ago The two main problems I see in Main are connected. Nothing should depend on Main (according to the book Clean Architecture). Global state is bad (according to many books and web sources). The public fields in Main are clearly global state. I have some ideas on how to convert them into fields of a separate class, but since there are so many public fields in Main, it takes a lot of changes to change them into private fields i a different class. Rather than trying to solve that i one go, perhaps I should demonstrate how one or a couple of public fields can be treated. Perhaps you don't like that change; then we can try some other solution.
Another source of inspiration regarding this are some articles and youtube videos by Misko Hevery. He talks about how to remove Singletons (as in the Singleton patterns, because Singletons are global state, and are bad for testing), and replace them with factories and dependency injection. I'm not sure I fully understand his ideas yet, and haven't seen a large scale implementation, so it takes some experimentation.
He distinguishes singletons from Singletons. A Singleton is an implementation of the Singleton pattern (as described by GOF in the Design Patterns book). A singleton is an object which is the only object of its class. If you do "new MySpecialClass()" only once, you have a singleton. That is totally OK.
He also wants to separate classes that creates objects from classes that do work. Factories create objects. Classes with business logic shouldn't; they should get the objects given to them. This makes the business logic easier to test.
One single factory class can create many different kinds of objects. According to Hevery, it's good to have classes/objects with the same lifetime created by the same factory.
Most of the public fields in Main seem to be created at the same time, and live as long as the application is running. They have the same lifetime. So they can be created by the same factory. The created objects can be used by other classes, passed to them as arguments. Or if an object or a constructor needs too many objects, a reference to the single instance of this factory can be passed to the method or constructor.
Have a look at what Hevery writes about singeltons: http://misko.hevery.com/?s=singleton, for example http://misko.hevery.com/2009/03/30/collaborator-vs-the-factory/
goxr3plus
commented
5 years ago I need to read your comment 5 times to understand :) Thank you for such detailed and informative comments.
I am opened to completely refractor the code. I know it's not a good practice to have all the dependencies in Main class.
In the book Clean Architecture by Robert C Martin, the author recommends that nothing should depend on Main. The purpose of Main should simply be to start the application. There could potentially be several different Main classes (for different use cases), and the rest of the application would be totally ignorant about this. The theme of the book is maintainability. A change in a file that should be easy to change (such as Main) should not force a lot of other files to have to be changed.
As a consequence, there cannot be any public fields in Main.
With that in mind, I have started to move fields from Main. I have moved fields that are written only once, by the MainLoader class, to that (MainLoader) class. I have also encapsulated the fields; changed them from public to private, and introduced getter methods in the client code.
See https://github.com/HelgeStenstrom/XR3Player/tree/moveFromMain (my repo and branch) https://github.com/HelgeStenstrom/XR3Player/commit/3283ca7443973754804f3f30a4e86f6762088e34 (the commit to my repo)