3
\$\begingroup\$

So I've got this module called risar, which draws. But that's not really of importance. I wrote this code which sets 20 flowers on the background.

Now they need to move downwards by 5 units. The code works fine but it looks horribly awkward to me. I'd like it to look more "fancy" or maybe that less loops would be used or I'd like to see simply another way to do it. I need help with 2nd function; the 1st one looks okay. I'm relatively new to Python.

def doFlowers(): 
    flowers = []
    colors = ["black_flower.svg","blue_flower.svg", "brown_flower.svg", "green_flower.svg","purple_flower.svg"]
    for i in range(20):
        x = random.randint(20, (risar.maxX-20))
        y = random.randint(20, 300)
        flower = risar.picture(x, y, barve[i//4]) # 0, 1, 2, 3, 4
        flowers.append(flower)
    return flowers
flowers = doFlower()


def moving(flower):
    for i in range(len(flower)):
        while flower[i].y() < risar.maxY:
            for p in flower: #p is position
                p.setPos(p.x(), p.y()+5)
\$\endgroup\$
2
  • \$\begingroup\$ So moving is supposed to animate the drawing by moving flowers every frame? \$\endgroup\$
    – theodox
    Jan 17, 2014 at 21:19
  • \$\begingroup\$ You probably mean flowers = doFlowers(). \$\endgroup\$ Jan 17, 2014 at 23:49

1 Answer 1

3
\$\begingroup\$

Let's start with your naming. flower is supposed to be a list, right? Let's make it plural, then.

def moving(flowers):
    for i in range(len(flowers)):
        while flowers[i].y() < risar.maxY:
            for p in flowers: #p is position
                p.setPos(p.x(), p.y()+5)

Next, notice that you don't actually care about i; you just want to operate on every single flower.

def moving(flowers):
    for f in flowers:
        while f.y() < risar.maxY:
            for p in flowers: #p is position
                p.setPos(p.x(), p.y()+5)

Hmm… not only are there nested loops, but two of them iterate over all flowers. What's going on? I think you're trying to move all flowers "simultaneously" until all of them have their y-coordinate ≥ risar.maxY. Then just say this, which I think is a bit more expressive:

def moving(flowers):
    while any([f.y() < risar.maxY for f in flowers]):
        for flower in flowers:
            flower.setPos(flower.x(), flower.y() + 5)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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