code critique requested - just 60 lines

[Lie Ryan]

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/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.