Is your code for your Seaside web-app using concise methods?

I've been working on a web-app with Seaside/Glorp for a while now
(although it's been on hold since November when I last worked on it) and
find that some of my administrative classes are getting a bit larger
than I really want.. I can certainly refactor the code to collect each
functional-area of the admin interface into its own class, but am
curious about whether others out there with Seaside apps have most or
almost all of your classes with methods containing <10 lines of code
(give or take)?   Some of my admin methods have >20-30+ lines of code --
many of which are laying out forms or handling callbacks..  In those
cases, I don't think there's much that can be done  -- correct?

Right now, my admin interface deals with the following sub-areas (with a
bit over 30 methods):

  o Category maintenance
  o Inventory maintenance
  o Order management
  o Vendor management
  o a few other misc. items

I'm probably going to try to refactor out each sub-area into its own
class which will take a bit of surgery to accomplish, but it should make
the code a bit easier to follow I think..

Anyway, I guess I'm just curious if my ST coding skills are leading me
down the wrong direction (I'm used to writing C++ code for my day-job
where it's not uncommon to find methods with 50-100 or more lines) and I
should be going for more simplicity in how my methods & objects are laid
out..

Comments?


_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

> Anyway, I guess I'm just curious if my ST coding skills are
> leading me down the wrong direction (I'm used to writing C++
> code for my day-job where it's not uncommon to find methods
> with 50-100 or more lines) and I should be going for more
> simplicity in how my methods & objects are laid out..
>
> Comments?

Any chance we take take one of them as an example, and see the code and make
suggestions?

Ramon Leon
http://onsmalltalk.com 

_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

Ramon Leon wrote: Sure.. I'll take a stab at posting some code that I can try to make
sense of for you -- the code below is for a page that generates an
offline (I'll have a different set for online orders that will work
similar but different) order form entered by an administrator from a
paper form given to them.. This form will eventually (I don't have some
of the mechanics in place completely yet) add items to the database via
Glorp but that part of the code isn't quite there yet...

Initially, we have the following which in turn renders other areas by
calling methods later (below further) -- this code is using SeasideAsync
as you'll notice near the bottom method (which is in a major state of
flux & experimentation)..  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


MSWLI_Admin_Organization>>renderFamilyAndOrgDivs: html withOrg: org

   html div style: 'border-bottom: solid 1px #555; height: 73px;'; with: [
      "Enclose both divs in an outer div to allow us to align the left &
right inner divs   "
      html div id: 'familyname'; with: [
         html label style: 'font-weight: bold; height: 76px'; with:
'Ship To: '. html break.
         html break; break; break.
      ].
      html div id: 'orgname'; with: [
         html label style: 'font-weight: bold; height: 76px'; with:
'Ship From: '. html break.
         html label with: org orgName. html break.
         html label with: org address. html break.
         html label with: org location city, ', ', org location state,
', ', org location zipcode.
      ]
   ].


MSWLI_Admin_Organization>>renderUserSelectList: html withUserList:
aUserList

   html div id: 'userListDropDown'; with: [
   html text: '1) Select Family for Order'. html break.
    (html select)
        list: aUserList;
        selected: aUserList first;
        labels: [:item |
           item familyId = 1
            ifTrue: [item nameLast]
            ifFalse: [item nameLast , ' (#' , item familyId asString , ')']
        ];
        callback: [self error: 'Should not come here'];
        liveCallback: [:item :h |
             h span id: 'familyname'; with: [
                h label style: 'font-weight: bold'; with: 'Ship Order
To: '. h break.
                h label with: item nameLast,', ',item nameFirst. h break.
                h label with: item streetAddress. h break.
                h label with: item location city,', ',item location
state,' ', item location zipcode.
             ].
           "(h span) id: 'familyname'; with: [ item nameLast  ]."
        ]
   ]



MSWLI_Admin_Organization>>renderScripSelectList: html withScripList:
aScripList
   | dummyDenom denomList blah |

   dummyDenom := Array withAll: #('select a Scrip Vendor first').
   blah := false.
   html text: '2) Select Scrip Vendor'. html break.
   (html select)
   list: aScripList;
      selected: aScripList first;
      labels: [:item |
         blah = false
            ifTrue: [ blah := true. 'Select a Vendor...']
            ifFalse: [item vendor name]
      ];
      callback: [self error: 'Should not come here'];
      liveCallback: [:item :h |
         (h span) id: 'denominationListDropDown'; with:
         [ "Once we get a vendor selected, let's fill in the div for the
denomination by replacing it with another selector"
          denomList  := (item denominations reject: [:each | each
denomination = 0]).
            h text: '3) Select Scrip Vendor first'. h break.
            (h select)
            list: denomList;
            selected: denomList first;
            labels: [:nitem | nitem denomination];
            liveCallback: [:nitem :h2 |
            ]
         ]
      ].


MSWLI_Admin_Organization>>


_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

2007/1/24, Rick Flower <[hidden email]>:
There is. Don't do the forms yourself, instead generate them with
something like Magritte.
And as brutal as this sounds now: use proper CSS and hire a web
designer. No more inline styles, no more consecutive breaks.

Cheers
Philippe
_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

Philippe Marschall wrote: I had looked into using Magritte, but at the time (6-9 months ago) it
wasn't yet ported to
VW which is what I'm using..

> And as brutal as this sounds now: use proper CSS and hire a web
> designer. No more inline styles, no more consecutive breaks.
Actually, I've got 99% of my styles in style methods or in plain old CSS
files,
but do have some spotty forced styles in various places that seemed
easier at
the time instead of coming up with a new item for the css file that may
or may
not be used elsewhere in my code.

_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

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



_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

Ramon Leon wrote:
>
> Ok, take everything I say with a grain of salt, no critiques is intended I'm
> simply offering my opinion.  
>  
I like some good constructive criticism.. I'm fairly new to ST and have
only been using it for <1 year
and I like to learn techniques from those more knowledgeable than I am
-- particularly here! Ok .. Sounds reasonable.. I probably would have done that eventually anyway. Excellent comments!  I guess I got so caught up in writing the code that
I wasn't seeing the forest for
the trees on that one.. Your suggestions make a lot of sense and do a
nice job of keeping things
more OO like and avoiding implementation details in places that don't
need to know about that sort
of stuff.
> 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.
>  
Sounds good.. Most of my CSS is external in separate files care of my
old PHP days.  I think I like doing
that better than having Seaside hitting the server a bunch of times for
those more complex pages with
lots of objects containing internal style methods that need to be
fetched during the drawing process. OK.. Sounds reasonable as well..
> 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
>  
Hmm.. I'm not sure I've ever really looked at ST symbols before.. I
gather (upon doing a little homework
just now) that I just need to use something like #MyDivName or similar
to get a symbol instead of a
string such as 'MyDivName' like I am doing currently?  I guess I didn't
really realize that ST was likely to
be keeping multiple copies of those strings for all of the different
methods that were using them.. Are there
any other pitfalls with using symbols?  I ran across a blog entry
suggesting that new ST'ers frequently use
gratuitous use of symbols and have big performance hits (see link below
-- particularly the last comment)
under certain conditions.. I'll have to admit that they indicate that
you should use "asSymbol" to get
the symbol object instead of creating what would sound (to me anyway)
like another symbol..
Am I barking up the wrong tree or just misunderstanding symbols?

http://www.cincomsmalltalk.com/userblogs/troy/blogView?showComments=true&entry=3313219321

Many thanks so far for the great info Ramon!

_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

No problem, I like seeing how other people code, you never know when you
might accidentally learn something from someone else's code.

If you want to tag something with an identifier, use a symbol.  A symbol is
just a globally unique string.  They allow for faster comparisons because
they can be compared with == rather than =.  However, I use them for
semantic reasons, not performance reasons.  Symbols are for keying and
identifying things, strings generally aren't.  Symbols also have the
advantage of being checked by the refactoring browser when you rename
methods.  Symbols are kind of like undeclared constants in other languages.

Ramon Leon
http://onsmalltalk.com 

_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

Ramon Leon wrote:
> If you want to tag something with an identifier, use a symbol.  A symbol is
> just a globally unique string.  They allow for faster comparisons because
> they can be compared with == rather than =.  However, I use them for
> semantic reasons, not performance reasons.  Symbols are for keying and
> identifying things, strings generally aren't.  Symbols also have the
> advantage of being checked by the refactoring browser when you rename
> methods.  Symbols are kind of like undeclared constants in other languages.
>  
Cool.. I'll try it out.. Thanks!
_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

Ramon Leon wrote:
Ok.. I ended up with something that very much resembled that code.. I
realized the other day that some of this might fit better as part of
a shopping cart that has a "header" and "footer" and some divs with
"content" in the middle.. I figured I could rip out the guts of the
code and move it into a new object (aka MSAShoppingCart in my case)
and have the caller specify the blocks to be used to deal with the
respective rendering of content for each div along with the respective
callbacks, etc to effectively make a generic shoppingcart that only
knows how to render itself at a high level and maintain some class
variables for quantities, what was purchased, user-record for purchaser,etc.

Anyway, I've gotten stuck now with the ST syntax and think I've gotten
lost in the forest and am hoping someone can kindly point me in the
right direction.  Perhaps I should not bother with the shoppingCart
paradigm in the same way I'm heading with it.. Any comments would be
greatly appreciated.. Below is the code mostly as I've got it loaded
into VW now (I took some liberties and edited some things I wanted to
change anyway):

Here's my code for the above :

MSWLI_Admin_Organization>>renderAddPaperOrderOn: html
   self shoppingCart renderCart: [ self renderFamilyAndOrg: (self session org) on: html]
            renderScripBlockAs:  [ self renderScrip: self scripListForOrg on: html]
            renderDenomBlockAs:  [ self renderDenomFiller: self on: html ]
            renderQtyBlockAs:    [ self renderQtyBox:      self on: html ]
            renderSubmitBlockAs: [ html submitButton liveCallback: [:r |  ]; text: 'Add to Cart' ]
            renderCartBlockAs:   [ ... ]
            on: html.

MSAShoppingCart>>renderCart: headerBlock renderScripBlock:  scripBlock  renderDenomBlock:  denomBlock
            renderQtyBlock:    qtyBlock renderSubmitBlock: submitBlock  renderCartBlock:   cartBlock
            on: html
   html div class: #fieldset_lookalike; with:
   [
     html form: [
       headerBlock on: html.
       html div id: #vendorListDropDown;        with: [ scripBlock ].
       html div id: #denominationListDropDown ; with: [ denomBlock ].
       html div id: #quantityEntry;             with: [ qtyBlock   ].
       html div id: #addToCart;                 with: [ submitDiv  ].
       html div id: 'cart-contents';            with: [ cartBlock  ].
    ]].

With this, I run into issues with the passing of the "on: html" args
(particularly for the headerBlock and likely for the others as well)
and how to properly deal with them.. In nosing around a bit with what
others have done, I could break out each block into separate classes
and just "render" them, but that doesn't seem right since it starts
breaking my code down too small (in my opinion)..

Anyway, I'm hoping someone can give me some insight for this sort of
refactoring..

Thanks --

Signed -- "Stuck in the woods"
_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

> Anyway, I'm hoping someone can give me some insight for this
> sort of refactoring..
>
> Thanks --
>
> Signed -- "Stuck in the woods"

Blocks work like this...

block := [Smalltalk beep].
block value. "beep".

block := [:a | Transcript show: a].
block value: "print".

block := [:a :b | Transcript show: a; show: b].
block value: "first arg" value: "second arg".

So... I'm not sure I like your approach, too much api and block magic to
avoid a couple of divs.  But here goes...

MSWLI_Admin_Organization>>renderAddPaperOrderOn: html
   self shoppingCart
       renderCart: [ self renderFamilyAndOrg: (self session org) on: html]
       renderScript: [ self renderScrip: self scripListForOrg on: html]
       renderDenom: [ self renderDenomFiller: self on: html ]
       renderQty: [ self renderQtyBox: self on: html ]
       renderSubmit: [ html submitButton liveCallback: [:r |  ]; text: 'Add
to Cart' ]
       renderCart: [ ... ]
       on: html

MSAShoppingCart>>renderCart: headerBlock
                 renderScript: scripBlock
                 renderDenom: denomBlock
                 renderQty: qtyBlock
                 renderSubmit: submitBlock  
                 renderCart: cartBlock
                 on: html

   html div class: #fieldset_lookalike; with:
     [html form:
       [headerBlock value.
       html div id: #vendorListDropDown; with: scripBlock.
       html div id: #denominationListDropDown; with: denomBlock.
       html div id: #quantityEntry; with: qtyBlock.
       html div id: #addToCart; with: submitDiv.
       html div id: 'cart-contents'; with: cartBlock]].

You don't need to wrap blocks inside blocks to pass them to #with:, they're
already blocks, just pass them.  

For what you're doing here, at the very least, I'd lose the component and
just make this a custom method on a custom renderer so it'd look like this
instead.

renderAddPaperOrderOn: html
   html
       renderCart: [ self renderFamilyAndOrg: (self session org) on: html]
       renderScript: [ self renderScrip: self scripListForOrg on: html]
       renderDenom: [ self renderDenomFiller: self on: html ]
       renderQty: [ self renderQtyBox: self on: html ]
       renderSubmit: [ html submitButton liveCallback: [:r |  ]; text: 'Add
to Cart' ]
       renderCart: [ ... ]

Because what you're doing doesn't really call for a component, just a custom
rendering method.

Ramon Leon
http://onsmalltalk.com 

_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

Ramon Leon wrote:

Thanks.. I knew that but just haven't needed to use them in my
own code until (perhaps) now -- at least in terms of accepting
blocks.

> So... I'm not sure I like your approach, too much api and block magic to
> avoid a couple of divs.  But here goes...

I not sure I'm following your comment about avoiding divs..
If you've got some other suggestions, I'd love to hear them.. (8->

I do see that I was wrapping my blocks in more blocks.. I guess it
was a bad habit since all of my Seaside code that uses "with:" always
has the blocks enclosures.. Thanks for pointing that out!

> For what you're doing here, at the very least, I'd lose the component and
> just make this a custom method on a custom renderer so it'd look like this
> instead.

In my case, the MSAShoppingCart was not a (Seaside) component (I should
have made that obvious) but more a helper class to hold onto anything
related to a shopping cart such as those items added to the cart, etc.
I was hoping to just be able to tell the cart logic what it needed to
use to write into the offending divs.  Obviously I didn't get far with
the whole concept.  It sounds like the code below has more or less done
away with the shopping cart altogether..correct?

I suppose I could just keep the shopping cart simple (KISS) and just
have the offending callback/block logic do the necessary updates of
items in the cart with the liveCallbacks as I've already got it.. Is
that my best approach for this sort of problem? If so, I think that's
more or less what I had before I started whacking things around late
last night..

Many thanks in advance for your assistance Ramon!




_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside

> In my case, the MSAShoppingCart was not a (Seaside) component
> (I should have made that obvious) but more a helper class to
> hold onto anything related to a shopping cart such as those
> items added to the cart, etc.

Wouldn't that just be the Cart object?

> I suppose I could just keep the shopping cart simple (KISS)
> and just have the offending callback/block logic do the
> necessary updates of items in the cart with the liveCallbacks
> as I've already got it.. Is that my best approach for this

Yes... MVC, the Seaside component is the view, the callback's or
liveCallbacks, are the controller methods (hopefully the callbacks just
invoke methods rather than contain them, and there should be some sort of
model object, i.e. the ShoppingCart domain object.

> sort of problem? If so, I think that's more or less what I
> had before I started whacking things around late last night..
>
> Many thanks in advance for your assistance Ramon!

Any time.

Ramon Leon
http://onsmalltalk.com


_______________________________________________
Seaside mailing list
[hidden email]
http://lists.squeakfoundation.org/cgi-bin/mailman/listinfo/seaside