2
\$\begingroup\$

This is my first ever program in my first language, Python. I wanted to know how I could improve my writing style.

#USER: TCG
#Noughts & Crosses

#Header
header = '''
------------------------------------------
|                                        |
|            KNOTS & CROSSES             |
|                                        |
------------------------------------------'''
print header
print

#Asks for input, no number are allowed
while True:
    name1 = raw_input("Enter name of player 1: ")
    if name1.replace(' ','').isalpha():
        break
    print '\n' "No numbers allowed" '\n'
    continue

while True:
    name2 = raw_input("Enter name of player 2: ")
    if name2.replace(' ','').isalpha():
        break
    print '\n' "No numbers allowed" '\n'
    continue

print
print "CHOOSING PLAYERS..."
import time
time.sleep(1)

#xplayer is assigned from randomly choosing between name1 & name2
import random
xplayer = random.choice((name1,name2))
print

#Display's who's playing with either naughts and crosses
print xplayer, "You are playing with Crosses"
if name1 != xplayer:
    name = name1
elif name2 != xplayer:
    name = name2
print name,"You are playing with Noughts"

print
print

#Defines Grid
#function is used so that the can be replaced into it's original form when user asks to replay
print "         HERE IS THE GRID :"
print
def grid():
    global p1,p2,p3,p4,p5,p6,p7,p8,p9,p10,p11,p12,p13,p14
    p1 = "                                    " '\n'
    p2 = "  ----------------------------------" '\n'
    p3 = "  |          |          |          |" '\n'
    p4 = "  |     1    |     2    |     3    |" '\n'
    p5 = "  |          |          |          |" '\n'
    p6 = "  ----------------------------------" '\n'
    p7 = "  |          |          |          |" '\n'
    p8 = "  |     4    |     5    |     6    |" '\n'
    p9 = "  |          |          |          |" '\n'
    p10 = "  ----------------------------------" '\n'
    p11 = "  |          |          |          |" '\n'
    p12 = "  |     7    |     8    |     9    |" '\n'
    p13 = "  |          |          |          |" '\n'
    p14 = "  ----------------------------------" '\n'
    print p1 + p2 + p3 + p4 + p5 + p6 + p7 + p8 + p9 + p10 + p11 + p12 + p13 + p14

grid()
print

#Initiliaize lists

turn1l = []
turn2l = []

#assigned values to avoid NameError's
nx = 0
i = 0

while i > -1:

    ############# USER INPUT #############

    def user_input(j,h):
        global nx,turn2,turn1
        print '\n', j,
        if nx == 1:
            k = raw_input("The space has already been taken \n \n"
                          "Enter it again, using a number from the grid : ")
        else:
            k = raw_input("It's your turn, enter a number from the grid : ")
        nx = 0
        if h == 1:
            turn1 = k
        elif h == 2:
            turn2 = k
        if k in turn2l or k in turn1l:
            nx = 1
            return 1

    ############# REPLAY GAME / EXIT GAME #############

    def replay():
        global i,turn1l,turn2l
        while True:
            restart = raw_input("\nWould you like to replay (yes/no) : ")
            if restart == "yes":
                i = 0
                turn1l = []
                turn2l = []
                grid()
                return 1
            elif restart == "no":
                print '\n' "Thanks for playing!"
                time.sleep(2)
                raise SystemExit()
            else:
                print '\n' "Incorrect input! Please enter either \"yes\" or \"no\""

    ############# CHECKS IF THERE IS A WINNER #############

    def winner_checker():

        def winner_checker_2(x,x1,x2):

            #All possilble wins
            if (("1" in x and"4" in x and "7" in x) or ("1"in x and"2"in x and"3" in x) or ("2"in x and"5" in x and"8" in x) or ("3"in x and"6"in x and"9" in x) or
                ("4"in x and"5"in x and"6" in x) or ("7" in x and"8"in x and"9" in x) or ("1"in x and"5"in x and"9" in x) or ("3"in x and"5"in x and"7" in x)):            

                print '\n' * 5, x1, "YOU HAVE WON! GG TO", x2, '\n'
                if replay() == 1:
                    return 3  

        winner_checker_2(turn1l,xplayer,name)
        winner_checker_2(turn2l,name,xplayer)


    ############# DISPLAY PLAYER'S MOVE #############

    def print_grid_thing(n1,n2,x1,x2,x3):
        global p1,p2,p3,p4,p5,p6,p7,p8,p9,p10,p11,p12,p13,p14
        if p3 in n1 and p4 in n2:
            p3 = x1
            p4 = x2
            p5 = x3
        elif p7 in n1 and p8 in n2:
            p7 = x1
            p8 = x2
            p9 = x3
        elif p11 in n1 and p12 in n2:
            p11 = x1
            p12 = x2
            p13 = x3
        print '\n' * 38
        print p1 + p2 + p3 + p4 + p5 + p6 + p7 + p8 + p9 + p10 + p11 + p12 + p13 + p14

    def display(x,px1,px2,px3,x1,x2,x3,x4):
        if x == turn1:
            #Replace the spaces with 'x'
            pxx1 = px1[:x1] + "\ /" + px1[x3:]
            pxx2 = px2[:x2] + "x" + px2[x4:]
            pxx3 = px3[:x1] + "/ \\" + px3[x3:]
            print_grid_thing(px1,px2,pxx1,pxx2,pxx3)
            return 2

    def display2(x,px1,px2,px3,x1,x2,x3,x4):
        if x == turn2:
            #Replace the spaces with 'o'
            pxx1 = px1[:x1] + "---" + px1[x3:]
            pxx2 = px2[:x2] + "|   |" + px2[x4:]
            pxx3 = px3[:x1] + "---" + px3[x3:]
            print_grid_thing(px1,px2,pxx1,pxx2,pxx3)
            return 2

    ############# Player 1 #############

    while True:

        if user_input(xplayer,1) == 1:
            continue

        #Goes through all of these until the conditon is met within the function
        if (display("4",p7,p8,p9,7,8,10,9) == 2 or display("1",p3,p4,p5,7,8,10,9) == 2 or display("7",p11,p12,p13,7,8,10,9) == 2 or
            display("2",p3,p4,p5,18,19,21,20) == 2 or display("5",p7,p8,p9,18,19,21,20) == 2 or display("8",p11,p12,p13,18,19,21,20) == 2 or
            display("3",p3,p4,p5,29,30,32,31) == 2 or display("6",p7,p8,p9,29,30,32,31) == 2 or display("9",p11,p12,p13,29,30,32,31) == 2):

            turn1l.append(turn1)
            break
        print '\n' "Invalid Input, enter a number from the grid"



    ############# CHECK WINNER AFTER TURN 1 #############

    if winner_checker() == 3:
         continue

    ############# IF THE MATCH DRAWS #############

    if i == 4:
        print '\n' * 5
        print "******* The match was a draw *******" '\n'
        if replay() == 1:
            continue

    ############# Player 2 #############

    while True:

        if user_input(name,2) == 1:
            continue

        #Goes through all of these until the conditon is met within the function
        if (display2("4",p7,p8,p9,7,6,10,11) == 2 or display2("1",p3,p4,p5,7,6,10,11) == 2 or display2("7",p11,p12,p13,7,6,10,11) == 2 or
            display2("2",p3,p4,p5,18,17,21,22) == 2 or display2("5",p7,p8,p9,18,17,21,22) == 2 or display2("8",p11,p12,p13,18,17,21,22) == 2 or
            display2("3",p3,p4,p5,29,28,32,33) == 2 or display2("6",p7,p8,p9,29,28,32,33) == 2 or display2("9",p11,p12,p13,29,28,32,33) == 2):

            turn2l.append(turn2)
            break
        print '\n' "Invalid Input, please enter an available a number from the grid"

    ############# CHECK WINNER AFTER TURN 2 #############

    if winner_checker() == 3:
        continue

    i += 1

Update: New version

\$\endgroup\$
2
\$\begingroup\$

First of all, I would like to compliment you on the results. I ran your program and it is quite user friendly. It is easy to play, easy to understand, and comes up with reasonable errors when I type the wrong thing instead of crashing with a traceback. The following is just improvements on how you came up with that result.

print '\n' "No numbers allowed" '\n'
continue

Your message isn't quite accurate. It isn't just numbers that aren't allowed. The user might type Johnny$. There are no numbers, but the error is that numbers aren't allowed. I would suggest Only alphabetic characters are allowed.

Your two loops are identical. I would suggest putting that code into a function and calling the function twice to get the two names.

...
import time

You shouldn't put your imports before the first uses. You might think, "I want to add a pause before asking for the second name". Therefore, you put time.sleep(1) up there. You get a NameError! Of course it's because you didn't import time early enough. All imports should be at the beginning of the file. PEP 8, the Python style guide, has some useful rules about imports.

def grid():
    ...

Function definitions should go near the top of the file. Usually, code is written like this:

  1. Imports
  2. Constants
  3. Functions and Classes
  4. Module level code

Often, the module level code is written in an if __name__ == '__main__: block.1

global p1,p2,p3,p4,p5,p6,p7,p8,p9,p10,p11,p12,p13,p14

With that many variables, you should be using a list instead. If you have a list of strings, you can concatenate them all with "".join().2 There is no reason to use global variables anyway. In fact, there is no reason to split up the string into multiple variables. You should use triple-quoted strings.3

while ...
    def user_input(j,h):
    ...
    def replay():
    ...

I've already mentioned that function definitions should be at the top. I mention these particular ones because they are within a loop. That means that the functions will be redefined for each iteration of the loop. The definitions aren't changing; you are just re-instating them. That causes unnecessary processing time. Also, you keep using global variables. Use global variables only when the variables must be accessed by code that is external to the function that defined them. You have some global variables that should actually be global, but some of them don't.

if (("1" in x and "4" in x and "7" in x) or ...

It's much simpler to create a list of the good combinations and use any() and all()4:

winning_combos = ("147", "123", "258", "369", "456", "789", "159", "367")
if any(all(y in x for y in combo) for combo in winning_combos):
    ...

I must admit I'm not quite sure how these win-checking functions work. Your winner_checker_2() function returns 3 sometimes, but winner_checker() doesn't do anything with what it returns. I would suggest making your functions return booleans (True and False) and have your main code take care of what to do with the information.

def print_grid_thing(...):

That's pretty funny, but why print_grid_thing? Maybe print_grid would be a better name. There, also, you don't need global variables. You should also be using a list here. You can use rows[2] instead of p3, for example. You could then use print "".join(rows). Well, actually, you could define your rows not to have new lines at the end and use print "\n".join(rows)

if winner_checker() == 3:
    continue

As I mentioned above, winner_checker() never returns anything. Those two lines, therefore, are useless.


Further reading:

1if __name__ == '__main__':

2str.join()

3strings and string literals

4generator expressions, any(), all()

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you, this means a lot! I have taken all of what you have said into consideration and here is the "new" program(I have also changed the while loops into a for loop): pastebin.com/RmfkqNF2 \$\endgroup\$ – TCG Apr 10 '16 at 16:01
  • \$\begingroup\$ That looks pretty good. I have two suggestions. You can simplify line 155 with in: if not move.isdigit() or int(move) == 0:. (You could do or not int(move) but using not ... == 0 makes it a little more obvious what's happening.) My second suggestion: the two if statements at lines 162 and 167 are long and hard to read. I would suggest splitting them up a little bit and adding comments at certain parts of the conditions. \$\endgroup\$ – zondo Apr 10 '16 at 16:45

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.