Problem
Because I’m rather new to PHP and Object Oriented Programming, I’m often looking for some practice. Thus I decided to focus on making a browser-based Mafia game that should work like a Dutch variant: http://www.bendes.nl/.
Before I go any further, I would like to put a disclaimer right here. I’m new to StackExchange and especially CodeReview. My project is not finished, but I still felt like my question suited to this platform. If it’s not, please feel free to give me the right directions.
I’m not trying to improve in MySQL right now, just the fundamentals of PHP and OOP
The concept is simple: the player needs to commit crimes in order to make money. They can spend their money on food/meds to generate health, weapons to increase probability and so on.
Firstly, I created a class player in player.php that has several methods that influence the player’s stats. For example, raiseXp() raises the experience points of the player, but calls the levelUp() function when it reaches the maximum value of the current rank (exp).
player.php
<?php
class Player {
var $name;
var $exp = 1;
var $xp = 0;
var $max_xp = 100;
var $wealth = 0;
var $health = 100;
function __construct($name) {
$this->name = $name;
}
function initXp() {
$factor = 1.11;
$this->max_xp *= $factor;
}
function raiseXp($amount) {
$this->xp += $amount;
if ($this->xp >= $this->max_xp) {
$this->levelUp($this->xp - $this->max_xp);
}
}
function levelUp($leftover) {
$reward = 5 * $this->rank;
$this->exp++;
$this->xp = $leftover;
$this->initXp();
}
function setHealth($value) {
if ($value > 0 && $value < 101) {
$this->health = $value;
}
}
function increaseHealth($amount) {
if (100 - $this->health >= $amount) {
$this->health += $amount;
} else {
$this->health = 100;
}
}
function decreaseHealth($amount) {
$this->health -= $amount;
if ($this->health <= 0) {
$this->goToHospital();
}
}
function goToHospital() {
$this->setHealth(70);
$this->forceDecreaseWealth(300);
}
function increaseWealth($amount) {
$this->wealth += $amount;
}
function decreaseWealth($amount) {
if ($this->wealth >= $amount) {
$this->wealth -= $amount;
} else {
// Error
}
}
function forceDecreaseWealth($amount) {
if ($this->wealth >= $amount) {
$this->wealth -= $amount;
} else {
$this->wealth -= $amount;
$debt = $this->wealth - $this->wealth * 2;
}
}
}
?>
At the top of my index file, I included the player class, checked for an open session and built a new player object inside a session variable:
index.php
<?php
// Include controllers
include "controller/player.php";
// Start session handler
session_start();
// Check if there's a login session active
if (isset($_SESSION["player"])) {
$player = $_SESSION["player"];
} else {
$_SESSION["player"] = new Player('Testspeler');
$player = $_SESSION["player"];
}
?>
You might have noticed I included something above session_start(). From what I know, you should always put session_start() at the very top of you document. The thing is, $_SESSION["player"] wouldn’t remember if I put it at the top.
Next, I created a Crime class. This is the main blueprint of a crime with a method (commit()).
<?php
class Crime {
var $reward;
var $xp_boost;
var $probability;
function __construct($reward, $xp_boost, $probability) {
$this->reward = $reward;
$this->xp_boost = $xp_boost;
$this->probability = $probability;
}
function commit() {
$result = mt_rand(0, 101);
if ($result <= $this->probability) {
return true;
} else {
return false;
}
}
}
$crimes = array(
"Winkelwagens stelen" => new Crime(1, $player->rank, 80)
);
?>
I put these in a table using a foreach loop. I can’t seem to understand how a user would select a crime and “launch” the commit function.
<table>
<tr>
<th colspan="100%">Misdaden</th>
</tr>
<tr>
<th>Misdaad</th>
<th>XP</th>
<th>Geld</th>
<th>Waarschijnlijkheid</th>
<th></th>
</tr>
<?php foreach ($crimes as $x => $y) : ?>
<tr>
<td><?php echo $x; ?></td>
<td><?php echo $y->xp_boost; ?></td>
<td><?php echo $y->reward; ?></td>
<td><?php echo $y->probability; ?></td>
<td><!-- Somehow call $y->commit() when the user clicks? --></td>
</tr>
<?php endforeach; ?>
My Question(s)
- Writing style. What would you change about my writing style? Is it neat? Is it inconsistent? Vague?
- Choices and approaches. Do you see any choices I made that you wouldn’t have dared to make at all?
Solution
Be careful when you are setting objects to session in PHP. I would refer you to the PHP Object Serialization Documentation where it is noted that you need to make sure you include your class file definition in ALL pages on your site which use session (we can’t tell if that is the case here or not). I would also always defined __sleep() and __wakeup() magic methods so you can provide clear intent on how to serialize/unserialize the object for session storage. This may not make much of a difference on these simple classes, but when you begin to work with classes that do things like store resource handles, nested objects, etc. this can be a big problem.
You should get in the habit of explicitly setting accessibility information on your class properties and methods. Without these, everything on your class is public. This may be OK for simple applications, but can be really bad in working with more complex applications.
This also helps force you to think about what should code calling the class really be able to change. For example, should code interacting with a Crime object be able to actually change the properties after the class is instantiated? My guess is that this would be undesirable in your application and that the Crime objects should be immutable after instantiation, meaning the properties should be protected/private.
When thinking about designing your classes, you really need to think about your methods as an action that the object can take. For example, a Crime can’t commit itself. You need Player to commit a crime, thus commitCrime should probably rightfully be method on the Player class with a signature like:
public function commitCrime(Crime $crime) {}
Does it seems silly to seemingly add complexity to the Player class by having to pass a Crime to it, when you could just do it the way you have? It might on the surface, but, what if you decide in the future that you want a higher level player or a player with certain characteristics to have a better chance of committing a given crime? What then?
You can’t define all that logic on the Crime::commit() method because Crime doesn’t have all the information it needs to determine the outcome in such a case. That is because the individual Player actually “commits” the Crime. Your code should be modeled this way.
Your code is very fragile in that you are doing no validation of inputs passed to your public methods. You should validate that you are getting the proper objects, strings, floats, integers, etc. passed to your parameters (both expected type and expected range of value) and throw exceptions when these conditions aren’t met.
Your mantra should be “Fail early. Fail loudly.” This meaning that you want to interrupt code operation as soon as you are getting unexpected input such that you are not later operating against objects that are not set up in a proper state. Not doing this will make your code much harder to maintain and debug.
Some of your method names could perhaps be changed to something more meaningful. For example, initXp() might better be named something like raiseMaxXp(), as you are not “iniatilizing” anything. The more you can get yourself in the habit of naming your variables and methods in a way that is meaningful, the more easy-to-read your code will become.
// in Player
var $name;
var $exp = 1;
var $xp = 0;
var $max_xp = 100;
var $wealth = 0;
var $health = 100;
// in Crime
var $reward;
var $xp_boost;
var $probability;
I am guessing all of these properties should be protected/private as they should only be modified using the methods exposed by the class. This means you might need to specify getters for these.
$factor = 1.11;
$factor here is, in essence, a class constant. Don’t hide this value declaration away in a method, declare as a constant for the class.
if (isset($_SESSION["player"])) {
Is this meaningful validation? You could get your application in a bad state here. Perhaps flip your conditional around and check for valid instance of Player.
if (empty($_SESSION['player']) || $_SESSION['player'] instanceof Player === false) {
$_SESSION["player"] = new Player('Testspeler');
}
$player = $_SESSION["player"];
Do you need a Hospital class? A Player can’t heal himself can he?
Alternatively, you could consider a receiveHealing() method that could take a Hospital or Medicine or any other item that can performing according to some Healing interface)
public function receiveHealing(Healing $healItem) {}
Similar for decreaseHealth and possibly also for wealth-related behaviors.
In your php code, the properties are defined as in JavaScript.
The properties must be mentioned with their accessibility. (public, protected or private)
And same in your method definition.