Adding getInstance() to the configurator will either force us to cast in
a bunch of different places or to expose the GlobalConfiguratorImpl's
api to the rest of the world (which I don't want to do because they are
applicable ONLY to global configurator. And it won't lock us into an
API we may have to expand later.
As for simply putting the Boolean on the request map, either I'll have
to make a protected constant on the Configurator interface (which has no
bearing on any of the configurators except the global configurator so it
shouldn't go into the ancestor) or I add a porotected (isDisabled)
method (which, again, is only applicable to the GlobalConfiguratorImpl
and therfore shouldn't do into it's Ancestor).
I've never been one to include a feature into an interface or a class
that is applicable in only one instance of that class because it
violated basics OO design principals. The only other option I see here
is to define the constant used to store the boolean in BOTH classes and
hope they remain in sync, but I've never been a big fan of that either.
Scott
Adam Winer wrote:
Scott,
What about the far simpler approach of:
public static final void disableConfiguratorService(ExternalContext
external)
{
external.getRequestMap().put(..., Boolean.TRUE);
}
public static final boolean
isConfiguratorServiceDisabled(ExternalContext external)
{
return Boolean.TRUE.equals(external.getRequestMap().get(....));
}
Eliminates all the introspection and ugly hidden impl dependencies.
*And*, add to Configurator:
public static Configurator getInstance(ExternalContext)
{
... use code like RequestContextFactory.getFactory(),
but instead of instantiating a hardcoded file, use
ClassLoaderUtils.getServices("org.apache.myfaces.trinidad.config.GlobalConfigurator");
and calling configurator.init(...) to boot
}
... with, of course,
META-INF/services/org.apache.myfaces.trinidad.config.GlobalConfigurator
pointing
at GlobalConfiguratorImpl. And put the onus on
GlobalConfiguratorImpl's init() to load (and initialize)
all the other Configurators.
I think this'd also eliminate any need for "isInitialized()" - put
the onus on the code that retrieves (and therefore might
create) the instance.
-- Adam
On 12/19/06, Scott O'Bryan <[EMAIL PROTECTED]> wrote:
Adam,
EVERYTHING will have to cast to it, since the entry point does not
return this class, but it's Configurator equivalent. Reflection and
casting are among the slowest operations in the Java language. And, we
do need access to this from the API package as well as the Impl (unless
you want me to move the resource servlet to the impl). These are facts.
Can it be done? Yes. But it's really ugly. So I'll tell you what.
I'll make an additional patch for this. Take a look at both and you
decide which is superior.
Scott
Adam Winer wrote:
> Scott,
>
> You're explaining very well why you want to put this in IMPL.
> And why you need a different instance that handles this on
> behalf of all other configurators. You're not yet explaining
> why you need a whole class to accomplish this, as opposed
> to a standard decorator or CoR pattern, etc. I just don't get it.
> This one global instance is going to load all other instances,
> and invoke all other instances. NO ONE needs to cast to it -
> it is all powerful since it is the first (and only) entry point,
> and it can decide whether all the others will run or not.
>
> (And "dog slow"? C'mon, you're exaggerating. Hugely.
> And describing one potential implementation of one
> potential design.)
>
> Yes, I do fight against adding extra code to our
> API. For good reason, ya know! Less public API is
> good. And, it really worries me when I see a design
> that is unlike all the other designs I've seen for this
> sort of pattern. I immediately get a gut feeling that
> it's not really necessary.
>
> -- Adam
