[Tm4j-developers] [PATCH] Some improvements for LocatorFactory

[Florian G. H. <f.g...@gm...>]

Hello.

I've hacked up some changes to LocatorFactory-related classes and would ask
anyone who's interested to evaluate them. Just take a look -- I'd greatly
appreciate any comments. Note that this is a patch against the current
status of the CVS tree, not a TM4J distribution, which is why I only post it
to this list, not the patch management system at SourceForge. What this
patch includes:

* A LocatorFactoryBase class, serving as an abstract base class for locator
factories (who would've guessed), with an implementation of the
createLocator() method, a generic getImplementation() method returning the
implementing class for a given notation, and overloaded
removeImplementation() and registerImplementation() convenience methods with
String instead of Class arguments.
* An in-memory LocatorFactoryImpl altered to extend LocatorFactoryBase.
* An additional test in LocatorFactoryTest with a dummy Locator
implementation to make sure everything works.

I think the LocatorFactoryBase class with the implemented createLocator()
method and the abstract getImplementation() method makes locator factory
implementations a lot more straightforward. Extending LocatorFactoryBase
means that the implementing class only has to define ways to register and
remove implementations, and to return a corresponding implementing class for
a locator notation. No need to worry about actually creating locators.

LocatorFactoryBase.createLocator() uses a bit of metaclass hacking in order
to actually instantiate the locator. If you're interested, please take a
look at it and tell me how you feel about this. What it assumes is that any
valid locator implementation has a constructor with a (LocatorFactory,
String, String) signature. I think this is reasonable because locators
should not be created outside the context of a factory and should always
have a notation and address. If it can't detect such a constructor,
createLocator() throws a LocatorFactoryException with an explanatory detail
message. With this approach, it obsoletes the use of initialise() and
setFactory() on the locator altogether, and I think that's a Good Thing
because it ultimately means that we could do away with these methods, and a
locator without a factory, notation, and address would never exist. The way
things are now, we allow locators to float in free space without an owning
factory and in an unitialized state, and this IMHO is downright ugly. :-)
There are obviously some disadvantages to my alternative, too -- such as the
fact that an implementation design error (not providing a decent
constructor) will only be detected at run-time, not compile-time. This calls
for making the constructor requirement absolutely clear in the Locator
interface documentation. So we have to weigh alternatives here. Certainly no
hard feelings on my part if we leave things the way they are now.

Furthermore, I think a getImplementation() method should actually be
specified by the Locator interface. But that's a detail.

- PATCH INSTRUCTIONS (just for completeness' sake)

* Applying the patch
1. Unzip/untar the attached archive in the root directory of your "tm4j" CVS
module.
2. Run "./tm4j-net-patch-2002-02-17" This will patch the ant buildfile and
call the "tm4j-net-patch-2002-02-17" target. On Windows, you'll have to
patch the build file manually and then run that target.
3. Build the "tm4j-net-build" and/or "tm4j-net-test" target.

* Un-applying the patch
Either restore the affected files
(src/org/tm4j/net/memory/LocatorFactoryImpl.java and
src/org/tm4j/net/test/LocatorFactoryTest.java) with "cvs update -C", or
overwrite the patched files with the ".orig" files in the same respective
directories. Then build tm4j-net-build.

Hope you like it! :-)

-- Florian