By using this site, you agree to our updated Privacy Policy and our Terms of Use. Manage your Cookies Settings.
446,143 Members | 1,919 Online
Bytes IT Community
+ Ask a Question
Need help? Post your question and get tips & solutions from a community of 446,143 IT Pros & Developers. It's quick & easy.

Re: Request a short code review

P: n/a
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
Jun 27 '08 #1
Share this Question
Share on Google+
5 Replies


P: n/a
On Apr 17, 7:11 pm, 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."""
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é
Jun 27 '08 #2

P: n/a
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
Jun 27 '08 #3

P: n/a
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
Jun 27 '08 #4

P: n/a
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.
Jun 27 '08 #5

P: n/a
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

Jun 27 '08 #6

This discussion thread is closed

Replies have been disabled for this discussion.