Simple game code review request

[NeooeN]

Hi!

I have written a simple game of sokoban in php and I would like to ask for a comment on my code. The very first thing to say is that it is a cli script and all starts with main.php.

 

<?php


system("stty -icanon");          //this line will cause php to recive every single character typed into terminal without
                                 //pressing enter, it is not the best way of doing this and limits usage of this program
                                 //to linux but it works


include "sokoban.php";           //this file contains class handling the game logic
include "cliClass.php";          //this file contains class extending sokoban class and adding draw function
include "mapFromBase.php";       //definition of a single function that converts maps from string format to one that
                                 //sokoban class uses


$game = new cliClass();          //declare a new sokoban object (cliClass is child of sokoban)
$game->loadMap(mapFromBase("BggEBgYGCQkGCQkQAAAAAKiqqip4VVUtWFVVJVhVVSVYVVUlWFVVJViVViVYlVYlWFVVJVhVVSVYVVUlWFVVJXhVVS2oqqoqAAAAAA=="));


echo $game->draw();              //lets print the first "frame" of the game


$moves = array (); $moves["w"] = 0; $moves["a"] = 3; $moves["s"] = 1; $moves["d"] = 2;//just an auxiliary array


while ($c = fread(STDIN, 1)) {   //iterate through all keys pressed by the user
    if ("w" == $c || "a" == $c || "s" == $c || "d" == $c) {//filter out anything but WASD
        $game->move($moves[$c]); //invoke move method with proper param
        echo $game->draw();      //and print next frame
        if ($game->check()) break;
    }
}


?>

 

It starts with a game initialization ("new cliClass()") and map loading ("$game->loadMap()"). Then we have a simple loop over upcoming datas, that processes them and passes to game object ("$game->move()"). The whole file is just to make the actual code running.

 

The sokoban.php contains sokoban class that has following public methods:

 

•  

• loadMap - as a parameter it takes an array of complex format

• move - takes one integer of range 0-3 which means movements done by player (up, down, left, right)

• check - checks if all "stones" stands on an "X" field (the goal of the game) and returns bool accordingly

•  

 

 

and is listed below:

<?php


  class sokoban {


    const stop = 0;
    const freeToGo = 1;
    const moveStone = 2;


    public $map = array ();
    //0 - void
    //1 - floor
    //2 - wall
    //3 - X
    public $stones = array ();
    public $player = array ();
    protected $moveDirection;
    //0 - array (-1, 0)
    //1 - array (1, 0)
    //2 - array (0, 1)
    //3 - array (0, -1)
    
    function loadMap ($map = array ()) {
      foreach ($map[2] as $rowId => $row) {  //mapa
        foreach ($row as $fieldId => $field) {
          if ($field < 0 || $field > 3) throw new exception ("Zła wartość w array'u mapy (\$rowId = $rowId ; \$fieldId = $fieldId)!");
          $this->map [$rowId][$fieldId] = $field;
        }
      }
      foreach ($map[1] as $stone) { //kamienie
        $this->stones[] = $stone;
      }
      $this->player = $map[0];
      return $this;
    }
    
    function move ($moveDirection) {
      $this->moveDirection = $moveDirection;
      if ($this->isFree ()) {
        $this->player = $this->translateMoveDirection ();
      }
      foreach ($this->stones as &$stone) {
        if ($stone[0] == $this->player[0] && $stone[1] == $this->player[1]) {
          $stone = $this->translateMoveDirection ();
        }
      }
    }


    function translateMoveDirection ($r = null) {    
      return array ($this->player[0] + ($this->moveDirection == 1 ?
                                                                   (isset ($r) ?
                                                                                2 :
                                                                                1
                                                                   ) :
                                                                   ($this->moveDirection == 0 ?
                                                                                               (isset ($r) ?
                                                                                                            -2 :
                                                                                                            -1
                                                                                               ) :
                                                                                               0
                                                                   )
                                       ),
                    $this->player[1] + ($this->moveDirection == 2 ?
                                                                   (isset ($r) ?
                                                                                2 :
                                                                                1
                                                                   ):
                                                                   ($this->moveDirection == 3 ?
                                                                                               (isset ($r) ?
                                                                                                            -2 :
                                                                                                            -1
                                                                                               ) :
                                                                                               0
                                                                   )
                                       )
                   );
    }


    function isFree ($r = null) {
     $x = $this->translateMoveDirection ($r)[0];
     $y = $this->translateMoveDirection ($r)[1];
      if ($this->map [$x][$y] == 2) {
        return false;
      }
      foreach ($this->stones as $stone) {
        if ($stone[0] == $x && $stone[1] == $y) {
          return (isset($r) ?
                             false :
                             $this->isFree (1)
                 );
        }
      }
      return true;
    }
    
    public function check () {
      foreach ($this->stones as $stone) {
        if (!($this->map[$stone[0]][$stone[1]] == 3)) {
          return false;
        }
      }
      return true;
    }


  }


?>

 

The move method invokes isFree, which checks if the player is trying to stand on a wall (which is forbidden) or if is trying to push two stones at once (forbidden as well). Both move and isFree methods invokes translateMoveDirection which is doing simple calculations to determine position of processed elements.

 

My first concerns concern the algorithm of move method and those invoked by it. The move method saves its parameter in a class field instead of passing it to invoked methods. This is due to the fact that I was trying to save memory (every invoked method uses this parameter so passing it all the time would cause the interpreter to copy the parameter a few times for no reason, in my way it is stored in a field once). Is it worth it or should the invoked methods (isFree, translateMoveDirection) takes the data as a parameter?

 

Another thing is it worth to make translateMoveDirection static, since it would return the value based only on the parameter not on an object internal state? My general question (at least on that file) is: is it possible to make it more object oriented, to simplify the whole? I was wondering if it would be possible to somehow break it to separated classes representing game, map and stones?

 

The next file is cliClass.php:

<?php


  require_once "sokoban.php";


  class cliClass extends sokoban {


    function draw () {
      $obj = array ();
      $drawing = array ();
      foreach ($this->map as $map) {
        $drawing[] = "";
        foreach ($map as $m) {
          switch ($m) {
            case 0: $drawing[count($drawing)-1] .= ".";
              break;
            case 1: $drawing[count($drawing)-1] .= " ";
              break;
            case 2: $drawing[count($drawing)-1] .= "W";
              break;
            case 3: $drawing[count($drawing)-1] .= "X";
              break;
          }
        }
      }
      foreach ($this->stones as $stone) {
        @$drawing[$stone[0]][$stone[1]] = "O";
      }
      $drawing[$this->player[0]][$this->player[1]] = "|";
      $toReturn = "";
      foreach ($drawing as $d) {
        $toReturn .= $d."\n";
      }
      return $toReturn;
    }


  }


?>

 

There is no much to say, it's basically a class extending sokoban class and adding draw method, that is returning a string that printed to a terminal represents a game "frame". It is more for debugging purposes but I would like to ask if this way of adding functionalities to existing classes is a good way to do OOP?

 

And the last file is mapFromBase.php:

<?php


  function mapFromBase ($text) {
    $text = str_split (base64_decode($text));
    foreach ($text as &$char) {
      $char = ord ($char);
    }
    $player = array (array_shift ($text), array_shift ($text));
    $stones = array ();
    for ($i = array_shift ($text); $i != 0; $i--) {
      $stones[] = array (array_shift ($text), array_shift ($text));
    }
    $map = array ();
    $line = array ();
    $wrap = array_shift ($text);
    foreach ($text as $byte) {
      for ($j = 0; $j < 4; $j++) {
        $line[] = ($byte & (3 << $j*2)) >> ($j*2);
        if (count ($line) == $wrap) {
          $map[] = $line;
          $line = array ();
        }
      }
    }
    if (count ($line) != 0) {
      $map[] = $line;
    }
    return array ($player, $stones, $map);
  }
  
  //print_r (mapFromBase ("BggEBgYGCQkGCQkQAAAAAKiqqip4VVUtWFVVJVhVVSVYVVUlWFVVJViVViVYlVYlWFVVJVhVVSVYVVUlWFVVJXhVVS2oqqoqAAAAAA=="));


?>

 

It stores a single function mapFromBase that is used to convert from one (string) format to another (array) datas about map (shape of the walls, stones, X's) and I have no idea how to do it properly. I would like my code to be prepared to create more formats and also keep it independent from the cliClass. I guess I have to use some pattern like factory or builder, but I am really confused by the choice so I ask for any advice on that as well.

 

In general I ask for any advices and comments on what can be done to make my code being better quality. I'll be very grateful for any help!

[ignace]

It's a good start but OOP-wise it's still lacking quite a few things. Your data is not encapsulated (using public instead of private). Your classes have too many responsibilities (loading a map, running the game, etc..). Wrong use of inheritance (ie cliClass extends Sokoban). Some key concepts are defined by array's instead of classes or are simple functions.

 

Instead of creating a function mapFromBase, create a class Map with a static function fromBase64:

 

class Map {
  public function __construct(array $tiles) {
    $this->tiles = $tiles;
  }

  public static function fromString($str, $sep = "\n") {
    $tiles = [[]];
    $row = 0;
    foreach (str_split($str) as $char) {
      if ($sep === $char) {
        $tiles[$row++] = array();
        continue;
      }
      $tiles[$row][] = $char;
    }
    return new static($tiles);
  }
  
  public static function fromBase64($str) {
    return static::fromString( base64_decode($str) );
  }
  
  public static function fromFile(SplFileInfo $file) {
    return static::fromString( file_get_contents($file->getPathName()) );
  } 
}

// Map::fromBase64('Hz8meZ..');
// Map::fromString('O . '); // ...

Now instead of loadMap you can simply do setMap(Map $map) or pass it through the constructor __construct(Map $map).

 

Also define a Player class to hold all player info. For the movement define a Move class and Tile like so:

 

class Move {
  const UP = 0;
  const RIGHT = 1;
  const DOWN = 2;
  const LEFT = 3;
  
  public static function fromString($str) {
    if (false === $pos = array_search(['n', 'e', 's', 'w'], $str)) {
      throw new InvalidMoveException();
    }
    return $pos;
  }
}

class Tile { // different kind of tiles on the map
  const TYPE_STONE = 'O';
  const TYPE_EMPTY = ' ';
  const TYPE_WALL = '|';
  const TYPE_PLAYER = '.';
}

// the actual map (holding all tiles)
class Map .. {
  public function doMove($move) { // $map->doMove(Move::UP);
    // translate move to direction
    // validate (throw exception if invalid move for example Tile::TYPE_WALL or map bounds)
    // execute
    return $this->tiles[$x][$y]; // returns Tile::TYPE_*
  }
}

To encapsulate everything create a Game class that holds the map, the player, the state of the current game, etc..

 

// abstract Game for different implementations
abstract class Sokoban {
  const STATE_NEW = 0;
  const STATE_STARTED = 1;
  const STATE_RUNNING = 3;
  const STATE_ENDED_WON = 2;
  const STATE_ENDED_LOST = -1;
  
  private $player;
  private $map;
  private $state = self::STATE_NEW;
  
  abstract public function execute();
}

// cli implementation of Game
class CliSokoban extends Sokoban {
  public function execute() {
    $input = Move::fromString($this->readInput());
    $tile = $this->map->doMove($input);
    // ..
  }
}

By defining all your concepts it makes it easier to understand your program and easier to maintain. Edited March 1, 2015 by ignace

[NeooeN]

Thanks a lot for your answer. I have a few questions. Could you write an examples of how initialization with your design would look like? I can't imagine how the classes should interact with each other. You have placed the function that decode map from my format but how is the map encoded passed to the it? Is it passed to constructor of the game object which is then passing it to map constructor or should I pass ready map object to constructor of game?

 

Another thing is that I do not understand what is the algorithm of doMove. I can't understand what actually it does and why is it returning the field type (what field? I do not understand what $x and $y are) and what the caller do with it? In general I can not understand what the two methods (execute and doMove) responsibilities are.

[ignace]

It all depends on how you want your game to work of course but the initializing code could be something like:

 

$map = Map::fromBase64('BggEBgYGCQkGCQkQAAAAAKiqqip4VVUtWFVVJVhVVSVYVVUlWFVVJViVViVYlVYlWFVVJVhVVSVYVVUlWFVVJXhVVS2oqqoqAAAAAA==');
// or load it from a file
$map = Map::fromFile('/path/to/some.map');

$player = new Player('Player 1');
$game = new CliGame($player, $map, new CliManager(/*read input, colorize output and whatnot*/));
$game->execute();

execute() simply executes the game for cli it uses the CLI commands, for web it uses HTML and GET/POST.

 

The doMove() translates the move to a Tile.

 

WWWWWWWWW
WXXOWXXXW
WWWXXXXXW
WWWWWWWWW

X (=row) and Y (=column) thus (1,3) would return O (or Tile::PLAYER). Since Map holds all the Tile's it's the one that translates the Move to a Tile so the Game can make decisions based on the player action.

[NeooeN]

That explains a lot, thank you very much for your effort! I am going back to work.