sabren.net   rants   archive   bio   portfolio


« index »

Refactoring the Product Page [0526.2000]

I admit it. I'm guilty of RapeAndPasteProgramming for BlogDrive/ZikeShop. I just wanted to get something working. But the more UserStories I work on, the nastier the code gets. It's time to refactor.

Let's look at what we've got to begin with. It's a form that lets you edit Products in a database. The top part of the form lets you add or edit a single product. The bottom part of the form shows two links for each product: one to edit it, one to delete it. (You don't have to try and understand the code. It's an ugly mess. That's why I'm refactoring.) 

""" 
adm_product.py - product administration for zikeshop 
 
$Id: adm_product.py,v 1.4 2000/05/26 06:53:39 sabren Exp $ 
""" 
 
import zikeshop 
import weblib 
import sys 
 
import zikeshop.admin.header 
 
if weblib.request.get("ID"): 
    prod = zikeshop.Product(ID=weblib.request["ID"]) 
else: 
    prod = zikeshop.Product() 
 
if weblib.request.get("action")=='save': 
    prod.code = weblib.request.get("code", '') 
    prod.product = weblib.request.get("product", '') 
    prod.descriptLong = weblib.request.get("descriptLong", '') 
    prod.save() 
elif weblib.request.get("action")=='delete': 
    print "<b>", prod.code, "deleted.</b><br>" 
    prod.delete() 
    prod = zikeshop.Product() 
 
 
if prod.ID: 
    print "<b>this is 'edit' mode</b>" 
else: 
    print "<b>this is 'add' mode</b>" 
    prod.descriptLong = '' 
    prod.product = '' 
    prod.code = '' 
 
print '<form action="adm_product.py" method="POST">' 
if prod.ID: 
    print '<input type="hidden" name="ID" value="%s">' % prod.ID 
print 'code: <input type="text" name="code" value="%s"><br>' % prod.code 
print 'product: <input type="text" name="product" value="%s"><br>' \
      % prod.product 
 
print 'categories:<br>' 
cur = zikeshop.dbc.cursor() 
sql = "SELECT ID, path, 0 from base_node order by path" 
cur.execute(sql) 
print weblib.selectBox("nodeIDs", cur.fetchall(), extra="MULTIPLE") 
print '<br>' 
 
 
print 'description:<br>' 
print '<textarea cols="30" rows="5" name="descriptLong">%s</textarea><br> \
      ' % prod.descriptLong 
print '<input name="action" value="save" type="submit">' 
print '</form>' 
 
 
print '<hr>' 
 
cur = zikeshop.dbc.cursor() 
cur.execute("SELECT ID, code, product FROM shop_product order by code") 
 
print '<b><a href="adm_product.py">add new</a></b><br>' 
 
for row in cur.fetchall(): 
    print '<A HREF="adm_product.py?action=delete&ID=%s">[x]</a>' % (row[0],) 
    print '<A HREF="adm_product.py?ID=%s">%s</a>' % (row[0], row[1]), 
    print row[2], "<br>" 
 
print '<hr>' 
print 'zikeshop alpha (c)2000 zike interactive, inc' 
 

Yuck, huh? Now, I've actually got several pages that look similar to this. My goal is to combine their functionality into a reusable class. This is my first "major" refactoring, so I wanted to write out my thoughts as I went along.

Okay. There's a couple glaring problems here:

  • First of all, there's several distinct actions going on, but they're presented as one long script. These actions ought to be broken down into functions, and eventually transformed into methods.
  • Second, we're mixing up HTML and code. This is a cardinal sin in my book. My intent is to eventually move all the HTML stuff into a zebra template. I'm not going to do that here, but it still needs to be cleaned up.
  • Third, this code is directly tied to the zikeshop.Product class. The other pages like this are tied to zikebase.Node and zikebase.Content. All three of these are subclasses of zdc.RecordObject, which is a subclass of zdc.Object. A generic ObjectEditor class would be able to handle any of these classes, plus many more that are sure to come. I'm not sure if I can accomplish this just yet, but it's a goal to shoot for.
  • Fourth, the lines at the top about "if action is this, elif action is that..." are just dying for a dispatch function. (That was intentional - I knew this refactoring was coming, and that ObjectEditor would extend an Actor class)

There's also a usability issue that probably isn't apparent from the code: if you submit the page, you always wind up in edit mode, editing the last product you worked with. This constantly trips me up. It should either show you an error message or put you back into add mode.

Finally, the "categories" area is useless. That's not a bug. It's just not finished. I was working on this area when I decided the rest of the code was too awful to live.

functional decomposition
Let's start by extracting a couple functions. The code flows pretty much straight down, with only four ifs and one loop, so I think we can pretty much just break it into chunks:

  • Get a (new or existing) Product object.
  • Do something with it. (which can be further broken down into Add or Delete)
  • Show an add/edit form.
  • Show a list of products.

Let's break this into functions and see what happens: 

"""
adm_product.py - product administration for zikeshop
 
$Id: adm_product.py,v 1.6 2000/05/26 08:10:19 sabren Exp $
"""
 
import zikeshop
import weblib
import sys
 
 
def fetchProduct():
    if weblib.request.get("ID"):
        return zikeshop.Product(ID=weblib.request["ID"])
    else:
        return zikeshop.Product()
 
 
def doAction(prod):
    if weblib.request.get("action")=='save':
        action_save(prod)
    elif weblib.request.get("action")=='delete':
        action_delete(prod)
 
 
def action_delete(prod):
    print "<b>", prod.code, "deleted.</b><br>"
    prod.delete()
    prod = zikeshop.Product()
 
 
def action_save(prod):
    prod.code = weblib.request.get("code", '')
    prod.product = weblib.request.get("product", '')
    prod.descriptLong = weblib.request.get("descriptLong", '')
    prod.save()
 
 
def showForm(prod):
    if prod.ID:
        print "<b>this is 'edit' mode</b>"
    else:
        print "<b>this is 'add' mode</b>"
        prod.descriptLong = ''
        prod.product = ''
        prod.code = ''
 
    print '<form action="adm_product.py" method="POST">'
    if prod.ID:
        print '<input type="hidden" name="ID" value="%s">' % prod.ID
    print 'code: <input type="text" name="code" value="%s"><br>' \
          % prod.code
    print 'product: <input type="text" name="product" value="%s"><br>' \
          % prod.product
 
    print 'categories:<br>'
    cur = zikeshop.dbc.cursor()
    sql = "SELECT ID, path, 0 from base_node order by path"
    cur.execute(sql)
    print weblib.selectBox("nodeIDs", cur.fetchall(), extra="MULTIPLE")
    print '<br>'
 
    print 'description:<br>'
    print '<textarea cols="30" rows="5" name="descriptLong">'
    print '%s</textarea><br>' % prod.descriptLong
    print '<input name="action" value="save" type="submit">'
    print '</form>'
 
 
def showProductLinks():
    cur = zikeshop.dbc.cursor()
    cur.execute(
        "SELECT ID, code, product FROM shop_product order by code")
 
    print '<b><a href="adm_product.py">add new</a></b><br>'
    for row in cur.fetchall():
        print '<A HREF="adm_product.py?action=delete&ID=%s">[x]</a>' \
              % (row[0],)
        print '<A HREF="adm_product.py?ID=%s">%s</a>' % (row[0], row[1]),
        print row[2], "<br>"
 
 
if __name__=="__main__":
    import zikeshop.admin.header
    prod = fetchProduct()
    doAction(prod)
    showForm(prod)
    print '<hr>'
    showProductLinks()
    print '<hr>'
    print 'zikeshop alpha (c)2000 zike interactive, inc'
 

And, I check the code into CVS, paste the code into this into this article, do some minor formatting changes, and THEN remember to run my test suite. It fails! The showProductLinks() definition said "print" instead of "def", and, in the __name__=="__main__" area (a python idiom - it's just the main block of code), I wrote getProduct() instead of fetchProduct()... Simple mistakes. Duh. :) Anyway, I cleaned them up and repasted the example as you see it above.

status check
The new version is 92 lines, compared to 74 in the original. This is mostly due to all the function definition lines, and a couple lines that I broke just for formatting reasons after I indented.

The code is now broken into smaller chunks, with function headings that describe what each piece does. This version is quite a bit more readable.

We're still mixing code and HTML, but it's mostly confined to two functions: showForm() and showProductLinks(), plus a single line in action_delete().

We're still tied to the Product class, though we're closer to to decoupling it. If you look in the __main__ section, you'll notice that the first three function calls all deal with a temporary variable called "prod", representing a Product object. It felt funny writing things this way, but passing in "prod" let me make most of these chunks into functions just by indenting the code.

What I really want is a separate class that lets the user act on Product and other RecordObjects through a web browser. Let's see what we can accomplish by creating an ObjectEditor class.

(an hour and forty-something minutes later...)

Here we go: 

"""
adm_product.py - product administration for zikeshop
 
$Id: adm_product.py,v 1.7 2000/05/26 09:51:30 sabren Exp $
"""
 
import zikeshop
import weblib
import sys
 
 
class ObjectEditor:
    """A class for editing descendents of zdc.Object"""
 
    ## attributes ########################################
 
    what   = None # what class?
    object = None # an instance of the class
    input  = {}
 
    ## constructor #######################################
 
    def __init__(self, what, **args):
        """ex: objed=ObjectEditor(Person, fName="fred", lName="smith")"""
        self.what = what
        self.object = apply(self.newObject, (), args)
 
 
    ## public methods ####################################
 
    def newObject(self, **which):
        """Create a new instance of whatever class we're editing."""
         
        ## this next bit is python magic that will
        ## create a "what" and pass "where" to the
        ## constructor, assigning the result to self.object
        ##
        ## in the example above, objed.object would be
        ## a Person object representing "fred smith".
         
        return apply(self.what, (), which)
 
 
    def act(self, input):
        """objed.act(input) #input=a dict with a key called 'action'."""
 
        self.input = input
        if input.has_key("action"):
            method = "act_" + input["action"]
 
            ## more python magic to call the method:
            if hasattr(self, method):
                apply(getattr(self, method), ())
            else:
                raise "Don't know how to %s!" % action
 
        else:
            pass # do nothing - no action given
 
 
    ## actions ###########################################
 
    def act_delete(self):
        self.object.delete()
        self.object = self.newObject()
 
 
    def act_save(self):
        for field in self.object.getEditableAttrs():
            if self.input.has_key(field):
                setattr(self.object, field, self.input[field])
        self.object.save()
 

(snipping showForm() and showProductLinks(), which didn't change..) 

if __name__=="__main__":
    import zikeshop.admin.header
 
    if weblib.request.get("ID"):
        ed = ObjectEditor(zikeshop.Product, ID=weblib.request["ID"])
    else:
        ed = ObjectEditor(zikeshop.Product)
 
    ed.act(weblib.request)
 
    showForm(ed.object)
    print '<hr>'
    showProductLinks()
    print '<hr>'
    print 'zikeshop alpha (c)2000 zike interactive, inc'
 

Much better! I even remembered to get the test suites working before checking it in this time. :)

status check
This version is 136 lines, almost twice the length of the original. However, the 60-something lines that represent ObjectEditor can (and should!) be moved into a seperate module and replaced with a single line. That'll leave us somewhere around 74 lines, the same length as the original.

Looking back at our original list of complaints:

  • The code is still broken down into discrete units.
  • The HTML is still limited to two functions. I even deleted the message from ObjectEditor.delete(), though this probably ought to be replaced with something similar so the user knows something's happened.
  • ObjectEditor should be able to work with any sort of zdc.Object that comes its way. We're no longer limited to Product objects.
  • We replaced the if/elif junk with a dispatch function ObjectEditor.act() ..

I'm not sure if I like everything about this code. Having to put an if statement around the ObjectEditor constructor kinda gets on my nerves, for example. I'll clean that up some other time.

There will definitely be other Actor objects besides ObjectEditor, and I may want them to be more closely coupled to the web library. But I'm not sure how I want to play that right now, so I'm not going to do anything yet.

Also, I didn't write any test cases while I was refactoring. I just relied on the existing ones. Now that I have a new class, I'll want to back up and write some more test cases for it to make sure it does everything I think it does.

Oh. I never did address the usability thing. I'll do it later.

Finally, .what is probably a bad choice for the class I'm editing. I may change it to ._class or something. Along those lines, I changed from using "where" to "which" in newObject, and forgot to update the comments.

With those caveats, I'm pretty happy with this refactoring. There's probably a lot more I can do on the HTML side, but I'll worry about that when I start integrating zebra. Meanwhile, I think once I start using ObjectEditor on my other admin pages, it'll be safe safe to go back in the water and continue adding features to zikeshop.


« index »