3
\$\begingroup\$

I wanted to create a function describing a sport game called "Leader". The idea is that you make as many push-ups as you can, increasing each repetition by 1 and as you reach your maximum, each next repetition is decreased by 1 until you reach 0 push-ups eventually.

I managed to do this using dictionaries, but I think this could be done in much easier way.

from typing import List, Tuple


def leader_step(max_pushups, step): # maximum pushups a person can do and a step of increment
    i = 0  # count of the repetitions
    pushups: List[Tuple[int, int]] = [(0, 0)]  # number of pushups at the beginning (at each repetition, it total)
    while pushups[i][0] <= max_pushups + abs(step): # +abs(step) in case step > 1
        if pushups[i][0] >= max_pushups:  # decrease push-ups as they reach max
            step = -step
        i += 1
        now = step + pushups[i - 1][0]
        sum = now + pushups[i - 1][1]  # counting the sum of all push-ups by adding previous sum and current pushups
        pushups.insert(i, (now, sum))
        if pushups[i][0] < 1:  # game stops when you reach 0 push-up
            break
    return pushups[1:-1]

Function should return 2 sequences:

  1. showing the number of push-ups at each repetition
  2. showing total sum of push-ups made at each repetition
\$\endgroup\$
  • \$\begingroup\$ I think you should have more descriptive variable names. \$\endgroup\$ – Justin Jun 22 at 8:51
  • 1
    \$\begingroup\$ Please don't change the question in a way that would invalidate an answer: codereview.stackexchange.com/help/editing \$\endgroup\$ – dfhwze Jun 22 at 14:03
  • 1
    \$\begingroup\$ Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 Jun 22 at 14:07
8
\$\begingroup\$

You can indeed simplify this quite a bit using a generator and the itertools module.

I would separate out the generating of the pushups to be done from the total pushups. For this you can use two range objects and the yield from (Python 3.3+) keyword combination:

def pushups(n):
    yield from range(1, n)
    yield from range(n, 0, -1)

The accumulation can be done using itertools.accumulate and itertools.tee to duplicate the generator:

from itertools import accumulate, tee

def leader_step(n):
    gen1, gen2 = tee(pushups(n))
    return list(gen1), list(accumulate(gen2))

if __name__ == "__main__":
    print(leader_step(5))
# ([1, 2, 3, 4, 5, 4, 3, 2, 1], [1, 3, 6, 10, 15, 19, 22, 24, 25])

As noted in the comments by @Peilonrayz, it is not actually necessary to split the generator (as long as it fits into memory, which is very likely, given that presumably a human will try to do this training):

def leader_step(n):
    training = list(pushups(n))
    return training, list(accumulate(training))
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks a LOT! \$\endgroup\$ – Константин Писаный Jun 22 at 14:09
  • \$\begingroup\$ @Peilonrayz Mostly because I did not think of it. Secondly because I only decided to return the consumed iterators at the end... \$\endgroup\$ – Graipher Jun 22 at 14:19
  • \$\begingroup\$ @Peilonrayz Indeed. I decided to directly mirror the interface of the OP, instead of returning generator objects and advising of the difference... \$\endgroup\$ – Graipher Jun 22 at 14:23
  • \$\begingroup\$ I would not use ´tee´ in this case. It is useful if both iterators are consumed in about the same pace, but it keeps all values in memory if one advances a lot before the other, so in this case, where you exhaust gen1 completely, it offers no advantages over the last approach. In case of a generator like this, I would just do ´return pushups(n), accumulate(pushups(n))´. If the caller needs the instantiated as a list, he can still do so. \$\endgroup\$ – Maarten Fabré Jun 23 at 9:51
  • 1
    \$\begingroup\$ @MaartenFabré Agreed. Which is why I added the alternative version without tee when Peilonrayz commented this. \$\endgroup\$ – Graipher Jun 23 at 9:59
7
\$\begingroup\$

Line by line:

def leader_step(max):
  • Type hints (verified using a strict mypy configuration) would be really helpful to understand what this method actually does.
  • Since the application is called "Leader" that part of the function name is redundant - step should be enough to understand it in the application context.

i = 0
  • Usually, i is an index of some sort. But index into what? Is it counting towards max? Basically, I shouldn't have to read the entire function to understand what this variable is used for.

psps = {0: 0}
  • Should this be pushups?
  • I read this as "a pushup with some property (key) "0" has some value "0". This doesn't tell me much. Is this how many pushups I have performed in each set? Something else entirely?

cnt = {0: 0}
  • Should this be count? counts? Something else?
  • Is it incidental that this has the same value as psps? Or are these data structures related in some way?

k = 1
  • After going through this and re-reading it, this variable is telling us whether we're incrementing or decrementing another number. You can instead use += 1 and -= 1 and remove this variable.

while max + abs(k) >= psps[i]:
  • This is checking whether we've reached max, but you should be able to refactor this to something like while pushups <= max_pushups.

if psps[i] >= max:      # decrease push-ups as they reach max
    k = -k
  • That's not what this does. You are negating k here, for still unknown reasons. which will later result in decreasing another counter.

i += 1
  • Why is this incremented here? Especially when you refer to the old value (i - 1) twice below.

psps[i] = k + psps[i-1]
  • OK, so you're setting the "current" pushups "key" adding or subtracting k (still unknown) to/from the previous pushups value. At this point it very much looks like psps should be a list rather than a dictionary, since you keep incrementing the index and setting that.

if psps[i] < 1:         # game stops when you reach 1 push-up
  • This is True when you've reached zero, not one. Is that a bug or an error in the comment?

del psps[i]
break
  • Rather than having to insert and then delete this index, I would refactor so that you break before inserting.

cnt[i] = cnt[i - 1] + psps[i] # counting the sum of all push-ups
  • Do you need all the intermediary values?

del psps[0]
del cnt[0]
  • So you don't actually want the initial values. If these were lists you could just use a slice like psps[1:] to get everything but the first element.

return psps.values(), cnt.values()
  • This reaffirms that both values should be lists, because the keys are thrown away at the end.
\$\endgroup\$
  • \$\begingroup\$ I`ve remade it with lists, but i still cant get, how to edit the whole (i - 1) thing. Is it more edible now? \$\endgroup\$ – Константин Писаный Jun 22 at 14:07

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.