7
\$\begingroup\$

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
\$\endgroup\$
3
\$\begingroup\$
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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.