bruno-brant
commented
4 years ago Another thing to add is the enum that defines the possible services. Well, the enum doesn't prevent us from doing a mismatch.
Take a look, for instance, here, AddToCache:
What prevents us from, let's say:
ILightRepository repo = null;
AddToCache(WorkBenchServiceType.Event, repo)Nothing (also, nothing prevents us to insert a null there, since null checking isn't done).
So this shouldn't be a public interface if we want to actually restrict what a user can insert there. But then again, this is just because #39, right? We shouldn't be really restricting stuff this way.
And this flaw happens here as well:
Inside this code, there's a (T)service, a code that can very easily fail.
So, one more vote to remove this class once and for all.
There's a large number of issues with Liquid's service locator,
WorkBench:It's static, which forbids a number of good OOP practices such as inheritance; It also makes testing very difficult (#74) because, once you exercise it, you can no longer change anything. It's also vulnerable to thread-safety issues because every thread will access the same data (although, I must admit, this is unlikely, since you should load instances only at application startup - after that you only read from it).
It's a glorified static dictionary. The whole interface is about restricting what can be loaded into it, which is something that the dictionary would do natively (minus the protection over setting something twice, which I wonder if it is really helpful...). The only thing that matters is that it will call Initialize on a service on the first read, which is a good feature.
It restricts what can be used as a service.
It violates SOLID in every manner I can think, well, besides SRP.
I think a redesign is in order.