はじめに
下記の記事執筆していたとき、ブラックジャックのソースコードについて、以下の課題がありました。
機能としては完成させて、やり切った思いはあります。ただ、もっとこうしたら良くなるという、自分では気付かないことが多々あるはず...という状態なので、第三者にコードレビューをしてもらう機会を得ようと思います。
今回、コードレビューしてもらう機会を得たので、レビューに基づいてリファクタリングしました。
ちなみに、PHPで作ったコンソール画面で操作するブラックジャックゲームのソースコードです。
自己紹介
- 私は公務員からWebエンジニアに転向して、 2 年目に入ったところです。
- 1 年目は、研修 3 ヶ月、実務 9 ヶ月という内訳で、実務は設計メインの SES 出向先であるため、コードの実装は実務では未経験です。
- そのため自己研鑽として、継続的にコードと向き合い技術を向上させるための課題を設定して取り組んでいます。
誰にコードレビューしてもらったか
自社の育成担当の方に目を通してもらいました。
育成担当の方とは業務で一緒の仕事しているわけではなく、月一で面談して学習の方向性などを話す、という関係です。
目的
- 記事化することで、自分自身の復習のため
- 他の人はこんなことでレビューされてるんだ、という初学者の方への情報共有のため
目次
コードレビューしてもらったうち、以下の 3 点について書いていきます。
1. メッセージ表示についての Message クラスへの一任
2. switch 文の利用
3. status 情報の判定ロジックの内包
1. メッセージ表示についての Message クラスへの一任
まずは、メッセージ表示についての Message クラスへの一任についてです。
レビュー内容
レビュー内容は以下です。
設計として Message クラスを作成したのであれば、アプリケーション内で横断的に利用できるようにするのがベストです。
そして、メッセージを出力するところを Message クラスに一任しましょう！
変更履歴
以下のコミットの変更差分が、メッセージ表示についての Message クラスへの一任に関する変更です。
変更内容
各クラスで文字列を直接書いて出力しているところを、 Message クラスに委譲しました。
元々、Message クラスは作成していたのですが、各クラスで文字列書いて出力しているところも混在していました。
混在させていた理由は、繰り返し利用しそうな文章だけ Message クラスでメソッド化しておこう、と考えていたためです。
変更箇所の抜粋
変更は複数箇所ありますが、一つ抜粋して修正前後を確認します。
修正前
/**
* ブラックジャックを開始する
*
* @return void
*/
private function startGame(): void
{
echo 'ブラックジャックを開始します。' . PHP_EOL;
修正後
/**
* ブラックジャックを開始する
*
* @return void
*/
private function startGame(): void
{
echo Message::getGameStartMessage();
/**
* ブラックジャックの開始のメッセージを返す
*
* @return string
*/
public static function getGameStartMessage(): string
{
return 'ブラックジャックを開始します。' . PHP_EOL;
}
今回リファクタリングしてみて、Message クラスにメソッド化しているのに、他のクラスで文字列書いて出力している重複コードが見つかりました。
早速、Message クラスに一任していないことによる弊害が確認されて、一任した方がいい、と実感できました。
2. switch 文の利用
続いて、 switch 文の利用についてです。
レビュー内容
レビュー内容は以下です。
同じ値を、 if, elseif で何度も聞く場合は、 switch 文も検討してみてください。スッキリ見えます。
変更履歴
以下のコミットの変更差分が、switch 文の利用に関する変更です。
switch 文の基本の確認
switch 文を使用してこなかったので、まずは公式ドキュメントで基本を確認しました。
注意:
switch/case が行うのは、 緩やかな比較 であることに注意しましょう。
例1
switch文の構造<?php // これが swtich 文です: switch ($i) { case 0: echo "iは0に等しい"; break; case 1: echo "iは1に等しい"; break; case 2: echo "iは2に等しい"; break; } // 上記は、下記と同じです: if ($i == 0) { echo "iは0に等しい"; } elseif ($i == 1) { echo "iは1に等しい"; } elseif ($i == 2) { echo "iは2に等しい"; } ?>
注意したいのは、 switch 文は緩やかな比較（$i == 0）であることです。厳密な比較（$i === 0）ではないことに注意しながら、実装する必要があります。
変更箇所の抜粋
同じく変更は複数箇所ありますが、一つ抜粋して修正前後を確認します。
変更前
/**
* 勝敗、特殊ルールに応じたプレイヤーのチップ残高を算出する
*
* @param Game $game
* @param Player $player
* @return void
*/
public function calcChips(Game $game, Player $player): void
{
if ($player->getSplitStatus() === $player::NO_SPLIT) {
$chipsByResult = $this->calcChipsByResult($player);
$player->changeChips($chipsByResult);
$this->displayMessageByResult($player);
$player->reset();
} elseif ($player->getSplitStatus() === $player::SPLIT_FIRST) {
echo Message::getSplitDeclarationMessage($player);
$chipsByResult = $this->calcChipsByResult($player);
$player->changeChips($chipsByResult);
$this->displayMessageByResult($player);
$player->reset();
} elseif ($player->getSplitStatus() === $player::SPLIT_SECOND) {
$this->calcChipsSplitSecondHand($game, $player);
$this->resetStatusSplitPlayer($game, $player);
$this->removePlayerSecondHand($game, $player);
}
}
変更後
/**
* 勝敗、特殊ルールに応じたプレイヤーのチップ残高を算出する
*
* @param Game $game
* @param Player $player
* @return void
*/
public function calcChips(Game $game, Player $player): void
{
switch ($player->getSplitStatus()) {
case $player::NO_SPLIT:
$chipsByResult = $this->calcChipsByResult($player);
$player->changeChips($chipsByResult);
$this->displayMessageByResult($player);
$player->reset();
break;
case $player::SPLIT_FIRST:
echo Message::getSplitDeclarationMessage($player);
$chipsByResult = $this->calcChipsByResult($player);
$player->changeChips($chipsByResult);
$this->displayMessageByResult($player);
$player->reset();
break;
case $player::SPLIT_SECOND:
$this->calcChipsSplitSecondHand($game, $player);
$this->resetStatusSplitPlayer($game, $player);
$this->removePlayerSecondHand($game, $player);
break;
}
}
$player->getSplitStatus() という同じ値を、 if, elseif で何度も聞く場合であるため、 switch 文に書き換えました。
抜粋すると、このような差分です。
if ($player->getSplitStatus() === $player::NO_SPLIT) {
// 処理
} elseif ($player->getSplitStatus() === $player::SPLIT_FIRST) {
// 処理
} elseif ($player->getSplitStatus() === $player::SPLIT_SECOND) {
// 処理
}
switch ($player->getSplitStatus()) {
case $player::NO_SPLIT:
// 処理
break;
case $player::SPLIT_FIRST:
// 処理
break;
case $player::SPLIT_SECOND:
// 処理
break;
}
厳密な比較でなくなる点は少しモヤっとしますが、 $player->getSplitStatus() という同じ値の比較であることが一目瞭然なので、可読性は上がったと感じています。
3. status 情報の判定ロジックの内包
最後に、 status 情報の判定ロジックの内包についてです。
レビュー内容
レビュー内容は以下です。
status 情報をいろんなところで判定していますが、どこかのクラスに判定ロジックごと内包できるなら内包する方がよいです。
変更履歴
以下のコミットの変更差分が、status 情報の判定ロジックの内包に関する変更です。
変更箇所の抜粋
/**
* 選択したアクション（ヒットかスタンド）により進行する
*
* @param Game $game
* @return void
*/
public function action(Game $game): void
{
- while ($this->getStatus() === self::HIT) {
+ while ($this->hasHitStatus()) {
echo Message::getScoreTotalMessage($this);
sleep(Message::SECONDS_TO_DISPLAY);
echo Message::getProgressQuestionMessage();
$inputYesOrNo = $this->selectHitOrStand();
if ($game->getDealer()->isFirstHand($this->getHand())) {
$message = $game->getDealer()->getSpecialRule()->applySpecialRule($inputYesOrNo, $game, $this);
}
if ($inputYesOrNo === 'Y') {
$game->getDealer()->dealOneCard($game->getDeck(), $this);
$game->getDealer()->getJudge()->checkBurst($this);
$message = Message::getCardDrawnMessage($this);
} elseif ($inputYesOrNo === 'N') {
$this->changeStatus(self::STAND);
$message = Message::getStopDrawingCardsMessage();
} elseif (!$game->getDealer()->getSpecialRule()->isSpecialRule($inputYesOrNo)) {
$message = Message::getInputErrorMessage();
}
echo $message;
sleep(Message::SECONDS_TO_DISPLAY);
}
}
呼び出される判定ロジックは以下です。
/**
* ステータスがヒットであるか否かを判定する
*
* @return boolean
*/
public function hasHitStatus(): bool
{
if ($this->getStatus() === self::HIT) {
return true;
}
return false;
}
status 情報の判定ロジックの内包についても、ここのコードが何をしているのかが分かりやすくなり、コードが追記されていっても読みやすくなりそうだと思えました。
おわりに
今回、コードレビューしてもらって、レビューに基づいてリファクタリングしましたが、そういった機会は成長を加速させるために重要だと感じました。
独学でプログラミング学習してきた身としては、自分が書いたコードが良いのか悪いのかが答え合わせできなかったため、この先は成長を加速させる機会を逃さず、人にレビューしてもらう流れを作っていきたいです。
ありがとうございました。