[Geotools-devel] Re: Proposed refactoring of AbstractFeatureStore

[Jody G. <jga...@re...>]

Ian:

Sorry for punting this email onto the mailing list without talking to 
you first.

It is just you had so many good ideas I thought others might want to 
play too. Especially since getBounds() optimizations keep cropping up 
everywhere.

So a quick plan based on your ideas:
- Add getListenerManager() to DataStore as public API
- Add CachedFeatureResults - like DefaultFeatureResults but with the 
Bounds code chris keeps writing
- Kill all the horrible Anonymous inner classes we can

And once again, a very smart move to untangle the 
FeatureSource/FeatureResults mess.

Thanks,
Jody

Ian Schneider wrote:

>Hi Jody,
>
>While working with shapefile I found a monstrosity of anonymous inner
>classes tacked on in attempts to correctly return the bounds from the
>FeatureSource. Digging further, I found the AbstractDataStore provides
>the same inner classes, minus the bounds functionality. I failed to
>discover the need for the abstraction found in AbsctractFeatureLocking,
>AbstractFeatureSource, and AbstractFeatureStore.
>  
>
The number of classes represent various attempts to "optimize" and cache 
the results of queries (such as get bounds).  Really I want to have two 
classes for DataStore implementations to choose from:
- DefaultFeatureResults
- CachedFeatureResults

Where they really only use DefaultFeatureResults - when things have gone 
wrong and they are debugging.

Currently only bounds has had any effort put into optimizing it - 
getBounds() is a bottle neck for Chris as I understand it. What really 
is needed is for CachedFeatureResults to be written once rather then 
several times as is happening now.

>Why is getDataStore() abstract, when all implementations I found simply
>return what could be a final DataStore.
>  
>
getDataStore() was abstract to allow JDBCDataStore - which does not 
extends AbstractDataStore to make use of it.
It should really be handled with a constructor I admit.


>Why is add/removeFeatureListener abstract... I found JDBCFeatureStore
>which uses its DataStores listenerManager to attach the listener to, but
>why not simply make the default behavior something like:
>
>public void addFeatureListener(FeatureListener l) {
>  getListenerManager().addFeatureListener(l);
>}
>  
>
The answer is that when I set up the api - getListenerManager() was not 
part of it yet.  I am not sure I wanted to tie the hands of all 
implementations with the
use of my listener manager. PostgisDataStore for instance may be able to 
detect updates from other applications and provide notification to 
geotools2. Oh wait that notification process could just use 
getListenerManager and call the fireX functions.

Okay that is a great idea :-)

If I can place getListenerManager() as part of the public DataStoreAPI - 
we can make use less Anonymous classes. Is this a worthwhile trade off?

>Where the listenerManager can simply be set in the constructor...
>
>I agree anonymous inner classes make sense sometimes, but this code is
>bulky, verbose, and harder to understand than a simple support class
>would be.
>
>I'd be happy to do the work, but I thought I'd ask first in case I
>missed something in the design...
>  
>
Nope you seem to be right on the money. Great Idea Ian - and thanks for 
letting me think it over.

Before we go nuts though we should have this conversation on the public 
email list.

Jody