Reduce Complexity in combine players

src/PlayersCollection.php

@@ -8,7 +8,7 @@
  * Class PlayersCollection
  * @package SecretSanta
  */
-class PlayersCollection implements \Iterator, \Countable
+class PlayersCollection implements \Countable
 {
     /** @var Player[] */
     private $players = [];
@@ -29,10 +29,15 @@ public function addPlayer(Player $player)
     /**
      * @param Player $player
      * @param Player $couple
+     * @throws PlayersCollectionException
      */
     public function addCouple(Player $player, Player $couple)
     {
-        if (!$this->isDuplicatePlayer($player) && !$this->isDuplicatePlayer($couple)) {
+        if (!$this->areDifferentPlayers($player, $couple)) {
+            throw  new PlayersCollectionException('The couple can not be the same player');
+        }
+
+        if (!$this->isDuplicatePlayer($player) && !$this->isDuplicatePlayer($couple) ) {
             $this->players[$player->id()] = $player;
             $this->players[$couple->id()] = $couple;
 
@@ -102,6 +107,14 @@ public function areExclude(Player $player, Player $player2)
         return false;
     }
 
+    /**
+     * @return int
+     */
+    public function count()
+    {
+        return count($this->players);
+    }
+
     /**
      * @return int
      */
@@ -125,6 +138,16 @@ private function isDuplicatePlayer(Player $player)
         return false;
     }
 
+    /**
+     * @param Player $player
+     * @param Player $otherPlayer
+     * @return bool
+     */
+    private function areDifferentPlayers(Player $player, Player $otherPlayer)
+    {
+        return $player->id() != $otherPlayer->id();
+    }
+
     /**
      * @param array $list
      * @return array
@@ -157,52 +180,4 @@ private function forceShuffle($list)
 
         return $shuffleList;
     }
-
-    /**
-     * @return Player
-     */
-    public function current()
-    {
-        return current($this->players);
-    }
-
-    /**
-     * @return void Any returned value is ignored.
-     */
-    public function next()
-    {
-        next($this->players);
-    }
-
-    /**
-     * @return int
-     */
-    public function key()
-    {
-        return key($this->players);
-    }
-
-    /**
-     * @return bool
-     */
-    public function valid()
-    {
-        return ($this->current() !== false);
-    }
-
-    /**
-     * @return void Any returned value is ignored.
-     */
-    public function rewind()
-    {
-        reset($this->players);
-    }
-
-    /**
-     * @return int
-     */
-    public function count()
-    {
-        return count($this->players);
-    }
 }

src/SecretSanta.php

@@ -53,23 +53,19 @@ public function addCouple($name, $email, $coupleName, $coupleEmail)
     }
 
     /**
-     * @return PlayersCollection
+     * @return Player[]
      * @throws SecretSantaException
      */
     public function play()
     {
         try {
             $this->combinePlayers();
 
-            return $this->players;
+            return $this->associatePlayers();
         } catch (SecretSantaException $exception) {
             throw  $exception;
         } catch (\Exception $exception) {
-            throw new SecretSantaException(
-                'Error during play, impossible to find secret santa, try again',
-                0,
-                $exception
-            );
+            throw new SecretSantaException('Error during play, impossible to find secret santa, try again');
         }
     }
 
@@ -84,34 +80,67 @@ private function combinePlayers()
 
         $retry = count($this->players) + $this->players->countExcludePlayers();
 
-        while (!$this->isValidCombination() && $retry > 0 ) {
-            $this->combination = [];
-            $secretPlayers = $this->players->shufflePlayers();
-            foreach ($this->players as $playerId => $player) {
-                foreach ($secretPlayers as $secretPlayer) {
-                    if ($player->id() != $secretPlayer->id() && !$this->players->areExclude($player, $secretPlayer)) {
-                        if (!in_array($secretPlayer->id(), $this->combination)) {
-                            $player->setSecretSanta($secretPlayer);
-                            $this->combination[$player->id()] = $secretPlayer->id();
-                            unset ($secretPlayers[$secretPlayer->id()]);
-                            break;
-                        }
-                    }
-                }
-            }
+        while (!$this->tryMatchSecretSantaPlayers() && $retry > 0 ) {
             $retry--;
         }
 
-        if (!$this->isValidCombination() && $retry <= 0) {
+        if (!$this->isValidCombination()) {
             throw new SecretSantaException("Not enough players to play");
         }
     }
 
+    /**
+     * @return bool
+     */
+    private function tryMatchSecretSantaPlayers()
+    {
+        $this->combination = [];
+        $secretPlayers = $this->players->shufflePlayers();
+        foreach ($this->players->players() as $playerId => $player) {
+            foreach ($secretPlayers as $secretPlayer) {
+                if ($this->isValidSecretSanta($player, $secretPlayer)) {
+                    $this->combination[$player->id()] = $secretPlayer->id();
+                    unset ($secretPlayers[$secretPlayer->id()]);
+                    break;
+                }
+            }
+        }
+
+        return $this->isValidCombination();
+    }
+
+    /**
+     * @param Player $player
+     * @param Player $secretPlayer
+     * @return bool
+     */
+    private function isValidSecretSanta($player, $secretPlayer)
+    {
+        if ($player->id() != $secretPlayer->id() && !$this->players->areExclude($player, $secretPlayer)) {
+            if (!in_array($secretPlayer->id(), $this->combination)) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
     /**
      * @return bool
      */
     private function isValidCombination()
     {
         return count($this->combination) > 0 && count($this->combination) == count($this->players);
     }
+
+    private function associatePlayers()
+    {
+        foreach ($this->combination as $playerId => $secretSantaId) {
+            $this->players->player($playerId)->setSecretSanta(
+                $this->players->player($secretSantaId)
+            );
+        }
+
+        return $this->players->players();
+    }
 }

tests/PlayersCollectionTest.php

@@ -43,6 +43,30 @@ public function testDuplicatePlayer()
         $playersCollection->addPlayer($player);
     }
 
+    /**
+     * @expectedException \SecretSanta\Exceptions\PlayersCollectionException
+     */
+    public function testDuplicatePlayerInCouple()
+    {
+        $expectedPlayer = Player::create('name', '[email protected]');
+        $expectedCouple = Player::create('nameCouple', '[email protected]');
+
+        $playersCollection = new PlayersCollection();
+        $playersCollection->addPlayer($expectedPlayer);
+        $playersCollection->addCouple($expectedPlayer, $expectedCouple);
+    }
+
+    /**
+     * @expectedException \SecretSanta\Exceptions\PlayersCollectionException
+     */
+    public function testSamePlayerInCouple()
+    {
+        $expectedPlayer = Player::create('name', '[email protected]');
+
+        $playersCollection = new PlayersCollection();
+        $playersCollection->addCouple($expectedPlayer, $expectedPlayer);
+    }
+
     public function testAddCouple()
     {
         $expectedPlayer = Player::create('name', '[email protected]');