Need assistance in refactoring this function

Question

I am currently in the process of completing a college assignment. We have been using John Zelle's graphics.py module for the first semester.

The task was to construct two different patterns, and then lay them out in a particular design, automatically adjusting depending if the window was 500 x 500, 700 x 700 or 900 x 900 pixels.

I have completed the task, however, one big part of my code is very inefficient and long, this therefore isn't ideal.

Below is the code that is long and inefficient:

def DrawPattern(width,size,win,colour):

    if size == 5:
        for x in range(0,width,100):
            drawCircleExpanse(win,x,400,colour)
        for j in range(100,400,100):
            drawCircleExpanse(win,j,300,colour)
        drawCircleExpanse(win,200,200,colour)
        for j in range(100,400,100):
            drawCircleExpanse(win,j,100,colour)
        for x in range(0,width,100):
            drawCircleExpanse(win,x,0,colour)
    if size == 7:
        for x in range(0,width,100):
            drawCircleExpanse(win,x,width-100,colour)
        for j in range(100,width-100,100):
            drawCircleExpanse(win,j,width-200,colour)
        for i in range(200,width-200,100):
            drawCircleExpanse(win,i,width-300,colour)        
        drawCircleExpanse(win,300,300,colour)
        for j in range(100,width-100,100):
            drawCircleExpanse(win,j,100,colour)
        for x in range(0,width,100):
            drawCircleExpanse(win,x,0,colour)
        for i in range(200,width-200,100):
            drawCircleExpanse(win,i,width-500,colour)   
    if size == 9:
        for x in range(0,width,100):
            drawCircleExpanse(win,x,width-100,colour)
        for j in range(100,width-100,100):
            drawCircleExpanse(win,j,width-200,colour)
        for i in range(200,width-200,100):
            drawCircleExpanse(win,i,width-300,colour) 
        for y in range(300,width-300,100):
            drawCircleExpanse(win,y,width-400,colour)
        drawCircleExpanse(win,400,400,colour)
        for j in range(100,width-100,100):
            drawCircleExpanse(win,j,100,colour)
        for x in range(0,width,100):
            drawCircleExpanse(win,x,0,colour)
        for i in range(200,width-200,100):
            drawCircleExpanse(win,i,width-700,colour) 
        for y in range(300,width-300,100):
            drawCircleExpanse(win,y,width-600,colour) 

The size variable takes the user's input; for example 5, 7 or 9. This translates to either 500 x 500, 700 x 700 or 900 x 900. Similarly with colour, which will just change the colour to one of the user's choice.

This code, will produce the following pattern, with the size 5 which translates to 500 x 500:

The drawCircleExpanse function, which constructs the pattern itself, contains the following code:

def drawCircleExpanse(win,x,y,colour):
    rad = 50
    for c in range(10):
        circle = Circle(Point(50+x,(50+(c * 10) / 2)+y), rad)
        circle.setOutline(colour)
        circle.draw(win)
        rad -= 5

I would be really grateful if someone could assist in shortening my massive block of code.

Answer 1

I agree that this is probably a better fit for codereview, but have two thoughts that may help you:

• For each value of size, the number of rows (for loops) before and after the center-element drawCircleExpanse is floor(size/2). Since you can compute the number of rows, you can use another loop in which the row loops are nested. Using nested loops before and after the center-element draw... call means you can get rid of your if statements entirely.
• To make the code easier to read, I would actually move the factor of 100 into drawCircleExpanse, so that the coordinates to drawCircleExpanse are on an imaginary grid rather than on screen pixels. Localizing the grid-to-pixel transformation also gives you more flexibility to change that transformation later if you need to.

Answer 2

Why aren't you making the computer do the math for you? For size == 5, this is a first pass at refactoring:

x_list = range(0, 500, 100) + \
         range(100, 400, 100) + \
         [200] + \
         range(100, 400, 100) + \
         range(0, 500, 100)

y_list = [400] * 5 + \
         [300] * 3 + \
         [200] + \
         [100] * 3 + \
         [0] * 5

def drawCircleExpanse(win, x, y, colour):
    print "%3d, %3d" % (x, y)

win = None
colour = None
for x, y in zip(x_list, y_list):
    drawCircleExpanse(win, x, y, colour)

Output

  0, 400
100, 400
200, 400
300, 400
400, 400
100, 300
200, 300
300, 300
200, 200
100, 100
200, 100
300, 100
  0,   0
100,   0
200,   0
300,   0
400,   0

You should be able to further refactor this by using the size to create x_list and y_list.

Answer 3

I would be really grateful if someone could assist in shortening my massive block of code.

Is this short enough:

from graphics import *

RADIUS = 50
NUMBER_CIRCLES = 10
DECREMENT = RADIUS // NUMBER_CIRCLES

def drawCircleExpanse(win, x, y, colour):
    for radius in range(RADIUS, 0, -DECREMENT):
        circle = Circle(Point(RADIUS + x, (2 * RADIUS - radius) + y), radius)
        circle.setOutline(colour)
        circle.draw(win)

def DrawPattern(width, size, win, colour):
    for y in range(size, 0, -2): # 5 3 1
        for x in range(width // 2 - y * RADIUS, width // 2 + y * RADIUS, RADIUS * 2):
            drawCircleExpanse(win, x, width // 2 - y * RADIUS, colour)
            if y > 1:
                drawCircleExpanse(win, x, width // 2 + (y - 2) * RADIUS, colour)

win = GraphWin("Optimize Drawing", 700, 700)

DrawPattern(700, 7, win, "red")

win.getMouse()

win.close()

I think you made it far more complicated than necessary. The above should also work for '3' (and 300) and possibly other values beyond 5, 7 & 9. There is a little issue of adjusting for the slight window border (chrome) width which I leave to you.