Overall, I feel that my code is a bit verbose.
game.py
#!/usr/bin/python3
import sys,os
from models import Board, GameState
"""
Game loop for the minesweeper game.
"""
class Game:
def __init__(self):
self.board = Board(rows=10, cols=10)
def play(self):
self.welcome()
while self.board.game_state in [GameState.on_going, GameState.start]:
self.board.print_board_wrapper(self.board.print_board_hook)
try:
raw = input("> ")
line = "".join(raw.split())
if line[0] == "f":
point = tuple(map(int, line[1:].split(",")))
self.board.flag_square(point[0], point[1])
else:
point = tuple(map(int, line.split(",")))
self.board.click_square(point[0], point[1])
except (IndexError, ValueError):
self.help()
except KeyboardInterrupt:
try:
sys.exit(0)
except SystemExit:
os._exit(0)
if self.board.game_state == GameState.lose:
print("\n\nYou hit a mine. :(\n")
else:
print("\n\nYou win!\n")
self.board.print_board_wrapper(self.board.print_board_end_hook)
def welcome(self):
print("\nWelcome to PySweep!")
self.help()
def help(self):
print("\nEnter coordinates")
print("> <row>,<column>")
print("> 1,1")
print("Flag and unflag coordinates")
print("> f <row>,<column>")
print("> f 1,1")
if __name__ == "__main__":
game = Game()
game.play()
models.py
""" Data models for a minesweeper CLI game. """
import random
import itertools
from enum import Enum
class GameState(Enum):
start = 0
win = 1
lose = 2
on_going = 3
class Board:
""" Represents a minesweeper board with squares. """
def __init__(self, rows, cols):
self.rows = rows
self.cols = cols
self.game_state = GameState.start
self.number_of_mines = 0
self.max_mines = (cols-1)*(rows-1)
mines_percentage = (100 * self.max_mines / (rows*cols))/3
self.__create_squares(self.cols, self.rows, mines_percentage)
def flag_square(self, row, col):
if not self.__valid_square(row, col) or self.__get_square(row, col).clicked:
return
square = self.squares[row][col]
square.flag = not square.flag
def click_square(self, row, col):
"""
Click the square and click its
neighbors which don't have neighboring mines.
"""
if not self.__valid_square(row, col) or self.__get_square(row, col).clicked:
return
square = self.squares[row][col]
if self.game_state == GameState.start:
square.mine = False
for neighbor in square.neighbors():
neighbor.mine = False
self.game_state = GameState.on_going
if square.mine:
self.game_state = GameState.lose
return
square.clicked = True
if square.mine_neighbors() == 0:
for neighbor in square.neighbors():
if not neighbor.mine:
if neighbor.mine_neighbors() == 0:
self.click_square(neighbor.row, neighbor.col)
neighbor.clicked = True
if self.__win():
self.game_state = GameState.win
def print_board_wrapper(self, print_hook):
print("\n")
col_print = " "
for i in range(0, self.cols):
col_print += str(i) + " "
print(col_print + "\n")
for i,row in enumerate(self.squares):
row_print = str(i) + " "
for square in row:
row_print += print_hook(square)
print(row_print + "\n")
def print_board_hook(self, square):
"""
Prints the board. If a square is clicked,
print the number of neighboring mines.
If the square is flagged, print "f".
Else print ".".
"""
if square.clicked:
return " " + str(square.mine_neighbors()) + " "
elif square.flag:
return " f "
return " . "
def print_board_end_hook(self, square):
if square.mine:
return " x "
return self.print_board_hook(square)
def __win(self):
for row in self.squares:
for square in row:
if not square.mine and not square.clicked:
return False
return True
def __get_square(self, row, col):
""" Return the square at the given row and column."""
return self.squares[row][col]
def __valid_square(self, row, col):
return (row < self.rows and row >= 0) and (col < self.cols and col >= 0)
def __create_squares(self, cols, rows, mines_percentage):
"""
Create a grid of squares of size rows by cols.
"""
self.squares = [[Square(self, row, col, mine=self.__is_mine(mines_percentage))
for col in range(cols)] for row in range(rows)]
def __is_mine(self, mines_percentage):
""" Determine if a square is a mine while generating the board. """
is_mine = random.randrange(100) < mines_percentage
if is_mine:
if self.number_of_mines >= self.max_mines:
return False
self.number_of_mines = self.number_of_mines + 1
return True
return False
class Square:
"""
Represents a single square in the minesweeper board.
A square may have or may not have a mine, may be clicked or unclicked.
"""
def __init__(self, board, row, col, mine):
self.board = board
self.row = row
self.col = col
self.mine = mine
self.flag = False
self.clicked = False
def mine_neighbors(self):
return len(list(filter(lambda square: square.mine, [self.board.squares[point[0]][point[1]] for point in self.__point_neighbors()])))
def neighbors(self):
return [self.board.squares[point[0]][point[1]] for point in self.__point_neighbors()]
def __point_neighbors(self):
row_neighbors = list(filter(lambda val: val >= 0 and val < self.board.rows, [self.row-1, self.row, self.row+1]))
col_neighbors = list(filter(lambda val: val >= 0 and val < self.board.cols, [self.col-1, self.col, self.col+1]))
neighbor_set = set(itertools.product(row_neighbors, col_neighbors))
neighbor_set.remove((self.row, self.col))
return list(neighbor_set)
I think there's lots of places where my code could be made a lot more readable. The hook for the print_board function seems a bit suspect. The __point_neighbors function could probably be improved (having a lambda seems to be overkill).
I'm also looking to improve the mine generation algorithm. Currently, each square has an individual probability of being a mine and mine generation is stopped when max_mines is reached. So, the bottom right corner would be likely to have less mines. My solution to the "first click shouldn't be a mine" problem is I clear the space around the first click so you always get the eight spaces around the first click for free.
Here's a link to the git repository.
1 Answer 1
Your Game class might not need to be a class at all. You create self.board in the __init__() method, and then only use it in the play() method. Since play() doesn't return until the game is over, board could simply be a local variable, created at the beginning of the play() function. If that is changed, then Game doesn't have any data - just methods - so doesn't need to be a class.
point = tuple(map(int, line[1:].split(",")))
self.board.flag_square(point[0], point[1])
This code seems a little verbose. You are creating a tuple to force the conversion of the return value of map into something you can directly use, from the mysterious map object is actually returns. How about:
row, col = map(int, line[1:].split(","))
self.board.flag_square(row, col)
This is a little clearer, bordering on self-documenting. We expect exactly 2 values ... a row and a col value. If the user types in "f 3,4,7", your code would muddle on using the 3,4 value, where as the new code will raise a Value Error: too many values to unpack, which your code would catch and prompt the user for better input.
The Square class holds too much information. While mine, flag and clicked all seem required, board, row and col are not necessary; adding them has actually complicated your code. Your squares know where they are, so they can tell you who their neighbours are, as well has how many mines are around them. But imagine if they didn't. Not a problem: Board.squares[row][col] holds each square. You just need to ask Board for the neighbours of (row, col) or the number of mine around (row, col). The code just needs to move from Square to Board, and as these are board-level operations, that makes sense.
How many times is square.mine_neighbors() called? If you start the game clicking (3,4), each time you print the board, you will need to print the number of mines around (3,4), so after n turns, you've called it n times for that square. But on the second turn, you'll have clicked another square, and will have called the function n-1 times for that new square. And n-2 times for the third square that got clicked, and so on. The result is you're calling square.mine_neighbours() \$O(n^2)\$ times. The mines don't move; the number of neighbours never changes. Perhaps you could store one additional piece of information in Square ... the number of neighbouring mines. Compute that for the entire board at the start.
Board.print_board_wrapper(self, print_hook).
This API requires the caller to pass in one of two functions, depending on whether the game is over or not. The Board object itself has a self.game_state member which knows whether the game is over. Why force the caller to provide the correct method?
class Board:
...
def print_board(self):
if self.game_state in (GameState.start, GameState.on_going):
print_hook = self.print_board_hook
else:
print_hook = self.print_board_end_hook
self.print_board_wrapper(print_hook)
Now the caller can just call print_board(), and the correct board format will be used depending on state.
But wait! Why two "hook" methods depending on the game state? The Board.print_board_hook method knows the game state. You could get rid of the two hook methods, and replace them with:
def print_square(self, square):
"""..."""
if self.game_state in (GameState.win, GameState.lose) and square.mine:
return " x "
elif square.clicked:
return " " + str(square.mine_neighbors()) + " "
elif square.flag:
return " f "
return " . "
Why does Board have a function to print the game board? It doesn't do any other printing. You could add a graphical UI to the program, and most of Board doesn't have to be changed. The Game would need to be changed, of course, as well as these Board.print_board_* methods.
Perhaps the print_board() function should be moved into Game? In fact, it looks like you intended this, but forgot.
""" Data models for a minesweeper CLI game. """
(And "Data models" wouldn't have any user I/O methods)
A simple mine placement algorithm:
def _add_mines(self, mine_percentage):
num_mines = self.rows * self.cols * mine_percentage // 100
count = 0
while count < num_mines:
row = random.randrange(self.rows)
col = random.randrange(self.cols)
if not self.squares[row][col].mine:
self.squares[row][col].mine = True
count += 1
But, how to ensure the user's starting location isn't a mine? Easy. Make it a mine to begin with. Add all the other mines. Then remove that initial mine.
self.squares[row][col].mine = True
self._add_mines(mine_percentage)
self.squares[row][col].mine = False
As long as the mine percentage is not high, the number of iterations through the loop shouldn't be much larger than num_mines. To handle larger mine percentages, a different strategy should be used:
- Create a list of all board locations
- Remove the user's starting location
- Shuffle the list
- Add mines to the first
num_mineslocations in the list.
Something like:
locs = [(row, col) for row in range(self.rows) for col in range(self.cols)]
locs.remove((initial_row, initial_col))
random.shuffle(locs)
for row, col in locs[:num_mines]:
self.squares[row][col].mine = True
As you suspected, your neighbors(self) could be improved. You don't need itertools.product, or filter or lambda, nor even the __point_neighbors helper function. List comprehension with a double for loop can replace the product and set creation. Just need a few min/max operations to ensure the loops stay within the confines of the board. Assuming the code is moved to class Board:
def neighbors(self, row, col):
neighbor_set = { (r, c) for r in range(max(0, row-1), min(self.rows, row+2))
for c in range(max(0, col-1), min(self.cols, col+2)) }
neighbor_set.remove((row, col))
return neighbor_set
There is no need to turn the set into a list.