patternpythonCritical
How clean is my snow?
Viewed 0 times
cleansnowhow
Problem
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.
For example, I don't know how to remove the duplicate line
I confess! While writing this it was rain, not snow!
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!
Solution
Let's look at the code.
Your imports are very minimal! Good.
This looks more like a docstring to me. It would be nice to render it as such. You can do this by dropping the
Global variables are not that nice. But this is a simple file, so maybe we can leave it like this for now? Let's.
I think something like a class would be better for this. Let's try
Of course, now we need to replace
That's better.
We're modifying
Here, for all the positions on the screen, you're looping over all the drops. Ideally we'd turn that around:
Now that's a bit faster and cleaner.
Makes sense. Except I'd suggest to not use the
This behaves the same, but does not have to create an intermediate list.
You want to get rid of the duplicated call to
But in my opinion, the extra
from random import randrange
import timeYour 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 = speedOf 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.speedWe'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 usedef 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.Code Snippets
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}class RainDrop(object):
def __init__(self, x, y, speed):
self.x = x
self.y = y
self.speed = speedContext
StackExchange Code Review Q#118538, answer score: 108
Revisions (0)
No revisions yet.