• Post Reply Bookmark Topic Watch Topic
  • New Topic

Is this code sane?  RSS feed

 
Daniel Prene
Ranch Hand
Posts: 241
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
In breif: It loads a properties files with mappings between names and classes , like foo = org.foo.Bar, and then creates a hashtable of constructors and strings to be used by a factory to create new instances of the objects identified by the names in the property file.

Heres my code:


Is what I'm doing sane/practical? Once I figure out how to parse xml I'll come up with a way to specify the constructor args within a xml file and do some other stuff to make it safer. Any comments?

Thanks for atleast reading this post,
-D.P.
 
Tony Morris
Ranch Hand
Posts: 1608
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Originally posted by Daniel Prene:
In breif: It loads a properties files with mappings between names and classes , like foo = org.foo.Bar, and then creates a hashtable of constructors and strings to be used by a factory to create new instances of the objects identified by the names in the property file.

Heres my code:


Is what I'm doing sane/practical? Once I figure out how to parse xml I'll come up with a way to specify the constructor args within a xml file and do some other stuff to make it safer. Any comments?

Thanks for atleast reading this post,
-D.P.


A highly subjective question. Here are some notables:
- you're using -source 1.5, and Class is parameterised, yet you don't use a
parameter.
- you assign locals to null.
- you have a static method that should be an interface operation declaration.
- you use a Hashtable reference (should be Map).
- you use Hashtable (should be HashMap under -source 1.5, which is implicitly -target 1.5).
- you have locals that are write once, that are not declared final.
- you use an Enumeration where you should use a foreach loop.
- you declare to catch Exception.
- you use a cast where you should be using generics for the Enumeration (I assume you are ignoring that compiler warning - it means something).
- you have locals that exceed required scope.
- you dereference public operation parameters (check for null first and fail immediately if appropriate before dereferencing).

Once you fix up the contractual interface (that should not be a static method), *how* it is done becomes entirely irrelevant. Express your requirements (what you are doing) first, before your implementation (how you are doing it).
 
Stan James
(instanceof Sidekick)
Ranch Hand
Posts: 8791
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The idea of building a factory like this is pretty good. It lets you add new implementation classes through configuration instead of coding them into your system, a classic way to replace a long if-else structure with polymorphism.

I've often done this kind of object factory by storing the fully qualified class name or by storing a singleton instance of the objects we're interested in. I usually have a short, easy to remember logical name for the map key rather than a concrete class name. That helps reduce coupling between the client that needs these classes and the actual implementations.

Tony's points are all good for code style and/or correctness. One good technique is to declare, initialize and use variables as close together as possible. If you moved these bits together instead of three distant lines ...

... you might see that loader is only used in these two lines. Tony says the variable is exposed to a larger scope than is necessary: the whole method. That's a hint that these lines might be happier in a method by themselves or maybe along with the actual properties.load(). When you move things close together, these opportunities become much easier to see and easier to "refactor" by moving code around.

All in all, be encouraged. The configuration-driven factory is good. Read over the style and goodness tips and show us what you make of it all.
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!