Simon memory game

Question

I have recently started to learn Python and I have decided to apply what I have learnt so far to creating a Simon Memory Game clone. I am now looking for help with improving the code. Any help with improving this code would be greatly appreciated and criticism is welcome. I'm using Python 3.5.

import tkinter
from random import choice
class Simon() :
 def __init__(self, master) :
 # Configure tkinter.Tk with some basic settings.
 self.master = master
 self.master.minsize(640, 480)
 self.master.resizable(False, False)
 self.master.title("Simon Memory Game")
 self.master.update() # Complete any outstanding tkinter tasks.
 # Create the canvas to be used to draw the rectangles that make up the game. Have it take up the entire window.
 self.game_canvas = tkinter.Canvas(self.master, width = self.master.winfo_width(), height = self.master.winfo_height(), highlightthickness = 0)
 self.game_canvas.pack()
 # Set up the four colors that will be used throughout the game.
 self.idle_colors = ("red", "blue", "green", "yellow")
 self.tinted_colors = ("#ff4d4d", "#4d4dff", "#4dff4d", "#ffff4d")
 self.current_colors = [color for color in self.idle_colors]
 self.rectangle_ids = []
 # Sequence of the colors for the current game and the position in the sequence for use when showing it to the user.
 self.sequence = [choice(self.idle_colors)]
 self.sequence_position = 0
 self.draw_canvas()
 self.show_sequence()
 self.master.mainloop()
 # Show the sequence to the player, so the player can repeat it.
 def show_sequence(self) :
 # Pass the current color of the sequence to the flash function.
 self.flash(self.sequence[self.sequence_position])
 # Check that we have not reached the end of the sequence.
 if(self.sequence_position < len(self.sequence) - 1) :
 # Since we haven't reached the end of the sequence increment the postion and schedule a callback.
 self.sequence_position += 1
 self.master.after(1250, self.show_sequence)
 else :
 self.sequence_position = 0 # Reset the position for next round.
 # Flash a single rectangle once. Used to indicate it is apart of the sequence to the player.
 def flash(self, color) :
 index = self.idle_colors.index(color) # Find the position in the tuple that the specified color is at.
 if self.current_colors[index] == self.idle_colors[index] : # Use the position of the color specified to compare if the current color on screen matches the idle one.
 # If so set the current color equal to that of the tinted color.
 self.current_colors[index] = self.tinted_colors[index]
 self.master.after(1000, self.flash, color) # Call this function again in 1 seconds time to revert back to the idle color
 else :
 self.current_colors[index] = self.idle_colors[index] # Revert the current color back to the idle color.
 self.draw_canvas() # Draw the canvas to reflect this change to the player.
 def check_choice(self) :
 color = self.idle_colors[self.rectangle_ids.index(self.game_canvas.find_withtag("current")[0])]
 if(color == self.sequence[self.sequence_position]) :
 if(self.sequence_position < len(self.sequence) - 1) :
 self.sequence_position += 1
 else :
 self.master.title("Simon Memory Game - Score: {}".format(len(self.sequence))) # Update the score.
 # Reached the end of the sequence append new color to sequence and play that.
 self.sequence.append(choice(self.idle_colors))
 self.sequence_position = 0
 self.show_sequence()
 else :
 # Game Over for the player as they got the sequence wrong reset the game back to level one.
 self.master.title("Simon Memory Game - Game Over! | Final Score: {}".format(len(self.sequence)))
 self.sequence[:] = [] # Empty the list of sequences
 self.sequence.append(choice(self.idle_colors)) # Add the first sequence of the new game to the list of sequences.
 self.sequence_position = 0
 self.show_sequence()
 def draw_canvas(self) :
 self.rectangle_ids[:] = [] # Empty out the list of ids.
 self.game_canvas.delete("all") # Clean the frame ready for the new one
 for index, color in enumerate(self.current_colors) : # Iterate over the colors in the list drawing each of the rectangles their respective place.
 if index <= 1 :
 self.rectangle_ids.append(self.game_canvas.create_rectangle(index * self.master.winfo_width(), 0, self.master.winfo_width() / 2, self.master.winfo_height() / 2, fill = color, outline = color))
 else :
 self.rectangle_ids.append(self.game_canvas.create_rectangle((index - 2) * self.master.winfo_width(), self.master.winfo_height(), self.master.winfo_width() / 2, self.master.winfo_height() / 2, fill = color, outline = color))
 for id in self.rectangle_ids :
 self.game_canvas.tag_bind(id, '<ButtonPress-1>', lambda e : self.check_choice())
def main() :
 root = tkinter.Tk()
 gui = Simon(root)
if __name__ == "__main__" : main()

Answer 1

Style

Aside of @syb0rg's answer, there are some comments in the text intended to describe the method that follows. These should be docstrings instead.

You also happen to have magic numbers and sequences that would be better as constants, or at least class-level attributes:

class Simon:
 IDLE = ("red", "blue", "green", "yellow")
 TINTED = ("#ff4d4d", "#4d4dff", "#4dff4d", "#ffff4d")
 FLASH_ON = 1000
 FLASH_OFF = 250
 def __init__(self):
 ...

Reinventing the wheel

Instead of drawing rectangles on a canvas and binding events to them, tkinter provides the Button widget which allows a lot out-of-the-box: changing the color is done with button.config(background='green'), binding an action is done with the command attribute; and you can also disable a button so user can't interact when the sequence is being displayed.

The only thing to take care of is retrieving the right button into check_choice: you'll need to add some kind of index parameter. But the command keyword used when building a button expects a no-arguments callable. You can overcome that by using helper methods that call check_choice with a fixed index, using a lambda, or using functools.partial.

Flashing the tiles

I find the way you handle showing the sequence to the player somewhat uncommon. Having flash being called twice (on + off) between two calls of show_sequence feels weird. It would be more straightforward to have show_sequence initiate stuff and then a flash_on and a flash_off method that are calling each other using self.master.after (and using the self.FLASH_ON and self.FLASH_OFF constants).

Iterating over the sequence

Not that your way of doing is that bad considering the constraints, but I just want to show an alternative approach: using iter and next. Instead of storing the current index, you store an iterator to you sequence using iter and the current button that should be pushed. After a successful button push, you update the current value using next. And if StopIteration is raised, then it means the player reached the end of the sequence.

This method can be used both when waiting for user input and when flashing the buttons.

Nitpicks

I find that flashing a tile for a whole second is a little bit slow. I personally would have used 600 or 750 ms.

I also find that the tinted colors are too close to the idle ones (except for green). You should use colors that are more distinct so the player can better see which one is next in the sequence.

Lastly, I feel that running self.master.mainloop() or even having the root created outside the class does not allow for much customization, I would keep things internal and provide a method to launch the game (that will run self.master.mainloop().

Proposed improvements

import tkinter as tk
import random
from functools import partial
class Simon:
 IDLE = ('red', 'blue', 'green', 'yellow')
 TINTED = ('#ff4d4d', '#4d4dff', '#4dff4d', '#ffff4d')
 FLASH_ON = 750 #ms
 FLASH_OFF = 250 #ms
 def __init__(self, title='Simon Memory Game'):
 self.master = tk.Tk()
 self.master.title(title)
 self.master.resizable(False, False)
 self.title = title
 self.buttons = [
 tk.Button(
 self.master,
 height=15,
 width=25,
 background=c,
 activebackground=c, # remove this line if you want the player to see which button is hovered
 command=partial(self.push, i))
 for i, c in enumerate(self.IDLE)]
 for i, button in enumerate(self.buttons):
 button.grid({'column': i % 2, 'row': i // 2})
 def reset(self):
 self.sequence = []
 self.new_color()
 def push(self, index):
 if index == self.current:
 try:
 self.current = next(self.iterator)
 except StopIteration:
 self.master.title('{} - Score: {}'
 .format(self.title, len(self.sequence)))
 self.new_color()
 else:
 self.master.title('{} - Game Over! | Final Score: {}'
 .format(self.title, len(self.sequence)))
 self.reset()
 def new_color(self):
 for button in self.buttons:
 button.config(state=tk.DISABLED)
 color = random.randrange(0, len(self.buttons))
 self.sequence.append(color)
 self.iterator = iter(self.sequence)
 self.show_tile()
 def show_tile(self):
 try:
 id = next(self.iterator)
 except StopIteration:
 # No more tiles to show, start waiting for user input
 self.iterator = iter(self.sequence)
 self.current = next(self.iterator)
 for button in self.buttons:
 button.config(state=tk.NORMAL)
 else:
 self.buttons[id].config(background=self.TINTED[id])
 self.master.after(self.FLASH_ON, self.hide_tile)
 def hide_tile(self):
 for button, color in zip(self.buttons, self.IDLE):
 button.config(background=color)
 self.master.after(self.FLASH_OFF, self.show_tile)
 def run(self):
 self.reset()
 self.master.mainloop()
if __name__ == '__main__':
 game = Simon()
 game.run()

• user112566: Thanks for the input. I understand that I shouldn't have reinvented the wheel and instead used the buttons which judging from your code would of made the project much easier to develop. Also thanks for suggesting the alternate approach of using iter and next as I will read up on this and try to incorporate them in my next project.

Answer 2

Most of this will come down to reading PEP8, however I'll state them here anyway.

• if statements don't need parentheses, so don't put them around them. You seem to go between doing this and not, remember to also be consistent

• Functions at module level should have two new lines between them and other things, rather than one as you have now

• In your function calls, you have unexpected spaces around keyword/parameter assignments

• There should also be two blank lines between the import statements and other code

• You should have at least two spaces before inline comment.

• Many of your lines are too long, they should only be 79 characters maximum

• In Whitespace in Expressions and Statements,

  Avoid extraneous whitespace in the following situations:

  ...

  Immediately before a comma, semicolon, or colon:

  Yes: if x == 4: print x, y; x, y = y, x
  No: if x == 4 : print x , y ; x , y = y , x 
  

• You shouldn't have multiple statements on one line (there should be a newline after the colon)

• There is no newline at end of file as seen here.