Request a short code review

Discussion in 'Python' started by james@reggieband.com, Apr 17, 2008.

  1. Guest

    Hi all,

    I am moving my server based scripting over to Python and I am
    attempting to write more idiomatic python. I have 8 years
    professional programming experience and I am looking to get to a very
    high standard of python coding. As such, I was wondering if it was
    appropriate to post code snippets I am not 100% happy with in order to
    find a more elegant python way for coding.

    Here is a method I came across that I would like to clean up:


    def output_random_lesson_of_type(self, type=None):
    """Output a lesson of a specific type - if no type is passed in
    then output any type."""
    if type:
    filtered_lessons = filter(lambda x: x["type"] == type,
    self.lesson_data["lessons"])
    if filtered_lessons:
    lesson = self.output_random(filtered_lessons)
    else:
    print "Unable to find lessons of type %s." % type
    else:
    lesson = self.output_random(self.lesson_data["lessons"])
    return lesson


    Where 'type' is a string, and 'self.lesson_data["lessons"]' is an
    array of dictionaries where each item is guaranteed to have string
    value with a key 'type'

    I am not necessarily looking to make the code shorter or more
    functional or anything in particular. However if you spot something
    to improve then I am happy to learn.

    Any and all comments appreciated.

    Cheers,
    James.
    , Apr 17, 2008
    #1
    1. Advertising

  2. Guest


    > I am not necessarily looking to make the code shorter or more
    > functional or anything in particular. However if you spot something
    > to improve then I am happy to learn.


    To give an example of what I mean I have already altered the code:

    def output_random_lesson_of_type(self, type=None):
    """Output a lesson of a specific type.
    If no type is passed in then output any type."""
    output_lessons = self.lesson_data["lessons"]
    if type:
    filtered_lessons = filter(lambda x: x["type"] == type,
    self.lesson_data["lessons"])
    if filtered_lessons:
    output_lessons = filtered_lessons
    else:
    print "Unable to find lessons of type %s." % type
    return self.output_random(output_lessons)

    Changes:
    - Respected a column width of 80
    - Created the output_lessons variable, assigned it to a default.
    This remove an else statement and reduced the call to
    self.output_random to a single instance in the return statement

    Cheers,
    James
    , Apr 17, 2008
    #2
    1. Advertising

  3. André Guest

    On Apr 17, 7:11 pm, wrote:
    > > I am not necessarily looking to make the code shorter or more
    > > functional or anything in particular. However if you spot something
    > > to improve then I am happy to learn.

    >
    > To give an example of what I mean I have already altered the code:
    >
    > def output_random_lesson_of_type(self, type=None):
    > """Output a lesson of a specific type.
    > If no type is passed in then output any type."""
    > output_lessons = self.lesson_data["lessons"]
    > if type:
    > filtered_lessons = filter(lambda x: x["type"] == type,
    > self.lesson_data["lessons"])
    > if filtered_lessons:
    > output_lessons = filtered_lessons
    > else:
    > print "Unable to find lessons of type %s." % type
    > return self.output_random(output_lessons)
    >
    > Changes:
    > - Respected a column width of 80
    > - Created the output_lessons variable, assigned it to a default.
    > This remove an else statement and reduced the call to
    > self.output_random to a single instance in the return statement
    >
    > Cheers,
    > James


    I prefer, especially for longer methods, to return as soon as possible
    (thereby indicating that the code below is irrelevant to a particular
    condition). Thus, I would replace
    output_lessons = filtered lessons
    by
    return self.output_random(filtered_lessons)

    André
    André, Apr 17, 2008
    #3
  4. John Machin Guest

    wrote:
    >> I am not necessarily looking to make the code shorter or more
    >> functional or anything in particular. However if you spot something
    >> to improve then I am happy to learn.

    >
    > To give an example of what I mean I have already altered the code:
    >
    > def output_random_lesson_of_type(self, type=None):
    > """Output a lesson of a specific type.
    > If no type is passed in then output any type."""


    "any" type? Perhaps you mean all types.

    > output_lessons = self.lesson_data["lessons"]
    > if type:
    > filtered_lessons = filter(lambda x: x["type"] == type,
    > self.lesson_data["lessons"])


    filter/map/reduce/lambda is not to everyone's taste; consider using a
    list comprehension:

    filtered_lessons = [x for x in self.lesson_data["lessons"] if x["type"]
    == type]

    Now you need to go up a level ... when you find yourself using
    dictionaries with constant string keys that are words, it's time to
    consider whether you should really be using classes:

    filtered_lessons = [x for x in self.lesson_data.lessons if x.type == type]


    > if filtered_lessons:
    > output_lessons = filtered_lessons
    > else:
    > print "Unable to find lessons of type %s." % type


    So the error action is to print a vague message on stdout and choose
    from all lessons?

    > return self.output_random(output_lessons)
    >
    > Changes:
    > - Respected a column width of 80


    If you really care about people who are still using green-screen
    terminals or emulations thereof, make it < 80 -- some (most? all?)
    terminals will produce an annoying blank line if the text is exactly 80
    bytes long.


    > - Created the output_lessons variable, assigned it to a default.
    > This remove an else statement and reduced the call to
    > self.output_random to a single instance in the return statement


    .... and also makes folk who are interested in what happens in the
    error(?) case read backwards to see what lessons will be used.

    HTH,

    John
    John Machin, Apr 18, 2008
    #4
  5. On Thu, 17 Apr 2008 15:11:40 -0700, james wrote:

    >> I am not necessarily looking to make the code shorter or more
    >> functional or anything in particular. However if you spot something to
    >> improve then I am happy to learn.

    >
    > To give an example of what I mean I have already altered the code:
    >
    > def output_random_lesson_of_type(self, type=None):
    > """Output a lesson of a specific type.
    > If no type is passed in then output any type."""
    > output_lessons = self.lesson_data["lessons"] if type:
    > filtered_lessons = filter(lambda x: x["type"] == type,
    > self.lesson_data["lessons"])
    > if filtered_lessons:
    > output_lessons = filtered_lessons
    > else:
    > print "Unable to find lessons of type %s." % type
    > return self.output_random(output_lessons)
    >
    > Changes:
    > - Respected a column width of 80
    > - Created the output_lessons variable, assigned it to a default.
    > This remove an else statement and reduced the call to
    > self.output_random to a single instance in the return statement
    >
    > Cheers,
    > James


    I would write it like this:

    def output_random_lesson_of_type(self, type=None):
    """\
    Output a lesson of a specific type.
    If no type is passed in then output any type.
    """
    if type:
    return self.output_random([x for x in self.lesson_data["lessons"]
    if x["type"] == type])
    return self.output_random(self.lesson_data["lessons"])


    --
    Ivan
    Ivan Illarionov, Apr 18, 2008
    #5
  6. Sam Guest

    Hello,

    I would personally avoid using "type" as variable name, or key,
    because built-in class type already exists. As I understand your code,
    it was not your intention to override it.

    ++

    Sam
    Sam, Apr 18, 2008
    #6
  7. Guest

    Thank you all for posting insightful and useful comments.

    Here is what I have based on your input:


    def output_random_lesson_of_type(self, lesson_type=None):
    """Output a lesson of a specific type.
    If no type is passed in then output all types."""
    lessons = self.lesson_data["lessons"]
    if lesson_type:
    lessons = [x for x in lessons if x["type"] == lesson_type]
    rand_lesson = choice(lessons)
    inputs = self.create_lesson_inputs(rand_lesson)
    print rand_lesson["output"] % inputs
    return rand_lesson, inputs

    Changes:
    - generator expression instead of filter
    - removed keyword 'type' in favor of lesson_type
    - wrap to 78 columns
    - remove error-check -- allow it to fail in the choice statement.
    I've decided to make a valid lesson_type an invariant anyway
    so the appropriate thing would be an assert - but I'll settle for
    a throw from choice if it receives an empty list
    - moved self.output_random logic into this function

    The lesson data is loaded from YAML which explains why it is inside a
    dictionary instead of a class. I am currently exploring ways to allow
    me to easily switch my data providers between SQLAlchemy and YAML.

    Cheers again. This is very valuable for me. I am a true believer
    that if one takes care in the smallest of methods then the larger code-
    base will be all the better.

    James.
    , Apr 18, 2008
    #7
  8. En Fri, 18 Apr 2008 15:10:09 -0300, <> escribió:

    > lessons = self.lesson_data["lessons"]
    > if lesson_type:
    > lessons = [x for x in lessons if x["type"] == lesson_type]
    >
    > Changes:
    > - generator expression instead of filter


    The above is not a generator expression, it's a list comprehension. They look similar.
    This is a list comprehension:

    py> a = [x for x in range(10) if x % 2 == 0]
    py> a
    [0, 2, 4, 6, 8]

    Note that it uses [] and returns a list. This is a generator expression:

    py> g = (x for x in range(10) if x % 2 == 0)
    py> g
    <generator object at 0x00A3D580>
    py> g.next()
    0
    py> g.next()
    2
    py> for item in g:
    .... print item
    ....
    4
    6
    8

    Note that it uses () and returns a generator object. The generator is a block of code that runs lazily, only when the next item is requested. A list is limited by the available memory, a generator may yield infinite items.

    > Cheers again. This is very valuable for me. I am a true believer
    > that if one takes care in the smallest of methods then the larger code-
    > base will be all the better.


    Sure.

    --
    Gabriel Genellina
    Gabriel Genellina, Apr 20, 2008
    #8
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. David Geering

    longs, long longs, short short long ints . . . huh?!

    David Geering, Jan 8, 2007, in forum: C Programming
    Replies:
    15
    Views:
    546
    Keith Thompson
    Jan 11, 2007
  2. Replies:
    4
    Views:
    803
    Kaz Kylheku
    Oct 17, 2006
  3. www
    Replies:
    51
    Views:
    1,456
  4. Ioannis Vranos

    unsigned short, short literals

    Ioannis Vranos, Mar 4, 2008, in forum: C Programming
    Replies:
    5
    Views:
    660
    Eric Sosman
    Mar 5, 2008
  5. John G Harris

    Crockford's 'The Good Parts' : a short review

    John G Harris, Jul 11, 2009, in forum: Javascript
    Replies:
    21
    Views:
    254
    John G Harris
    Jul 15, 2009
Loading...

Share This Page