8
\$\begingroup\$

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?

200_success
146k22 gold badges190 silver badges479 bronze badges
asked Oct 2, 2015 at 1:03
\$\endgroup\$
0

3 Answers 3

4
\$\begingroup\$

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.

answered Oct 2, 2015 at 7:10
\$\endgroup\$
1
  • \$\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\$ Commented Oct 2, 2015 at 8:16
3
\$\begingroup\$

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.

answered Oct 2, 2015 at 6:33
\$\endgroup\$
1
  • \$\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\$ Commented Oct 2, 2015 at 8:12
2
\$\begingroup\$

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?

answered Oct 2, 2015 at 11:50
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.