473,322 Members | 1,409 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,322 software developers and data experts.

code critique requested - just 60 lines

Hi, I would like some feedback on how you would improve the following
program:
http://www.bitbucket.org/metaperl/pt...074f/payout.py

Basically, using non-strict dictionary keys can lead to bugs, so that
worried me. Also, I'm not sure that my code is as crisp and concise as
it could be. I also did not like the long string representation in the
Scenerio class. It is hard to read and error-prone to code.

Any feedback on how you would've written this differently is welcome,
either by commenting below, or by checking out the repo and checking
it back in!

class Rates:

def __init__(self, per_click, per_ref_click):
self.per_click = per_click
self.per_ref_click = per_ref_click

def __str__(self):
return 'per_click: %.2f per_ref_click: %.2f' %
(self.per_click, self.per_ref_click)
ad_rate = 200 # 2 dollars for 100 clicks

# http://code.activestate.com/recipes/278259/
def sumDict(d):
return reduce(lambda x,y:x+y, d.values())
rates = {}
rates['std'] = Rates(per_click=1, per_ref_click=0.5)
rates['vip'] = Rates(per_click=1.25, per_ref_click=1.25)

class Scenario:

def __init__(self, std_clicks, vip_clicks, upline_status):
self.clicks = {}
self.payout = {}
self.ad_rate = 200

self.clicks['std'] = std_clicks
self.clicks['vip'] = vip_clicks
self.upline_status = upline_status

def calc_profit(self):
for member_type in rates:
self.payout[member_type] = self.clicks[member_type] *
rates[member_type].per_click
if self.upline_status is None:
self.payout['upline'] = 0
else:
self.payout['upline'] = sumDict(self.clicks) *
rates[upline_status].per_ref_click
#print "rates: %s self.clicks: %d upline payout: %.1f\n" %
(rates[upline_status], sumDict(self.clicks), self.payout['upline'])
return ad_rate - sumDict(self.payout)
def __str__(self):
profit = self.calc_profit()
return 'upline_status: %s upline_payout: %.1f\n\tstd_clicks:
%d std_payout %.1f vip_clicks: %d vip_payout: %.1f\n\t\tProfit: %.1f
\n' % (self.upline_status, self.payout['upline'], self.clicks['std'],
self.payout['std'], self.clicks['vip'], self.payout['vip'], profit)

scenario = []

for upline_status in [None, 'std', 'vip']:
for vip_clicks in [0, 25, 50, 75, 100]:
std_clicks = 100 - vip_clicks
scenario.append(Scenario(std_clicks, vip_clicks,
upline_status))

# really, we could've printed the objects as they were made, but for
debugging, I kept creation and
# printing as separate steps
for s in scenario:
print s
Oct 2 '08 #1
8 1088
On Thu, 02 Oct 2008 07:51:30 -0700, Terrence Brannon wrote:
Hi, I would like some feedback on how you would improve the following
program:
http://www.bitbucket.org/metaperl/pt...074f/payout.py

Well, for starters, I'd say that's the WORST implementation of Quicksort
I've every seen!!!

*wink*

Seriously, the first thing I'd say is that you need to document what the
program is supposed to do! It's a bit much to expect us to guess what it
does, and then see if it does what we think it should do. What's the
purpose of the program?
Basically, using non-strict dictionary keys can lead to bugs, so that
worried me.
What's a "non-strict dictionary key"?
--
Steven
Oct 2 '08 #2
Terrence Brannon, I suggest you to shorten a lot some of those very
long lines.
# http://code.activestate.com/recipes/278259/
def sumDict(d):
return reduce(lambda x,y:x+y, d.values())
Not all recipes are good, and that looks bad in various ways. Try
this:

def sumDictValues(d):
return sum(d.itervalues())

But probably it's better to inline that operation.

Bye,
bearophile
Oct 2 '08 #3
On Thu, 02 Oct 2008 07:51:30 -0700, Terrence Brannon wrote:
Hi, I would like some feedback on how you would improve the following
program:
http://www.bitbucket.org/metaperl/pt...074f/payout.py
Okay, I've read over the code, and tried to guess from context what it is
supposed to do. I've added documentation (based on my guesses!) and
modified the code in a few places. I've tried not to radically change the
overall design of your code.

Some comments:

Readable is more important than concise. There's no prize for reducing
the number of lines or leaving out comments.

I'm not really sure that your Scenario code gains much by being based on
class instances. Perhaps ordinary functions would be easier to
understand? That's probably a matter of taste though.

I don't consider it a good idea for __str__ to be used for complicated
formatted results. In general, I would expect str(obj) to return a simple
string version of the object. (Note that "simple" doesn't necessarily
mean short.)

Your Rates class is simple enough that you could probably replace it with
a plain tuple:

rates = { # lookup table for standard and VIP payment rates.
'std': (1, 0.5), 'vip': (1.25, 1.25)}

And then later in the code rates[member_type].per_click would become
rates[member_type][0]. I wouldn't do that for more than two fields per
record.

An even better idea would be the NamedTuple found here:
http://code.activestate.com/recipes/500261/

which is included in Python 2.6. For your simple purposes, the class Rate
is probably good enough.

Here's my code, untested so there's no guarantee it will work. Also,
because I'm lazy I haven't tried to fix long lines that have word-
wrapped, sorry.

I make no claim of copyright on the following, feel free to use it or
discard it.

======

class Rates:
"""Record holding two fields.

The fields are payment rates in cents per click and per
referred(?) click. (FIXME : what is per_ref_click???)
"""
def __init__(self, per_click, per_ref_click):
self.per_click = per_click
self.per_ref_click = per_ref_click

def __str__(self):
return '<Record: (%.2f, %.2f)>' % (self.per_click,
self.per_ref_click)

ad_rate = 200 # 2 dollars for 100 clicks FIXME: is this actually used?

rates = { # lookup table for standard and VIP payment rates.
'std': Rates(per_click=1, per_ref_click=0.5),
'vip': Rates(per_click=1.25, per_ref_click=1.25)
}

class Scenario:
"""Scenerio is a financial What-If calculation."""
def __init__(self, std_clicks, vip_clicks, upline_status):
"""Initialise an instance with:
std_clicks = the number of standard clicks to be paid (???)
vip_clicks = the number of VIP clicks to be paid (???)
upline_status = one of None, "std" or "vip"
(but I don't know what the different statuses mean...)
"""
self.clicks = {'std': std_clicks, 'vip' = vip_clicks}
self.payout = {}
self.ad_rate = 200
self.upline_status = upline_status
self.setup_payup()

def setup_payup(self):
"""Set up the payout dictionary."""
for member_type in rates: # FIXME: should rates be global?
self.payout[member_type] = self.clicks[member_type] * rates
[member_type].per_click
if self.upline_status is None:
self.payout['upline'] = 0
else:
self.payout['upline'] = sum(self.clicks.values()) * rates
[upline_status].per_ref_click

def calc_profit(self):
"""Return the profit made."""
return self.ad_rate - sum(self.payout.values())

def display(self):
"""Pretty print a nicely formatted table of the scenario."""
profit = self.calc_profit()
template = """
Scenario: <no description>
================================================== =========
Upline status: %5s Upline payout: %.1f
Std clicks: %5d Std payout: %.1f
VIP clicks: %5d VIP payout: %.1f
Profit: %.1f
"""
s = self # alias to save space
return template % (s.upline_status, s.payout['upline'],
s.clicks['std'], s.payout['std'], s.clicks['vip'],
s.payout['vip'], profit)

if __name__ == '__main__':
# Set up some scenarios.
scenarios = []
for upline_status in [None, 'std', 'vip']:
for vip_clicks in [0, 25, 50, 75, 100]:
std_clicks = 100 - vip_clicks
scenarios.append(
Scenario(std_clicks, vip_clicks, upline_status)
)
# And display them.
for s in scenarios:
print s.display()
Oct 2 '08 #4
On Thu, 02 Oct 2008 07:51:30 -0700, Terrence Brannon wrote:
Hi, I would like some feedback on how you would improve the following
program:
http://www.bitbucket.org/metaperl/pt...074f/payout.py

Basically, using non-strict dictionary keys can lead to bugs, so that
worried me. Also, I'm not sure that my code is as crisp and concise as
it could be. I also did not like the long string representation in the
Scenerio class. It is hard to read and error-prone to code.

Any feedback on how you would've written this differently is welcome,
either by commenting below, or by checking out the repo and checking it
back in!

class Rates:

def __init__(self, per_click, per_ref_click):
self.per_click = per_click
self.per_ref_click = per_ref_click
You could use a better name, something that contains "price" or "cost" or
something on that line, it is not immediately obvious what is per_click.
def __str__(self):
return 'per_click: %.2f per_ref_click: %.2f' %
(self.per_click, self.per_ref_click)
ad_rate = 200 # 2 dollars for 100 clicks

# http://code.activestate.com/recipes/278259/ def sumDict(d):
return reduce(lambda x,y:x+y, d.values())
rates = {}
rates['std'] = Rates(per_click=1, per_ref_click=0.5) rates['vip'] =
Rates(per_click=1.25, per_ref_click=1.25)
It's not a really good idea to interleave module-level code and class/
function declarations. Python's code usually put all module-level code
below all declarations. (of course there are exceptions too, such as
higher level functions, although now there are decorators to avoid it)
class Scenario:

def __init__(self, std_clicks, vip_clicks, upline_status):
self.clicks = {}
self.payout = {}
self.ad_rate = 200

self.clicks['std'] = std_clicks
self.clicks['vip'] = vip_clicks
self.upline_status = upline_status

def calc_profit(self):
for member_type in rates:
Global variables... avoid them unless you have to...
It's better (in this case) to pass rates to the __init__ function.
self.payout[member_type] = self.clicks[member_type] *
rates[member_type].per_click
if self.upline_status is None:
self.payout['upline'] = 0
else:
self.payout['upline'] = sumDict(self.clicks) *
rates[upline_status].per_ref_click
#print "rates: %s self.clicks: %d upline payout: %.1f\n" %
(rates[upline_status], sumDict(self.clicks), self.payout['upline'])
return ad_rate - sumDict(self.payout)
def __str__(self):
profit = self.calc_profit()
return 'upline_status: %s upline_payout: %.1f\n\tstd_clicks:
%d std_payout %.1f vip_clicks: %d vip_payout: %.1f\n\t\tProfit: %.1f \n'
% (self.upline_status, self.payout['upline'], self.clicks['std'],
self.payout['std'], self.clicks['vip'], self.payout['vip'], profit)
Ewww.... NEVER WRITE A LINE THAT LONG... (and with \n)
Instead python have multi-line string: '''blah blah''' or """blah blah"""

A good practice is to limit a line to be < 80 chars.
scenario = []

for upline_status in [None, 'std', 'vip']:
rather than using None, you should use another string (perhaps 'N/A',
'Nothing'), this way the code in your class above wouldn't have to
special case None.
for vip_clicks in [0, 25, 50, 75, 100]:
std_clicks = 100 - vip_clicks
scenario.append(Scenario(std_clicks, vip_clicks,
upline_status))

# really, we could've printed the objects as they were made, but for
debugging, I kept creation and
# printing as separate steps
for s in scenario:
print s
Separating object creation (model) and printing (view) is a good thing,
it's a step toward MVC (Model, View, Controller).

Your code could use some documentation/comments.

Oct 2 '08 #5
On Oct 2, 11:56*am, bearophileH...@lycos.com wrote:
Terrence Brannon, I suggest you to shorten a lot some of those very
long lines.
yes, I wanted to, but was not sure how to continue a line on the next
line in Python.
Oct 3 '08 #6
On Oct 2, 11:09*am, Steven D'Aprano <st...@REMOVE-THIS-
cybersource.com.auwrote:
On Thu, 02 Oct 2008 07:51:30 -0700, Terrence Brannon wrote:
>
Basically, using non-strict dictionary keys can lead to bugs, so that
worried me.

What's a "non-strict dictionary key"?
In Perl, you can pre-define what keys are allowed in a dictionary.
That way, mis-spelling the dict key doesnt lead to accessing something
didnt mean to.

Oct 3 '08 #7
In message
<cd**********************************@x41g2000hsb. googlegroups.com>,
Terrence Brannon wrote:
On Oct 2, 11:56Â*am, bearophileH...@lycos.com wrote:
>Terrence Brannon, I suggest you to shorten a lot some of those very
long lines.

yes, I wanted to, but was not sure how to continue a line on the next
line in Python.
Did you check the manual?

<http://docs.python.org/reference/lexical_analysis.html>
Oct 5 '08 #8
En Fri, 03 Oct 2008 05:07:41 -0300, Terrence Brannon <me******@gmail.com>
escribió:
On Oct 2, 11:56*am, bearophileH...@lycos.com wrote:
>Terrence Brannon, I suggest you to shorten a lot some of those very
long lines.

yes, I wanted to, but was not sure how to continue a line on the next
line in Python.
Having ANY open () or [] or {} is enough to implicitely continue a line
(being it a function call, a list definition, a generator expression,
whatever...)

tags = { 'S': 'Small',
'M': 'Medium',
'L': 'Large',
'XL': 'Extra large',
}

Also, you may use \ as the LAST character (immediately preceding the
newline) to continue the logical line on the next physical line:

result = coef[0] * sum_deliverd + \
coef[1] * max_income + \
coef[2] * min_delay

--
Gabriel Genellina

Oct 6 '08 #9

This thread has been closed and replies have been disabled. Please start a new discussion.

Similar topics

7
by: Cynthia Turcotte | last post by:
Hi all -- A client has hired me to, among other things, optimize her web site for search engine submission. So being the dutiful SEO geek that I am, I went through and optimized each and every...
49
by: lime | last post by:
I have addressed most of the issues listed in the responses to my last post "Critique CSS layout (1st go - fingers crossed)". I'm rapt with the progress I have made, thanks to all for your past...
54
by: Martin Eisenberg | last post by:
Hi, I've written a program that I'd like to hear some opinions on regarding my C++ usage. As a program it's smallish, but on Usenet 300 lines seem a bit much. Do you think it's appropriate to...
19
by: TC | last post by:
Are there any good sites or forums for a web critique? I went to alt.html.critique and it's pretty dead.
4
by: rihad | last post by:
Hi there. If you remember my recent posting regarding fslurp(), well, this is its continuation :) A collection of useful and semi-useful file-and-line-slurping functions: fslurp() - slurp a text...
188
by: christopher diggins | last post by:
I have posted a C# critique at http://www.heron-language.com/c-sharp-critique.html. To summarize I bring up the following issues : - unsafe code - attributes - garbage collection -...
26
by: Martin Jørgensen | last post by:
Hi, I don't understand these errors I get: g++ Persort.cpp Persort.cpp: In function 'int main()': Persort.cpp:43: error: name lookup of 'j' changed for new ISO 'for' scoping Persort.cpp:37:...
10
by: vonbreslau | last post by:
Hello C-landers, can you find any improvement or critique to this program? #include <stdio.h> #define ZER 0 #define LIM 7 /* Sort numbers according to its increasing size */ void e(void);...
3
by: cs | last post by:
Hi, I'm new to C and would appreciate any feedback on the following program, asplit, which splits a file into 2 new files, putting a certain number of lines in the first file, and all the rest...
0
by: DolphinDB | last post by:
Tired of spending countless mintues downsampling your data? Look no further! In this article, you’ll learn how to efficiently downsample 6.48 billion high-frequency records to 61 million...
0
by: ryjfgjl | last post by:
ExcelToDatabase: batch import excel into database automatically...
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: Vimpel783 | last post by:
Hello! Guys, I found this code on the Internet, but I need to modify it a little. It works well, the problem is this: Data is sent from only one cell, in this case B5, but it is necessary that data...
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.