473,326 Members | 2,196 Online
Bytes | Software Development & Data Engineering Community
Post Job

Home Posts Topics Members FAQ

Join Bytes to post your question to a community of 473,326 software developers and data experts.

Re: Request a short code review

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
5 1603
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
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
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
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
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 thread has been closed and replies have been disabled. Please start a new discussion.

Similar topics

2
by: Dave Patton | last post by:
I'd appreciate any feedback on http://www.elac.bc.ca/ particularly in regards to how the pages are marked up. The markup is valid HTML 4.01 strict, but that doesn't mean I've done things using...
72
by: B McDonald | last post by:
http://www.galtsvalley.com Hi all. I've recently made some major stylistic changes to my site and now it is essentially a new design with some new CSS plumbing. I am hoping that a few hardy...
18
by: Ben Hanson | last post by:
I have created an open source Notepad program for Windows in C++ that allows search and replace using regular expressions (and a few other extras). It is located at...
9
by: Adam Monsen | last post by:
I kindly request a code review. If this is not an appropriate place for my request, where might be? Specific questions are in the QUESTIONS section of the code. ...
136
by: Merrill & Michele | last post by:
A derangement is a mapping of a set onto itself leaving no element fixed. I realized that that was what I was programming when I was asked to randomly determine who buys presents for whom this...
0
by: Rick | last post by:
Hello, I'm six months into asp.net/c# and enjoying it. A couple weeks ago I needed to write a "simple" high performance counter, the short story is it turned into a mini project. Many lesson...
0
by: Sam | last post by:
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. ++ ...
3
by: vijaykumardahiya | last post by:
Dear Sir, I have two queries. First question is: I have a Html page. On which Have two buttons Submit and Issue. I want when I click on Sumit button request should be go to submit.jsp. and When I...
0
isladogs
by: isladogs | last post by:
The next Access Europe meeting will be on Wednesday 6 Mar 2024 starting at 18:00 UK time (6PM UTC) and finishing at about 19:15 (7.15PM). In this month's session, we are pleased to welcome back...
1
isladogs
by: isladogs | last post by:
The next Access Europe meeting will be on Wednesday 6 Mar 2024 starting at 18:00 UK time (6PM UTC) and finishing at about 19:15 (7.15PM). In this month's session, we are pleased to welcome back...
0
by: jfyes | last post by:
As a hardware engineer, after seeing that CEIWEI recently released a new tool for Modbus RTU Over TCP/UDP filtering and monitoring, I actively went to its official website to take a look. It turned...
1
by: PapaRatzi | last post by:
Hello, I am teaching myself MS Access forms design and Visual Basic. I've created a table to capture a list of Top 30 singles and forms to capture new entries. The final step is a form (unbound)...
1
by: CloudSolutions | last post by:
Introduction: For many beginners and individual users, requiring a credit card and email registration may pose a barrier when starting to use cloud servers. However, some cloud server providers now...
1
by: Defcon1945 | last post by:
I'm trying to learn Python using Pycharm but import shutil doesn't work
1
by: Shællîpôpï 09 | last post by:
If u are using a keypad phone, how do u turn on JavaScript, to access features like WhatsApp, Facebook, Instagram....
0
by: Faith0G | last post by:
I am starting a new it consulting business and it's been a while since I setup a new website. Is wordpress still the best web based software for hosting a 5 page website? The webpages will be...
0
isladogs
by: isladogs | last post by:
The next Access Europe User Group meeting will be on Wednesday 3 Apr 2024 starting at 18:00 UK time (6PM UTC+1) and finishing by 19:30 (7.30PM). In this session, we are pleased to welcome former...

By using Bytes.com and it's services, you agree to our Privacy Policy and Terms of Use.

To disable or enable advertisements and analytics tracking please visit the manage ads & tracking page.