I've been working on the functions that handle when a player's sprite interacts with either a wall (0) or say the crate to push (£) I'm just wondering how this looks and if there alternative methods, as I've been told I can make things more complicated than they really need to be.

def MOVE_UP():
    x = MapGrid.getCharLocation(PlayerMan.getRow() - 1, PlayerMan.getCol())
    y = MapGrid.getCharLocation(Boxes.getRow() - 1, Boxes.getCol())
    if x == '0':
    elif x == '£':
        if y == '0':
            MapGrid.despawn(Boxes.getRow(), Boxes.getCol())
            MapGrid.spawnBoxes(Boxes.getChar(), Boxes.getRow(), Boxes.getCol())

            MapGrid.despawn(PlayerMan.getRow(), PlayerMan.getCol())
            MapGrid.spawnMan(PlayerMan.getChar(), PlayerMan.getRow(), PlayerMan.getCol())

        MapGrid.despawn(PlayerMan.getRow(), PlayerMan.getCol())
        MapGrid.spawnMan(PlayerMan.getChar(), PlayerMan.getRow(), PlayerMan.getCol())

This code, in Python 3.6, is repeated for the other three directions.


closed as off-topic by Billal Begueradj, Daniel, t3chb0t, Dannnno, Raystafarian Jun 11 '18 at 4:15

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Billal Begueradj, Daniel, t3chb0t, Dannnno, Raystafarian
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ what is getCharLocation, PlayerMan, Boxes. Please share more of the code. If you think it is too long to post here, put it on github or so. You seem to be keeping the locations of everything both in the grid and on the individual element, which complicates your code immensely. \$\endgroup\$ – Maarten Fabré Jun 8 '18 at 12:02
  • \$\begingroup\$ also checking out how @property works will make your code cleaner \$\endgroup\$ – Maarten Fabré Jun 8 '18 at 12:10
  • \$\begingroup\$ you should also make clear what should happen when there are more boxes behind each other. As it is now, they would merge \$\endgroup\$ – Maarten Fabré Jun 8 '18 at 12:12


I seems your grid is a nested list of strings. It would be clearer, if you change that to Enums

from enum import Enum

class Tiles(Enum):
    WALL = '0'
    PLAYER = "*"
    BOX = '£'
    EMPTY = '.'

so you can do something like this:

w = Tiles.WALL
e = Tiles.EMPTY
b = Tiles.BOX
p = Tiles.PLAYER
grid = [

getter and setters

Python does not have the Java tradition of setters and getters. If you want to work with a calculated property, of protect a property, you can use @property. This can work like this

class Boxes:
    def __init__(self, row, ...):
        self._row = row

    def row(self):
        return self._row

and so on for char and column

double work

The position of the Boxes and the player is kept both in the MapGrid , and in PlayerMan and Boxes. This is double work, and prone to errors.

Now to move something, you have to adapt it both in the grid (.despawn and .spawnXXX and on the PlayerMan and Boxes, resulting in a lot of unnecessary code.

All you really need to capture the state of the board is this:

class SokobanGrid():
    def __init__(self, grid, position, strength=None):
        grid = list(map(list, grid))  # make a copy of the grid
        assert grid[position[0]][position[1]] == Tiles.PLAYER
        num_players = sum(
            sum(tile == Tiles.PLAYER for tile in row) 
            for row in grid
        assert num_players == 1

        self._grid = grid

        self._dimensions = len(grid), len(grid[0])
        self.position = position
        self.strength = strength

    def copy(self):
        return SokobanGrid(self._grid, position=self.position)


instead of making a method that returns a string representation of the board, you can define __str__ and/or __repr__

    def __str__(self):
        rows = (
            f'x{idx}: ' + ''.join(tile.value for tile in row)
            for idx, row in enumerate(self._grid)
        header = ' ' * 4 + ''.join(map(str, range(self._dimensions[1]))) + '\n'
        return header + '\n'.join(rows)

you can omit the position parameter, and go look for it yourself.

g = SokobanGrid(grid=grid, position=(3, 3))
x0: 00000
x1: 0...0
x2: 0..£0
x3: 0££*0
x4: 0..£0
x5: 0..£0
x6: 0...0
x7: 00000

The interactions with the tile then go like this:

    def tile(self, x, y):
        return self._grid[x][y]

    def iswall(self, x, y):
        return self.tile(x, y) == Tiles.WALL

    def isbox(self, x, y):
        return self.tile(x, y) == Tiles.BOX

    def isempty(self, x, y):
        return self.tile(x, y) == Tiles.EMPTY

    def set_tile(self, x, y, tile):
        self._grid[x][y] = tile

    def swap_tiles(self, old, new):
        old_tile = self.tile(*old)
        new_tile = self.tile(*new)
        self.set_tile(*old, new_tile)
        self.set_tile(*new, old_tile)


Instead of a move method for each direction, you can make a generic one, that accepts a direction. The directions can be an Enum, with a value the change in coordinates it entails (UP is (-1, 0), because row 0 is the top row)

class Direction(Enum):
    UP = (-1, 0)
    DOWN = (1, 0)
    LEFT = (0, -1)
    RIGHT = (0, 1)

The first helper method yields all the moves in a certain direction until it hits a wall (not included) or an empty spot (included)

    def _moves(self, Direction):
        x, y, = self.position
        x_max, y_max = self._dimensions
        dx, dy = Direction.value
        while 0 <= x < x_max and 0 <= y < y_max:
            if self.iswall(x, y):
            yield x, y
            if self.isempty(x, y):
            x, y = x + dx, y + dy

the second helper method checks whether the player can move in a certain direction:

    def can_move(self, direction):
        for boxes, position in enumerate(self._moves(direction)):
            if self.isempty(*position):
                return True
            if self.iswall(*position):
                return False
            if self.strength and boxes > self.strength:
                return False
        return False

This raises a class IllegalMove(Exception): pass to signal to the user interface that a move is not allowed. You might use a ValueError instead

This can be tested with

d = Direction.DOWN
u = Direction.UP
l = Direction.LEFT
r = Direction.RIGHT

assert g.can_move(u)
assert g.can_move(d)
assert not g.can_move(l)
assert not g.can_move(r)

The meaning of the strength parameter, is the maximum amount of boxes the player can push. If we set this to 1, it should not be able to push down anymore:

g_1 = g.copy()
g_1.strength = 1
assert g_1.can_move(u)
assert not g_1.can_move(d)
assert not g_1.can_move(l)
assert not g_1.can_move(r)

Now to move, we must start from the first empty tile to the player, and swap the tiles piece by piece

    def move(self, direction):
        if not self.can_move(direction):
            raise IllegalMove
        moves = self._moves(direction)
        moves = reversed(list(moves))
        for old, new in pairwise(moves):
            self.swap_tiles(old, new)

pairwise is from the itertools recipes

from itertools import tee, takewhile
def pairwise(iterable):
    "s -> (s0,s1), (s1,s2), (s2, s3), ..."
    a, b = tee(iterable)
    next(b, None)
    return zip(a, b)


g_move = g.copy()
x0: 00000
x1: 0...0
x2: 0..£0
x3: 0££.0
x4: 0..*0
x5: 0..£0
x6: 0..£0
x7: 00000

Not the answer you're looking for? Browse other questions tagged or ask your own question.