I have this code ready and it works just fine. But what are the ways by which I can improve this code? I have no CS background and this is my first coding project. I could not paste my entire code so I'm posting my github project link. Any recommendations on that would also be appreciated.
import pygame
pygame.init()
window = pygame.display.set_mode((600,600))
pygame.display.set_caption("Tic Tac Toe")
white = (255,255,255)
Circle = pygame.image.load('Cross.png')
Cross = pygame.image.load('Circle.png')
clicks = []
load1 = False
load2 = False
load3 = False
load4 = False
load5 = False
load6 = False
load7 = False
load8 = False
load9 = False
load10 = False
load11 = False
load12 = False
load13 = False
load14 = False
load15 = False
load16 = False
load17 = False
load18 = False
def Background():
window.fill(white)
pygame.draw.rect(window, (0,0,0),( 198,0,4,600))
pygame.draw.rect(window, (0,0,0),( 398,0,4,600))
pygame.draw.rect(window, (0,0,0),( 0,198,600,4))
pygame.draw.rect(window, (0,0,0),( 0,398,600,4))
pygame.display.flip()
run = True
while run:
pygame.time.delay(100)
for event in pygame.event.get():
if event.type == pygame.QUIT:
run = False
mouse = pygame.mouse.get_pressed()[0]
mouse_x,mouse_y = pygame.mouse.get_pos()
if len(clicks)%2 != 0:
if mouse:
if mouse_x > 0 and mouse_x < 198 and mouse_y > 0 and mouse_y < 198:
clicks.append(mouse)
load1 = True
elif mouse_x > 0 and mouse_x < 198 and mouse_y > 202 and mouse_y < 398:
clicks.append(mouse)
load2 = True
elif mouse_x > 0 and mouse_x < 198 and mouse_y > 402 and mouse_y < 600:
clicks.append(mouse)
load3 = True
elif mouse_x > 202 and mouse_x < 398 and mouse_y > 0 and mouse_y < 198:
clicks.append(mouse)
load4 = True
elif mouse_x > 202 and mouse_x < 398 and mouse_y > 202 and mouse_y < 398:
clicks.append(mouse)
load5 = True
elif mouse_x > 202 and mouse_x < 398 and mouse_y > 402 and mouse_y < 600:
clicks.append(mouse)
load6 = True
elif mouse_x > 402 and mouse_x < 600 and mouse_y > 0 and mouse_y < 198:
clicks.append(mouse)
load7 = True
elif mouse_x > 402 and mouse_x < 600 and mouse_y > 202 and mouse_y < 398:
clicks.append(mouse)
load8 = True
elif mouse_x > 402 and mouse_x < 600 and mouse_y > 402 and mouse_y < 600:
clicks.append(mouse)
load9 = True
elif len(clicks)%2 == 0:
if mouse:
if mouse_x > 0 and mouse_x < 198 and mouse_y > 0 and mouse_y < 198:
clicks.append(mouse)
load10 = True
elif mouse_x > 0 and mouse_x < 198 and mouse_y > 202 and mouse_y < 398:
clicks.append(mouse)
load11 = True
elif mouse_x > 0 and mouse_x < 198 and mouse_y > 402 and mouse_y < 600:
clicks.append(mouse)
load12 = True
elif mouse_x > 202 and mouse_x < 398 and mouse_y > 0 and mouse_y < 198:
clicks.append(mouse)
load13 = True
elif mouse_x > 202 and mouse_x < 398 and mouse_y > 202 and mouse_y < 398:
clicks.append(mouse)
load14 = True
elif mouse_x > 202 and mouse_x < 398 and mouse_y > 402 and mouse_y < 600:
clicks.append(mouse)
load15 = True
elif mouse_x > 402 and mouse_x < 600 and mouse_y > 0 and mouse_y < 198:
clicks.append(mouse)
load16 = True
elif mouse_x > 402 and mouse_x < 600 and mouse_y > 202 and mouse_y < 398:
clicks.append(mouse)
load17 = True
elif mouse_x > 402 and mouse_x < 600 and mouse_y > 402 and mouse_y < 600:
clicks.append(mouse)
load18 = True
if load1:
window.blit(Circle, (0,0))
if load2:
window.blit(Circle, (0,201))
if load3:
window.blit(Circle, (0,401))
if load4:
window.blit(Circle, (201,0))
if load5:
window.blit(Circle, (201,201))
if load6:
window.blit(Circle, (201,401))
if load7:
window.blit(Circle, (401,0))
if load8:
window.blit(Circle, (401,201))
if load9:
window.blit(Circle, (401,401))
if load10:
window.blit(Cross, (0,0))
if load11:
window.blit(Cross, (0,201))
if load12:
window.blit(Cross, (0,401))
if load13:
window.blit(Cross, (201,0))
if load14:
window.blit(Cross, (201,201))
if load15:
window.blit(Cross, (201,401))
if load16:
window.blit(Cross, (401,0))
if load17:
window.blit(Cross, (401,201))
if load18:
window.blit(Cross, (401,401))
if ((load1 and load2 and load3) or (load2 and load5 and load8) or (load3 and load6 and load9) or (load1 and load4 and load7) or (load4 and load5 and load6) or
(load7 and load8 and load9) or (load1 and load5 and load9) or (load3 and load5 and load7)):
print("Circle Wins")
elif ((load10 and load11 and load12) or (load13 and load14 and load15) or (load16 and load17 and load18) or (load10 and load13 and load16) or
(load11 and load14 and load17) or(load12 and load15 and load18) or (load10 and load14 and load18) or (load13 and load14 and load16)):
print("Cross Wins")
pygame.display.update()
Background()
pygame.quit()
1 Answer 1
These points are in order of how I went through your program. They are not ordered in matter of importance.
Your code is suspicious from the start:
Circle = pygame.image.load('Cross.png')
Cross = pygame.image.load('Circle.png')
Why? What prompted you to define a a circle as a cross and vice versa? Either way, the variable names start with a Capital while all other variables do not. Considering Python is a case-sensitive language, please keep all variable names lowercase to prevent confusion.
Your code does not specify anywhere what the expected sizes of Cross.png and Circle.png are. Which means my first game looked like this:
Tic Tac Toe with crosses and circles too large
The pictures aren't correctly centred in their square, so the second game (with modified cross/circle files) looked like this:
Tic Tac Toe with cross misaligned
The cross in the middle square is misaligned, too far on the top-left of the corner. With the earlier game, it didn't do this.
If you ship/share code without shipping/sharing the pictures, like you did when posting your question, make sure you either:
- leave a note somewhere either in the documentation or the code itself which states the required sizes.
- enforce the program won't start by checking the dimensions of the pictures.
- scale the images in your program so they'll always be made to fit.
Your code only counts the clicks made. Not whether any of those clicks were correct. If I tap an already selected square again, I can make this game happen:
Or this:
Tic Tac Toe cheats, circle always wins
Because you're keeping track of each square twice, once for circle and once for cross, it sometimes happens that I can overwrite a square with a circle. Or that the logic of the program no longer corresponds with the visual representation, leading to a very confusing win:
Tic Tac Toe not won, but program thinks so anyway
Long story short, your program is very easy to break. 5+ games in I got more rounds that didn't go according to expectations than games that did.
This is how the program currently keeps track of the squares:
load1 = False
load2 = False
load3 = False
load4 = False
load5 = False
load6 = False
load7 = False
load8 = False
load9 = False
load10 = False
load11 = False
load12 = False
load13 = False
load14 = False
load15 = False
load16 = False
load17 = False
load18 = False
load is an incredibly bad name for a variable. My first guess would be that it loads something externally, not that it's the state of an internal part. state or square would have been better. And while we're at it, might as well turn it into a list. While we could start with an empty list and append a False 18x times, we can also do it the Python way and use a list comprehension:
states = [False for x in range(18)]
Which can be called like states[3] (0-indexed, so 0..17 are valid).
However, we don't want 18 values. There are only 9 squares and each square should only be fillable once. You're currently tracking each square twice, which is a good way to get bugs. Instead of 2 bool per square, we can give each square a State using Enum.
The following example:
from enum import Enum
class State(Enum):
FREE = 1
CIRCLE = 2
CROSS = 3
states = [State.FREE for x in range(9)]
states[4] = State.CIRCLE
states[6] = State.CROSS
for i in states:
print(i, type(i))
Produces the following output:
State.FREE <enum 'State'> State.FREE <enum 'State'> State.FREE <enum 'State'> State.FREE <enum 'State'> State.CIRCLE <enum 'State'> State.FREE <enum 'State'> State.CROSS <enum 'State'> State.FREE <enum 'State'> State.FREE <enum 'State'>
Now a square can no longer be a circle and a cross at the same time, not even by accident. Before overwriting, the square can be checked whether it's free. If it's not free, it can't be written to. Only after a successful write, should the game change players. This means we'll have to do away with the clicks list, stop appending the mouse location to anywhere and just handle the input. If it was right, act. If it was wrong, don't act.
The easiest (not necessarily the best) way to keep track of a player, is using a player variable. We could use a bool, but why not make it an Enum instead? After all, we can toggle (invert the value) of an Enum just as easily as we can with a bool, if we pick the values right.
class Player(Enum):
CIRCLE = 1
CROSS = -1
Now, if we want to switch players, all we have to do is:
player = Player(player.value * -1)
Don't mind my poor naming, but functionally we're making progress.
When checking for a player win, you print who won. However, you don't pause/exit the game and continue to accept input. This is unwanted. All we need to fix this using the already existing while run loop. You do have a pygame.quit() at the end of your program, but by the time the code reaches there the program will be terminated anyway. You don't need it. Just turn run to False.
Then the validation of the win can be simplified:
def is_3_in_row(a, b, c, sign):
return states[a] == State.CIRCLE and states[b] == State.CIRCLE \
and states[c] == sign
Your board also doesn't recognize a tie (9 filled squares without winner). If we account for all that, we can wrap most of the program into a main guard. This will prevent the game from accidentally running if you later decide to import the file in a different program.
All remaining global variables that will not be changed after being defined are now UPPER_CASE, see PEP 8. Background is now background. Your comma's (,) also got a bit of breathing room, making the code easier to read.
The score so far:
import pygame
from enum import Enum
class State(Enum):
CIRCLE = 1
CROSS = 2
FREE = 3
class Player(Enum):
CIRCLE = 1
CROSS = -1
def background(window):
window.fill(WHITE)
pygame.draw.rect(window, (0, 0, 0), (198, 0, 4, 600))
pygame.draw.rect(window, (0, 0, 0), (398, 0, 4, 600))
pygame.draw.rect(window, (0, 0, 0), (0, 198, 600, 4))
pygame.draw.rect(window, (0, 0, 0), (0, 398, 600, 4))
WHITE = (255, 255, 255)
CIRCLE = pygame.image.load('Circle.png')
CROSS = pygame.image.load('Cross.png')
def main():
pygame.init()
pygame.display.set_caption("Tic Tac Toe")
window = pygame.display.set_mode((600, 600))
pygame.display.flip()
states = [State.FREE for x in range(9)]
player = Player.CIRCLE
run = True
def is_3_in_row(a, b, c, sign):
return states[a] == State.CIRCLE and states[b] == State.CIRCLE \
and states[c] == sign
while run:
pygame.time.delay(100)
for event in pygame.event.get():
if event.type == pygame.QUIT:
run = False
mouse = pygame.mouse.get_pressed()[0]
mouse_x, mouse_y = pygame.mouse.get_pos()
if mouse:
if mouse_x > 0 and mouse_x < 198 and mouse_y > 0 and mouse_y < 198:
if states[0] is State.FREE:
if player == player.CIRCLE:
states[0] = State.CIRCLE
elif player == player.CROSS:
states[0] = State.CROSS
player = Player(player.value * -1)
elif mouse_x > 0 and mouse_x < 198 and mouse_y > 202 and mouse_y < 398:
if states[1] is State.FREE:
if player == player.CIRCLE:
states[1] = State.CIRCLE
elif player == player.CROSS:
states[1] = State.CROSS
player = Player(player.value * -1)
elif mouse_x > 0 and mouse_x < 198 and mouse_y > 402 and mouse_y < 600:
if states[2] is State.FREE:
if player == player.CIRCLE:
states[2] = State.CIRCLE
elif player == player.CROSS:
states[2] = State.CROSS
player = Player(player.value * -1)
elif mouse_x > 202 and mouse_x < 398 and mouse_y > 0 and mouse_y < 198:
if states[3] is State.FREE:
if player == player.CIRCLE:
states[3] = State.CIRCLE
elif player == player.CROSS:
states[3] = State.CROSS
player = Player(player.value * -1)
elif mouse_x > 202 and mouse_x < 398 and mouse_y > 202 and mouse_y < 398:
if states[4] is State.FREE:
if player == player.CIRCLE:
states[4] = State.CIRCLE
elif player == player.CROSS:
states[4] = State.CROSS
player = Player(player.value * -1)
elif mouse_x > 202 and mouse_x < 398 and mouse_y > 402 and mouse_y < 600:
if states[5] is State.FREE:
if player == player.CIRCLE:
states[5] = State.CIRCLE
elif player == player.CROSS:
states[5] = State.CROSS
player = Player(player.value * -1)
elif mouse_x > 402 and mouse_x < 600 and mouse_y > 0 and mouse_y < 198:
if states[6] is State.FREE:
if player == player.CIRCLE:
states[6] = State.CIRCLE
elif player == player.CROSS:
states[6] = State.CROSS
player = Player(player.value * -1)
elif mouse_x > 402 and mouse_x < 600 and mouse_y > 202 and mouse_y < 398:
if states[7] is State.FREE:
if player == player.CIRCLE:
states[7] = State.CIRCLE
elif player == player.CROSS:
states[7] = State.CROSS
player = Player(player.value * -1)
elif mouse_x > 402 and mouse_x < 600 and mouse_y > 402 and mouse_y < 600:
if states[8] is State.FREE:
if player == player.CIRCLE:
states[8] = State.CIRCLE
elif player == player.CROSS:
states[8] = State.CROSS
player = Player(player.value * -1)
if states[0] == State.CIRCLE:
window.blit(CIRCLE, (0, 0))
elif states[0] == State.CROSS:
window.blit(CROSS, (0, 0))
if states[1] == State.CIRCLE:
window.blit(CIRCLE, (0, 201))
elif states[1] == State.CROSS:
window.blit(CROSS, (0, 201))
if states[2] == State.CIRCLE:
window.blit(CIRCLE, (0, 401))
elif states[2] == State.CROSS:
window.blit(CROSS, (0, 401))
if states[3] == State.CIRCLE:
window.blit(CIRCLE, (201, 0))
elif states[3] == State.CROSS:
window.blit(CROSS, (201, 0))
if states[4] == State.CIRCLE:
window.blit(CIRCLE, (201, 201))
elif states[4] == State.CROSS:
window.blit(CROSS, (201, 201))
if states[5] == State.CIRCLE:
window.blit(CIRCLE, (201, 401))
elif states[5] == State.CROSS:
window.blit(CROSS, (201, 401))
if states[6] == State.CIRCLE:
window.blit(CIRCLE, (401, 0))
elif states[6] == State.CROSS:
window.blit(CROSS, (401, 0))
if states[7] == State.CIRCLE:
window.blit(CIRCLE, (401, 201))
elif states[7] == State.CROSS:
window.blit(CROSS, (401, 201))
if states[8] == State.CIRCLE:
window.blit(CROSS, (401, 401))
elif states[8] == State.CROSS:
window.blit(CROSS, (401, 401))
if (is_3_in_row(0, 1, 2, State.CIRCLE)
or is_3_in_row(1, 4, 7, State.CIRCLE)
or is_3_in_row(2, 5, 8, State.CIRCLE)
or is_3_in_row(0, 3, 6, State.CIRCLE)
or is_3_in_row(3, 4, 5, State.CIRCLE)
or is_3_in_row(6, 7, 8, State.CIRCLE)
or is_3_in_row(0, 4, 8, State.CIRCLE)
or is_3_in_row(2, 4, 6, State.CIRCLE)):
print("Circle wins")
run = False
elif (is_3_in_row(0, 1, 2, State.CROSS)
or is_3_in_row(1, 4, 7, State.CROSS)
or is_3_in_row(2, 5, 8, State.CROSS)
or is_3_in_row(0, 3, 6, State.CROSS)
or is_3_in_row(3, 4, 5, State.CROSS)
or is_3_in_row(6, 7, 8, State.CROSS)
or is_3_in_row(0, 4, 8, State.CROSS)
or is_3_in_row(2, 4, 6, State.CROSS)):
print("Cross wins")
run = False
elif all((state == State.CIRCLE or state == State.CROSS)
for state in states):
print("It's a tie")
run = False
pygame.display.update()
background(window)
if __name__ == "__main__":
main()
Is this as good as it can get? Absolutely not. We can simplify the part setting the new state of the square, by putting the logic behind a function. Including the player = Player(player.value * -1), but not without a long explanation about scope. So I will leave that for another time.
If you work out what the relationships between the various coordinates of a square are, you can make a pre-defined datastructure that works out the coordinates for you instead of having to do all the adjustments in all 9 cells when something has to be adjusted (for example, if you ever increase your window-size to anything bigger than 600x600.
Explore related questions
See similar questions with these tags.