incubator-adffaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam Winer" <awi...@gmail.com>
Subject Re: [PORTAL] Changes and API review?
Date Wed, 20 Dec 2006 01:28:21 GMT
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


On 12/19/06, Scott O'Bryan <darkarena@gmail.com> wrote:
> It's API bloat and I'm also going to have to store some extra privates
> on some of these classes as well as expose some additional api's to
> support this.  I ran into another issue with not implementing the Global
> configurator.  Take a look at this code.
>
> When used inside of GlobalConfiguratorImpl, the code looks something
> like this:
>
>   public void disableConfiguratorServices(final ServletRequest request)
>   {
>     if(Boolean.TRUE.equals(request.getAttribute(_IN_REQUEST)))
>     {
>         throw new IllegalStateException("Request is already begun.");
>     }
>
>     request.setAttribute(_IN_REQUEST, _DISABLED);
>   }
>
> Now all the IN_REQUEST stuff is basically there to handle proper cleanup
> and some API enforcement when calling into the GlobalConfigurator's
> getExternalContext method.  It is only exposed through the
> GlobalConfiguratorImpl.  There is no reason for anyone to know about it
> outside of the impl package and even then in only a few locations.
> Really though, people should keep their hands off of this.  If I have on
> the normal configurator a getGlobalConfigurator method which returns a
> configurator, I have to go though much hokeyness in order to tell
> whether I'm in a request or not.  Because API cannot depend on classes
> in Impl, I'll need to use reflection to get at the private methods
> unless I want to expose this as an API on the configurator in general
> and I don't think we want to do that.
>
> We can also just skip the validity checking and modify the attribute,
> but I would think that if someone is trying to disable the Configurator
> and the response has already begun, then they should be notified as soon
> as possible before hokey little errors creep up.  There is much in the
> impl package that depends on the instance of the GlobalConfiguratorImpl
> and it's silly in my opinion to have to cast it every time we need to
> use an arbitrary function.  And when we need functionality from this guy
> in the API package (like with the ResourceServlets) the implementation
> begins to look REAL ugly.
>
> So my question is why should we have to go through all this reflection
> and casting, making this system dog slow, rather then supporting the
> proper API's.  The amount of work I've had to put in simply to code
> "around" these issues is amazing.  And I still havn't heard any
> compelling reason why THIS implementation is not good.
>
> Scott
>
> Adam Winer wrote:
> > Well, in this specific instance, it therefore doesn't "bloat" every
> > configurator, since it only appears in one location.
> >
> > -- Adam
>
>

Mime
View raw message