5
\$\begingroup\$

I'm planning to transition from years of forced procedural programming to OOP. I decided I'd start off small with a little gumball machine object to get my bearings. Everything seems to run ok, but I feel strange having all these globals in my class.

Code in action here.

Two questions:

  1. Is there a way to avoid globals in my code?

  2. Are there any glaring issues with the way I've used OOP to put this gumball machine together?

<?php
/* Gumball Machine */
class gumballMachine {
    /* Class Variables */
    public $pricePerTurn = .25;
    private $maxGumballs = 200;
    private $numColors = 5;
    private $estimateError = .25;



    public function __construct() {
        global $redGumballs;
        global $blueGumballs;
        global $whiteGumballs;
        global $greenGumballs;
        global $yellowGumballs;
        global $totalGumballs;
        global $iTotalGumballs;
        $redGumballs = rand(1,($this->maxGumballs / $this->numColors));
        $blueGumballs = rand(1,$this->maxGumballs/$this->numColors);
        $whiteGumballs = rand(1,$this->maxGumballs/$this->numColors);
        $greenGumballs = rand(1,$this->maxGumballs/$this->numColors);
        $yellowGumballs = rand(1,$this->maxGumballs/$this->numColors);
        $totalGumballs = $redGumballs+$blueGumballs+$whiteGumballs+$greenGumballs+$yellowGumballs;
        $iTotalGumballs = $totalGumballs;
        echo 'You walk up to a gumball machine.<br><br>';
    }

    public function estimateTotalGumballs() {
        global $totalGumballs;
        echo 'You glance at the gumball machine and guess it is about '. round(($totalGumballs/$this->maxGumballs *100+5/2)/5)*5  .'% full. You know the gumball machine can hold about '.$this->maxGumballs.' gumballs total.<br><br>';
    }

    public function estimateTotalColorGumballs($color) {
        $color = strtolower($color);
        switch ($color) {           
            case 'red': 
                global $redGumballs;
                $colorGumballs = $redGumballs;
                break;              
            case 'blue':
                global $blueGumballs;
                $colorGumballs = $blueGumballs;
                break;              
            case 'white':
                global $whiteGumballs;
                $colorGumballs = $whiteGumballs;                
                break;              
            case 'green':
                global $greenGumballs;
                $colorGumballs = $greenGumballs;
                break;              
            case 'yellow':
                global $yellowGumballs;
                $colorGumballs = $yellowGumballs;               
                break;              
        }

        global $totalGumballs;

        if ($colorGumballs == 0) {
            echo "You glance at the gumball machine and don't see any ".$color." gumballs remaining. <br>";
            return 0;
        } elseif ($colorGumballs == 1) {
            echo "You glance at the gumball machine and only see a single ".$color." gumball remaining. <br>";
            return 1;
        } else {            
            $remaining = $colorGumballs + round((rand(-1,1)) * $totalGumballs * (($colorGumballs/$totalGumballs)* $this->estimateError),0);
            echo "You glance at the gumball machine and estimate there are ". $remaining  ." ".$color." gumballs remaining. <br>";          
            return $remaining;
        }
    }

    public function insertQuarter() {
        echo '<br>You insert a quarter into the machine<br>';
    }

    public function twistHandle() {
        echo 'You twist the gumball machine handle and a gumball is chosen.<br>';
    }

    public function openDoor() {
        global $redGumballs;
        global $blueGumballs;
        global $whiteGumballs;
        global $greenGumballs;
        global $yellowGumballs;
        global $totalGumballs;
        $redSelector = $redGumballs;
        $blueSelector = $redSelector+$blueGumballs;
        $whiteSelector = $blueSelector+$whiteGumballs;
        $greenSelector = $whiteSelector+$greenGumballs;
        $yellowSelector = $greenSelector+$yellowGumballs;
        $gumballSelector = rand(1,$totalGumballs);

        if ($totalGumballs > 0) {
            if ($gumballSelector <= $redSelector) {
                echo "You open the gumball machine door and out pops a <font color=red><b>red</font></b> gumball!<br><br>";
                $redGumballs--;
                $totalGumballs--;
                return 'red';
            } elseif ($gumballSelector <= $blueSelector) {
                echo "You open the gumball machine door and out pops a <font color=blue><b>blue</font></b> gumball!<br><br>";
                $blueGumballs--;
                $totalGumballs--;
                return 'blue';
            } elseif ($gumballSelector <= $whiteSelector) {
                echo "You open the gumball machine door and out pops a <font color=black><b>white</font></b> gumball!<br><br>";
                $whiteGumballs--;
                $totalGumballs--;
                return 'white';
            } elseif ($gumballSelector <= $greenSelector) {
                echo "You open the gumball machine door and out pops a <font color=green><b>green</font></b> gumball!<br><br>";
                $greenGumballs--;
                $totalGumballs--;
                return 'green';
            } elseif ($gumballSelector <= $yellowSelector) {
                echo "You open the gumball machine door and out pops a <font color=orange><b>yellow</font></b> gumball!<br><br>";
                $yellowGumballs--;
                $totalGumballs--;
                return 'yellow';
            }           

        } else { 
            echo "You already took the last gumball!<br><br>";
            return 'none';
        }       
    }
}

$gumballMachine = new gumballMachine;


$gumballMachine->estimateTotalGumballs();

$eRed = $gumballMachine->estimateTotalColorGumballs('Red');
$eBlue = $gumballMachine->estimateTotalColorGumballs('Blue');
$eWhite = $gumballMachine->estimateTotalColorGumballs('White');
$eGreen = $gumballMachine->estimateTotalColorGumballs('Green');
$eYellow = $gumballMachine->estimateTotalColorGumballs('Yellow');

$reds = 0;
$blues = 0;
$whites = 0;
$greens = 0;
$yellows = 0;

for ($i=0;$i<$iTotalGumballs; $i++) {
    $gumballMachine->insertQuarter();
    $gumballMachine->twistHandle();
    $returned = $gumballMachine->openDoor();
    switch ($returned) {        
        case 'red':
            $reds++;
            break;
        case 'blue':
            $blues++;
            break;
        case 'white':
            $whites++;
            break;
        case 'green':
            $greens++;
            break;
        case 'yellow':
            $yellows++;
            break;          
    }
}

echo "After $i quarters (\$".number_format($i * .25,2).")...<br>";
echo "You have <font color=red><b>$reds</font></b> red gumballs after estimating there were <font color=red><b>$eRed</font></b> reds.<br>";
echo "You have <font color=blue><b>$blues</font></b> blue gumballs after estimating there were <font color=blue><b>$eBlue</font></b> blues.<br>";
echo "You have <b>$whites</b> white gumballs after estimating there were <b>$eWhite</b> whites.<br>";
echo "You have <font color=green><b>$greens</font></b> green gumballs after estimating there were <font color=green><b>$eGreen</font></b> greens.<br>";
echo "You have <font color=orange><b>$yellows</font></b> yellow gumballs after estimating there were <font color=orange><b>$eYellow</font></b> yellows.<br><br>";
?>
\$\endgroup\$
  • \$\begingroup\$ Yes, I know this is pretty terrible OOP code. I wanted to know just how terrible. Thanks for all the feedback! Especially guy who emailed me about this :) \$\endgroup\$ – DrewP84 Apr 16 '14 at 2:50
4
\$\begingroup\$

Is there a way to avoid globals in my code?

Yes, of course there is! (more on that in my actual answer).

Are there any glaring issues with the way I've used OOP to put this gumball machine together?

Yes. If I'm honest, there are loads of issues.
I don't want to be rude, but it is my opinion that CR has to be blunt to be good. This is something I've explained at length some time ago.

Anyway, onwards:
Avoid the need for globals by using properties. Classes (and thus objects) allow you to couple state to functionality. You need a given value, and your methods will be using that value (altering it, using it for computation) throughout, then that data needs to be tucked away inside that class, where no other code can touch it.

class MyText
{
    public $string = 'my text';//a terrible example
    public function scream()
    {
        return strtoupper($this->string);
    }
}

Ok, but how do we get a value in that class, and use it as a property? Enter the __construct method:

class MyText
{
    public $string = null;//no value (yet)
    public function __construct($value)
    {
        $this->string = $value;
    }
    public function scream()
    {
        return strtoupper($this->string);
    }
}

You then create an instance like so:

$instance = new MyText('This is the value');
echo $instance->scream();//THIS IS THE VALUE
$another = new MyText('And now for something completely different');
echo $another->scream();//AND NOW FOR SOMETHING...

Each instance has its own property, and can do the same thing without this affecting the other instances of the same class. They are all self-contained units.

Now, the property here isn't neatly tucked away, of course, it's public, so other code can simply change the value of the string property:

$instance->string = 'foobar';
echo $instance->scream();//FOOBAR

That's not ideal, especially considering that this could happen:

$instance->string = array('foobar');
//strtoupper on an array shouldn't be allowed

Enter access modifiers. By defining a property as being protected or private you can prevent other (external, as in: not contained within the class) from accessing and altering the properties you need directly:

class MyText
{
    protected $string = null;
    //all other methods are identical
}
$instance = new MyText('I am protected');
echo $instance->scream();
$instance->string = 'not allowed';//error!

Of course, sometimes you do find yourself wanting to change the value of a property of an instance that already exists. Rather than creating a new instance, a simple setter method can be used to do just that:

class MyText
{
    protected $string = null;
    public function __construct($value = null)
    {
        $this->string = $value;
    }
    public function setText($value)
    {
        $this->string = (string) $value;//ensure $value is a string!
        return $this;
    }
}

Now you can do something like this:

$instance = new MyText();//<-- don't set a value here, it's been made optional
echo $instance->setText('Hello')
    ->scream();//because setText returns $this, we can chain method calls

And indeed, this'll echo HELLO. Setters have the added advantage of giving you the chance to validate the data that the user (the one calling the method) is trying to assign to a given property. A method like this:

public function setData(array $data)
{//note the type-hint!
    $this->data = $data;
    return $this;
}

Will result in a fatal error when this method is called with anything else than an array. Your class expects the $data property to be an array, so your method only excepts an array argument. This makes your API a lot less error-prone, reveals bugs in the calling code more quickly and produces something that is beginning to resemble self-documenting code.

That's what you need for you gumball counters, too. Pass all those variables through to your constructor, and assign them to properties. Then you no longer rely on those global variables even existing:

$blueGumballs = 123;
$foo = new Gumball($blueGumballs, 123, 123, 123);//<-- assign all colours
$foo->estimateTotalColorGumballs('blue');
$blueGumballs = 456;//<-- has no effect on the instance!
$foo->estimateTotalColorGumballs('blue');

Ideally, you don't use a separate argument for each color/gumball counter value, but you'd pass them all as an array (which allows for some type-hinting).
As your objects become more complex, you soon find yourself writing classes for data, too. These class names can be used for type-hints, too.

For example, I have a db table full of user data. An SQL table is a rigid data structure, that can, for example contain: a user name, nickname, status, age and some timestamps.
Each of these fields imply a specific data type: strings for the name and nickname, an email is a special string, that should be validated using filter_var($email, FILTER_VALIDATE_EMAIL) and thus only accepted if it's a valid email address.
Age is an integer, the timestamps could be instances of the DateTime object and the status (active or not) could be a bool (true/false).

That class, representing a user could be filled by a query result or a form submission (a new user registered). Once the instance is created and the data set, you pass that instance around, to ensure the data stays intact (setters that validate data), and nothing is lost along the way (an object is a single unit)...

Perhaps think about going down that route for your gumballs: they all have a count (number of gumballs left), and a distinct colour. Furthermore, type-hinting makes life just so much easier when debugging, or maintaining code someone else wrote.

Basically, an object should be self-contained What happens inside the object, only affects the object. What happens outside of the object doesn't concern the object. A global variable is an object's worst enemy.
If you want to write OO code, pour your data into objects, and think about your data as being instances, instead of variables.


I'll leave you with this sneak-preview of rants to follow: SOLID is easy!
Perhaps you can check out a couple of my other answers on this site where I explain, at length, why a method should never ever echo, for example this answer of mine. I know, I tend to write long answers, but at least then I'm sure people are likely to understand why I say way I say, and where my critiques stem from.

To be continued

\$\endgroup\$
  • \$\begingroup\$ Please be as blunt as necessary. I threw this together in an hour, and it is my first OOP project. I didn't expect it to be a work of art. I mainly wanted to see everything I need to fix from a pro in a basic example before I move, so any and all feedback is extremely helpful! \$\endgroup\$ – DrewP84 Apr 15 '14 at 14:31
5
\$\begingroup\$

First of all, all your gumballs counters have no good reason to be global. As they are part of the state of the gumball machine, why don't you make the counters instance variables?

Also, there might be a much more concise way to handle this counters : instead of having multiple variables, you could use a associate array to map colors to the corresponding number of gumball. As a cool side-effect, you don't need to maintain a variable containing the total number of gumballs : just use sum and you'll be allright.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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