3
\$\begingroup\$

I had to do a small application using a menu system and Turtle. I have finished it and worked perfect. Just to make sure, can you please check it if it has done properly?

#!/usr/bin/python

## draw_shape.py

import turtle as t


def how_many():
    while True:  
        print "  How many of then do you want to draw?"
        print "  -Range is from 1 to 5-"
        global shape_no
        shape_no = raw_input('  Enter your choice: ')
        try: 
            shape_no = int(shape_no)
            if (1 <= shape_no <= 5):
                print "Your number is ok"
                break
            else:
                print
                print "from 1 to 5 only"
        except:
            print "Only numbers allowed - Please try again"
    return True


#=========#=========#=========#=========#=========#=========#=========#
def start_point():
    t.penup() 
    t.setpos(-240,0) 
    t.pendown()

def draw_square(howmanyof):
    start_point()
    for a in range(howmanyof): 
        for a in range(4): 
            t.forward(80) 
            t.left(90)
        t.penup() 
        t.forward(100)
        t.pendown()

def draw_triangle(howmanyof):
    start_point()
    for a in range(howmanyof): 
        for a in range(3): 
            t.forward(80) 
            t.left(120)
        t.penup() 
        t.forward(100)
        t.pendown()

def draw_rectangle(howmanyof):
    start_point()
    for a in range(howmanyof):

        for a in range(2): 
            t.forward(80) 
            t.left(90)
            t.forward(40)
            t.left(90)
        t.penup() 
        t.forward(100)
        t.pendown()

def draw_hexagon(howmanyof):
    start_point()
    for a in range(howmanyof): 
        for a in range(6): 
            t.forward(50) 
            t.left(60)
        t.penup() 
        t.forward(110)
        t.pendown()

def draw_octagon(howmanyof):
    start_point()
    for a in range(howmanyof): 
        for a in range(8): 
            t.forward(40) 
            t.left(45)
        t.penup() 
        t.forward(110)
        t.pendown()

def draw_circle(howmanyof):
    start_point()
    for a in range(howmanyof): 
        t . circle(50)
        t.penup() 
        t.forward(120)
        t.pendown()

#=========#=========#=========#=========#=========#=========#=========#
def main():
    while True:
        print
        print "  Draw a Shape"
        print "  ============"
        print
        print "  1 - Draw a square"
        print "  2 - Draw a triangle"
        print "  3 - Draw a rectangle"
        print "  4 - Draw a hexagon"
        print "  5 - Draw an octagon"
        print "  6 - Draw a circle"
        print
        print "  X - Exit"
        print

        choice = raw_input('  Enter your choice: ')

        if (choice == 'x') or (choice == 'X'):
            break
        elif choice == '1':
            how_many()
            draw_square(shape_no)
        elif choice == '2':
            how_many()
            draw_triangle(shape_no)
        elif choice == '3':
            how_many()
            draw_rectangle(shape_no)
        elif choice == '4':
            how_many()
            draw_hexagon(shape_no)
        elif choice == '5':
            how_many()
            draw_octagon(shape_no)
        elif choice == '6':
            how_many()
            draw_circle(shape_no)
        else:
            print
            print '  Try again'
            print



#=========#=========#=========#=========#=========#=========#=========#
if __name__ == "__main__":
    main()

#=========#=========#=========#=========#=========#=========#=========#
\$\endgroup\$
1
\$\begingroup\$
  1. Why are you using a global? There seems to be no need for it, and in general it's something to avoid.

  2. Why not factor out the common stuff? In your main loop you have a bunch of if cases which all start in the same way and then all call methods which are very similar. If you start by checking that it's a valid input you could pull out the calls to how_many() and start_point() and even the loop for a in range(howmanyof):.

\$\endgroup\$
  • \$\begingroup\$ 1- when i take off global shape_no i can not get the user input. It draws 4 times regardless of user input. \$\endgroup\$ – eMRe Dec 23 '11 at 17:20
  • \$\begingroup\$ @baris22 As mentioned in the SO answers, return shape_no from how_many(). \$\endgroup\$ – Daniel Fischer Dec 23 '11 at 19:29
  • \$\begingroup\$ Thanks alot. I got it working and i kind of understood what you meant \$\endgroup\$ – eMRe Dec 23 '11 at 21:23
0
\$\begingroup\$

(You haven't updated the question based on Peter's answer, so I can't really comment on how_many(), and will primarily focus on the rest.)

First of all, your function names are misleading: draw_triangle should draw a triangle, while yours draws a certain number of triangles. I suggest pulling the triangle-drawing logic into a separate function called draw_triangle and then renaming the current draw_triangle to draw_n_triangles. You might even be able to make a draw_n_shapes function which takes a function for drawing one shape -- I haven't looked too closely, but it seems like this could work:

def draw_n_shapes(n, draw_func):
    start_point()
    for a in range(n): 
        draw_func()
        t.penup() 
        t.forward(120)
        t.pendown()

Note that the spacing may be off (you use 100, 110, and 120 in the different functions -- either pass it as a parameter, or choose one).

Secondly, instead of the long if..else chain, you could use a dictionary. For example:

def main():
     draw_functions = {
         1: draw_square,
         2: draw_triangle,
         3: draw_rectangle,
         4: draw_hexagon,
         5: draw_octagon,
         6: draw_circle
     }
     while True:
         # stuff that shows the message, takes input, etc.
         if choice.lower() == 'x':
             break
         if int(choice) in draw_functions:
             draw_functions[int(choice)](shape_no)
         else:
             print "Try again"

You instead could use a list, but then the indices would start at 0, which I don't think you want.

Thirdly, although your functions are all short and fairly self-documenting, I would suggest adding docstrings to your functions, as well as comments whenever you feel like you have to make an important decision. Even if you don't expect the code to be read by anyone at all, spotting similarities may be simpler in text than in code. You may also want to name your constants -- the aforementioned 100 is a good example, as well as the -240, 0 point you use in start_point (why did you choose that point?).

Speaking of which, start_point may be better-named jump_to_start_point, if I understood the purpose correctly. In general, you may want to have function names start with a verb.

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