Re: Request a short code review

[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

[=?ISO-8859-1?Q?Andr=E9?=]

On Apr 17, 7:11 pm, [e-mail] 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é

[John Machin]

ja***@reggieband.com 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

[Ivan Illarionov]

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

[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]

En Fri, 18 Apr 2008 15:10:09 -0300, <ja***@reggieband.comescribió:

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:

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

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

pyg = (x for x in range(10) if x % 2 == 0)
pyg
<generator object at 0x00A3D580>
pyg.next()
0
pyg.next()
2
pyfor 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