Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I just wrote a snow animation in Python. I think this is fairly clean but I have a few things that I don't like.

from random import randrange
import time

# Snow animation
# Snow is simply the `#` symbol here. Half of the snow moves at 1 char
# per frame, while the other half moves at 0.5 chars per frame. The
# program ends when all the snow reaches the bottom of the screen.
# The viewing area is 80x25. It puts 100 snow flakes to output, half
# fast, and half slow. Every frame it dispenses 4 flakes, 2 fast and
# 2 slow ones, at random locations at the top of the viewing area.

screen = {'x': 80, 'y': 20}
drops = []

def createRainDrop(x, y, speed):
    return {'x': x, 'y': y, 'speed': speed}

def createRandomDrops():
    dropCount = 4
    for i in range(dropCount):
        yield createRainDrop(randrange(0, screen['x']), 0, min((i % 2) + 0.5, 1))

def moveDrops():
    for drop in drops:
        speed = drop['speed']
        drop['y'] = drop['y']+speed

def drawDrops():
    out = [''] * screen['y']
    for y in range(screen['y']):
      for x in range(screen['x']):
        out[y] += '#' if any([drop['x'] == x and int(drop['y']) == y for drop in drops]) else ' '

    return '\n'.join(out)


def dropsOnScreen():
    return any([drop['y'] < screen['y'] for drop in drops])

drops += createRandomDrops()

while dropsOnScreen():
    if len(drops) < 100:
        drops += createRandomDrops()

    print(drawDrops())
    moveDrops()
    time.sleep(0.100)

For example, I don't know how to remove the duplicate line drops += createRandomDrops(), and drawDrops() feels a bit like a hack.

I confess! While writing this it was rain, not snow!

share|improve this question
3  
createRainDrop are you sure that's snow falling? – njzk2 Feb 1 at 23:23
1  
Unfortunately yes. While writing this it was rain, not snow. – J Atkin Feb 1 at 23:30
25  
The only answer you really need to the question "how clean is my snow?" is "don't eat it. Even if it's not yellow." – Mason Wheeler Feb 2 at 21:01
    
I would love to see the animation. What do I need to follow to "load" the code on a Windows 7 machine please? Can I just install Python and let the file run with it or do I need an IDE and run it inside that? Any link to a tut on how to get this started would be nice. Currently it is "snow-raining" here, half snow half rain. – lowtechsun Feb 3 at 11:07
    
Just install python 3.x, go to the console and type python C:\path\to\snow.py. You may need to shrink the console window for the animation to be smooth(ish). – J Atkin Feb 3 at 14:14
up vote 67 down vote accepted

Let's look at the code.

from random import randrange
import time

Your imports are very minimal! Good.

# Snow animation
# Snow is simply the `#` symbol here. Half of the snow moves at 1 char
# per frame, while the other half moves at 0.5 chars per frame. The
# program ends when all the snow reaches the bottom of the screen.
# The viewing area is 80x25. It puts 100 snow flakes to output, half
# fast, and half slow. Every frame it dispenses 4 flakes, 2 fast and
# 2 slow ones, at random locations at the top of the viewing area.

This looks more like a docstring to me. It would be nice to render it as such. You can do this by dropping the # signs, and surrounding it in """ quotes.

screen = {'x': 80, 'y': 20}
drops = []

Global variables are not that nice. But this is a simple file, so maybe we can leave it like this for now? Let's.

def createRainDrop(x, y, speed):
    return {'x': x, 'y': y, 'speed': speed}

I think something like a class would be better for this. Let's try

class RainDrop(object):
    def __init__(self, x, y, speed):
        self.x = x
        self.y = y
        self.speed = speed

Of course, now we need to replace createRainDrop(...) with RainDrop(...), and drop['...'] with drop.....

def createRandomDrops():
    dropCount = 4
    for i in range(dropCount):
        yield RainDrop(randrange(0, screen['x']), 0, min((i % 2) + 0.5, 1))

That's better.

def moveDrops():
    for drop in drops:
        drop.y = drop.y + drop.speed

We're modifying drop here, instead of asking it to modify itself. We should be writing something like drop.moveDown() here, or maybe drop.tick() ('tick' is what's commonly used to notify an event about stepping forward in time).

def drawDrops():
    out = [''] * screen['y']
    for y in range(screen['y']):
      for x in range(screen['x']):
        out[y] += '#' if any([drop.x == x and drop.y == y for drop in drops]) else ' '

    return '\n'.join(out)

Here, for all the positions on the screen, you're looping over all the drops. Ideally we'd turn that around:

def drawDrops():
    out = [[' ' for _ in range(screen['x'])] for _ in range(screen['y'])]
    for drop in drops:
        if int(drop.y) < screen['y']:
            out[int(drop.y)][drop.x] = '#'

Now that's a bit faster and cleaner.

def dropsOnScreen():
    return any([drop.y < screen['y'] for drop in drops])

Makes sense. Except I'd suggest to not use the [...], which creates a list. Better to use

def dropsOnScreen():
     return any(drop.y < screen['y'] for drop in drops)

This behaves the same, but does not have to create an intermediate list.

drops += createRandomDrops()

while dropsOnScreen():
    if len(drops) < 100:
        drops += createRandomDrops()

    print(drawDrops())
    moveDrops()
    time.sleep(0.100)

You want to get rid of the duplicated call to drops += createRandomDrops().

while True:
    if len(drops) < 100:
        drops += createRandomDrops()

    if not dropsOnScreen():
        break

    print(drawDrops())
    moveDrops()
    time.sleep(0.100)

But in my opinion, the extra createRandomDrops is not that bad.

share|improve this answer
    
Thanks for the answer! This does look much better than my original code. A minor point: in createRandomDrops you point out the min((i % 2) + 0.5, 1) and change it to min(int(i % 2 + 0.5), 1), the original was actually intended to use floats, it changes a number to 1 or 0.5 so I could get a half step snow flake, though it is a bit hard to tell... – J Atkin Feb 1 at 16:30
    
A rather off topic question, is it more common in python to see snake_cased_names or camelCaseNames? (I think the python style guide uses snake_case, though personally I like camelCase much better) – J Atkin Feb 1 at 18:07
4  
@JAtkin: snake-case, except for classes which should be PascalCase. – Sjoerd Job Postmus Feb 1 at 20:00
1  
@SjoerdJobPostmus I'm not sure what the intent of the OP is regarding the speed. It looks like it should be 0.5 or 1, so maybe (1.0, 0.5)[i%2] – njzk2 Feb 1 at 23:31
1  
@njzk2: after re-reading, the statement is min((i % 2) + 0.5, 1). This takes values (1.0, 1.5)[i % 2]. It might not be what is intended, but it is the current value. – Sjoerd Job Postmus Feb 1 at 23:41

Cool animation!

Let's get some linting out of the way. As per PEP 8, you should use 4 spaces of indentation consistently, and function names should be snake_case.

Scalability

The main weakness of your design is scalability. If you extend the loop to run indefinitely, then you will eventually run into performance issues.

One problem is that the drops list grows with each iteration, and is never pruned. The drops don't disappear after falling to the ground; they keep falling forever, invisibly, off-screen. The solution is to have moveDrops() delete drops when they fall beyond the bottom. (That's a smarter strategy than having dropsOnScreen() re-examine every drop on every animation frame.)

Furthermore, to place the drops on the grid, you do an O(n) scan for each position on the screen with '#' if any([drop['x'] == x and int(drop['y']) == y for drop in drops]). I would rewrite drawDrops() so that each drop places itself, using a dictionary or a 2-D array. I would also prefer to use comprehensions than repeated append operations, but that is mostly a style preference.

Data types

Your comment says that the screen dimensions are 80×25, but your code says screen = {'x': 80, 'y': 20}. Ideally, the dimensions should be detected at runtime using the curses library. Since screen is used as a global variable, I would like to see it named SCREEN and made immutable. A namedtuple would make it immutable, with the additional benefit of allowing the dot accessor rather than the clumsy [] notation. I think that width and height would be more appropriate names than x and y.

Similarly, defining a class for the raindrops would avoid the drop['x'] notation. Furthermore, the createRainDrop() function cries out to be a constructor.

Creating drops and looping

The rest of the code is an exercise in Pythonic iteration. Everything can be handled with liberal usage of iterators.

In createRandomDrops(), instead of the cryptic formula min((i % 2) + 0.5, 1), use itertools.cycle([0.5, 1]). I would turn createRandomDrops() into an infinite generator.

In the solution below, parameters such as the speed, intensity, and duration are all centrally tweakable by modifying drop_params and precipitation. For example, precipitation = drop_generator(**drop_params) would result in an infinite loop with just one new drop per frame.

Suggested solution

from collections import namedtuple
import curses
from itertools import chain, cycle, islice, repeat
from random import randrange
import sys
import time

SCREEN = namedtuple('Screen', 'height width')(*curses.initscr().getmaxyx())
curses.endwin()

class Raindrop:
    def __init__(self, x, y, speed):
        self.x, self.y, self.speed = x, y, speed

def drop_generator(batch_size=1, **drop_params):
    while True:
        yield [
            Raindrop(**{key: next(gen) for key, gen in drop_params.items()})
            for _ in range(batch_size)
        ]

def move_drops(drops):
    """Move each drop down according to its speed, and remove drops from the
       set that have fallen off."""
    for drop in drops:
        drop.y += drop.speed
    drops.difference_update([drop for drop in drops if drop.y >= SCREEN.height])

def render_drops(drops, char='#'):
    """Return a string representation of the entire screen."""
    scr = {
        int(drop.y) * SCREEN.width + int(drop.x): char for drop in drops
    }
    return '\n'.join(
        ''.join(scr.get(y * SCREEN.width + x, ' ') for x in range(SCREEN.width))
        for y in range(SCREEN.height)
    )


drop_params = {
    'x': (randrange(0, SCREEN.width) for _ in repeat(True)),
    'y': repeat(0),
    'speed': cycle([0.5, 1]),
}
precipitation = chain.from_iterable([
    islice(drop_generator(batch_size=4, **drop_params), 25),
    repeat([])  # ... then generate nothing as existing drops keep falling
])
drops = set(next(precipitation))
while drops:
    drops.update(next(precipitation))
    print(render_drops(drops))
    # Python 2.7 seems to have a curses bug that necessitates flushing
    sys.stdout.flush()
    move_drops(drops)
    time.sleep(0.100)
share|improve this answer
1  
Very nice! Apparently my cryptic formula was really bad, because it returns 0.5 and 1... codereview.stackexchange.com/questions/118538/… I personally dislike the snake_case very much (I come from Java/Scala) and that is why I was using camelCase. Edit, woah, I can hardly read your solution. I'm gonna go and find my python book... – J Atkin Feb 2 at 13:35
1  
@JAtkin since you're using Python3, why not just use the builtin terminal size? – Wayne Werner Feb 3 at 22:07
    
2 reasons. 1) I didn't know about it. 2) I came by the current numbers by trial and error to get the least flickering on my machine. – J Atkin Feb 3 at 22:54

You don't get drops of snow! Clearly it should be

  for flake in flurry:
share|improve this answer
21  
I'm a bit confused as to why (what seems to be to me) pedantry is the second most up-voted answer to this question. Granted, the English lexical difference is important to the next code maintainer, a 100-character answer regarding 2 variable names seems to be of less value than every other answer here. CR community, you confuse me. – Chris Cirefice Feb 3 at 2:38
11  
@ChrisCirefice You are not the only one confused by this. If you want, feel free to post a question on our meta. Unfortunately(?), nitpicking on a variable name is a valid answer. As to why it has been upvoted this much, I can't say. – Simon Forsberg Feb 3 at 13:29
    
@ChrisCirefice and Simon well the original question is asking about some nitpicks in a script that draws snow on the screen. most people probably don't see the serious vibe in that either – im so confused Feb 3 at 13:47
17  
Naming things is one of the two hard problems in computer science so 'nit picking' names is quite important. For reference the hard problems are 1) Naming things 2) Cache expiry 3) off by one errors. I think the alliteration got an upboat or two too. – Loofer Feb 3 at 13:49
2  
@SimonForsberg I posted a question on Meta. I don't think that this answer should be deleted or anything, but answers like this that seem to be funny remarks should really be comments in my opinion, and not shadow other complete answers. – Chris Cirefice Feb 3 at 17:19
def createRandomDrops():
    dropCount = 4
    for i in range(dropCount):

Magic number 4 in middle of the code

def drawDrops():

I would expect this method to actually draw drops, not return string to print

share|improve this answer

I'm surprised no one has talked about your character choice! Why are their a bunch of hashtags falling? Nah, I kid, it was an ok choice of character, but we can do better! What about changing the # to the unicode (which Python 3 supports!) . Now it really looks like snow!

Also, your code at the moment is backwards compatible with Python 2. My change of character would break that. If you want to fix that add:

# -*- coding: utf8 -*-

To the top of your file.

share|improve this answer
6  
In case you're not sure how to type that, or don't feel like searching, print('\N{SNOWFLAKE}') should help. Also it works on Python2, if you make that u'\N{SNOWFLAKE}' ;) – Wayne Werner Feb 3 at 22:11
    
Nice, I didn't know that utf-8 had a snow symbol. – J Atkin Feb 4 at 16:37
from random import randrange

I would suggest using

import random

Then in your code use random.randrange. In your particular case maybe it doesn't really matter, but I've found for myself that it's a good rule to import the module, when possible, instead of names from it.

https://google.github.io/styleguide/pyguide.html#Imports

Use imports for packages and modules only.

Importing modules sometimes helps you to prevent circular import errors.

It helps importing too many names when your module grows.

It helps you solving name clashes. For example several modules have ValidationError class (mongoengine, wtforms, etc.) and your code uses all of them.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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