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 Thu, 21 Dec 2006 22:00:41 GMT
On 12/21/06, Scott O'Bryan <darkarena@gmail.com> wrote:
>
> Adam,
>
> Well, you basically implemented one of the solutions I said I didn't
> like earlier, but oh well.  And there are a number of places you need to
> cast.  So the concerns are still valid.


Well, I don't get that claim, as I didn't add a single cast in my
patch at all, and removed references to the impl class, so I can't imagine
how I've made things worse.  And I didn't really see any casts that were
already there.

The one question I do have is why does getInstance take in an
> ExternalContext?  I'm assuming it's because your executing the init
> inside of the getInstance object and I'm not sure this is the correct
> place for this.  My hope in all of this was to be able to explicitly
> handle initialization of this object.  Plus, nine times out of ten, the
> ExternalContext object is ignored in the call to this method.


A create + initialize construct is more robust than a create-and-hope-
it-gets-initialized-by-someone-else construct.

-- Adam



Either way, don't see as I really have the time to argue.  So is it
> acceptable to everyone?
>
> Also, there is no
>
>
> Adam Winer wrote:
> > Scott,
> >
> > OK, well, I just went ahead and implemented what I was trying
> > to say, to see if I'd run into the problems you're describing.  I
> > didn't...
> > (It's possible I've broken something in portlet land - I only tested
> > the changes in a servlet environment.)
> >
> > On 12/21/06, Scott O'Bryan <darkarena@gmail.com> wrote:
> >> Adding getInstance() to the configurator will either force us to cast
> in
> >> a bunch of different places
> >
> > No, it doesn't.  No casts were required at all, much
> > less in a bunch of places, and most of the code now
> > doesn't even have references to the impl class at all.
> >
> >> 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).
> >
> > See the code checked in, which does not suffer these probems.
> >
> >> 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.
> >
> > Again, see the code checked in.
> >
> > -- Adam
> >
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message