Deep Simplicity Part 8

Okay, back to the rules! Rule #8 is: Use First-Class Collections. Unlike the previous rule, I really like this one. Basically the rule states that any class that contains a collection should have no other instance variables. This is similar to other Deep Simplicity rules in that it makes it clear what the responsibility of the class is.

On top of this, a simple array has very little semantic intent other than the name that a developer gives the variable that contains it. As we have discussed before lazy programmers can take your well-meaning code and pervert it into meaningless spaghetti by simply chopping a few characters off of a variable name. By making the collection a full-blown object in it's own right it also becomes clear where you should put helper code like filtering or applying a function to all members of the collection. Let's take a look at some code:
class Card { protected $value; protected $suit; } $all = array(); $all[] = new Card('Ace','Spades'); $all[] = new Card('Ace','Diamonds'); // continue for all cards in a standard deck $deck = $all; $p1 = array(); for ($i=0; $i<8; $i++) { $random = rand(0, count($deck) - 1); $p1[] = $deck[$random]; unset($deck[$random]) } $p2 = array(); for ($i=0; $i<8; $i++) { $random = rand(0, count($deck) - 1); $p2[] = $deck[$random]; unset($deck[$random]) } // Do one exchange $ask = random(0, count($p1)-1); $has = false; foreach ($p2 as $i => $c) { if ($p1[$ask]->value == $c->value) { $p1[] = $c; unset($p2[$i]); $has = true; } } if (! $has) { echo 'Go fish!'; }
So, this is some seriously lazy programming. Although there are a few comments, they are very sparing. The short variable names give you no real clue about what the four arrays represent - at least not without having to expend some brain power figuring out what all of this means. I know you may be saying to yourself that this is a poor example, but I assure you, I have seen code like this in production all the time. I think it's a legacy of the PHP developer lifecycle, starting with one-off single page scripts and eventually evolving into more. Let's see the code again with some more comments and better variable naming:
class Card { protected $value; protected $suit; } $allCards = array(); $allCards[] = new Card('Ace','Spades'); $allCards[] = new Card('Ace','Diamonds'); // continue for all cards in a standard deck // Copy all cards because we want to deal some out $gameDeck = $allCards; $playerOneHand = array(); for ($i=0; $i<8; $i++) { // find a random card from the game deck $randomCard = rand(0, count($gameDeck) - 1); // Add card to player two $playerOneHand[] = $gameDeck[$randomCard]; // Remove card from game deck unset($gameDeck[$randomCard]) } $playerTwoHand = array(); for ($i=0; $i<8; $i++) { // find a random card from the game deck $randomCard = rand(0, count($gameDeck) - 1); // Add card to player two $playerTwoHand[] = $gameDeck[$randomCard]; // Remove card from game deck unset($gameDeck[$randomCard]) } // Do one exchange $askForCard = random(0, count($playerOneHand) - 1); $hasCard = false; foreach ($playerTwoHand as $cardIndex => $card) { if ($playerOneHand[$askForCard]->value == $card->value) { // Move the card to the asking player's hand $playerOneHand[] = $card; unset($playerTwoHand[$cardIndex]); $hasCard = true; } } if (! $hasCard) { echo 'Go fish!'; }
So this example is much more clear in terms of intent. Although you can now determine what each of the arrays is representing, you have to do a lot of parsing in order to determine what is going on. On top of the difficulty in understanding what is going on, there are lots of bug-prone areas where you deal with the fact that arrays are 0 indexed. such as this line:
$askForCard = random(0, count($playerOneHand) - 1);
Now, some of you are saying that getting a random card from an array of cards should be extracted into a method. I agree with you completely! The questions is, where? Should we put all of this in a Game class which has a Game::getRandomCard(array $cards) method? When you are playing "Go Fish" is there some game device that a player passes their hand into to get a random card? I don't think so. If however, you take the advice of this rule and make all of these arrays into first class objects, it becomes quite clear where that method belongs:
class Card { protected $value; protected $suit; } class Game { public function getCardDeck() { $deck = new Deck(); $deck->append(new Card('Ace', 'Spades')); $deck->append(new Card('Ace', 'Diamonds')); // continue for all cards in a standard deck return shuffle($deck); } } class Deck extends ArrayObject { protected function deal($count) {} } class PlayerHand extends ArrayObject { protected function getCardToAskFor() {} protected function has(Card $card) {} protected function give($hand, $value) {} } $game = new Game(); $gameDeck = $game->getDeck(); $playerOneHand = new PlayerHand($gameDeck->deal(8)); $playerTwoHand = new PlayerHand($gameDeck->deal(8)); // Do one exchange $askFor = $playerOneHand->getCardToAskFor(); if ($playerTwoHand->has($askFor)) { $playerTwoHand->give($playerOneHand, $askFor); } else { echo 'Go fish!'; }
I don't know about you, but this last example seems very clear in terms of what's going on. Even if a lazy developer where to come along and started changing the variable names to three or less characters in length, things still make sense with minimal effort because the logic is hidden away in the Collection itself rather than getting in the way of getting work done.

One more rule to go: #9 Don't user any Getters/Setters/Properties