Basic "bee game" labeled as messy

Question

Could you tell me how to make it quality code?

<?php
class Game {
    public function __construct() {
        session_start();
    }
    public function play(){
        return '<input type="button" value="Play" onclick="window.location = \'?play=1\'">';
    }
    public function start($msg=null) {
        unset($_SESSION['bees']);
        $bee = new Bee();
        $bee->setType('queen');
        $this->addBee($bee,1);
        $bee->setType('worker');
        $this->addBee($bee,5);
        $bee->setType('drone');
        $this->addBee($bee,8);
        if($msg)
            return $msg;
        else {
            var_dump($_SESSION['bees']); // debug
            return 'Game started <input type="button" value="Hit" onclick="window.location = \'?hit=1\'">';
        }
    }
    public function hit() {
        $msg = '';
        if($_SESSION['bees'])
            $rd = array_rand($_SESSION['bees'],1);
        else
            $this->start('Game will start again');
        $_SESSION['bees'][$rd]['lifespan'] = $_SESSION['bees'][$rd]['lifespan'] - $_SESSION['bees'][$rd]['hit'];
        if($_SESSION['bees'][$rd]['lifespan']<1) {
            $_SESSION['bees'][$rd]['bees'] -=1;
            $_SESSION['bees'][$rd]['lifespan'] = $_SESSION['bees'][$rd]['life'];
            switch($rd)
            {
                case 'queen': $msg .= $this->start('Game start again, the Queen is dead.'); break;
                default: $msg .= 'One \'' . $rd . '\' is gone';
            }
        }
        if($_SESSION['bees'][$rd]['bees']<1){
            unset($_SESSION['bees'][$rd]);
            $msg = 'The \'' . $rd . '\' team is gone';
        }
        var_dump($_SESSION['bees']); // debug
        return '<input type="button" value="Hit" onclick="window.location = \'?hit=1\'"> ' . $msg;
    }
    public function addBee(Bee $newbee, $number = 1) {
        $tipo = $newbee->getType();
        $bee = $newbee->get();
        $_SESSION['bees'][$tipo] = $bee;
        $_SESSION['bees'][$tipo]['bees'] = $number;
        $_SESSION['bees'][$tipo]['life'] = $newbee->getLifespan($tipo);
    }
}

class Bee
{
protected $_type = null;
protected $_types = array(
    'queen' => array(
    'hit' => 8,
    'lifespan' => 100
    ),
    'worker' => array(
    'hit'   =>  10,
    'lifespan' => 75
    ),
    'drone' => array(
    hit'    =>  12,
    'lifespan' => 50
    )
    );
    public function setHit(){}
    public function setLifespan(){}
    public function addNewType(){}
    protected function _set_hit(){}
    protected function _set_lifespan(){}
    protected function _add_new_type(){}
    public function getLifespan($type){
        if(array_key_exists($type,$this->_types))
            return $this->_types[$this->_type]['lifespan'];
        else
            throw new Exception('The Bee need to be a Queen, Worker or Drone');
    }
    public function getTypes() {
        return $this->_types;
    }
    public function getType() {
        return $this->_type;
    }
    public function setType($type) {
        if(array_key_exists($type,$this->_types))
            $this->_type = $type;
        else
            throw new Exception('The Bee need to be a Queen, Worker or Drone');
    }
    public function get() {
        return $this->_types[$this->_type];
    }
}
?>
<html>
<head>
<title>play Bee</title>
</head>
<body>
    <?php                                               
        $game = new Game();
        if(isset($_GET['play']) && $_GET['play']) {
            echo $game->start();
        } else if(isset($_GET['hit']) && $_GET['hit']) {
            echo $game->hit();
        } else {
            echo $game->play();
        }
    ?>
</body>
</html>

One point I know is about the non-existence of documentation (comments) on it, but was a test for 1 hour which I didn't give the concern for it. — John Smith Optional, Jul 30, 2013 at 17:41
MVC, that's all you need to really know, but I need 15 chars in a comment. Also Single Responsabilitiy (which comes naturally when using the MVC pattern), and declare classes in separate files... — Elias Van Ootegem, Aug 1, 2013 at 10:48

Malachi · Accepted Answer · 2014-04-04 17:32:45Z

One big issue with this code is that you've got a tight coupling between the business logic and the presentation. For example, the method play() really jumps out:

public function play(){

    return '<input type="button" value="Play" onclick="window.location = \'?play=1\'">';

}

From its name I'd think that this method plays something (a game? A piece of music? A role in a play? Another issue is clear, unambiguous naming of classes, methods and variables). I wouldn't expect it to output the HTML markup for a button with a JavaScript event attached to it.

This might not seem like a big deal, and in this particular case it probably isn't. But ask yourself this, how much work would you need to do if whoever set you this project decided half way though that it should run through the command line instead of a web browser?

Ideally, your PHP classes should contain no presentation logic at all. They should return data structures (arrays, scalar types, simple data transfer objects, etc) to the outside world, and then a separate process should take that information and render it to the user in a suitable presentation. The classes that do the actual work (the business objects) should implement all the logic and rules needed to accomplish a task without any concern as to how the results of that task are presented.

This concept is known as separation of concerns. A concern is a particular part of the software package you're developing which shouldn't be tightly bound (coupled in the lingo) to other parts of your package (sorry I can't really word it better than that). Implementing the rules and maintaining the state of the game is one concern, getting user input and displaying output to the user is a different concern. While there obviously has to be some coupling you should try to minimize the degree that one is bound to the other. Additionally, it's generally considered preferable for the part of the program responsible for presentation (which will typically be unique to that program) to know something about the business objects than it is for the business objects (which may get used across a wide range of applications) to know something about the presentation.

A related issue is reliance on $_SESSION inside your class. Again this has to do with separation of concerns. The $_SESSION array and its associated methods (session_start (), etc) are a data storage mechanism. Data storage is a separate concern from the business logic, so again you should try to separate the code that handles storing the game state out from the code that implements the business logic. In the case of $_SESSION there's the additional problems of it being tightly coupled to HTTP and that the session_start () function can't be called once headers have been sent to the client. Again, ask yourself how hard would it be to adapt your code to work in a command line or to use a database instead of the PHP session to store state?

Quonux · Accepted Answer · 2013-07-30 23:50:28Z

Indentation is important
dont use the same bee object again and again
embrace braces (after every if, else, and so on keyword comes a brace pair)
after if or switch should be a space or/and there should bespaces inside e.g.
if( X ) { or if ( X ) {
controversial point, don't use nondescriptive names, use names with meaning, e.g. don't use names like $msg, $rd

you can cache the value of $_SESSION['bees'][$rd] into a variable and give it a good name, this healps a lot

class Game {
    public function __construct() {
        session_start();
    }

    public function play(){
        return '<input type="button" value="Play" onclick="window.location = \'?play=1\'">';
    }

    public function start($msg=null) {
        unset($_SESSION['bees']);

        $bee = new Bee();
        $bee->setType('queen');
        $this->addBee($bee,1);

        $bee = new Bee();
        $bee->setType('worker');
        $this->addBee($bee,5);

        $bee = new Bee();
        $bee->setType('drone');
        $this->addBee($bee,8);

        if($msg) {
            return $msg;
        }
        else {
            var_dump($_SESSION['bees']); // debug
            return 'Game started <input type="button" value="Hit" onclick="window.location = \'?hit=1\'">';
        }
    }

    public function hit() {
        $msg = '';

        if ($_SESSION['bees']) {
            $rd = array_rand($_SESSION['bees'],1);  
        } 
        else {
            $this->start('Game will start again');
        }

        $_SESSION['bees'][$rd]['lifespan'] = $_SESSION['bees'][$rd]['lifespan'] - $_SESSION['bees'][$rd]['hit'];

        if ($_SESSION['bees'][$rd]['lifespan']<1) {
            $_SESSION['bees'][$rd]['bees'] -=1;
            $_SESSION['bees'][$rd]['lifespan'] = $_SESSION['bees'][$rd]['life'];

            switch($rd)
            {
                case 'queen':
                $msg .= $this->start('Game start again, the Queen is dead.');
                break;

                default:
                $msg .= 'One \'' . $rd . '\' is gone';
            }
        }

        if ($_SESSION['bees'][$rd]['bees']<1) {
            unset($_SESSION['bees'][$rd]);
            $msg = 'The \'' . $rd . '\' team is gone';
        }

        var_dump($_SESSION['bees']); // debug

        return '<input type="button" value="Hit" onclick="window.location = \'?hit=1\'"> ' . $msg;
    }

    public function addBee(Bee $newbee, $number = 1) {

        $tipo = $newbee->getType();
        $bee = $newbee->get();

        $_SESSION['bees'][$tipo] = $bee;
        $_SESSION['bees'][$tipo]['bees'] = $number;
        $_SESSION['bees'][$tipo]['life'] = $newbee->getLifespan($tipo);
    }
}


class Bee {
    protected $_type = null;

    protected $_types = array(  'queen' => array(
                                                'hit'   =>  8,
                                                'lifespan' => 100
                                                ),
                                'worker' => array(
                                                'hit'   =>  10,
                                                'lifespan' => 75
                                                ),
                                'drone' => array(
                                                'hit'   =>  12,
                                                'lifespan' => 50
                                                ));

    public function setHit(){}
    public function setLifespan(){}
    public function addNewType(){}

    protected function _set_hit(){}
    protected function _set_lifespan(){}
    protected function _add_new_type(){}

    public function getLifespan($type) {
        if (array_key_exists($type,$this->_types)) {
            return $this->_types[$this->_type]['lifespan'];
        }
        else {
            throw new Exception('The Bee need to be a Queen, Worker or Drone');
        }
    }

    public function getTypes() {
        return $this->_types;
    }

    public function getType() {
        return $this->_type;
    }

    public function setType($type) {
        if(array_key_exists($type,$this->_types)) {
            $this->_type = $type;
        }
        else {
            throw new Exception('The Bee need to be a Queen, Worker or Drone');
        }
    }

    public function get() {
        return $this->_types[$this->_type];
    }
}

Malachi · Accepted Answer · 2014-04-04 17:35:36Z

Inheritance, ever heard of it?

A good rule I tend to use a lot is the following: Is there an if in my class function? Does the outcome of the function depend on the if? Then it is often a good idea to create a Sub class.

Your Bee classes look really messy without even looking at the code. For every Bee you set a 'type'. So what if I forget to set a type? Errors? is there a default? No way to know unless you dive into the code = Bad.

So better would be:

new Bee('worker');

By passing in the type in the construct we are now sure that the Bee is of a certain type, it also makes it easier to read.

But then remains the question. Does the type of the bee change it behaviour? I guess it does. A queen can lay eggs but the other beers can't. This would give you an 'abstract class Bee' with all default functions. Then you would create 3 different subclasses: WorkerBee, QueenBee and DroneBee

Then again, to much tight coupling. Say I want to test the 'Game' class, but no I have no idea what it needs. It needs session, it needs 'Bee',...

A really good place to start is SOLID

Apply that to your code and it will not be as messy anymore

Shazman · Accepted Answer · 2014-04-04 17:18:51Z

I had to take this test a couple of days ago during an interview process. I wrote mine as procedural and paid the price (didn't get selected for the next stage).

Looking at your effort, I've spent a couple of hours on it and have adapted it. Here is my offering:

class bee
{
    private $type = NULL;
    private $qty = 0;
    private $life = 0;

    public static $available_types = array('queen' => array(
                                                            'hit'   =>  8,
                                                            'lifespan' => 100
                                                            ),
                                            'worker' => array(
                                                            'hit'   =>  10,
                                                            'lifespan' => 75
                                                            ),
                                            'drone' => array(
                                                            'hit'   =>  12,
                                                            'lifespan' => 50
                                                            ));

    public function __construct($type, $qty) 
    {
        //create new bee set collection

        //does this bee type exist?
        if (array_key_exists($type, self::$available_types)) 
        {
            //set default properties
            $this->type = $type;
            $this->qty = $qty;
            $this->life = self::$available_types[$type]['lifespan'];
        }
        else 
        {
            throw new Exception('The Bees need to be a Queen, Worker or a Drone');
        }       

    }

    public function get_type()
    {
        return $this->type;
    }

    public function get_life()
    {
        return $this->life;
    }

    public function get_qty()
    {
        return $this->qty;
    }   

}

class game 
{
    public $message;

    public function __construct() 
    {
        session_start();
    }

    public function reset_game($supress_message = FALSE)
    {
        //reset each type of bees
        $bee = new bee('queen', 1);
        $this->add_bees($bee);

        $bee = new bee('worker', 5);
        $this->add_bees($bee);  

        $bee = new bee('drone', 8);
        $this->add_bees($bee);  

        if (! $supress_message)
        {
            $this->message = 'Game has been reset. All bees have been respawned.';
        };
    }

    public function add_bees(bee $new_bee) 
    {
         $_SESSION['bees'][$new_bee->get_type()] = array('qty' => $new_bee->get_qty(),
                                                        'life' => $new_bee->get_life(),
                                                        'hit_points' => bee::$available_types[$new_bee->get_type()]['hit']
                                                        ); 
    }   

    public function hit()
    {
        $bees = (array) $_SESSION['bees'];

        //is the session object still valid?
        if ($bees) 
        {
            //get random bee from session
            $seed = array_rand($bees, 1); 

            //check if there are any bees of selected type actually left
            if ($bees[$seed]['life'] == 0 && $bees[$seed]['qty'] == 0)
            {
                //apply hit again
                $this->hit();
            };

            //deduct appropriate hit points from selected bee
            $bees[$seed]['life'] -= $bees[$seed]['hit_points'];

            //if this bee has no life left
            if ($bees[$seed]['life'] < 0)
            {
                //then reset its life to designated lifespan
                $bees[$seed]['life'] = bee::$available_types[$seed]['lifespan'];    

                //reduce bee quantity by 1
                $bees[$seed]['qty'] -= 1;                           
            };

            //how many bees are remaining for this type
            if ($bees[$seed]['qty'] >0 )
            {   
                //there is at least one bee still left
                $this->message =  $seed.' bee has been hit. There are '.$bees[$seed]['qty'].' bees left.';  
            }
            else //no, either the queen is dead, or all workers are dead, or all drones are dead
            {
                    switch ($seed)
                    {
                        case 'queen': //the queen is dead, end game
                            $this->message =  'The queen is dead, therefore all bees are dead. The game has been reset';
                            $this->reset_game(TRUE);
                            return;
                            break;

                        default: 
                            $bees[$seed]['life'] = 0;
                            $this->message =  $seed.' bee has been hit and killed. There are no more '.$seed.' bees left.';
                            break;
                    };
            };

            //update session data with changes
            $_SESSION['bees'][$seed] = $bees[$seed];

        } 
        else //session no longer exists
        {
            $this->reset_game();
        }
    }

    public function show_view()
    {
        //set view data 
        $view = new stdClass();
        $view->message = $this->message;
        $view->queen = $_SESSION['bees']['queen'];
        $view->worker = $_SESSION['bees']['worker'];
        $view->drone = $_SESSION['bees']['drone'];

        ?>
            <!DOCTYPE html>
            <html>
            <body>

            <h1>Bee Test</h1>
                <p><?=$view->message?></p>

                <form name="input" action="beetest_revised.php" method="post">
                    <input type="submit" name ="action" value="hit">
                </form>

                <p>Queen Bees</p>
                <?=var_dump($view->queen)?>

                <p>Worker Bees</p>
                <?=var_dump($view->worker)?>

                <p>Drone Bees</p>
                <?=var_dump($view->drone)?>

            </body>
            </html>
        <?
    }

}

$controller = new game();

if ($_POST)
{
    $controller->hit();
    $controller->show_view();
}
else
{   
    $controller->message = 'Game has been reset.';
    $controller->reset_game();
    $controller->show_view();
}

Could you explain how this can benefit the asker's code? Code-only answers aren't considered reviews and are subject to deletion. — Jamal, Apr 4, 2014 at 17:21

Stack Exchange Network

Basic "bee game" labeled as messy

4 Answers 4

Inheritance, ever heard of it?

Your Answer

Not the answer you're looking for? Browse other questions tagged
php
game
or ask your own question.

Hot Network Questions

Basic "bee game" labeled as messy

4 Answers 4

Inheritance, ever heard of it?

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged phpgame or ask your own question.

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
php
game
or ask your own question.