In extension of my work on refactoring the HarvestScheduler assignment in the unit test class, which I described in the More informative naming, less inline comments thread, I've struggled with the instantiation, assignment and cleanup of the HarvestSchedule'er. It has finally dawned on me that the the problem is 2 conflicting characteristics of the HarvestScheduler:
- The class is meant to be a Singleton . This means that only one instance of the class should exist a one time, which the Singleton pattern enforces by hiding the possibility of construction/creating instances from users of the class.
- The class extends the CleanupIF: This interface implies that it is possible to invoke this operation when closing down the class. This works fine in normal class, where the operation is symmetric to the construction of the class. Eg. an objects life cycle is: create -> living -> cleanup.
The conflict between the 2 aspects of the HarvestScheduler arises because a Singleton pattern hides the object creation control from its API control, where the CleanupIF is logically a functionality associated with the destruction of a object.
The problem can be seen in the follow HarvestScheduler code:
This code enables a user to create multiple instances of the HarvestScheduler as simple as this:
This means the that the contract for the Singleton-pattern has been broken. It also means that application behaviour will now depend on when the created HarvestScheduler instances are garbage collect.
What is even worse is that this is used in the unit test to emulate contruction of a new HarvestScheduler instance in each test case. Eg.
Note: The HarvestSchedulers close call delegated to the Cleanup method.
Conclusion: The CleanupIF should never be used on Singletonic classes as this leads to complicated and brittle code, and may lead to inconsistent application behavior. A solution could be to split the HarvestScheduler into a plain thread execution scheduler singleton, and a 'JobManager' class containing the business logicextending the CleanupIF. Another solution might be to replace the singleton pattern with a Inversion-of-control pattern, delegation the creation control to a class/container external to the HarvestScheduler and its users.