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?

\$\endgroup\$
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.

\$\endgroup\$
  • \$\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 Oct 2 '15 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.

\$\endgroup\$
  • \$\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 Oct 2 '15 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?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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