14
\$\begingroup\$

In my coding interview for a company, I got the question to write a Minesweeper game. I did not manage to finish the game in 1 hour at that time, so now I have written it again after the interview. Could you please help me to check if my code follows good practices for a game-program ? I appreciate any ideas.

import random
import pdb
from os import system
from enum import Enum


class GameStatus(Enum):
    PLAYING = 0
    LOSE = 1
    WIN = 2

class MineBoard(object):

    def __init__(self, w, h, k):
        # Create a new board with size w x h
        self.w = w
        self.h = h
        self.board = [[0 for i in range(w)] for j in range(h)]
        self.allocateMines(w, h, k)
        self.status = GameStatus.PLAYING
        self.cellsToOpen = w * h - k

    def allocateMines(self, w, h, numOfMines):
        allocIndexes = self.getRandomPos(w * h, numOfMines)
        for i in allocIndexes:
            self.setMine(int(i / w), i % h)
            self.setAdjacentMines(int(i / w), i % h)

    def printLayout(self):
        print(' ' * 7 + ''.join(map(lambda x : '{:^7d}'.format(x + 1), range(self.w))))
        print(' ' * 7 + '-' * (self.w * 7))
        for (i, row) in enumerate(self.board):
            print('{:^7d}'.format(i + 1) + '|' + ' |'.join(list(map(lambda x : '{:^5s}'.format(self.display(x)), row))) + ' | ')
            print(' ' * 7 + '-' * (self.w * 7))

    def click(self, row, col):
        value = self.reveal(row, col)
        if value:
            self.cellsToOpen -= 1
            if self.cellsToOpen == 0:
                self.status = GameStatus.WIN
            if self.hasMine(row, col):
                self.status = GameStatus.LOSE
            elif self.isBlank(row, col):
                for dr in range(row - 1, row + 2):
                    for dc in range(col - 1, col + 2):
                       self.click(dr, dc)

    def flag(self, row, col):
        if self.isValidCell(row, col) and self.isHidden(row, col):
            self.toggleFlag(row, col)

    def isValidCell(self, row, col):
        return row >= 0 and row < self.h and col >= 0 and col < self.w


    def getRandomPos(self, n, k):
        res = []
        remains = [i for i in range(n)]
        while k > 0:
            r = random.randint(0, len(remains) - 1)
            res.append(r)
            del remains[r]
            k -= 1
        return res

    #Convention for cell values:
    #    - 0 : Hidden Blank
    #    - 10 : Revealed Blank
    #    - -1 : Hidden Bomb
    #    - 9 : Revealed Bomb
    #    - 1 ~ 8 : number of adjacent bomb (hidden)
    #    - 11 ~ 18 : adjacent bomb (revealed)
    #    - x + 100 : Flagged
    #

    def setMine(self, row, col):
        self.board[row][col] = -1

    def setAdjacentMines(self, row, col):
        for dr in range(row - 1, row + 2):
            for dc in range(col - 1, col + 2):
                if self.isValidCell(dr, dc) and not self.hasMine(dr, dc):
                    self.board[dr][dc] += 1

    def toggleFlag(self, row, col):
        if self.isFlagged(row, col):
            self.board[row][col] -= 100
        else:
            self.board[row][col] += 100

    # Open a cell and return its value
    def reveal(self, row, col):
        if not self.isValidCell(row, col) or not self.isHidden(row, col):
            return None
        if self.isFlagged(row, col):
            self.toggleFlag(row, col)
        self.board[row][col] += 10
        return self.board[row][col]

    def isHidden(self, row, col):
        return self.board[row][col] < 9

    def hasMine(self, row, col):
        return self.board[row][col] % 10 == 9

    def isBlank(self, row, col):
        return self.board[row][col] % 10 == 0

    def isOver(self):
        return self.winGame() or self.loseGame()

    def loseGame(self):
        return self.status == GameStatus.LOSE

    def winGame(self):
        return self.status == GameStatus.WIN

    def isFlagged(self, row, col):
        return self.board[row][col] > 90

    def revealAll(self):
        for i in range(len(self.board)):
            for j in range(len(self.board[0])):
                self.reveal(i, j)

    def display(self, ip):
        if ip > 90:
            return 'F'
        elif ip == 9:
            return '*'
        elif ip == 10:
            return '.'
        elif ip > 10:
            return str(ip - 10)
        return ''


def cls():
    system('clear')

def play():
    w = int(input('Enter width of board: '))
    h = int(input('Enter height of board: '))
    m = int(input('Enter number of mines : '))
    while m >= w * h - 1:
        m = int(input('Too many mines. Enter again : '))
    game = MineBoard(w, h, m)
    while not game.isOver():
        cls()
        game.printLayout()
        command = nextCommand()
        splits = command.split(' ')
        row = int(splits[0]) - 1
        col = int(splits[1]) - 1
        if command[-1] == 'F':
            game.flag(row, col)
        else:
            game.click(row, col)

    game.revealAll()
    cls()
    game.printLayout()
    if game.loseGame():
        print('You lose !!')
    elif game.winGame():
        print('You win !!. Congradulations !!')


def nextCommand():
    instruction = 'Commands : \n<row> <col> : open cell\n<row> <col> F : flag cell\nq : quit\n'
    return input(instruction).strip()

play()
\$\endgroup\$
20
\$\begingroup\$

Linter, style checker, static analyzer, code formatter

You should use a linter and/or a static analyzer, preferably one with an auto-correct functionality. I actually have multiple linters and multiple static analyzers configured in my editor, and they are set up so that they analyze my code while I type, and automatically correct whatever they can auto-correct when I save.

When I save your code into a file and open the file in my editor, I get a whopping

  • 157(!!!) Errors
  • 44 Warnings
  • 21 Infos

Now, to be fair, a lot of these are duplicates, because as I mentioned, I have multiple linters and analyzers set up. Also, I have them set to pretty aggressive settings, which can sometimes be annoying and overwhelming if you work with code that you haven't freshly written yourself. (OTOH, it is tremendously helpful if you have them turned on from the start, since you will be immediately notified and can thus avoid letting the count ever get this high.)

In particular, I have type checking turned on, and almost 130 of the Errors are from Pylance complaining it can't fully determine the static type of some variable, parameter, or function. Now, if you care about static type safety or not, that is a subjective opinion, so you may ignore these Errors.

The nice thing about style checkers, linters, and static analyzers with auto-correction support, and automatic code formatters is that they do (part of) your work for you. For example, as mentioned, if I simply save your code into a file and open that file in an editor, I get 157 Errors, 44 Warnings, and 21 Infos.

If, instead, I copy&paste the code into my editor, even during the "paste" operation, it already starts automatically applying fixes, and I only get 139 Errors, 30 Warnings, and 21 Infos. And I get the code formatted according to my preferences (e.g. using " instead of ').

Consistency

Sometimes, you use two blank lines between methods, sometimes only one.

You should choose one style and stick with it. If you are editing some existing code, you should adapt your style to be the same as the existing code. If you are part of a team, you should adapt your style to match the rest of the team.

In general, if you use two different ways to write the exact same thing, the reader will think that you want to convey a message with that. So, you should only use two different ways of writing the same thing IFF you actually want to convey some extra information.

Note that PEP8 mandates two lines after classes, one line after methods and functions.

PEP8 violations

The standard community coding style for the Python community is defined in Python Enhancement Proposal 8 – Style Guide for Python Code. You should always follow the guidelines of PEP8. There are plenty of tools available that can flag and even auto-correct violations of PEP8.

Here's just a couple that my editor flagged:

  • No space before colon: In some places, you have space before the colon that starts a new block. There should be no space.
  • Line length: PEP8 is rather strict about line length. Some of your lines are too long.
  • Sometimes, you have 1 blank line after the function. There should be 2 blank lines after a function or class.
  • Docstring: All your public modules, classes, methods, and functions should have a docstring explaining their usage.
  • snake_case naming: The standard naming convention in Python is PascalCase for classes, snake_case for modules, functions, methods, attributes, variables, and parameters, and SCREAMING_SNAKE_CASE for constants. You are using camelCase for functions, methods, attributes, variables, and parameters. Use snake_case instead. For example, allocateMines should be allocate_mines, and so on.

Note that, if we ignore the afore-mentioned undefined types, then the naming accounts for a vast majority of the remaining issues my editor reports. So, let's fix those names.

Vertical whitespace

Your code is all bunched up together. Some empty lines would allow the code room to breathe, for example in the play function. The function is clearly separated into a series of steps: setup, game loop, finish. The same applies to the game loop itself, it also has distinct steps. Some whitespace would help draw attention to those steps:

    w = int(input("Enter width of board: "))
    h = int(input("Enter height of board: "))
    m = int(input("Enter number of mines : "))
    while m >= w * h - 1:
        m = int(input("Too many mines. Enter again : "))

    game = MineBoard(w, h, m)

    while not game.isOver():
        cls()
        game.printLayout()

        command = nextCommand()

        splits = command.split(" ")
        row = int(splits[0]) - 1
        col = int(splits[1]) - 1

        if command[-1] == "F":
            game.flag(row, col)
        else:
            game.click(row, col)

    game.revealAll()
    cls()
    game.printLayout()

    if game.loseGame():
        print("You lose !!")
    elif game.winGame():
        print("You win !!. Congradulations !!")

Actually, it would make even more sense to extract the various separate steps into separate functions.

Chained comparison

Python supports chained comparisons, i.e.

a < b and b < c

can be written as

a < b < c

You could use that here:

return row >= 0 and row < self.h and col >= 0 and col < self.w

could be

return 0 <= row < self.h and 0 <= col < self.w

Unnecessary comprehension

remains = [i for i in range(n)]

is unnecessary. You can pass any iterable to the list constructor to create a list:

remains = list(range(n))

Unused import

You import pdb but never use it. Remove the import.

Explicit inheritance from object

Your MineBoard class explicitly inherits from object. That is unnecessary in Python 3. Python 2 is no longer supported since 1 January 2020 (i.e. over 1.5 years), and Python 3 has been supported since 3 Dec 2008 (i.e. over 12.5 years). There is absolutely no reason to use Python 2 for new code in 2021.

So, your class declaration should just be

class MineBoard:

Unused variables

In one of your list comprehensions, you have unused variables:

self.board = [[0 for i in range(w)] for j in range(h)]

Neither i nor j are used. They should be called _ to make clear that they are deliberately ignored:

self.board = [[0 for _ in range(w)] for _ in range(h)]

No else / elif after return

In this piece of code

if ip > 90:
    return "F"
elif ip == 9:
    return "*"
elif ip == 10:
    return "."
elif ip > 10:
    return str(ip - 10)
return ""

All the elifs can just be ifs, since in all of the conditionals in this method, we either exit the method or the conditional was false. In other words: if we reach the if at all, we know that all the ifs before it were false, because otherwise we would already have returned from the method.

if ip > 90:
    return "F"
if ip == 9:
    return "*"
if ip == 10:
    return "."
if ip > 10:
    return str(ip - 10)
return ""

Naming

There are a couple of names in your code that could be clearer, for example ip, m, and k. In particular, it seems that the parameter k in __init__, the parameter num_of_mines in allocate_mines, and the local variable m in play mean the same thing, but the parameter k in get_random_pos does not mean the same thing as the parameter k in __init__.

They should really have more intention-revealing names. A good name should be intention-revealing. They should convey meaning.

I also noticed something strange about the MineBoard. It appears that MineBoard is not actually a board of mines. It is also a game of minesweeper. In fact, when you instantiate it, you actually assign it to a variable named game!

So, this implies two things: one, the class should probably have a different name (e.g. Game). But more importantly, the reason why it is hard to give it a proper name is that it appears to be doing too much. In particular, it represents two totally different concepts: a map / board, and a game. It should probably be split into two classes.

Mixing I/O and computation

I am not a big fan of mixing I/O and computation.

An example of what I mean is the print_layout method. It mixes responsibilities of creating the string representation and printing it. This makes it hard to reuse and hard to test. You can't just call it and check its result value in a test, for example, you actually have to capture the output from the terminal.

Instead, this method should be split into two methods. One which just creates the string representation of the board, and a second one which prints it. The first one should probably just be MineBoard's __str__ method, and the second one should probably be part of the game logic rather than the board logic.

Something like this:

def __str__(self) -> str:
    return "\n".join(
        [
            " " * 7 + "".join(map(lambda x: "{:^7d}".format(x + 1), range(self.w))),
            " " * 7 + "-" * (self.w * 7),
            "\n".join(
                (
                    "{:^7d}".format(i + 1)
                    + "|"
                    + " |".join(
                        list(map(lambda x: "{:^5s}".format(self.display(x)), row))
                    )
                    + " | "
                )
                + "\n"
                + (" " * 7 + "-" * (self.w * 7))
                for (i, row) in enumerate(self.board)
            ),
        ]
    )

And then in play, the two calls to game.print_layout() can simply be replaced by print(game).

Comments

Generally speaking, comments are a code smell. Mostly, comments should not exist:

  • If your code is so complex that you need to explain it in a comment, you should rather try to refactor your code to be less complex so that it needs no explanation.
  • If you have a comment that explains what something is, you can rather give it a name that tells the reader what the thing is.
  • If you have a comment that explains how the code does something, just delete it: the code explains how the code does something.

The only acceptable thing for a comment is to explain why the code does something in a specific non-obvious way. So, for example, there is an obvious way that looks like it should work, but you tried it and it didn't work for a non-obvious reason. Then you can add a comment explaining that you are specifically using solution B even though it looks like much simpler solution A should also work, but it actually doesn't work because of issue X. Ideally, you would add a link to the pull request / code review / bug ticket where this issue is discussed in greater detail and maybe a link to a wiki page with a detailed explanation.

The major offender is this comment here:

# Convention for cell values:
#    - 0 : Hidden Blank
#    - 10 : Revealed Blank
#    - -1 : Hidden Bomb
#    - 9 : Revealed Bomb
#    - 1 ~ 8 : number of adjacent bomb (hidden)
#    - 11 ~ 18 : adjacent bomb (revealed)
#    - x + 100 : Flagged
#

This comment is problematic for many reasons.

For one, it is placed in an awkward sport, in the middle of the class. It should probably be part of the class documentation proper, i.e. of the docstring.

However, it really should not exist at all. Such important information, and such an encoding should be encapsulated in an object. It looks like you are missing an abstraction, probably something like a Cell (which could be a namedtuple or a dataclass).

This abstraction would also allow us to move some of the methods out of MineBoard. For example, display should be an instance method of Cell. In fact, it should probably be Cell's __str__ method instead.

A big clue is the fact that you have multiple comments talking about "cells" but you have no abstraction called "cell" in your code. That is often a dead giveaway that you are missing an abstraction.

if __name__ == "__main__":

It is generally recommended to guard your main entry point using the familiar if __name__ == "__main__": construct. This way, the main entry point will only be automatically executed if the module is run as a script, but not if it is imported:

if __name__ == "__main__":
    play()

Shebang line

Since you intend to run this as a script, it should have a shebang line, something like this:

#!/usr/bin/env python3

(Interim) result

#!/usr/bin/env python3

import random
from enum import auto, Enum
from subprocess import run
from textwrap import dedent
from typing import Iterable, Optional


class GameStatus(Enum):
    PLAYING = auto()
    LOSE = auto()
    WIN = auto()


class MineBoard:
    status: GameStatus = GameStatus.PLAYING
    w: int
    h: int
    board: list[list[int]]
    cells_to_open: int

    def __init__(self, w: int, h: int, k: int) -> None:
        # Create a new board with size w x h
        self.w = w
        self.h = h
        self.board = [[0 for _ in range(w)] for _ in range(h)]
        self.allocate_mines(w, h, k)
        self.cells_to_open = w * h - k

    def allocate_mines(self, w: int, h: int, num_of_mines: int) -> None:
        alloc_indexes = self.get_random_pos(w * h, num_of_mines)
        for i in alloc_indexes:
            self.set_mine(int(i / w), i % h)
            self.set_adjacent_mines(int(i / w), i % h)

    def __str__(self) -> str:
        return "\n".join(
            [
                " " * 7 + "".join(map(lambda x: "{:^7d}".format(x + 1), range(self.w))),
                " " * 7 + "-" * (self.w * 7),
                "\n".join(
                    (
                        "{:^7d}".format(i + 1)
                        + "|"
                        + " |".join(
                            list(map(lambda x: "{:^5s}".format(self.display(x)), row))
                        )
                        + " | "
                    )
                    + "\n"
                    + (" " * 7 + "-" * (self.w * 7))
                    for (i, row) in enumerate(self.board)
                ),
            ]
        )

    def click(self, row: int, col: int) -> None:
        value = self.reveal(row, col)
        if value:
            self.cells_to_open -= 1
            if self.cells_to_open == 0:
                self.status = GameStatus.WIN
            if self.has_mine(row, col):
                self.status = GameStatus.LOSE
            elif self.is_blank(row, col):
                for dr in range(row - 1, row + 2):
                    for dc in range(col - 1, col + 2):
                        self.click(dr, dc)

    def flag(self, row: int, col: int) -> None:
        if self.is_valid_cell(row, col) and self.is_hidden(row, col):
            self.toggle_flag(row, col)

    def is_valid_cell(self, row: int, col: int) -> bool:
        return 0 <= row < self.h and 0 <= col < self.w

    def get_random_pos(self, n: int, k: int) -> Iterable[int]:
        res: list[int] = []
        remains = list(range(n))
        while k > 0:
            r = random.randint(0, len(remains) - 1)
            res.append(r)
            del remains[r]
            k -= 1
        return res

    # Convention for cell values:
    #    - 0 : Hidden Blank
    #    - 10 : Revealed Blank
    #    - -1 : Hidden Bomb
    #    - 9 : Revealed Bomb
    #    - 1 ~ 8 : number of adjacent bomb (hidden)
    #    - 11 ~ 18 : adjacent bomb (revealed)
    #    - x + 100 : Flagged

    def set_mine(self, row: int, col: int) -> None:
        self.board[row][col] = -1

    def set_adjacent_mines(self, row: int, col: int) -> None:
        for dr in range(row - 1, row + 2):
            for dc in range(col - 1, col + 2):
                if self.is_valid_cell(dr, dc) and not self.has_mine(dr, dc):
                    self.board[dr][dc] += 1

    def toggle_flag(self, row: int, col: int) -> None:
        if self.is_flagged(row, col):
            self.board[row][col] -= 100
        else:
            self.board[row][col] += 100

    # Open a cell and return its value
    def reveal(self, row: int, col: int) -> Optional[int]:
        if not self.is_valid_cell(row, col) or not self.is_hidden(row, col):
            return None
        if self.is_flagged(row, col):
            self.toggle_flag(row, col)
        self.board[row][col] += 10
        return self.board[row][col]

    def is_hidden(self, row: int, col: int) -> bool:
        return self.board[row][col] < 9

    def has_mine(self, row: int, col: int) -> bool:
        return self.board[row][col] % 10 == 9

    def is_blank(self, row: int, col: int) -> bool:
        return self.board[row][col] % 10 == 0

    def is_over(self) -> bool:
        return self.win_game() or self.lose_game()

    def lose_game(self) -> bool:
        return self.status == GameStatus.LOSE

    def win_game(self) -> bool:
        return self.status == GameStatus.WIN

    def is_flagged(self, row: int, col: int) -> bool:
        return self.board[row][col] > 90

    def reveal_all(self) -> None:
        for i in range(len(self.board)):
            for j in range(len(self.board[0])):
                self.reveal(i, j)

    def display(self, ip: int) -> str:
        if ip > 90:
            return "F"
        if ip == 9:
            return "*"
        if ip == 10:
            return "."
        if ip > 10:
            return str(ip - 10)
        return ""


def cls() -> None:
    run(["/usr/bin/env", "clear"], check=True)


def play() -> None:
    w = int(input("Enter width of board: "))
    h = int(input("Enter height of board: "))
    m = int(input("Enter number of mines : "))
    while m >= w * h - 1:
        m = int(input("Too many mines. Enter again : "))

    game = MineBoard(w, h, m)

    while not game.is_over():
        cls()
        print(game)

        command = next_command()

        splits = command.split(" ")
        row = int(splits[0]) - 1
        col = int(splits[1]) - 1

        if command[-1] == "F":
            game.flag(row, col)
        else:
            game.click(row, col)

    game.reveal_all()
    cls()
    print(game)

    if game.lose_game():
        print("You lose !!")
    elif game.win_game():
        print("You win !!. Congradulations !!")


def next_command() -> str:
    instruction = dedent(
        """\
            Commands :
            <row> <col> : open cell
            <row> <col> F : flag cell
            q : quit
        """
    )
    return input(instruction).strip()


if __name__ == "__main__":
    play()

Note: In order to make this answer useful for future readers, I have mostly assumed Python 3.10, which is about to be released soon. However, I don't think I have used anything that is not available in Python 3.9, and the code can be trivially made to work with at least Python 3.8.

\$\endgroup\$
4
  • \$\begingroup\$ Funny that we came to the dual layer / dual classes approach seperately. There must be something in that :) \$\endgroup\$ Sep 12 at 15:07
  • \$\begingroup\$ A minor comment: if you've ever worked with multilingual applications, _ is conventionally used with gettext (_ = gettext.gettext). By default, Pylint will ignore an unused variable called "dummy" or beginning with "unused_" (can be controlled with the "dummy-variables-rgx" setting). \$\endgroup\$
    – Aaron F
    Sep 13 at 10:37
  • \$\begingroup\$ I've always find it incredulous that comments are discouraged in a blanket fashion. You could certainly make a case that OP's code doesn't need comments, but that's not true in general. \$\endgroup\$
    – Passer By
    Sep 14 at 10:10
  • 1
    \$\begingroup\$ It's also possible to implement is_valid_cell as return row in range(self.h) and col in range(self.w) \$\endgroup\$
    – Aemyl
    Sep 14 at 11:25
5
\$\begingroup\$

Generally the code shows a consistent style, so in that regard I think it looks good. I like the way the status is explicitly kept using the enum; it makes everything that more easy to follow.


def __init__(self, w, h, k):

I could guess the w and h, but how could a caller know that k is the number of mines? Please use descriptive variable names.


self.setAdjacentMines(int(i / w), i % h)

This goes entirely unexplained in the code. First you create a list of indices, set the mines and then.. setAdjacentMines - why?


def printLayout(self):

I would expect that a method called printLayout prints... just the layout. However, it seems that it prints the entire board & board state. I think this may be a method that got expanded and never renamed.

The literal 7 appears a few times in printLayout. Why not create a constant value such as MARGIN for it? At least I presume it is a margin of sorts.


def click(self, row, col):

It seems that a click is also opening mines around the clicked location. What I find strange is that it seems those clicks can also explode mines.

click is used as a method name. This becomes a bit troublesome if you also allow "virtual clicks", as we find out later in the method. probe would maybe be a better name.

elif self.isBlank(row, col):

Personally I don't like it when click hides other functionality, I'd put that in a calling function. All in all, it doesn't adhere to the principle of least surprise to me.


def setAdjacentMines(self, row, col):

This method uses higher level functions to detect the state of a position, but then uses += 1 to set the state. I don't exactly get what it is supposed to do at first glance, even after looking at the conventions. I presume it is trying to count bombs.


#Convention for cell values:
#    - 0 : Hidden Blank
#    - 10 : Revealed Blank
#    - -1 : Hidden Bomb
#    - 9 : Revealed Bomb
#    - 1 ~ 8 : number of adjacent bomb (hidden)
#    - 11 ~ 18 : adjacent bomb (revealed)
#    - x + 100 : Flagged
#

Beware that comments that are somewhere within the code tend to get lost. This is especially true for environments that allow for reordering or refactoring of methods. Generally I would make those specific to the class; you need this to understand most of the methods in it anyway.


def isOver(self):
    return self.winGame() or self.loseGame()

def loseGame(self):
    return self.status == GameStatus.LOSE

def winGame(self):
    return self.status == GameStatus.WIN

What's wrong with getStatus?


def loseGame(self):
    return self.status == GameStatus.LOSE

def winGame(self):
    return self.status == GameStatus.WIN

These methods should definitely be private. But I honestly don't see why they exist at all, in that case.


while m >= w * h - 1:

Ow, I wonder how you would reveal those mines. Oh well, a bit of unfairness never hurt :)


while not game.isOver():

That one was expected after seeing isOver being defined. Try while game.getStatus == Playing Always try and use positive tests.


command = nextCommand()
splits = command.split(' ')
row = int(splits[0]) - 1
col = int(splits[1]) - 1
if command[-1] == 'F':
    game.flag(row, col)
else:
    game.click(row, col)

This should definitely be in a separate method. I'd use regular expressions here, if just to weed out invalid commands. Then you can use groups 1, 2 and 3 to retrieve the values.


game.revealAll()

I like this, and the fact that you use a separate call to print the board.

About the design

As indicated in other questions: using a position type would make sense, e.g. by randomly "allocating" mines.

The idea to have one board with an integer to represent states is a nice idea. However, it is also rather dangerous. For instance, it would allow you to flag already revealed positions, or maybe call setMine after the setup stage. It is therefore quite easy to move the board into an invalid state or to make invalid moves. In general I would prefer a game where the methods make sure you cannot cheat.

Another method is to have multiple layers, e.g. one with mines (and mine counts, for convenience) and one layer that shows if the position has been revealed or flagged.

I would certainly perform a clear split between setting up the board and playing the game. Factories, factory methods and/or private methods could play a role here. The split could be virtual (just private methods called when setting up the board, otherwise not separated) or explicit (a separate builder class).

All that said, after I concluded the review I understood the class design and would be able to alter it. So it definitely passed that test. I'd have to print out the board to understand printLayout fully, but that's OK.

Congradulations

You're weldone!

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for taking your time ! I love how you help to suggest some other names for my variables. I always struggle to name things while coding. \$\endgroup\$
    – Felicity
    Sep 14 at 2:07
  • \$\begingroup\$ I got an edit request that fixed the misspelling of "Congradulations" & "You're weldone" which is of course a joke on the misspelling of "Congratulations" in the code of the question. \$\endgroup\$ Sep 14 at 13:06
3
\$\begingroup\$

(I've taken the liberty of converting all identifiers to PEP8 style.)

Board state

The state of a cell on a board is encoded with a single integer, which combines the following information:

  • Is this cell revealed?
  • Does this cell have a mine?
  • Does this cell have a flag?
  • How many neighbours of this cell are mines?

This results in complicated code to check those properties, numerous magic numbers, and a lot of crevices where bugs can creep in. On the upside, it's fairly space efficient, but unless you're planning on allowing giant boards, that shouldn't make much of a difference. Here's my proposal:

@dataclass
class Cell:
    revealed: bool = False
    has_mine: bool = False
    has_flag: bool = False
    neighbouring_mines: int = 0

    def is_blank(self) -> bool:
        return not self.has_mine and self.neighbouring_mines == 0

(I like using dataclasses for things like this, but of course there are plenty of other options, like attrs or a plain Python class!)

This allows you to make various MineBoard methods less complex, for example:

def toggle_flag(self, row, col):
    self.board[row][col].has_flag = not self.board[row][col].has_flag

allocate_mines

In all other places, you use row and column indexing, but in this method you're using an index. For consistency, I'd use a list of tuples for the mine locations. Additionally, you don't need to generate this list yourself, you can use random.sample:

def allocate_mines(self, width, height, number_of_mines):
    all_indices = [(row, col) for row in range(height) for col in range(width)]
    mines = random.sample(all_indices, number_of_mines)
    for row, col in mines:
        self.set_mine(row, col)
        self.set_adjacent_mines(row, col)

is_valid_cell

You're using this method in several places inside loops. Instead of looping unnecessarily over out-of-bound cells, try instead adjusting the range boundaries:

def set_adjacent_mines(self, row, col):
    for dr in range(max(0, row - 1), min(self.height, row + 2)):
        for dc in range(max(0, col - 1), min(self.width, col + 2)):
            self.board[dr][dc].neighbouring_mines += 1

Item access

This is just a spur-of-the-moment idea, but you could implement __getitem__ for the MineBoard class. It could access Cell objects and -- when passed slices --- could even return an iterable over the Cells. For example:

def toggle_flag(self, row, col):
    self[row, col].has_flag = not self[row, col].has_flag

def set_adjacent_mines(self, row, col):
    for cell in self[row - 1:row + 2, col - 1:col + 2]:
        cell.neighbouring_mines += 1

... Suitable implementation of __getitem__ left as an exercise for the reader.

\$\endgroup\$
1
  • \$\begingroup\$ I know that represent everything in just one single number makes things much more complex here. It must be the result of doing many leetcode exercises recently and I just tend to save memory anytime possible. Love the idea of 'Item access'. Thanks !! \$\endgroup\$
    – Felicity
    Sep 14 at 2:11
2
\$\begingroup\$

Thanks Felicity for your post. You wrote that you're going for coding interviews, so I'll focus on various aspects that will be looked at by interviewers rather than pieces of code which other respondents already have. This might be a little extensive, but it's good to make you aware of what could be covered when submitting code during the interview process.

  • Python-specific

    • Not using PEP8 naming conventions
    • Not using the __main__ entry point
    • Not using f-string formatting
    • pdb is not used, be aware of unused imports in the final version
  • Input validation

    • No checking of inputs
    • No catching/handling of exceptions raised e.g.:IndexError list assignment index out of range
  • Coding Style

    • Using .strip() on a string which is hard-coded
    • Comments in the code explaining what the code does when the code expresses this already
    • Classes exposing private attributes as public
    • Variable naming
    • Mixing game logic with board logic (and instantiating the board as game changing it's role)
    • Enum for GameStatus but then not using the same logic with tile values?
    • Returning values from functions that aren't used - but as a way to exit the function
    • Duplicated code
    • Violating "tell-don't-ask" with lots of if statements when (see the next point...)
    • Not using a proper data structure to represent the tiles and their behaviour
    • Spelling/Grammar mistakes in the information presented to the user
  • Game Itself

    • Game not acting properly when flagging a single mine (3x3, 1 mine) - finishing automatically
    • Game not acting properly when flagging a single mine (5x5, 1 mine) due to lower-case f

Python Specific

These items are something you should be aware of when writing Python code.
PEP8: PEP8 talks about using snake_case for variable/function naming (whilst class naming is CamelCase) and a few other things. Be aware of the major standard for each language, and follow the style rules in each organisation.

Entry point: As we're writing a script for execution, and not as a library, the entry point if __name__ == "__main__": should be used. For example, if you pushed your script into the repository, and a code documentor such as Sphinx ran over it, it would freeze because it would start playing the game. This is because the code begins running as soon as Python loads it, when the intent of the documentor was just to analyse the code.

F-strings: Python 3.6 and later have this capability; f-strings can make reading print statements much easier. It's recommended to use them when writing any string statement that contains variables.

Imports: Unused imports hint that perhaps you're not fully aware of all the actions of your scripts? If the IDE doesn't highlight these, possibly change your IDE.

Input Validation

Input validation is a very important topic in programming, due to all sorts of bugs and attacks like Cross-Site-Scripting (XSS) and SQL Injection. When needing user input, ensure it's specific, that it's limited, and that you give responses to assist the user to provide the correct input, or allow them to exit the stage where they are.
This will enable avoiding runtime errors which crash the program (such as IndexError list assignment index out of range which I encountered) and avoid having try/except/finally statements due to limiting possible inputs.

Coding Style

.strip(): Normally .strip() is chained at the end of a string where the data can have extraneous spacing, but this one is your own string. A book called "Code Complete" can be useful in learning different patterns of common mistakes made by programmers, I recommend grabbing a copy.

Code Comments: Comments, if used at all, should be a "why you're doing it this way" and not a "how you're doing this". The code already explains the "how". The danger is when the code changes (due to bugs or requirement changes) from what the comment says, another coder who sees the code, and sees the comment, says "The code doesn't do that, I'll be helpful and make it do that" (I've seen this happen).

Classes exposing private attributes as public: Proper OOP ensures that the internals of how classes achieve their magic are hidden. Most other languages enforce this by statements such as private and public before their type and variable name. For classes, be aware of what variables which are internal/private, and place an underscore _ before them. This is important because when you put out code for others to use, if they begin accessing/modifying internal class variables and you release a new version with modified internals, it will break their implementation.

Variable Naming: line 21 states self.cellsToOpen = w * h - k, but the comment says # Create a new board with size w x h, and the caller is MineBoard(w, h, m). So we have w h k x m variables here. Do you see how this might be confusing to someone that is reading your code? Always use words that explain to readers what the code does through proper variable names. Obviously I've read through your code several times and I understand what your code does - but I shouldn't have to read it more than once to fully comprehend the statements. Something like: MineBoard(width, height, num_mines) and self.cellsToOpen = width * height - num_mines is much easier to understand.

Duplicated code: I see multiple calls to self.isValidCell and other functions inside the class. Python famously has a concept of DRY (Don't Repeat Yourself), which means that when you're starting to see multiple calls to a function, or repeating the same lines, that there is an opportunity for refactoring. Refactoring covers not only lines of code into a function, but of data objects into different structures.

Tell - Don't Ask: When your code has a lot of if-this, then-that statements in it, it's clear the logic belongs with the data rather than continually asking the data "are you this?" "oh you're not?" "what about, are you this instead?" "you are? okay, I'll do this action then".

A good example is a set of code checking every minute "is it now 7am?" (probably with a loop that blocks the rest of the code from running). Rather than doing that, the set_alarm(self, hour, minute) function would spawn a thread which waits for hour/minute and then activates a call-back to the activate_alarm(self) function. You tell the function when to do something, not ask it if it's ready to do it/if it has it.

Tiles data structure: Each tile on the board has multiple states (hidden/revealed/flagged) and data (empty/has mine) which is complicated behaviour. Coupled with tell-don't-ask, users perform actions to each tile that can alter the game state and surrounding tile states. It's clear that an enum for state and data is needed per tile, as well as the tile having the capability of call-backs into the board say when a mine was triggered. This point might be a little complicated, but patterns like Observer can simplify this process. Also, mentioning that you know one or two patterns during your interview (you should know them well enough to write them on a whiteboard) can make you stand out from the crowd.

Game Itself

Single mine flagging: In typical minesweeper, even when there is one mine remaining (flagged or unflagged), tiles that are unclicked still require clicking. The game rushes to a finish when flagging the correct tile, it doesn't leave the user in suspense whether they have chosen correctly or not.

Single mine flagging due to lower-case f: I was surprised when I flagged a tile and the game ended with a mine going off. It took me a few seconds to understand that it required an upper-case F to correctly flag a tile. On subsequent games, I failed again because of this input-handling problem. Upper or lower case, it shouldn't matter.

The End

I hope the other answers as well as mine are enough to give you lots to study before your next interview. I wish you the best of luck with the interviewing process and hope you get the job. Cheers!

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for taking your time to write such an detail answer. I learnt tons of things in just one single post. That was amazing !. Just a minor thing, the "strip" function I used is on the input from the user, not the 'instruction' itself. \$\endgroup\$
    – Felicity
    Sep 14 at 2:16
  • 1
    \$\begingroup\$ Yes, you are correct. Looking at the line after having a coffee :) it's a good idea to separate the messge to the user (use print(msg)), and what input you're receiving (input("row?");input("column?")). Makes it easier to obtain the choices. Good effort. \$\endgroup\$
    – C. Harley
    Sep 14 at 7:07

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.