Matthew Bevan wrote:
> I'd forgotten I'd written that!
>
>> I think I'm following the meat of this very interesting page
>> (http://trac.turbogears.org/turbogears/wiki/PassingArgumentsToCallables),
>> except for this:
>>
>> def make_list(c, none=None, *args, **kwargs):
>> def _call():
>> return [ [], [('', none)] ][none is not None] + \
>> c.build_list(*args, **kwargs)
>> return _call
>
>
> I agree, there needs to be more documentation on what the arguments do.
A better choice of name than "none" would help. How about
"defaultChoice" or "initialSelection"? "none" obfuscates the code by
making it difficult to intuit the purpose of the index expression. It
was also somewhat obfuscatory to prefer
[ [], [('', none)] ][none is not None]
to
[[('', none)], [] ][none is None]
> However, in the above, by passing an argument named "none" you can
> define an empty list item which appears at the front of the list. Take
> an example of a list of countries... with no none argument:
>
> [Australia,
> Canada,
> United States]
>
> If you wanted it to have an empty item at the top, set none to "" (an
> empty string). Doing so would produce:
>
> [,
> Australia,
> Canada,
> United States]
>
> If you wanted something a little more verbose, just pass a string that
> isn't empty:
>
> [Please select a country.,
> Australia,
> Canada,
> United States]
>
> Passing that argument to the make_list function allows for the top
> "empty" value to be customized on each call.
>
I will go out on a limb and suggest that while the code is perfectly
functional it isn't really Pythonic. Otherwise an apparently competent
newcomer would have been able to understand it. The fact that it took so
long to explain is a sign of some sort of code smell.
I wonder why it was considered preferable to the more obvious
def make_list(c, none=None, *args, **kwargs):
def _call():
if none is None:
l = []
else:
l = [('', none)]
return l + c.build_list(*args, **kwargs)
return _call
If efficiency were any kind of consideration (which I am prepared to
believe without examining context that it isn't) then the generated code
might lead you to prefer the published version:
>>> f1 = make_list(c)
>>> dis.dis(f1)
3 0 BUILD_LIST 0
3 LOAD_CONST 1 ('')
6 LOAD_DEREF 1 (none)
9 BUILD_TUPLE 2
12 BUILD_LIST 1
15 BUILD_LIST 2
18 LOAD_DEREF 1 (none)
21 LOAD_CONST 0 (None)
24 COMPARE_OP 9 (is not)
27 BINARY_SUBSCR
4 28 LOAD_DEREF 0 (c)
31 LOAD_ATTR 1 (build_list)
34 LOAD_DEREF 2 (args)
37 LOAD_DEREF 3 (kwargs)
40 CALL_FUNCTION_VAR_KW 0
43 BINARY_ADD
44 RETURN_VALUE
Comparing this with the "more comprehensible" version we see:
Now, look at the code that you generate in the original:
>>> f1 = make_list(c)
>>> dis.dis(f1)
3 0 LOAD_DEREF 2 (none)
3 LOAD_CONST 0 (None)
6 COMPARE_OP 8 (is)
9 JUMP_IF_FALSE 10 (to 22)
12 POP_TOP
4 13 BUILD_LIST 0
16 STORE_FAST 0 (l)
19 JUMP_FORWARD 16 (to 38)
>> 22 POP_TOP
6 23 LOAD_CONST 1 ('')
26 LOAD_DEREF 2 (none)
29 BUILD_TUPLE 2
32 BUILD_LIST 1
35 STORE_FAST 0 (l)
7 >> 38 LOAD_FAST 0 (l)
41 LOAD_DEREF 0 (c)
44 LOAD_ATTR 1 (build_list)
47 LOAD_DEREF 1 (args)
50 LOAD_DEREF 3 (kwargs)
53 CALL_FUNCTION_VAR_KW 0
56 BINARY_ADD
57 RETURN_VALUE
So there's more code, even though it doesn't necessarily execute at a
vastly different speed. However it would surely be even better, and
hardly less comprehensible, to use two entirely separate functions:
def make_list(c, none=None, *args, **kwargs):
if none is None:
def _call():
return c.build_list(*args, **kwargs)
else:
def _call():
return [('', none)] + c.build_list(*args, **kwargs)
return _call
Bear in mind that only one of the def's will be executed here, so we
should look at the two cases separately:
>>> f1 = make_list(c)
>>> dis.dis(f1)
4 0 LOAD_DEREF 0 (c)
3 LOAD_ATTR 0 (build_list)
6 LOAD_DEREF 1 (args)
9 LOAD_DEREF 2 (kwargs)
12 CALL_FUNCTION_VAR_KW 0
15 RETURN_VALUE
>>> f2 = make_list(c, "Please select")
>>> dis.dis(f2)
7 0 LOAD_CONST 1 ('')
3 LOAD_DEREF 0 (none)
6 BUILD_TUPLE 2
9 BUILD_LIST 1
12 LOAD_DEREF 2 (c)
15 LOAD_ATTR 0 (build_list)
18 LOAD_DEREF 1 (args)
21 LOAD_DEREF 3 (kwargs)
24 CALL_FUNCTION_VAR_KW 0
27 BINARY_ADD
28 RETURN_VALUE
However, I believe (without testing) this could - and should! - be
further refactored to
def make_list(c, none=None, *args, **kwargs):
if none is None:
return c.build_list
else:
def _call():
return [('', none)] + c.build_list(*args, **kwargs)
return _call
In this case the same result is given for the case when none is not
None. But look at the no-default choice:
>>> f1 = make_list(c)
>>> dis.dis(f1)
3 0 BUILD_LIST 0
3 RETURN_VALUE
I realise that this will probably seem like nit-picking over a minor
code sample, but it's important to remember why we are using Python in
the first place. Simplicity and readability *do* matter, particularly
when you are trying to establish a user base for a project where the
code *is* the documentation.
TG has chosen a web server which essentially comes without
documentation, and this makes it very hard to get into the project even
when one has coding skills. So let's not make things worse by slinging
obfuscatory code around. The improved performance can just be considered
a minor bonus <0.8 wink>.
I've seen remarks to the effect that "only lack of documentation stops
TG from going to 1.0", and I look forward to seeing (and using) the
finished project. I do feel, though, that both CherryPy and other TG
components have underestimated the effort required to produce good
usable documentation. In its absence, the examples need to be as clean
and helpful as possible.
Just so's you know I am not taking aim solely at TG, I was recently
frustrated to see that the SVN version of CherryPy still has a tutorial
example for the default method that includes the comment
"""
Using this mechanism you can easily simulate virtual URI structures
by parsing the extra URI string, which you can access through
cherrypy.request.virtualPath.
"""
despite the fact that the virtualPath attribute no longer appears to be
used and the code in the example doesn't attempt to use it. It's this
kind of sloppiness that ultimately discourages the sort of users that it
would be helpful to recruit.
Sorry, this didn't start out as a rant. Maybe I should put it on my
blog ...
regards
Steve
--
Steve Holden +44 150 684 7255 +1 800 494 3119
Holden Web LLC/Ltd http://www.holdenweb.com
Skype: holdenweb http://holdenweb.blogspot.com
Recent Ramblings http://del.icio.us/steve.holden
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"TurboGears" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at http://groups.google.com/group/turbogears
-~----------~----~----~----~------~----~------~--~---
