Best way to do this?

[3raser]

I've been itching to learn OOP for PHP, and I've learned quite a bit. But for me the hardest part seems to be actually applying OOP to a real project.

 

I made a simple player creating script just to get the feel of properly designing the structure of a project. Can anyone give me any feedback on the following code?

 

index.php

<?php
include('class_lib.php');

$player = new player(array('name' => 'Justin', 'str' => 100,'def' => 60, 'health' => 70));
echo '<br/><b>'. $player->getStats() .'</b><hr>';
echo '<br/>'.$player->incrHealth(30).'<br/>';
echo $player->incrStr(-40).'<br/>';
echo $player->getStats();
?>

 

class_lib.php [iGNORE THE NAME]

<?php

class player
{
    /*
     * @PARAM   [CONSTANT] required_fields
     * @DESC    the amount of fields required to create a new character
     */
    const required = 4;
    
    //player details
    public  $name;
    private $_str       = 0;
    private $_def       = 0;
    private $_health    = 0;
    
    /*
     * @PARAM   $information
     * @DESC    Array that holds the values of the player being constructed
     * @DESC    values: name,str,def,health
     */
    
    function __construct(array $information)
    {
        $validated = $this->isTrue($information);
        
        if($validated['error'] == 0)
        {
            //SUCCESSFUL CREATION OF PLAYER
            //set the values of our new player
            $this->name     = $information['name'];
            $this->_str     = $information['str'];
            $this->_def     = $information['def'];
            $this->_health  = $information['health'];
            
             echo 'Player '. $information['name'] .' created!';
        }
        else
        {
            //error received
            echo $validated['message'];
        }    
    }
    
    /*
     * @METHOD  isTrue
     * @DESC    Validates the new character information
     */
    
    public function isTrue(array $passed_details)
    {
        $x = 0;
        $i = 0;
        
        foreach($passed_details as $key => $value)
        {
            $x++;
            switch($key)
            {
                case 'name':
                    $i++;
                    break;
                case 'str':
                    $i++;
                    break;
                case 'def':
                    $i++;
                    break;
                case 'health':
                    $i++;
                    break;
            }
        }
        
        if($x > self::required)
        {
            return array('error' => 1, 'message' => 'Too many character values set. Fields set: '. $x .' of '. self::required);
        }
        elseif($i < self::required)
        {
            return array('error' => 1, 'message' => 'Missing a character value. Fields set: '. $x .' of '. self::required);
        }
        else
        {
            return array('error' => 0);
        }
    }
    
    /*
     * @METHOD  getStats
     * @DESC    Returns the statistics/stats of the player
     */
    
    public function getStats()
    {
        return 'Showing statistics of '. $this->name .': Str = '. $this->_str .', Def = '. $this->_def .', Health = '. $this->_health;
    }
    
    /*
     * @PARAM   $modified_value
     * @DESC    the amount to add to the strength of the player
     */
    
    public function incrStr($modified_value)
    {
        $this->_str += $modified_value;
        return 'Strength successfully increased by '. $modified_value;
    }
    
    public function incrDef($modified_value)
    {
        $this->_def += $modified_value;
        return 'Defence successfully increased by '. $modified_value;
    }
    
    public function incrHealth($modified_value)
    {
        $this->_health += $modified_value;
        return 'Health successfully increased by '. $modified_value;
    }
}

?>

[Adam]

Your incrStr(), incrDef() and incrHealth() methods shouldn't return a string dictating what happened. There's actually nothing happening in the methods that could go wrong, so you know at the point of invoking the method that it will be successful, and of course you have the value passed in available. It would be better to return $this so that you can chain the calls:

 

$player->incrHealth(30)->incStr(-40);

 

Similarly, the getStats() method shouldn't return the data concatenated into a string and the constructor shouldn't be echoing anything. The problem with returning strings that you output, is that you're mixing your application logic, or more specifically your model, with your view logic. This can introduce a whole range of problems when you later make changes. The object representing a player shouldn't determine what output is shown to the user -- just have it return the data, so the view can decide. You may think that would make it less reusable, but you can always reuse the view.

 

The MVC pattern is a really good example of how to split such concerns, and is what the majority of decent PHP frameworks out there have adopted.