Request a short code review

[John Machin]

james at 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