473,231 Members | 1,503 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,231 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 1085
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: VivesProcSPL | last post by:
Obviously, one of the original purposes of SQL is to make data query processing easy. The language uses many English-like terms and syntax in an effort to make it easy to learn, particularly for...
0
by: abbasky | last post by:
### Vandf component communication method one: data sharing ​ Vandf components can achieve data exchange through data sharing, state sharing, events, and other methods. Vandf's data exchange method...
0
by: fareedcanada | last post by:
Hello I am trying to split number on their count. suppose i have 121314151617 (12cnt) then number should be split like 12,13,14,15,16,17 and if 11314151617 (11cnt) then should be split like...
0
by: stefan129 | last post by:
Hey forum members, I'm exploring options for SSL certificates for multiple domains. Has anyone had experience with multi-domain SSL certificates? Any recommendations on reliable providers or specific...
0
Git
by: egorbl4 | last post by:
Скачал я git, хотел начать настройку, а там вылезло вот это Что это? Что мне с этим делать? ...
1
by: davi5007 | last post by:
Hi, Basically, I am trying to automate a field named TraceabilityNo into a web page from an access form. I've got the serial held in the variable strSearchString. How can I get this into the...
0
by: MeoLessi9 | last post by:
I have VirtualBox installed on Windows 11 and now I would like to install Kali on a virtual machine. However, on the official website, I see two options: "Installer images" and "Virtual machines"....
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: Aftab Ahmad | last post by:
Hello Experts! I have written a code in MS Access for a cmd called "WhatsApp Message" to open WhatsApp using that very code but the problem is that it gives a popup message everytime I clicked on...

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.