Leon asked a question which got closed both here and at Stack Overflow, but I kind of liked the idea, so I implemented a working version of his code, which I now want reviewed.
With regards to Leon's original code I've done the following improvement (in case he reads this new post):
- Renamed variables so that they have a meaning
- Used tuple assignment, like
red, green, blue = random(), random(), random()
to shorten the code, whilst still maintaining readability - Removed some magic numbers. Including the one related to 100 vs 1000 which caused the previous version to crash from time to time as one of the color indexes got to be negative
- Added action to left and right mouseclick. Left click changes the direction, whilst right clicks kills the turtle
- Increased the area allowed for the turtle to move in to be the default screen size for the turtle
- Changed to using
randrange()
with ranges from negative through positive values, to ease setting coordinate increments
Here is the working code:
import turtle
from random import random, randrange
import math
MIN_COLOR_STEP = 30
MAX_COLOR_STEP = 100
class MyTurtle(turtle.Turtle):
""" Helper turtle class to handle mouse clicks and keep state"""
def __init__(self, **args):
turtle.Turtle.__init__(self, **args)
self._alive = True
self._change_increment = False
def kill_turtle(self, x, y):
self._alive = False
def is_alive(self):
return self._alive
def wants_to_change_direction(self):
if self._change_increment:
self._change_increment = False
return True
return False
def change_increment(self, x, y):
# print "change increment"
self._change_increment = True
def draw_turtle(turtle, x1, y1, x2, y2, red, green, blue):
"""Change color of turtle, and draw a new line"""
turtle.color(red, green, blue)
turtle.up()
turtle.goto(x1, y1)
turtle.down()
turtle.goto(x2, y2)
def random_increment():
""" Return an increment to be used in x or y direction.
To avoid no movement in any coordinate make sure that this
never returns 0. That is that MIN_INCR + n*INCR_STEP != 0 for
all n's
"""
MIN_INCR = -22
MAX_INCR = 23
INCR_STEP = 5
return randrange(MIN_INCR, MAX_INCR, INCR_STEP)
def draw_turtle_screensaver():
""" Draw random lines on the screen that bounce around
If left mouse button is clicked, bob the turtle changes
direction. If right mouse button is clicked, bob the turtle
is killed.
"""
# Define or hard working turtle
turtle_screen = turtle.Screen()
bob_the_turtle = MyTurtle()
bob_the_turtle.shape('blank')
bob_the_turtle.speed(0)
turtle_screen.bgcolor('black')
turtle_screen.onclick(bob_the_turtle.kill_turtle, btn=2)
turtle_screen.onclick(bob_the_turtle.change_increment, btn=1)
# Get the limits for the turtle movement
MAX_WIDTH = bob_the_turtle.window_width() // 2
MAX_HEIGHT = bob_the_turtle.window_height() // 2
# Set initial coordinates to the middle of the screen
x1, y1 = 0, 0
x2, y2 = 0, 0
# Find random increments for change of every coordinate
x1_incr=random_increment()
y1_incr=random_increment()
x2_incr=random_increment()
y2_incr=random_increment()
# Setup initial colors, new colors and steps between changes
steps_before_change = randrange(MIN_COLOR_STEP, MAX_COLOR_STEP)
red, green, blue = random(), random(), random()
new_red, new_green, new_blue = random(), random(), random()
red_incr = (new_red - red)/steps_before_change
green_incr =(new_green - green)/steps_before_change
blue_incr = (new_blue - blue)/steps_before_change
color_steps=0
while(bob_the_turtle.is_alive()):
# Change color toward new color in incremental steps
red += red_incr
green += green_incr
blue += blue_incr
color_steps += 1
# If we've reached the new color, find a new color to go towards
if color_steps >= steps_before_change:
color_steps = 0
# Switch color, find new color and new color increments
red, green, blue = new_red, new_green, new_blue
new_red, new_green, new_blue = random(), random(), random()
steps_before_change = randrange(MIN_COLOR_STEP, MAX_COLOR_STEP)
red_incr = (new_red - red)/steps_before_change
green_incr =(new_green - green)/steps_before_change
blue_incr = (new_blue - blue)/steps_before_change
if bob_the_turtle.wants_to_change_direction():
# Find new random increments for change of every coordinate
x1_incr=random_increment()
y1_incr=random_increment()
x2_incr=random_increment()
y2_incr=random_increment()
# Increment all coordinates
x1 += x1_incr
y1 += y1_incr
x2 += x2_incr
y2 += y2_incr
# If any of the coordinates is off-screen, revert increment
if abs(x1) > MAX_WIDTH:
x1_incr *= -1
if abs(y1) > MAX_HEIGHT:
y1_incr *= -1
if abs(x2) > MAX_WIDTH:
x2_incr *= -1
if abs(y2) > MAX_HEIGHT:
y2_incr *= -1
# Draw the new line, in the current color
draw_turtle(bob_the_turtle, x1, y1, x2, y2, red, green, blue)
def main():
draw_turtle_screensaver()
if __name__ == '__main__':
main()
Do you have suggestion for improvement, or general review comments?
3 Answers 3
draw_turtle_screensaver()
is rather tedious, with many variables and some repetitive code. For example, you have red
, green
, blue
, as well as their new_...
counterparts. The code to manipulate those three color channels is written in triplicate. Furthermore, the color-changing code is somewhat copy-and-pasted from the color-initialization code.
Therefore, it would pay off to have a better color abstraction. I would write a class like this:
from collections import namedtuple
MIN_COLOR_STEP = 30
MAX_COLOR_STEP = 100
class Color(namedtuple('Color', 'r g b')):
def __new__(cls, r=None, g=None, b=None):
return super(cls, Color).__new__(cls,
r or random(), g or random(), b or random()
)
def __add__(self, color):
return Color(self.r + color.r, self.g + color.g, self.b + color.b)
def __sub__(self, color):
return Color(self.r - color.r, self.g - color.g, self.b - color.b)
def __div__(self, factor):
return Color(self.r / factor, self.g / factor, self.b / factor)
@staticmethod
def progressions():
"""
A generator that yields colors from an endless somewhat-random
sequence of gradual color changes.
"""
def progression(steps, start_color=None, end_color=None):
color = start_color or Color()
delta = ((end_color or Color()) - color) / steps
for _ in xrange(steps):
yield color
color += delta
color = Color()
while True:
for color in progression(randrange(MIN_COLOR_STEP, MAX_COLOR_STEP), color):
yield color
Then, you could relieve draw_turtle_screensaver()
of the task of generating colors. The original nine color-related variables now become just a generator:
def draw_turtle_screensaver():
...
colors = Color.progressions()
while bob_the_turtle.is_alive():
if bob_the_turtle.wants_to_change_direction():
x1_incr = ...
...
x1 += x1_incr
...
if abs(x1) > MAX_WIDTH:
x1_incr *= -1
...
draw_turtle(bob_the_turtle, x1, y1, x2, y2, *next(colors))
Having cleaned up the red
, green
, and blue
variables that way, I would then use the same technique to eliminate x1
, y1
, x2
, y2
, and the ..._incr
variables. That should leave you with a very simple and readable main loop.
-
\$\begingroup\$ Last night after posting my original code, I looked over the code, and it hit me that the
draw_turtle_screensaver()
was way to long, and I rebuilt it to using classes for both cases you mention. I did however not make that neat generator and calculation overload! :-D \$\endgroup\$holroy– holroy2015年10月02日 08:16:20 +00:00Commented Oct 2, 2015 at 8:16
Overall looks pretty good, but here are a few suggestions:
Read PEP 8; there are still a few areas where you could tidy up. In particular: two lines between function definitions, ALL CAPS variables are only global constants, sorting of imports at the top of the file.
In the constructor for your MyTurtle class, you should use
super()
to call the parent constructor, instead of hard-coding the name of the parent class. This is a good practice, because it allows for mocking and cleaner class hierarchies.I recommend Raymond Hettinger’s blog post Python’s super() consider super! and his PyCon talk of the same name.
There isn’t a
__repr__()
method on your Turtle class; having one is often useful for debugging.There aren’t any comments or docstrings to explain what the internal attributes of the class are, or why they might be useful. I don’t know how to use your class.
In the random_increment() function, I think a better approach would just be to check you aren’t getting a 0 result; that gives you a more even distribution over the range. i.e.,
def random_increment(min_increment, max_increment): """Returns a random increment for motion in an x/y direction""" increment = 0 # To ensure that we always move, make sure this never returns 0 while not increment: increment = random.randrange(min_increment, max_increment) return increment
In the
draw_turtle_screensaver()
function, the parentheses in the while statement are visual noise and can be removed.
-
\$\begingroup\$ Was a little uncertain regarding ALL_CAPS for constants within classes/methods, have thought about some of the other points, and then some new points! Cool! Will implement most, if not all, of these. \$\endgroup\$holroy– holroy2015年10月02日 08:12:19 +00:00Commented Oct 2, 2015 at 8:12
Last night I kept on coding, after realising the point now also made by 200_success, that my main method draw_turtle_screensaver()
was too large. So I implemented a version using classes, but I got somewhat carried away regarding the turtle association...
The code has some flaws, so I didn't post it last night. Today however, this is much better, and I've also incorporated most of the comments from the answer by alexwlchan. I really like the neat generators and calculation shortcuts provided in the answer by 200_success, but they didn't fit in the themed version given below. Will strongly consider his suggestions later on, as they are really good advice for more proper code.
from random import random, randrange
import turtle
""" A turtle based Python screensaver
Once upon a time there was a turtle, Bob, destined for a greater mission
of saving screens. The turtle decided that a good way to do this was to
get help from some of his friends:
* Adam & Eve : Who walk around in the world
* Alex : A painter just loving to mix paint color
Allowing adam & eve to walk around bumping into walls and turning direction,
bob decided to paint a line between them every now and then using the color
which Alex gives him.
Sometime the outside world could interfere with this happy little world, by
'clicking' with the left mouse button to change the direction of Adam &
Eve, or with the right mouse button to terminate Bob's short life.
"""
MIN_COLOR_STEP = 30
MAX_COLOR_STEP = 100
MIN_INCREMENT = -17
MAX_INCREMENT = 18
INCREMENT_STEP = 5
DEFAULT_INCREMENT = 8
class MyTurtle(turtle.Turtle):
"""An educated turtle capable of coordination and connection building.
The turtle has three goals in life. Connect walkers, coordinate
the change of direction for the walkers, and sadly enough to kill
it self in due time.
"""
def __init__(self, **args):
#turtle.Turtle.__init__(self, **args)
super(turtle.Turtle, self).__init__(**args)
self._alive = True
self._change_direction = False
def __repr__(self):
return '{}(alive={}, change={})'.format(self.__class__,
self._alive,
self._change_direction)
def kill_turtle(self, x, y):
"""Indicate self termination."""
self._alive = False
def is_alive(self):
"""Let the world now if turtle is still alive"""
return self._alive
def wants_to_change_direction(self):
"""Check if turtle wants to change direction.
If it wants to turn, it assumes proper actions are taken,
so it resets it own flag regarding changing directions
"""
if self._change_direction:
self._change_direction = False
return True
return False
def change_direction(self, x, y):
self._change_direction = True
def connects_walkers(self, a_walker, another_walker, the_painter):
"""Draw a line between walkers in the painters color"""
self.color(the_painter.show_color())
self.up()
self.goto(a_walker.tell_location())
self.down()
self.goto(another_walker.tell_location())
class PaintMixer():
"""Mixes paint to be used when painting stuff.
A painter living to change the color to a new color in given number
of steps, and when found find a new color to move towards. And if
someone asks tell what the color to use just now.
"""
def __init__(self, min_step=MIN_COLOR_STEP, max_step=MAX_COLOR_STEP):
self._min_steps = min_step
self._max_steps = max_step
self._step_counter = 0
self._new_red, self._new_green, self._new_blue = random(), random(), random()
self._find_new_color_and_increments()
def _find_new_color_and_increments(self):
"""The painters internal process of paint mixing."""
# Steps before color is completely changed, and we select a new color
self._steps_before_change = randrange(self._min_steps, self._max_steps)
self._step_counter = 0
self._red, self._green, self._blue = self._new_red, self._new_green, self._new_blue
self._new_red, self._new_green, self._new_blue = random(), random(), random()
self._red_incr = (self._new_red - self._red)/self._steps_before_change
self._green_incr = (self._new_green - self._green)/self._steps_before_change
self._blue_incr = (self._new_blue - self._blue)/self._steps_before_change
def mixes_paint(self):
"""Changes the color to be used when painting."""
self._step_counter += 1
self._red += self._red_incr
self._green += self._green_incr
self._blue += self._blue_incr
if self._step_counter > self._steps_before_change:
self._find_new_color_and_increments()
def show_color(self):
"""Return the color currently used by painter"""
return (self._red, self._green, self._blue)
class CoordinateWalker():
"""Walks around within the coordinate system.
A walker of the world destined to roam around within the limits
of the world. Keeping the direction reflecting of edges, and only
changes direction when explicitly told so. And if someone cares to
know, tells the location
"""
def __init__(self, max_width, max_height,
min_increment = MIN_INCREMENT,
max_increment = MAX_INCREMENT,
increment_step = INCREMENT_STEP,
default_increment = DEFAULT_INCREMENT):
self._min_increment = min_increment
self._max_increment = max_increment
self._increment_step = increment_step
self._default_increment = default_increment
self._max_width = max_width
self._max_height = max_height
self._x, self._y = 0, 0
self._x_incr, self._y_incr = 0, 0
self.changes_direction()
def _random_increment(self):
""" Return an increment to be used in x or y direction."""
random_increment = randrange(self._min_increment,
self._max_increment,
self._increment_step)
return random_increment if random_increment != 0 else default_increment
def walks(self):
"""Take a step, and turn if going to far."""
self._x += self._x_incr
self._y += self._y_incr
if abs(self._x) > self._max_width:
self._x_incr *= -1
if abs(self._y) > self._max_height:
self._y_incr *= -1
def changes_direction(self):
"""Changes direction, as ordered."""
self._x_incr = self._random_increment()
self._y_incr = self._random_increment()
def tell_location(self):
"""Let the surroundings know where I'm at."""
return (self._x, self._y)
def make_the_world():
"""Make the dark world and a turtle to inhabit it.
Define a dark world for the walkers to roam and fill with colors, and
bring bob the turtle alive and connect him so he can connect the walkers
"""
turtle_screen = turtle.Screen()
bob_the_turtle = MyTurtle(canvas = turtle_screen)
bob_the_turtle.shape('blank')
bob_the_turtle.speed(0)
turtle_screen.bgcolor('black')
turtle_screen.onclick(bob_the_turtle.kill_turtle, btn=2)
turtle_screen.onclick(bob_the_turtle.change_direction, btn=1)
# Get the limits for the turtle movement
max_width = bob_the_turtle.window_width() // 2
max_height = bob_the_turtle.window_height() // 2
return (max_width, max_height, bob_the_turtle)
def draw_turtle_screensaver():
"""A turtle based screensaver.
A screensaver involving bob the turtle which coordinates the
walkers, Adam and Eve, and connects them using the colors provided
by Alex, the painter.
"""
my_world_width, my_world_height, bob_the_turtle = make_the_world()
# Lets get some more inhabitants into the world
adam_the_walker = CoordinateWalker(my_world_width, my_world_height)
eve_the_walker = CoordinateWalker(my_world_width, my_world_height)
alex_the_painter = PaintMixer()
while bob_the_turtle.is_alive():
alex_the_painter.mixes_paint()
adam_the_walker.walks()
eve_the_walker.walks()
if bob_the_turtle.wants_to_change_direction():
adam_the_walker.changes_direction()
eve_the_walker.changes_direction()
bob_the_turtle.connects_walkers(adam_the_walker,
eve_the_walker,
alex_the_painter)
def main():
draw_turtle_screensaver()
if __name__ == '__main__':
main()
PS: Code given here is not meant for a new review (that would have been a new post), it is more for a future reference if someone wants to extend this screensaver. Maybe you would want to add new walkers and let them all connect?
Explore related questions
See similar questions with these tags.