I'll still come back to this thread. I have to agree with Peter below
- while setupRender() and/or using multiple event handlers would
technically produce cleaner code, it's proven out to be a more safe
way for me to use single onActivate() with multiple if (property ==
null) {...} blocks. The code doesn't look as clean, but it's all in
one place and it's easier to refactor. Repeatedly, I've run into cases
where I've started with data supposedly for display only, but then I
add a few actions which causes refactoring if I don't have have the
initialized in onActivate(). The persistence options can be added as
an afterthought when context is (conditionally) initialized in
onActivate(). It's a safer practice and the performance hit because of
the conditionals is likely negligible.
Kalle
On Fri, Sep 4, 2009 at 12:52 AM, <p.stavrini...@albourne.com> wrote:
> Hi Geoff,
>
> Great post, some good points made, I agree with most of your comments, there
> is only one thing I prefer to do differently... while onActivate should only
> be used to set context variables for a page, I feel setupRender() is not the
> place for context checking.
>
> Why? two reasons:
> 1. The nature of setupRender() (its a rendering method) so it has the
> characteristic of being able to iterate which implies repetitive calls could
> potentially be made to initialization code, which just sits a little
> uncomfortably with me, especially if it involves database calls, I so happen
> to use database calls on occasion to check these
> 2. Its too late in the page lifecycle as onActivate has already been
> called... while this may not appear to be a big deal for most cases,
> certainly it allows room for error particularly (but not limited to) not
> being able to handle something like a coercion error!
>
> I thought about this a lot and tried various options, by far the most
> successful solution for me was to have pages and components extend a
> completely generic base page, which uses:
> onActivate(EventContext eventContext)
>
> This has several advantages:
> - It allows you to check the contexts validity before the context is set in
> the page! (a big win)
> - It allows you to handle any activation errors before the page renders
> (even most coercion errors)
> - It prevents duplicate onActivate methods in pages (this cleaned up my code
> a lot)
> - It reduces the complexity of dealing with optional or a variable number of
> context parameters
> - base pages have all the advantages of inheritance / abstract classes
> making for good reuse of common logic, and enforcing a design pattern (good
> for working in teams).
> - You can use a single onException handler! removing a heap of code, also
> ensuring correct logging and reporting or errors without polluting your pages
> (enforced error handling...I don't have to worry about dealing with errors
> separately in every page)
> - Reuse! my pages are kept very clean, and are wired by simply extending the
> base page.
> - Abstract methods can be used for things like redirection where the
> implementation is provided in the page
>
> So thats my two cents then,
>
> Kind Regards,
> Peter
>
> ----- Original Message -----
> From: "Geoff Callender" <geoff.callender.jumpst...@gmail.com>
> To: "Tapestry users" <users@tapestry.apache.org>
> Sent: Thursday, 3 September, 2009 18:47:28 GMT +02:00 Athens, Beirut,
> Bucharest, Istanbul
> Subject: Re: [ANN] JumpStart 4.4 released
>
> Kalle,
>
> I'm really glad you asked this question because it got me thinking,
> and I've realised I'm putting too much into onActivate(..). The
> problem is that onActivate() is called regardless of whether the user
> clicks on submit or an ActionLink. The database activity in
> onActivate() is not usually necessary for an ActionLink.
>
> I think this is better...
>
> * Use onActivate() to read in the context and nothing more.
>
> * Use setupRender() to set up display-only objects and fields. You can
> also use getters if they have low overhead or you take care to lazy-
> load.
>
> * Use onPrepare() to set up editable objects and fields. onPrepare()
> is triggered by form render and form submit. If there's more than one
> form then use multiple onPrepares, eg. onPrepareForFormX(),
> onPrepareForFormY().
>
> So the examples become...
>
> UserCreate.java:
>
> void onPrepare() {
> // Instantiate a User for the page data to overlay.
> _user = new User();
> _user.setActive(true);
> }
>
> UserEdit.java:
>
> Long onPassivate() {
> return _userId;
> }
>
> void onActivate(Long id) {
> _userId = id;
> }
>
> void onPrepare() {
> try {
> _user = getSecurityFinderService().findUser(_userId);
> }
> catch (DoesNotExistException e) {
> // Handle null user in the template
> }
> }
>
> void setupRender() {
> _userRoles =
> getSecurityFinderService().findUserRolesShallowishByUser(_userId);
> }
>
> I've almost finished reworking JumpStart to suit this pattern and it
> works much better. I'll release a few days from now as version 4.5.2.
>
> The previous technique is a hangover from when I thought onActivate()
> should validate the context and throw an exception if it was invalid.
> I changed my mind on that quite a while ago and wrote this:
>
>
> http://jumpstart.doublenegative.com.au:8080/jumpstart/examples/infrastructure/handlingabadcontext/1
>
> Thanks again,
>
> Geoff
>
> On 03/09/2009, at 4:53 AM, Kalle Korhonen wrote:
>
>> Thanks Geoff, no can't see any major issues. Agree on the
>> initialization of roles in setupRender() since they really are used
>> for rendering only. Heavy use of lazy getter-based initialization has
>> gotten me into trouble before, but I may revisit my coding best
>> practice. And that's really the only thing I was after - to review my
>> understanding and practices against others'.
>>
>> Kalle
>>
>>
>> On Wed, Sep 2, 2009 at 8:12 AM, Geoff
>> Callender<geoff.callender.jumpst...@gmail.com> wrote:
>>> To set the scene: in the EditUser example, the user is displayed in
>>> a form
>>> and the user roles are displayed below it in a grid with
>>> ActionLinks for
>>> View, Edit, and Delete on each row.
>>>
>>> The reason it is OK to get the user roles in setupRender() is
>>> because they
>>> are not editable - all we need is the id for the context of each
>>> ActionLink
>>> in the row. If you come back to this screen and hit a link then it
>>> will
>>> still work.
>>>
>>> Actually, if the user roles were editable you'd probably get them in
>>> onPrepare() rather than onActivate(), just as in the EditableLoop1
>>> example.
>>> I can't recall why I preferred onPrepare() over onActivate() but I
>>> think it
>>> was because it's called exactly as often as it is needed whereas
>>> onActivate() is called often.
>>>
>>> If a ValueEncoder is used with the loop then it becomes OK to get the
>>> entities in setupRender() and encode them in onPrepare(). This is
>>> demonstrated in EditableLoopUsingEncoder1.
>>>
>>> Back in the user form, user.salutation is chosen from a Select
>>> list. The
>>> list doesn't need to be built in onActivate() either. It's done in
>>> getSalutations() and works just fine with the Back button.
>>>
>>> Can you see a hole in this that I've missed?
>>>
>>> Geoff
>>> http://jumpstart.doublenegative.com.au:8080/jumpstart/
>>>
>>> On 02/09/2009, at 11:56 PM, Kalle Korhonen wrote:
>>>
>>>> On Tue, Sep 1, 2009 at 11:30 PM, Geoff
>>>> Callender<geoff.callender.jumpst...@gmail.com> wrote:
>>>>>
>>>>> The key to it is this snippet: "if the stuff you are setting up
>>>>> is not
>>>>> needed for component event requests, consider putting it
>>>>> elsewhere". If I
>>>>> understand your example correctly, the object you are creating IS
>>>>> needed
>>>>> for
>>>>> a component event request so DO put it in onActivate(...).
>>>>
>>>> Yes, that's just the thing. Whether it's entities or translators or
>>>> anything, pretty much all of that "stuff" is needed for event
>>>> requests. Can you come up with a good example for initializing
>>>> something that is safe to do in setupRender()? Obviously if that
>>>> object is really needed just for rendering (as the operation name
>>>> suggests) then it's the right place for it, but those cases are few
>>>> and far between. Even in your example, the userRoles are most
>>>> certainly needed in event requests - obviously you can just return
>>>> error "user doesn't have the proper role for the operation" but it'd
>>>> be more usable to just do that in onActivate as well.
>>>>
>>>>> But with edit, if you want optimistic locking then you have to
>>>>> include
>>>>> the
>>>>> entity's version attribute in the form:
>>>>> in which case if you submit, press Back, then submit, you'll get an
>>>>> exception thrown by the persistence mechanism about optimistic
>>>>> locking -
>>>>> it
>>>>> tells us that the User has changed since the page was first
>>>>> displayed.
>>>>> It's
>>>>> correct, and all you do is hit refresh and try again. If you
>>>>> don't want
>>>>> optimistic locking then don't put the entity's version attribute
>>>>> in the
>>>>> form.
>>>>
>>>> Agree completely, that's a good pattern to follow.
>>>>
>>>> Kalle
>>>>
>>>>
>>>>> On 02/09/2009, at 4:03 PM, Kalle Korhonen wrote:
>>>>>
>>>>>> On Tue, Sep 1, 2009 at 9:50 PM, Geoff
>>>>>> Callender<geoff.callender.jumpst...@gmail.com> wrote:
>>>>>>>
>>>>>>> Good question. Yes, it does still seem to me to be best
>>>>>>> practice and
>>>>>>> no,
>>>>>>> I
>>>>>>> don't see it breaking the back button. Can you give an example?
>>>>>>
>>>>>> Assuming you use something else than session or client
>>>>>> persistence and
>>>>>> you initialize (create) an object, set it as a value of some page
>>>>>> property in your setupRender(), then submit the form, press the
>>>>>> back
>>>>>> button and re-submit the formit, the object will be null
>>>>>> (because it
>>>>>> was initialized in setupRender() that was never invoked rather
>>>>>> than
>>>>>> onActivate()).
>>>>>>
>>>>>> Kalle
>>>>>>
>>>>>>> On 01/09/2009, at 8:37 AM, Kalle Korhonen wrote:
>>>>>>>
>>>>>>>> Hey Geoff,
>>>>>>>>
>>>>>>>> I recall reading at some point that you had recommended not
>>>>>>>> using
>>>>>>>> onActivate() for all initialization purposes, and sure enough,
>>>>>>>> I dug
>>>>>>>> it up and at
>>>>>>>>
>>>>>>>>
>>>>>>>> http://jumpstart.doublenegative.com.au:8080/jumpstart/examples/navigation/onactivateandonpassivate/3
>>>>>>>> you say "It can be tempting to put lots of setup code into
>>>>>>>> onActivate(...). However, if the stuff you are setting up is not
>>>>>>>> needed for component event requests, consider putting it
>>>>>>>> elsewhere,
>>>>>>>> such as setupRender() or getter methods."
>>>>>>>>
>>>>>>>> Does that still reflect your current understanding of the best
>>>>>>>> practices? The caveat with using setupRender() (or anything else
>>>>>>>> except for onActivate()) is that even for non-ajax
>>>>>>>> applications, it
>>>>>>>> "breaks the back button", i.e. if a user goes back in history
>>>>>>>> and say
>>>>>>>> re-submits a form (for one reason or another) the objects
>>>>>>>> required by
>>>>>>>> the page/form are not initialized. Obviously it depends on the
>>>>>>>> case
>>>>>>>> what the application should do, but you loose half the
>>>>>>>> benefits of
>>>>>>>> redirect-after-post if your require a refresh before a page is
>>>>>>>> usable
>>>>>>>> again. Do you simple prefer rendering an an error in the back
>>>>>>>> button/direct form submit case or do you generally do all
>>>>>>>> initialization in onActivate()?
>>>>>>>>
>>>>>>>> Kalle
>>>>>>>>
>>>>>>>>
>>>>>>>> On Tue, Aug 4, 2009 at 7:59 PM, Geoff
>>>>>>>> Callender<geoff.callender.jumpst...@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> JumpStart 4.4 is now available. It's a tidying up release:
>>>>>>>>> the
>>>>>>>>> structure's
>>>>>>>>> a bit neater and it uses the latest OpenEJB, ie. 3.1.1.
>>>>>>>>>
>>>>>>>>> Use it live:
>>>>>>>>>
>>>>>>>>> http://jumpstart.doublenegative.com.au:8080/jumpstart/
>>>>>>>>>
>>>>>>>>> or download it:
>>>>>>>>>
>>>>>>>>> http://jumpstart.doublenegative.com.au
>>>>>>>>>
>>>>>>>>> And if someone can figure how to get Jetty/OpenEJB in Eclipse
>>>>>>>>> to use
>>>>>>>>> the
>>>>>>>>> libs and classes in the WAR instead of having to be spoon-fed a
>>>>>>>>> classpath,
>>>>>>>>> then please let me know. I suspect it's a class-loading issue
>>>>>>>>> that
>>>>>>>>> might
>>>>>>>>> soon be solved by http://code.google.com/p/embed-openejb-in-eclipse/
>>>>>>>>> .
>>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>>
>>>>>>>>> Geoff
>>>>>>>>
>>>>>>>> ---------------------------------------------------------------------
>>>>>>>> To unsubscribe, e-mail: users-unsubscr...@tapestry.apache.org
>>>>>>>> For additional commands, e-mail: users-h...@tapestry.apache.org
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> ---------------------------------------------------------------------
>>>>>>> To unsubscribe, e-mail: users-unsubscr...@tapestry.apache.org
>>>>>>> For additional commands, e-mail: users-h...@tapestry.apache.org
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> ---------------------------------------------------------------------
>>>>>> To unsubscribe, e-mail: users-unsubscr...@tapestry.apache.org
>>>>>> For additional commands, e-mail: users-h...@tapestry.apache.org
>>>>>>
>>>>>
>>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: users-unsubscr...@tapestry.apache.org
>>>> For additional commands, e-mail: users-h...@tapestry.apache.org
>>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: users-unsubscr...@tapestry.apache.org
>>> For additional commands, e-mail: users-h...@tapestry.apache.org
>>>
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: users-unsubscr...@tapestry.apache.org
>> For additional commands, e-mail: users-h...@tapestry.apache.org
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscr...@tapestry.apache.org
> For additional commands, e-mail: users-h...@tapestry.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscr...@tapestry.apache.org
For additional commands, e-mail: users-h...@tapestry.apache.org
