On Wed, Mar 9, 2011 at 11:54 AM, Maarten Billemont <lhun...@gmail.com> wrote:
> On 09 Mar 2011, at 18:56, Igor Vaynberg wrote:
>>
>> On Wed, Mar 9, 2011 at 1:15 AM, Maarten Billemont <lhun...@gmail.com> wrote:
>>>> yep, calling overridable methods from constructors is bad - you just
>>>> made the case for making page.oninitialize() final...
>>>
>>> No, I've made the case for this issue. Either add() should be disallowed
>>> from the constructor (meaning onInitialize won't get called from it),
>>> onInitialize should be made final (meaning onInitialize will do nothing
>>> when it gets called), or Page should stop calling initialize() as a result
>>> of a component being added, and behave like any other component does.
>>> Would you mind addressing this third option? What exactly is the reason
>>> for Page behaving in this special way?
>>
>> page.add() is where the oninitialize cascade is initiated because that
>> is the earliest point at which the page instance is known. doing it
>> from there ensures that the page is more or less fully constructed
>> after it is instantiated allowing you to call methods on it.
>
> Why should Page be a special case? Why not for Panels too?
oninitialize() is an atomic callback that notifies components when the
page becomes available. the page is special because since a page is
always available to itself it doesnt need such a callback. how is that
for a rationalization... :)
you are mixing a generic post-construct callback with a more specific
usecase of oninitialize()
> In fact, if you take away this special casing for Page, this issue probably
> wouldn't exist at all and we could go back to letting the constructor-favored
> users use constructors and the others use onInitialize.
that may very well be true. but weve come a long way from the
beginning of this thread which was: forbid add() in constructors.
>
>>
>> eg MyPage page=new MyPage();
>> page.foo() { where foo manipulates state of page's child components }
>>
>> i know that is not the style you prefer to use in your applications,
>> but that doesnt mean no one should be able to use it.
>
> This issue is about the fact that this style, which I've argued is flawed, is
> basically causing you to finalize onInitialize, meaning no one will be able
> to use the alternative. It cuts both ways, and I really don't see why
> onInitialize should come out as the looser when it condones safer code.
>
>>
>>>
>>>
>>> On 09 Mar 2011, at 09:15, Igor Vaynberg wrote:
>>>>
>>>>> Firstly, it's my opinion that you really shouldn't be doing anything to
>>>>> components directly, especially not from outside your class. As for why,
>>>>> see Encapsulation and Law of Demeter.
>>>>
>>>> neither of the two apply because nameeditor explicitly exposes the two
>>>> components as part of its contract via public getters.
>>>
>>> Allow me to remind you:
>>>
>>>> More formally, the Law of Demeter for functions requires that a method M
>>>> of an object O may only invoke the methods of the following kinds of
>>>> objects:
>>>>
>>>> • O itself
>>>> • M's parameters
>>>> • any objects created/instantiated within M
>>>> • O's direct component objects
>>>> • a global variable, accessible by O, in the scope of M
>>>
>>> Making your components available via getters does not excuse anything. The
>>> list does not include "invoke methods on any objects created by any objects
>>> created within M", for good reason.
>>
>> yes, and if we follow this "good" reason then i would have to add the
>> following methods:
>>
>> nameditor {
>> setfirstnamevisible() {}
>> setlastnamevisible() {}
>> setfirstnameenabled() {}
>> setlastnameenabled() {}
>> setfirstnamerequired() {}
>> setlastnamerequired() {}
>> addfirstnamevalidator() {}
>> addlastnamevalidator() {}
>> setfirstnamelabel() {}
>> setlastnamelabel() {}
>> }
>>
>> just to name a few, and all they do is forward stuff to the two inner
>> components. sounds like a lot of fun...
>
> Actually, that would be a workaround to hack away a warning your IDE might
> give you about the Law of Demeter. The point of the practice is that outside
> of NameEditor, you shouldn't be doing anything that requires knowledge of how
> NameEditor works. Which means you shouldn't be exposing any of its
> components, and definitely none of its components' properties. If you want
> to do something to your NameEditor that changes the way it shows your name,
> you should express that in terms that make sense for a name editor.
>
> new NameEditor(NameEditorMode.FIRST_NAME_ONLY);
>
> Or whatever API makes sense to you. Just don't expose NameEditor's guts.
the two components are not considered "guts". the nameeditor
encapsulates the markup used to render the two components in my
application, as well as a good set of defaults. however, it leaves
room open for overriding those defaults. there is nothing wrong with
this design.
>
>>
>>>>> Also, I really don't like condoning get(String), it's nasty and very
>>>>> type-unfriendly. It also breaks as soon as you do any refactoring in
>>>>> your component hierarchy.
>>>>
>>>> yes, i traded off some refactorability for better memory footprint.
>>>> imho a perfectly valid trade off when memory and space are costly.
>>>
>>>> there are still problems with this:
>>>> *you still have the two extra memory slots that are a complete waste
>>>> here - component references
>>>
>>>
>>> You're talking about the cost of a reference to a component. It's not like
>>> you've got a COPY of the component in your field. And no, it's not a good
>>> trade-off. You're writing Java applications, you'll be deploying in Java
>>> application containers, you're working with a web framework that goes to
>>> great lengths to help the container provide solid clustering. Your memory
>>> footprint is absolutely subordinate to decently maintainable and
>>> compile-time checked code. If you're a developer that doesn't appreciate
>>> the latter, you'll find that you can be a lot more flexible using a
>>> language with weak type checking, at the cost of maintainable code. Any
>>> Java developer should not be willing to make this trade-off, especially
>>> when it's as light as this one. Your component gets added to your page
>>> anyway!
>>
>> in a perfect world where your servers have unlimited amounts of memory
>> and your cluster's backplane is infinitely fast that would be true.
>> apparently youve missed some recent threads on this list about people
>> trying to optimize state. when you have a table with 100-200 rows and
>> each row has 2-3 instances of such editors, all of a sudden those two
>> extra slots add up.
>>
>> if you dont need to optimize your state thats great, but we should
>> leave the door open for when you do.
>
> You're making it sound a little black and white. You don't need unlimited
> memory and infinite bandwidth to support holding state and/or components in
> instance variables instead of casting results of get() operations on fragile
> strings. I think the grey zone is quite large, and while the exceptional
> case where you might want to optimize the maintainability out of your code
> would be nice to support, it really shouldn't be condoned as the standard way
> of doing things.
im not making things black or white, nor am i condoning the regular
use of get() to retrieve component instances. i am simply making sure
that we leave the door open for me to do so when necessary.
>>>> *the lazy init code is cumbersome to write
>>>
>>> Hardly, but you shouldn't be doing it anyway except for special cases where
>>> you can forgive yourself the bad design.
>>
>> well, in that case you shouldnt mind writing code like this:
>>
>> class mypage extends page {
>>
>> private boolean initialized=false;
>>
>> protected void onconfigure() { if (!initialized) {
>> initialized=true; myOneTimeInitializationCode(); }}
>>
>> }
>
> I would if it weren't a "special cases where I'll have to forgive myself the
> bad design". And I think you're implying I could do this per default in all
> my pages.
yes, you can trivially create your own oninitialize() for pages, it is
what i am implying. and yes, it feels dirty because its special casing
something that you feel may not need to be special cased, i know the
feeling well. howeever, as framework designers we have to weigh
perfection against practicality. we need to evaluate whether
eliminating this special case will remove certain functionality that
people may need. its not an easy decision because it is not black and
white.
>>>> *you are missing the two slots needed to pass models from constructor
>>>> to the two textfields - so thats four extra slots total that you have
>>>> now forced on users
>>>
>>> You really shouldn't be having references to your components and have only
>>> two slots for your models, but if you want to provide getters for your
>>> components, yes, you need instance fields for them. Because get(String) is
>>> nasty. This really doesn't have anything to do with the difference between
>>> constructor and onInitialize initialization, except for the added bonus
>>> that you can't do nasty get(String) in the latter case before your
>>> component is initialized. The second option is looking better and better
>>> to me.
>>
>> it has to do with the fact that if i use a constructor and add those
>> components then i do not need the references. i can always get them
>> via get().
>>
>>>> * the components i get from the getters are not added to their parent
>>>> - for more complex components that need access to their markup this is
>>>> a problem, eg if
>>>> ("text".equals(getFirst().getMarkupTag().get("type")).......not
>>>> necessarily a good example taking this particular custom component as
>>>> context, but a valid usecase in plenty of other situations.
>>>
>>> Irrelevant, even if you added them in your constructor, you don't have
>>> access to your markup before you add your newly created NameEditor to a
>>> page hierarchy. This example does help to confirm the need for an
>>> onInitialize: Don't touch your components until onInitialize is called
>>> indicating that the page hierarchy and markup are available.
>>
>> we are working on enabling this. if NameEditor is a panel or a border
>> then its children should be able to access its markup before
>> NameEditor instance is added to the page since its markup is loaded
>> independently of its parents.
>
> In the ideal world, putting your component initialization code in your
> constructors has no issues or side-effects. You'd have access to your markup
> and your parent, you'd have a fully initialized object, and you wouldn't have
> to make the mental leap that post-constructing actually makes sense.
> Unfortunately, it seems we can't have that. Using constructors for component
> initialization does have issues:
> 1. You don't know your markup.
not an issue for a vast majority of components out there
> 2. You don't know your parent.
again, not an issue for a vast majority
> 3. Your subclasses haven't initialized yet.
this problem has existed as long as oo languages have. interestingly
in its long lifetime java has not provided us with a generic
post-construct callback, so perhaps the problem is not as big as you
make it out to be.
> And while it obviously also has advantages to be able to use the constructor,
> it doesn't make sense to blindly argue the advantages without proposing a way
> of dealing with the issues.
interesting because in the beginning of this thread you wanted to
blindly castrate constructors :) i have proposed a way to deal with
the issue. you can create your own one-time callback in the page from
inside onconfigure().
anyways, the reason i replied to this thread was to:
* play devil's advocate
* flush out the reasoning for both sides of the argument and give
other people reading this a deeper understanding
* arguing helps me think through the problem
in the end i dont think my mind has been changed much. if i were to
vote on this issue after your initial posting my vote would be
* -1 to forbid add() from constructors
* +0 to delay oninitialize() cascade until right before the first call
to onconfigure()
and it would remain the same now. my main reason for arguing was to
flush out any problems surrounding delaying the initial oninitialize()
cascade. i dont think it will cause too many problems, especially
considering pages are not the unit of reusability. possible problems
it may cause, however, is that now components will remain
uninitialized for longer and that may make more of their methods
unavailable post construction possibly making them harder to
configure. this is why my vote is +0 and not +1.
-igor
> I've proposed a way of dealing with the issues: Use onInitialize always.
> This also has issues, and I've proposed solutions for all of them. You'll
> find this thread documents them well, and in that regard, the onInitialize
> side of the argument has actually been worked out better. The disadvantages
> are clear and the way you should design your code to cope with them have been
> explained fully.
>
> You say you're working on a way of dealing with issue [1] for
> constructor-based component initialization. That'd be grand. But that
> leaves [2] and [3]. I can only think of one way to resolve [2]: In addition
> to taking an `String id` in components' super calls, also take a
> `MarkupContainer parent`. But that would make all component constructors
> even more verbose. And it still leaves [3] unresolved.
>
> That's ignoring the troubles you get when you use onInitialize and
> constructors in your Page. However, that can be documented in the JavaDocs,
> if need be: <b>If you add components in your constructor, don't use
> onInitialize</b>.
>
> I see the only recurring case for constructor-based initialization being: We
> shouldn't prohibit those that want to do it that way from doing it that way.
> What about the fact that you're prohibiting me from using onInitialize in
> pages?
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org
> For additional commands, e-mail: users-h...@wicket.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org
For additional commands, e-mail: users-h...@wicket.apache.org
