Request a short code review

[james]

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.

 

[james]

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

 

[André]

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é

 

[John Machin]

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

 

[Ivan Illarionov]

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"])

 

[Sam]

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

 

[james]

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.

 

[Gabriel Genellina]

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.