Movement function for a Sokoban game using Python [closed]

Question

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': pass elif x == '£': if y == '0': pass else: MapGrid.despawn(Boxes.getRow(), Boxes.getCol()) Boxes.pushUp() MapGrid.spawnBoxes(Boxes.getChar(), Boxes.getRow(), Boxes.getCol()) MapGrid.despawn(PlayerMan.getRow(), PlayerMan.getCol()) PlayerMan.MOVE_UP() MapGrid.spawnMan(PlayerMan.getChar(), PlayerMan.getRow(), PlayerMan.getCol()) print(Boxes.displayOnScreen()) else: MapGrid.despawn(PlayerMan.getRow(), PlayerMan.getCol()) PlayerMan.MOVE_UP() MapGrid.spawnMan(PlayerMan.getChar(), PlayerMan.getRow(), PlayerMan.getCol()) print(MapGrid.displayOnScreen()) print(PlayerMan.displayOnScreen()) 

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

Answer 1

strings

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 = [ [w,w,w,w,w], [w,e,e,e,w], [w,e,e,b,w], [w,b,b,p,w], [w,e,e,b,w], [w,e,e,b,w], [w,e,e,e,w], [w,w,w,w,w], ] 

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 ... @property 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) 

displayOnScreen()

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)) print(g) 

 01234 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) 

moving

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): return yield x, y if self.isempty(x, y): return 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) 

so:

g_move = g.copy() g_move.move(d) print(g_move) 

 01234 x0: 00000 x1: 0...0 x2: 0..£0 x3: 0££.0 x4: 0..*0 x5: 0..£0 x6: 0..£0 x7: 00000