Problem
The background is my question is SO How to create a list of totals for durations?
where I found a recursive solution that I want to review. I could not test it for higher levels than 1 but to the best of my knowledge it should work.
What I’m trying to do is calculate a kind of “highscore”, i.e. a level based on sales during the two historical months where sales where the most. So I find the oldest purchase, use the helper methods to calculate the bonus for that period and then check to see whether that period is the entitling to a level upgrade. It’s basically going through all the historical orders.
Is my recursion written correctly?
def level(self):
startdate = model.Order.all().filter('status =', 'PAID'
).filter('distributor_id =',
self._key.id()).get().created.date()
last_day_nextmonth = startdate + relativedelta(months=2)
- timedelta(days=1)
total = self.personal_silver(startdate, last_day_nextmonth) + self.non_manager_silver(startdate, last_day_nextmonth)
if total >= 125:
level = 5
elif total >= 75:
level = 4
elif total >= 25:
level = 3
elif total >= 2:
level = 2
else:
level = 1
return self.levelHelp(level, last_day_nextmonth + timedelta(days=1))
def levelHelp(self, level, startdate):
last_day_nextmonth = startdate + relativedelta(months=2)
- timedelta(days=1)
total = self.personal_silver(startdate, last_day_nextmonth) + self.non_manager_silver(startdate, last_day_nextmonth)
if total >= 125:
newlevel = 5
elif total >= 75:
newlevel = 4
elif total >= 25:
newlevel = 3
elif total >= 2:
newlevel = 2
else:
newlevel = 1
maxlevel = level if level > newlevel else newlevel
nextstart = last_day_nextmonth + timedelta(days=1)
now = datetime.now().date()
if nextstart > now: #next start in is the future
return maxlevel
else: return self.levelHelp(maxlevel, nextstart)
The helper functions are
def non_manager_silver(self, startdate, enddate):
silver = 0
timeline = date(startdate.year, startdate.month, 1)
downline = User.query(User.sponsor == self._key).fetch()
distributor = self
while distributor.has_downline():
downline = User.query(User.sponsor
== distributor.key).fetch()
for person in downline:
orders = model.Order.all().filter('distributor_id =',
person.key.id()).filter('created >',
startdate).filter('created <',
enddate).filter('status =', 'PAID'
).fetch(999999)
for order in orders:
for (idx, item) in enumerate(order.items):
purchase = model.Item.get_by_id(long(item.id()))
amount = int(order.amounts[idx])
silver = silver + amount * purchase.silver
/ 1000.000
distributor = person
return silver
def personal_silver(self, startdate, enddate):
p_silver = 0
timeline = date(startdate.year, startdate.month, 1) # first day of month
orders = model.Order.all().filter('distributor_id =',
self._key.id()).filter('created >',
timeline).filter('created <', enddate).filter('status ='
, 'PAID').fetch(999999)
for order in orders:
for (idx, item) in enumerate(order.items):
purchase = model.Item.get_by_id(long(item.id()))
amount = int(order.amounts[idx])
p_silver = p_silver + amount * purchase.silver
/ 1000.000 # remember amounts
return p_silver
Solution
def level(self):
startdate = model.Order.all().filter('status =', 'PAID'
).filter('distributor_id =',
self._key.id()).get().created.date()
last_day_nextmonth = startdate + relativedelta(months=2)
- timedelta(days=1)
Continuing lines with is generally frowned upon. Either split it onto multiple lines or wrap the expression with parenthesis
total = self.personal_silver(startdate, last_day_nextmonth) + self.non_manager_silver(startdate, last_day_nextmonth)
I think writing a self.silver(start_date, end_date) function would make sense to get the sum of those two.
if total >= 125:
level = 5
elif total >= 75:
level = 4
elif total >= 25:
level = 3
elif total >= 2:
level = 2
else:
level = 1
I’d store all of these levels in a list like [0, 2, 25, 75, 125]. Then I’d use functions from bisect to figure out which level I was at.
return self.levelHelp(level, last_day_nextmonth + timedelta(days=1))
def levelHelp(self, level, startdate):
last_day_nextmonth = startdate + relativedelta(months=2)
- timedelta(days=1)
total = self.personal_silver(startdate, last_day_nextmonth) + self.non_manager_silver(startdate, last_day_nextmonth)
if total >= 125:
newlevel = 5
elif total >= 75:
newlevel = 4
elif total >= 25:
newlevel = 3
elif total >= 2:
newlevel = 2
else:
newlevel = 1
Deja Vu! You’ve got the exact same code again, at the very least, move the common code into another function.
maxlevel = level if level > newlevel else newlevel
Use maxlevel = max(level, newlevel)
nextstart = last_day_nextmonth + timedelta(days=1)
now = datetime.now().date()
if nextstart > now: #next start in is the future
return maxlevel
else: return self.levelHelp(maxlevel, nextstart)
You shouldn’t really be using recursion here. This is very naturally a while loop. I think if you rewrite this as a while loop, it’ll be clearer and you’ll be able to combine these two functions.
def non_manager_silver(self, startdate, enddate):
silver = 0
timeline = date(startdate.year, startdate.month, 1)
I’m not clear why you back this up to the start of the month rather then using the startdate.
downline = User.query(User.sponsor == self._key).fetch()
This has no effect, because you throw it away as soon as you enter the loop
distributor = self
while distributor.has_downline():
downline = User.query(User.sponsor
== distributor.key).fetch()
for person in downline:
orders = model.Order.all().filter('distributor_id =',
person.key.id()).filter('created >',
startdate).filter('created <',
enddate).filter('status =', 'PAID'
).fetch(999999)
Why 999999?
for order in orders:
for (idx, item) in enumerate(order.items):
No need for those parens.
purchase = model.Item.get_by_id(long(item.id()))
amount = int(order.amounts[idx])
Using int and long seems a little odd. Are you converting from strings?
silver = silver + amount * purchase.silver
/ 1000.000
Use silver += amount * purchase.silver / 1000.000
distributor = person
See, here is where you should be using recursion. I assume that you are trying to go through the downlines of all the downlines. However, as your code is written, you’ll only go through the downlines of the last member of the downline.
return silver
def personal_silver(self, startdate, enddate):
p_silver = 0
timeline = date(startdate.year, startdate.month, 1) # first day of month
orders = model.Order.all().filter('distributor_id =',
self._key.id()).filter('created >',
timeline).filter('created <', enddate).filter('status ='
, 'PAID').fetch(999999)
for order in orders:
for (idx, item) in enumerate(order.items):
purchase = model.Item.get_by_id(long(item.id()))
amount = int(order.amounts[idx])
p_silver = p_silver + amount * purchase.silver
/ 1000.000 # remember amounts
return p_silver
This function is pretty much exactly the same as the previous function. You should be able to move common parts into a function.
I’d approach this whole thing quite differently.
def find_downline(distributor):
"""
Given a distributor, collect all the members of his downline into a list
"""
downline = [distributor]
immediate_downline = User.query(User.sponsor == distributor.key)
for member in immediate_downline:
downline.extend( find_downline(member) )
return downline
def find_orders(distributor):
return model.Order.all().filter('distributor_id =',
self._key.id()).filter('status ='
, 'PAID').fetch()
def silver_in_order(order):
total = 0
for item, amount in zip(order.items, order.amounts):
purchase = model.Item.get_by_id(long(item.id()))
total += amount * purchase.silver
return total
def silver_bimonth(distributor):
bimonths = collections.defaultdict(int)
for member in find_downline(distributor):
for order in find_orders(member):
date = order.created.date
bimonth_index = (date.year * 12 + date.month) / 2
bimonths[bimonth_index] += silver_in_order(order)
return = max(bimonths.value)
def level(distributor):
silver = silver_bimonth(distributor)
return bisect.bisect_left(LEVELS, silver) + 1
Zero testing. Probably doesn’t work. But it should give an idea of an alternate approach. Basically, I figure that since you are going to fetch all the orders anyways, it makes more sense to fetch them all figure which bimonthly period they belong to, and figure out the maximum that way.