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.
Code: Select all
<?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;
}
}
?>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
Code: Select all
<?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;
}
}
?>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:
Code: Select all
<?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;
}
}
?>And the last file is mapFromBase.php:
Code: Select all
<?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=="));
?>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!