[Seaside] Is your code for your Seaside web-app using concisemethods?

[Ramon Leon]

> Please take it easy on  my coding style -- this code is still 
> being developed and is likely to have  some areas that could 
> be simplified or whatever.
> 
> MSWLI_Admin_Organization>>renderAddPaperOrderOn: html
>    | aDB org userList scripList |
> 
>    aDB        := self session db.
>    org        := self session org.
>    userList   := aDB readManyOf: MSADBUser  where: [:each | 
> each org id 
> = org id].
>    scripList  := aDB readManyOf: MSADBScrip where: [:each | 
> each org id = org id].
> 
>    html div class: 'fieldset_lookalike'; with:
>    [
>       html break.
>       self renderFamilyAndOrgDivs: html withOrg: org.
>       html break.
>       html form: [
>          self renderUserSelectList:   html withUserList: userList.
>          html div id: 'vendorListDropDown'; with:
>          [
>             self renderScripSelectList: html withScripList: scripList.
>          ].
>          html div id: 'denominationListDropDown'; with:
>          [
>             html text: '3) Select Scrip Denomination'. html break.
>             (html select)
>             list: #('Select a Vendor First' );
>                callback: [self error: 'Should not come here'].
>             html div id: 'cart-contents'; with: [ ].
>          ].
>          html break.
>       ].
>       html break. html break.
>    ].
>    html break

Ok, take everything I say with a grain of salt, no critiques is intended I'm
simply offering my opinion.  

So, lets start with this method.  First, I'd separate the controller code
from the view code so that I could categorize and reuse those queries, and
remove all temp vars from my rendering.  I don't like declaring variables in
the render methods, it too easily leads to logic in rendering methods.  It
also makes it hard to find things later.  I find it much easier to find a
query by selecting the "query" method category and looking at method names,
rather than digging through view code. So...

userList
    ^self session db 
        readManyOf: MSADBUser 
        where: [:each | each org id = org id]

scripList
    ^self session db
        readManyOf: MSADBScrip 
        where: [:each | each org id = org id].

Ok, with those removed, some methods renamed, most of the breaks removed
from the rendering, and a more Smalltalkish code formatting where blocks
don't begin/end on their own lines, we could shorten the render method to
this.

renderAddPaperOrderOn: html 
    html div class: #fieldsetLookalike; with: 
        [self renderOrg: org on: html.
        html form: 
            [self renderUsers: self userList on: html.
            html div id: #vendorListDropDown; with: 
                [self renderScripts: self scripList on: html].
            html div id: #denominationListDropDown; with: 
                [html div: '3) Select Scrip Denomination'.
                html select 
                    list: #('Select a Vendor First' );
                    callback: [self error: 'Should not come here'].
                html div id: 'cart-contents']]]

Now, one thing you'll notice I changed was your method names.  The style you
chose is rather repetitive because you chose to pass the canvas first,
rather than last and you end up repeating your arg name twice in the
selector.  So this...

self renderUserSelectList: html withUserList: userList

Becomes...

self renderUsers: self userList on: html

So I don't have to say user list twice in the selector.  Personally, I
always pass the canvas last with #on: because I'm thinking "Draw this stuff
on that canvas".  

I also try to avoid using technology names in my selectors, they add tend to
be longer and less able to cope with change that simple domain language
selectors.  For example, #renderFamilyAndOrgDivs:withOrg: becomes
#renderOrg:on: and #renderUserSelectList:withUserList: becomes
#renderUsers:on:.  

Now if I later change the layout to not use divs, or to use a Set or
Dictionary of users instead of a list, I don't need to rename my methods
because they were written in the language of the domain rather than the
language of the implementation technology.

As for CSS, I extract "all" CSS to external files with the exception of
#style:'display:none;', which I use a lot when doing ajaxy stuff where I
render something hidden for later.  Removing all CSS will clean up the code
a bunch, and give you a more flexible skinable application to boot.

As for Ajax, one thing I see in your code that I think could simplify
things, would be to always extract the body of a liveCallback into its own
rendering method.  This will save you from having to write the code twice
(denominationListDropDown), once for the initial render, and then once again
in the liveCallback each time the drop down list changes.  By factoring this
duplicate code into a single render method, you can call it on the initial
render with the seaside canvas and the list #('Select a Vendor First' ), and
call it from the live callback with the ajax canvs and the list from (item
denominations reject: [:each | each 
denomination = 0]).

One final thing, I tend to use symbols for my #class: and #id: selectors
rather than strings because I "think" it uses less memory and I feel it
looks much better.  Seaside at one time, had an issue with too many strings
building up.  See http://www.nabble.com/Seaside2.7a1-avi.10-t2319420.html,
but that was fixed, but I never changed the habit.

Ramon Leon
http://onsmalltalk.com