Movement function for a Sokoban game using Python [closed]

The name of the pictureThe name of the pictureThe name of the pictureClash Royale CLAN TAG#URR8PPP





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;







up vote
4
down vote

favorite












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.







share|improve this question













closed as off-topic by Billal BEGUERADJ, Daniel, t3chb0t, Dannnno, Raystafarian Jun 11 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




    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.
    – Maarten Fabré
    Jun 8 at 12:02










  • also checking out how @property works will make your code cleaner
    – Maarten Fabré
    Jun 8 at 12:10










  • you should also make clear what should happen when there are more boxes behind each other. As it is now, they would merge
    – Maarten Fabré
    Jun 8 at 12:12
















up vote
4
down vote

favorite












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.







share|improve this question













closed as off-topic by Billal BEGUERADJ, Daniel, t3chb0t, Dannnno, Raystafarian Jun 11 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




    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.
    – Maarten Fabré
    Jun 8 at 12:02










  • also checking out how @property works will make your code cleaner
    – Maarten Fabré
    Jun 8 at 12:10










  • you should also make clear what should happen when there are more boxes behind each other. As it is now, they would merge
    – Maarten Fabré
    Jun 8 at 12:12












up vote
4
down vote

favorite









up vote
4
down vote

favorite











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.







share|improve this 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.









share|improve this question












share|improve this question




share|improve this question








edited Jun 7 at 21:32









Phrancis

14.6k644137




14.6k644137









asked Jun 7 at 20:52









James D

211




211




closed as off-topic by Billal BEGUERADJ, Daniel, t3chb0t, Dannnno, Raystafarian Jun 11 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.




closed as off-topic by Billal BEGUERADJ, Daniel, t3chb0t, Dannnno, Raystafarian Jun 11 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




    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.
    – Maarten Fabré
    Jun 8 at 12:02










  • also checking out how @property works will make your code cleaner
    – Maarten Fabré
    Jun 8 at 12:10










  • you should also make clear what should happen when there are more boxes behind each other. As it is now, they would merge
    – Maarten Fabré
    Jun 8 at 12:12












  • 1




    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.
    – Maarten Fabré
    Jun 8 at 12:02










  • also checking out how @property works will make your code cleaner
    – Maarten Fabré
    Jun 8 at 12:10










  • you should also make clear what should happen when there are more boxes behind each other. As it is now, they would merge
    – Maarten Fabré
    Jun 8 at 12:12







1




1




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.
– Maarten Fabré
Jun 8 at 12:02




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.
– Maarten Fabré
Jun 8 at 12:02












also checking out how @property works will make your code cleaner
– Maarten Fabré
Jun 8 at 12:10




also checking out how @property works will make your code cleaner
– Maarten Fabré
Jun 8 at 12:10












you should also make clear what should happen when there are more boxes behind each other. As it is now, they would merge
– Maarten Fabré
Jun 8 at 12:12




you should also make clear what should happen when there are more boxes behind each other. As it is now, they would merge
– Maarten Fabré
Jun 8 at 12:12










1 Answer
1






active

oldest

votes

















up vote
2
down vote













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'xidx: ' + ''.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






share|improve this answer






























    1 Answer
    1






    active

    oldest

    votes








    1 Answer
    1






    active

    oldest

    votes









    active

    oldest

    votes






    active

    oldest

    votes








    up vote
    2
    down vote













    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'xidx: ' + ''.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






    share|improve this answer



























      up vote
      2
      down vote













      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'xidx: ' + ''.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






      share|improve this answer

























        up vote
        2
        down vote










        up vote
        2
        down vote









        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'xidx: ' + ''.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






        share|improve this answer















        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'xidx: ' + ''.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







        share|improve this answer















        share|improve this answer



        share|improve this answer








        edited Jun 8 at 14:20


























        answered Jun 8 at 14:11









        Maarten Fabré

        3,204214




        3,204214












            Popular posts from this blog

            Chat program with C++ and SFML

            Function to Return a JSON Like Objects Using VBA Collections and Arrays

            Will my employers contract hold up in court?