code critique requested - just 60 lines

[Terrence Brannon]

Hi, I would like some feedback on how you would improve the following
program:
http://www.bitbucket.org/metaperl/ptc_math/src/21979c65074f/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

 

[Steven D'Aprano]

Hi, I would like some feedback on how you would improve the following
program:
http://www.bitbucket.org/metaperl/ptc_math/src/21979c65074f/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"?

 

[bearophileHUGS]

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

 

[Steven D'Aprano]

Hi, I would like some feedback on how you would improve the following
program:
http://www.bitbucket.org/metaperl/ptc_math/src/21979c65074f/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()

 

[Lie Ryan]

Hi, I would like some feedback on how you would improve the following
program:
http://www.bitbucket.org/metaperl/ptc_math/src/21979c65074f/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.

 

[Terrence Brannon]

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.

 

[Gabriel Genellina]

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

 

[Terrence Brannon]

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.

 

[Lawrence D'Oliveiro]

In message

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>