best practices when writing controllers

[cd34]

When writing a pylons app, how much code do you put into your
controller?

One thought I had was to put enough code in to handle basic
operations, but, if the controller required more than 20 or so lines
of code, put that into a class or function so that the controller
itself was reasonably lightweight and easy to follow. But some
controllers only have a few actions, and putting 100 or so lines of
code into 3 actions seems functional and easier to follow with less
indirection.

Some of my actions have 3-5 lines of code after the decorator and
definition, but, some actions are quite complex.

Any suggestions or guidelines?

[Ben Bangert]

The nice thing about keeping a controller small, is when it has a lot of actions. I have a few controllers in one of my projects that are very large. Though I consider this less than ideal and when I have some time I'll likely go in and refactor to break it down a bit. I guess the issue is that while a controller might start with 3 actions, it could grow... and if you didn't keep it brief to begin with who knows when you might next have some time to refactor it.

I think your thought about keeping it to 20 or less lines of code is a pretty good rule of thumb, though sometimes for a big action, I can easily have 10-15 lines merely of assigning 'c' variables, so that prolly shouldn't be a hard limit. :)

Cheers,
Ben

[cd34]

On Apr 3, 12:19 pm, Ben Bangert <b...@groovie.org> wrote:
> The nice thing about keeping a controller small, is when it has a lot of actions. I have a few controllers in one of my projects that are very large. Though I consider this less than ideal and when I have some time I'll likely go in and refactor to break it down a bit. I guess the issue is that while a controller might start with 3 actions, it could grow... and if you didn't keep it brief to begin with who knows when you might next have some time to refactor it.
>
> I think your thought about keeping it to 20 or less lines of code is a pretty good rule of thumb, though sometimes for a big action, I can easily have 10-15 lines merely of assigning 'c' variables, so that prolly shouldn't be a hard limit. :)

I have one controller with 21 actions, most of which are a form/
formsave, but, some of the formsave actions do 8-10 'tasks' before
rendering the response. I thought consolidating tasks into an
external class/function would lend to better readability, but, each
action isn't duplicated, so, I'm just moving code from one place to
another. I think IBM once stated that 23 lines of code was the limit
for bugfree code which sort of corresponds to a single screen and I've
used that as a guideline. Converting 90k lines of php without a
framework to Pylons is allowing me to refactor quite a bit but, I was
just wondering if my approach was a bit extreme.

I'll have something like:

@validate(dadv_form, error_handler='add')
def addsave(self):
user =
meta.Session.query(users).filter(users.user_id==request.params['user_id']).one()
user.newval = request.params['newval']
user_log = user_log(somefunction,'C',user)
user.user_log.append(user_log)
secondtable =
meta.Session.query(blah),filter(blah.comment_id==request.params['comment_id']).one()
secondtable.comment = whatever
meta.Session.flush()
(task_id,status) = task().bigtask().add(get_client_id(), \
request.params['device_id'], \
request.params['username'], \
request.params['ip'], \
request.params['email'], \
request.params['path'], \
request.params['include'])
h.flash("whatever updated. Task %d, status %d" %
(task_id,status))
redirect('/blah/add')

While that isn't typical of every action, there are some actions that
require a lot more processing. Moving that out of the controller
would make the action something like:

@validate(dadv_form, error_handler='add')
def addsave(self):
(task_id,status) = do_addsave(request.params)
h.flash("whatever updated. Task %d, status %d" %
(task_id,status))
redirect('/blah/add')

It is certainly more compact, but, is it really any more readable? I
had been using sprox in TurboGears, but, had less success with it in
Pylons and haven't tried it recently, but, it would certainly change a
bit of the code. Replacing a number of lines of code with something
like:

provider.add(users, values=request.params)

or

provider.edit(users, values=request.params)

is certainly cleaner and provides a bit of readability.

The issue I'm facing is, I'm converting 90k lines of spaghetti code to
Pylons and looking towards the future from people that have maintained
large webapps. Just having a decent forms library (ToscaWidgets) has
made things a lot easier, though, I do miss ad_form from openacs. :)

I do appreciate the input. At least I know I'm not too far off base.

[Mike Orr]

On Sat, Apr 3, 2010 at 11:51 AM, cd34 <mcd...@gmail.com> wrote:
>  I think IBM once stated that 23 lines of code was the limit
> for bugfree code which sort of corresponds to a single screen and I've
> used that as a guideline.

I've heard something similar, that a function should be no longer than
a screenful, and a module no longer than a few printed pages. I'm not
strict about it, some of my functions go up to two or three screens,
but 20ish lines is where I start to think about extracting part of it
to another function.

It all depends on the local variables. If there's a chunk that uses
several of its own variables, it goes into a function. But if it has
lots of interactions with local variables from the surrounding code, I
don't split it. Because you can't take the whole context with you
unless you pass a lot of arguments or wrap them in an object.
(I did create a context object for an import routine yesterday, so
that I could pass several values as a unit between functions.)

Another factor is how complex the code is. Somebody (Linus Torvalds?)
said that if it's a long if-elif chain that just sets simple flags,
there's no reason to split it. But if it's complex nested loops,
that's a good reason to factor out the inner loops.

--
Mike Orr <slugg...@gmail.com>

[cd34]

My feeling is that refactoring code for the purpose of shrinking
something to an arbitrary 10-23 lines of code may also negatively
impact maintainability. Breaking a procedure into parts just for the
sake of making small pieces where you aren't violating DRY principals
just seemed counterproductive.

At some point, the complexity of a particular action in a controller
would almost demand moving some of that code to help readability, but,
I don't think I'll be overly concerned if a few actions are 40-60
lines inside the controller.

[cd34]

[Jonathan Vanasco]

On Apr 3, 2:51 pm, cd34 <mcd...@gmail.com> wrote:
>         (task_id,status) = task().bigtask().add(get_client_id(), \
>                            request.params['device_id'], \
>                            request.params['username'], \
>                            request.params['ip'], \
>                            request.params['email'], \
>                            request.params['path'], \
>                            request.params['include'])

I could be wrong, but this looks just like the stuff that i'd put into
a helper library to keep my controllers a bit cleaner...

(task_id,status) = h.newtask('add')

helpers/tasks.py
def newtask( method ):
t= task().bigtask().
if method == 'add':
return t.add(get_client_id(), \

[cd34]

On Apr 4, 1:48 pm, Jonathan Vanasco <jonat...@findmeon.com> wrote:
> I could be wrong, but this looks just like the stuff that i'd put into
> a helper library to keep my controllers a bit cleaner...
>
> (task_id,status) = h.newtask('add')
>
> helpers/tasks.py
> def newtask( method ):
>      t= task().bigtask().

While the controller is cleaner, does that really make it more
readable? From a maintainability standpoint, would someone new to the
code be able to see what that action was doing reasonably quickly?

At that point, you don't really know what newtask does unless you end
up writing a more descriptive function name, or, pass the class within
the parameter, and the action... and the parameters... :)

While the controller at that point would have 4-5 lines of code before
the response, considering that action is only used once and it isn't
duplicating code used elsewhere, all I would really be doing is moving
code from one place to another. I realize this is a philosophical
discussion and that everyone may have different opinions, but, it is
good to hear different viewpoints.

[Tim Black]

cd34 wrote:

While the controller at that point would have 4-5 lines of code before
the response, considering that action is only used once and it isn't
duplicating code used elsewhere, all I would really be doing is moving
code from one place to another.

I'm beginning to think that when I write controllers named things like getTaskHTML() or setTaskHTML() when I have a model.Task object but it doesn't have an attribute named HTML, I should refactor those methods to this form:  model.Task.HTML where HTML is a Python property or is accessible via __setattr__ and __getattribute__.  In general, if I'm writing controller methods that are merely basic manipulations of model objects, I'm thinking I should put those methods into the model rather than the controller.  Is that correct?

Tim Black

[Mike Orr]

Philosophically, the model shouldn't have any structural (WSGI- or
HTTP-specific) or UI (HTML-specific) code in it. The input values
are coming from POST parameters, which the model shouldn't know about.
The model should make sense outside a web context. So there should
be at least a thin layer validating/translating the input to generic
arguments.

--
Mike Orr <slugg...@gmail.com>

[Matt Feifarek]

On Mon, Apr 5, 2010 at 10:34 AM, Mike Orr <slugg...@gmail.com> wrote:

Philosophically, the model shouldn't have any  structural (WSGI- or
HTTP-specific) or UI (HTML-specific)  code in it.  The input values
are coming from POST parameters, which the model shouldn't know about.
 The model should make sense outside a web context.  So there should
be at least a thin layer validating/translating the input to generic
arguments.

Yeah, I'm no expert, nor zealot, but I like the above.

Sometimes by controller methods are pretty large, but sometimes managing/parsing the request is pretty large. And sometimes setting "c" for the templates is pretty large, too. To pull that out into someplace else seems pointless; gluing the request to the "business logic" is what the controller should do.

Basically, anything for me that talks to "Request" needs to be in the controller. I see helpers as basically an extension of controllers (for shortcuts) and indeed, try to keep the so-called "business logic" in model/* or even lib/*.

Of course, this is in theory; in practice, I tend to work for a panicked client and have to cruft up my controllers and templates Real-Good and think "I'll fix this later" but later rarely comes; there's always some new cruft to make on something else.

I noticed one thing that may be worth bringing up... some calm, deliberate forethought about routes.py seems to go a long way to helping keep things nicer later. It's also likely to be the first place for another dev to start learning about the app; good dispatch design will really help keep the controllers nice. FWIW.

[Tim Black]

I agree.  I should have used a more appropriate example--the one I'm working on right now is that I have a model class named Task which contains tasks arranged in a tree structure represented by means of a column named "parent" which contains the parent task's id.  I'm displaying the total of the estimated and actual hours for that task by adding up those columns for all of each task's children.  So, instead of my current controller methods named getTaskEstimatedHours() and setTaskEstimatedHours(), because the values those methods return can naturally be conceived of as attributes of a model object instance, I think I should put those methods under an accessor named model.Task.estimated_hours.  At first I left such methods out of the model, but now I'm thinking it's better to put them in the model to make the controllers less cluttered.  Is that a wise direction to go?

Tim Black

[Mike Orr]

If it's high-level logic based on model data (not on parameters or
HTML strings), the model is a good place for it. You can use ORM
class methods to return a query or result based on multiple records,
or separate functions. So, your "get" function would add up the hours
in the various records, and the "set" function would distribute the
hours to the appropriate records.

There are two approaches to the model. The minimal approach is to put
only the table definitions in it (and everything else in the
controller). The maximal approach is to push as much high-level
business logic as possible into the model. I prefer the maximal
approach.

So basically, the controller converts parameters to model structures,
and sets 'c' variables for the template. Pylons doesn't have a
separate view-logic level, so sometimes view logic is put into the
controller for convenience (rather than using a complex Python escape
in the template, which is harder to debug).

Shared controller code can be made into methods of the controller
class, put into the base controller, or into an immediate subclass. I
sometimes use such methods to copy a bunch of form result values to
'c' variables.

There are other ways to structure things if you write your own code.
For instance, you could define "view modules" -- functions the
controllers call, which in turn invoke the templates. That provides a
separate place for view logic.

You can also change the MVC model. "MVC" itself was defined for a
totally different context than web applications, and we see it's not a
perfect fit. There are several different interpretations of MVC, each
equally valid. Django calls the controller the "view", the view merely
the "template", and the framework the "controller". Some Java and
wxPython applications have a totally different way, in which the
controller passes a model object to the view, and the view manipulates
it directly to produce the result. (This goes further than just
passing an ORM object or query object to the template.) You can do
these in Pylons but you'd have to write your own code for it.

'helpers' is for little functions that are used across a wide
cross-section of templates and perhaps controllers. They tend to be
data-formatting functions, although they could also do lookups and
other things. In some ways they allow a bypass of the MVC structure
when it's worthwhile.

--
Mike Orr <slugg...@gmail.com>

[cd34]

On Apr 5, 9:36 am, Tim Black <timbla...@gmail.com> wrote:
> I'm beginning to think that when I write controllers named things like
> getTaskHTML() or setTaskHTML() when I have a model.Task object but it
> doesn't have an attribute named HTML, I should refactor those methods to
> this form:  model.Task.HTML where HTML is a Python property or is
> accessible via __setattr__ and __getattribute__

> <http://www.builderau.com.au/program/python/soa/Less-painful-getters-a...>.

> In general, if I'm writing controller methods that are merely basic
> manipulations of model objects, I'm thinking I should put those methods
> into the model rather than the controller.  Is that correct?

I started experimenting with inner classes on this current project
which is what made me wonder what other people were doing. I'm having
a slight issue with the inner class accessing a private function
defined in the outer class, but, I think overloading a base class
actually makes the code less readable in my case.

I have a class that communicates with a distributed management
system. Tasks are passed with encrypted XMLRPC. I could have done
something like:

class Tool(object):
def _communicate(self, params):

class Hammer(Tool):

class Saw(Tool):

But, that seemed less logical than:

class Tool(object):
def _communicate(self, params):

class Hammer(object):

class Saw(object):

Grouping the tasks within the main class seemed less intuitive to me.
If I used Hammer, I wouldn't know it was overloading Tool using the
first method, but, if I used inner classes, Tool().Hammer()
immediately told me that Hammer was a Tool. From a readability
standpoint, I've inferred more information. The only real issue I've
had with inner classes is calling private methods from the parent
class and that is probably something I'm doing wrong.

As for the initial question, what I've been doing is putting the code
in the controller that does the actual work. Any form or schema goes
into the model (this becomes a bit odd when dealing with callables),
and classes/functions go into lib. Last night I was debating whether
to move callables to a library because two different controllers might
use the same callable. The alternative I thought of was having
generic callables since they are populated from the controller and are
somewhat agnostic to the value that they pass to the form. That does
introduce some readability concerns, but, I haven't figured out which
way to go there.

MVC was fairly well defined by Smalltalk, but, the lines are somewhat
blurred. A strict MVC would never put code in a template, but, there
are many instances where I've broken that, and, Mako makes it
extremely easy to do so. I try not to do things that will impact
maintainability down the road and am trying to do things 'the right
way' before refactoring a ton of code.

[cd34]

controller/whatever.py
contains the functions that do the actual work, populating
tmpl_context.*, deals with the database, etc.

model/whatever.py
contains the schema for forms

model/database.py
contains the schema for the 1170 tables in this project

lib/whatever.py
contains the classes to deal with communications with the
distributed daemon, functions that massage data in predictable ways,
etc. Anything that might be called from the controller

templates/*
.mako files, etc.

What I've always debated is whether the form schemas should be a model
or in the controller. I have put the form schemas in the controllers
for testing or debugging things just to make it easier to track things
down, but, any deployed/final code is placed in model/*. I've
followed PEP-8 which has required me to make a few changes to my
coding style, but, four spaces rather than two, not using () around if
conditions, etc are not dealbreakers.

[Jonathan Vanasco]

On Apr 4, 4:28 pm, cd34 <mcd...@gmail.com> wrote:

> While the controller is cleaner, does that really make it more
> readable?  From a maintainability standpoint, would someone new to the
> code be able to see what that action was doing reasonably quickly?

If i were going for code readability, I don't know.

Usually I go for application maintainability. Looking at the original
code , this bigtask() class looked to be a logging functionality that
is used across multiple controllers and actions. i'm inclined to
refactor code like that into helper functions , so I only need to
update them in the smallest amount of places when the objects
interface / methods change. assuming bigtask() adds a new column, and
is called in a dozen places, I'd rather only edit 1 invocation of the
method to pull the request params -- not 12.

[Marius Gedminas]

On Mon, Apr 05, 2010 at 04:02:18PM -0700, cd34 wrote:
> What I've always debated is whether the form schemas should be a model
> or in the controller.

The controller, I'd say.

Marius Gedminas
--
Cool. Does it also recode ISO10646-1 pcf files into the funny
permutations and subsets used a long time ago in a galaxy far far away
on the planets Isolatinus XV and Koiruski VIII ...
-- Markus Kuhn inquires about libXft

[cd34]

On Apr 6, 10:54 am, Jonathan Vanasco <jonat...@findmeon.com> wrote:
> Usually I go for application maintainability.  Looking at the original
> code , this bigtask() class looked to be a logging functionality that
> is used across multiple controllers and actions.  i'm inclined to
> refactor code like that into helper functions , so I only need to
> update them in the smallest amount of places when the objects
> interface / methods change.  assuming bigtask() adds a new column, and
> is called in a dozen places, I'd rather only edit 1 invocation of the
> method to pull the request params -- not 12.

It actually calls a task, and each action that calls it uses different
parts of the request object. I thought about writing a wrapper class
around the wrapper class, but, adding another layer of indirection
seems like it would harm maintainability.

I thought about doing something like

(task_id,status) = task().bigtask().add(request.params)

and having .add figure out what arguments it needed.. but then I need
to pass client_id... and this calculated field... and another..

there are 8 different bigtask() classes, and each class has 8-10
methods. They are all related to task() which is the master class for
task dispatch. Each action in the controller calls each class
differently, so, there is no real repetition. I think this is one of
those situations where it can't be any cleaner while maintaining
readability.

[Ian Jamieson]

I REALLY like to keep things separated and I like my functions to be small, within reason. However I am making the assumption that myself and anyone else who comes back to change the code will be using tools that let them index or tag the source, enabeling them to hit some shortcut on a function to jump into that code to see what it does and where it comes from or where else it is used. I know lots of people work very well without these conveniences, for me though, I find it really helps me work out where things are a lot quicker.  

Either way with or without the above I don not like to scan through reams of sqlalchemy constructs in the controller....

--
You received this message because you are subscribed to the Google Groups "pylons-discuss" group.
To post to this group, send email to pylons-...@googlegroups.com.
To unsubscribe from this group, send email to pylons-discus...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/pylons-discuss?hl=en.