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. -- http://mail.python.org/mailman/listinfo/python-list
