Config refactoring proposal
Dmitri Dolguikh
dmitri at redhat.com
Tue Feb 23 21:24:00 UTC 2010
pls. see answers inline.
On 02/23/2010 04:36 PM, Jesus M. Rodriguez wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> While working on Pinsetter component of Candlepin, I have the need to
> configure both Quartz (via org.quartz.* properties) and Pinsetter itself
> (i.e. needing a list of task classnames as well as their cron schedules).
> I went through the config code to see if I could get a better
> understanding of what is going on with the application and wondering
> why we had so many "Configuration" classes. So to relieve some of my
> confusion I'm proposing the changes listed below. Thoughts?
>
> 1) Rename JerseyGuiceConfiguration to CandlepinContextListener -
> it will go along way to remove my confusion as this is the basic
> entry point into the guice configuration. I understand the lifecycle
> of the ContextListener over JerseyGuiceConfiguration which
> confuses me.
sure.
> 2) Rename CandlepinProductionConfiguration to CandlepinModule -
> this is a guice module for candlepin. Again the current name
> confuses me.
no objections here either.
> 3) Make Config class injectable into other parts of the application
> so that we may get access to configuration data when needed. Plus
> there should only be one Config, right now I see two instances where
> we created a "new Config()" class.
internally it behaves as a singleton. i tend to avoid static methods, as
they are impossible to test (or make components that depend on them
untestable). while current appearance maybe deceiving in regards to its
internals, i don't think it's a major point of confusion. in short, i
think it's better to keep the class free of static members.
> 4) Roll the ConfigFileLoader into the Config class, it's simple enough
> and Config basically comes from a file if it exists, so I think
> it should live in there.
more or less the same reasoning (testable is good). single
responsibility makes classes easier to test (and understand).
> 5) JPAConfiguration seems pretty generic and could be rolled into Config
> as well. As part of this make CandlepinModule call
> config.getNamespaceProperties("jpa.config") instead of new
> Config().jpaConfiguration()
at this point you already know my opinion about small classes. i like
them :). i would strongly suggest not to use strings in actual code to
retrieve bits of configuration. actual methods are easier to
locate/maintain. at this point (pls. correct me if i'm wrong) we have a
limited number of namespaces, which makes the approach (methods vs.
string constants) tractable (& preferable).
> 6) Move JPAInitializer into CandlepinModule
sure.
> 7) I'm still working on undestanding CustomizableModules
this allows to define what external modules should be loaded (via
candlepin.conf file). needed for implementations of extension points by
third parties (such as IT).
> 8) add some utility methods to Config i.e. getString(), getInt() etc
> (maybe when I needed them)
sure?
-dmitri
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/candlepin/attachments/20100223/21902c07/attachment.html>
More information about the candlepin
mailing list