code critique requested - just 60 lines

[Steven D'Aprano]

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

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()