tm4j-developers

[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

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

[Kal A. <ka...@te...>]

Hi Florian,

This patch looks good - it certainly cleans up implementation of new 
Locator types for the in-memory backend. I am not sure how easy it will be 
to port this to the Ozone backend. The LocatorBase abstract class should 
probably be modified too (to remove setFactory() and initialise(), although 
it might be necessary to retain something like this for the 
OzoneLocatorBase class (because it is easier to have no-arg constructors 
with Ozone objects) but the necessary casts that would be required would be 
localised within the OzoneLocatorFactoryImpl class, so that shouldn't be a 
problem.

Could you apply the patches and check them in ?

Cheers,

Kal

At 18:19 17/02/2002 +0100, Florian G. Haas wrote:
>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

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

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

Hello.

[Kal]
| [...]
| Could you apply the patches and check them in ?

I've done this, and from what ViewCVS tells me, everything shows up fine in
the repository. I have not received a syncmail message on the tm4j-commits
list, though... perhaps a config error on SF's part, or a simple delay. You
might want to do a "cvs update" in src/org/tm4j/net if you want to make sure
you've got the latest versions.

Later,
-- Florian

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

[Kal A. <ka...@te...>]

At 23:07 18/02/2002 +0100, Florian G. Haas wrote:
>Hello.
>
>[Kal]
>| [...]
>| Could you apply the patches and check them in ?
>
>I've done this, and from what ViewCVS tells me, everything shows up fine in
>the repository. I have not received a syncmail message on the tm4j-commits
>list, though... perhaps a config error on SF's part, or a simple delay.

Probably just a delay - I've got the commit messages from SourceForge.

>You
>might want to do a "cvs update" in src/org/tm4j/net if you want to make sure
>you've got the latest versions.

Will do!

Cheers,

Kal