Simon game / Four tiles game

When I read the community challange, I couldn't wait. So here I go with the very first Four tiles game:

```
import pygame as pg
import sys
import random
import time
import tkMessageBox

if sys.version_info.major == 3:
import tkinter as tk
else:
import Tkinter as tk

from pygame.locals import *

def game(LENGHT):
pg.init()
pg.display.set_caption("Four tiles game")

WIDTH = 600
HEIGHT = 400
DISPLAY = pg.display.set_mode((WIDTH, HEIGHT), 0, 32)

WHITE = (255, 255, 255)
RED = (255, 0, 0)
green = (0, 255, 0)
blue = (0, 0, 255)
yellow = (255, 255, 0)
COLORS = [RED, blue, green, yellow]

WAITING_TIME = 500

def generate_correct_sequence(length):
return [random.randint(1, 4) for _ in range(length)]

def draw_button(n, color):
if n == 1:
pg.draw.rect(DISPLAY, color, (0, 0, WIDTH / 2, HEIGHT / 2))
if n == 2:
pg.draw.rect(DISPLAY,color,
(WIDTH / 2, 0, WIDTH / 2, HEIGHT / 2))
if n == 3:
pg.draw.rect(DISPLAY,color,
(0, HEIGHT / 2, WIDTH / 2, HEIGHT / 2))
if n == 4:
pg.draw.rect(DISPLAY,color,
(WIDTH / 2, HEIGHT / 2, WIDTH / 2, HEIGHT / 2))

def draw_buttons():

for i in range(1, 5):
draw_button(i, COLORS[i - 1])

def what_button_is_clicked(mouse_pos):
"""
The buttons are numbered in the following way
1 | 2
_____

3 | 4
"""
if mouse_pos[0] WIDTH / 2 and mouse_pos[1] HEIGHT / 2:
return 3

elif mouse_pos[0] > WIDTH / 2 and mouse_pos[1] > HEIGHT / 2:
return 4

def flash(button_number, waiting_time):
draw_button(button_number, WHITE)
pg.display.update()
pg.time.wait(waiting_time)
draw_button(button_number, COLORS[button_number - 1])
pg.display.update()

def inform_user(correct_sequence, delay):
for button in correct

Missing the specs

You're missing a very important rule of the game:


The pattern gets longer each time the player completes the pattern. If the player presses a wrong button, the game ends.

You implemented only a one-shot round, which is not so interesting.

Mutually exclusive conditions

These conditions are mutually exclusive:

if n == 1:
    # ...
if n == 2:
    # ...
if n == 3:
    # ...

So you should use elif for the 2nd and later conditions.

Use consistent names

It's odd to name some constants with all caps, and others with all lowercase like this:

WHITE = (255, 255, 255)
RED = (255, 0, 0)
green = (0, 255, 0)
blue = (0, 0, 255)
yellow = (255, 255, 0)

Be consistent, name all of them all caps!

Avoid hard-coding

This function relies on having 4 buttons:

def generate_correct_sequence(length):
    return [random.randint(1, 4) for _ in range(length)]

It would be better to avoid hardcoding the number 4.
You could for example use len(COLORS) instead.
Even better would be to use a dedicated constant,
and derive both the elements of colors from it,
and use it everywhere else where it's relevant, as in this function.

Use local variables to explain logic

In this code:

if mouse_pos[0]  WIDTH / 2 and mouse_pos[1] < HEIGHT / 2:
    return 2
# ...

mouse_pos[0] and mouse_pos[1] are repeated many times,
which is tedious, and not as clear as it can be.
I don't actually have tkinter now,
but I'm guessing these stand for x and y coordinates.

I would either change the method to take parameters named x and y,
or introduce the local variables x and y early in the method:

x = mouse_pos[0]
y = mouse_pos[1]

The rest of the code would become shorter, and clearer.

Why not use 0-base indexing everywhere?

This method is slightly more complex than it needs to be due to the 1-based indexing:

def draw_buttons():
    for i in range(1, 5):
        draw_button(i, COLORS[i - 1])

I bet you could change to this simpler implementation,
and adjust the rest of the code to work with it without sacrificing anything:

def draw_buttons():
    for i in range(4):
        draw_button(i, COLORS[i])

Notice the hardcoded number 4 again. This highlights again the importance to avoid hardcoding. The range(1, 5) came from the fact of the hard limit 4,
and if you decided to change 4 at some point, searching in the code for "4" you would never find range(1, 5).

So, really, replace the number 4 with a constant. len(COLORS) may look tempting, but as I explained above, I suggest to use something else as the single authoritative point of control.

StackExchange Code Review Q#71702, answer score: 13