On 2/11/10 2:41 PM, Aaron Kimball wrote:
There are an enormous number of examples of the following line in user-side code:Configuration conf = new Configuration(); ... This is going to need to still work transparently after any refactoring. The new Configuration in this case needs to be populated with values from the appropriate defaults files. Maybe instead of using a singleton ConfigurationFactory (e.g. ConfigurationFactory.getConfigFactory().newConfiguration()), you could instead have Configuration's constructor use a service locator to get a configuration "populator." e.g: class ConfigServiceLocator { static ConfigServiceLocator getServiceLocator(); /** this class is a singleton */ ConfigService getConfigService() void setConfigService(ConfigService); } class ConfigService { getAllThatStuffThatWasPreviouslyStaticState() } then in Configuration#Configuration() { initWith(ConfigServiceLocator.getServiceLocator().getAllThatStuffThatWasPreviouslyStaticState()); } Then there can be a static block which initializes the ConfigServiceLocator with a default ConfigService instance that does everything as normal. For testing, though, you could instantiate a new ConfigService, put whatever state you want in it, and then update the ServiceLocator to use this new instance instead. Does that sound like a helpful alternative? - Aaron
I definitely see what you're getting at here and it is an alternative. My feeling is that there's still a weird static dependency that doesn't do exactly what you (I) want. There was someone once who said you should never modify the behavior of a constructor. A constructor should always produce a simple new instance. If you want different behavior, make the constructor private and use a static method to indicate non-standard behavior. I tend to think this is a good take. By not embedding static-dependent behavior you enable people to do more with dependency management. In the above example, there's a hard coded dep on ConfigServiceLocation within Configuration#Configuration with no point of interjection which is ultimately what I'm trying to avoid.
I'm sure this will end with there being simply too much existing inertia to change it, but it hurts us.
Now, all of that said, I think you could introduce the ConfigurationFactory and still allow people to directly create Configuration instances directly. The problem is that the Configuration constructor has the overlay logic where as it should probably be at least in a static method of Configuration (i.e. outside of the constructor proper).
One could do a phased replacement by introducing the CFactory, pushing code internally toward it, and eventually removing the layering within the Configuration, instead having the CF handle it (which I think is a better place for it), or at least opting for a static factory method within Configuration so people may use their own Factories (or Spring, for instance) externally.
Just jumping back for a second, I think the larger issue is the tendency to use static state in the code base. This is indicative of a larger pattern. I think that much of the code base can be simplified by having simpler objects at the potential expense of having a few extra classes.
I wanted to judge the relative reaction to these kinds of changes. I'm happy to submit patches even if they get rejected if it means people see the type of problem I'm trying to get at.
Thanks for your comments, Aaron! -- Eric Sammer e...@lifeless.net http://esammer.blogspot.com
