incubator-adffaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Scott O'Bryan" <>
Subject Re: [PORTAL] Changes and API review?
Date Thu, 21 Dec 2006 16:56:26 GMT
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.


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 <> 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

View raw message