<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
  <meta content="text/html; charset=ISO-8859-1"
 http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#ffffff">
<div class="moz-text-flowed"
 style="font-family: -moz-fixed; font-size: 12px;" lang="x-western">pls.
see answers inline.
<br>
<br>
On 02/23/2010 04:36 PM, Jesus M. Rodriguez wrote:
<br>
<blockquote type="cite" style="color: rgb(0, 0, 0);">-----BEGIN PGP
SIGNED MESSAGE-----
  <br>
Hash: SHA1
  <br>
  <br>
While working on Pinsetter component of Candlepin, I have the need to
  <br>
configure both Quartz (via org.quartz.* properties) and Pinsetter
itself
  <br>
(i.e. needing a list of task classnames as well as their cron
schedules).
  <br>
I went through the config code to see if I could get a better
  <br>
understanding of what is going on with the application and wondering
  <br>
why we had so many "Configuration" classes. So to relieve some of my
  <br>
confusion I'm proposing the changes listed below. Thoughts?
  <br>
  <br>
1) Rename JerseyGuiceConfiguration to CandlepinContextListener -
  <br>
&nbsp;&nbsp;&nbsp; it will go along way to remove my confusion as this is the basic
  <br>
&nbsp;&nbsp;&nbsp; entry point into the guice configuration. I understand the
lifecycle
  <br>
&nbsp;&nbsp;&nbsp; of the ContextListener over JerseyGuiceConfiguration which confuses
me.
  <br>
&nbsp;&nbsp; </blockquote>
sure.
<br>
<br>
<blockquote type="cite" style="color: rgb(0, 0, 0);">2) Rename
CandlepinProductionConfiguration to CandlepinModule -
  <br>
&nbsp;&nbsp;&nbsp; this is a guice module for candlepin. Again the current name
  <br>
&nbsp;&nbsp;&nbsp; confuses me.
  <br>
&nbsp;&nbsp; </blockquote>
no objections here either.
<br>
<br>
<blockquote type="cite" style="color: rgb(0, 0, 0);">3) Make Config
class injectable into other parts of the application
  <br>
&nbsp;&nbsp;&nbsp; so that we may get access to configuration data when needed. Plus
  <br>
&nbsp;&nbsp;&nbsp; there should only be one Config, right now I see two instances
where
  <br>
&nbsp;&nbsp;&nbsp; we created a "new Config()" class.
  <br>
&nbsp;&nbsp; </blockquote>
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.
<br>
<br>
<blockquote type="cite" style="color: rgb(0, 0, 0);">4) Roll the
ConfigFileLoader into the Config class, it's simple enough
  <br>
&nbsp;&nbsp;&nbsp; and Config basically comes from a file if it exists, so I think
  <br>
&nbsp;&nbsp;&nbsp; it should live in there.
  <br>
&nbsp;&nbsp; </blockquote>
more or less the same reasoning (testable is good). single
responsibility makes classes easier to test (and understand).
<br>
<br>
<blockquote type="cite" style="color: rgb(0, 0, 0);">5)
JPAConfiguration seems pretty generic and could be rolled into Config
  <br>
&nbsp;&nbsp;&nbsp; as well.&nbsp; As part of this make CandlepinModule call
  <br>
&nbsp;&nbsp;&nbsp; config.getNamespaceProperties("jpa.config") instead of new
  <br>
&nbsp;&nbsp;&nbsp; Config().jpaConfiguration()
  <br>
&nbsp;&nbsp; </blockquote>
at this point you already know my opinion about small classes. i like
them <span class="moz-smiley-s1" title=":)"><span>:)</span></span>. 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 (&amp; preferable).
<br>
<br>
<blockquote type="cite" style="color: rgb(0, 0, 0);">6) Move
JPAInitializer into CandlepinModule
  <br>
&nbsp;&nbsp; </blockquote>
sure.
<br>
<br>
<blockquote type="cite" style="color: rgb(0, 0, 0);">7) I'm still
working on undestanding CustomizableModules
  <br>
&nbsp;&nbsp; </blockquote>
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).
<br>
<br>
<blockquote type="cite" style="color: rgb(0, 0, 0);">8) add some
utility methods to Config i.e. getString(), getInt() etc
  <br>
&nbsp;&nbsp;&nbsp; (maybe when I needed them)
  <br>
&nbsp;&nbsp; </blockquote>
sure?
<br>
<br>
-dmitri
<br>
</div>
</body>
</html>