弊社で毎月開催し、PHPエンジニアの間で好評いただいているPHP TechCafe。
2022年10月のイベントでは「PHPのリーダブルなコード」について語り合いました。
弊社のメンバーが事前にまとめてきたコードの書き方の事例にしたがって、他の参加者に意見を頂いて語り合いながら学びました。
今回はその内容についてレポートします。
特集:PHPのリーダブルなコード
この回では弊社が用意したBADコードをトピックに、「どこが悪いのか」・「どうすれば良くなるか」を議論しました。 BADコードは難易度別に初級・中級・上級に分かれており、全部で8問ございます。
元ネタ:GitHub - piotrplenik/clean-code-php: Clean Code concepts adapted for PHP
初級
Sample 1
BADコード
<?php if ($foo === $bar) { return true; } else { return false; }
良くない理由
- bool 値を返したいときにif文を書くのは冗長
解消例
結果をbool値で返したい場合、明示的にif-else
文を書かなくとも、return 文に条件式を書くことで比較結果を戻り値にできます。
<?php return $foo === $bar;
参加者からは次のようなご意見を頂きました。
- あまり馬鹿にできなくて、何回か現場でも実際に見たことがあります
- 初学者は「"比較" = "IF文"」と錯覚しがちですね
Sample 2
BADコード
<?php if ($isOK && !hasPermission && $hoge !== “sample”) //何らかの処理 }
良くない理由
- if文の条件が複雑になると、単純に読みにくくなるだけでなく、不具合も起こりやすくなる
isOK
という変数名が微妙
解消例
パターン1:早期リターンを活用
<?php if (!$isOK){ return "NG"; } if (hasPermission){ return "OK"; } if ($hoge !== "sample"){ //なんらかの処理 }
パターン2:条件を関数に閉じ込める
<?phpif (条件がわかりやすく言語化された関数名()){ // なんらかの処理 }
Sample 3
BADコード
<?php if ($input == 0) { echo(‘0です!’); }
良くない理由
==
の場合、判定が曖昧なのでバグのもとになりやすいPHP8.0以前の場合、文字列と数値の比較をする際に文字列が数値にキャストされるので危険
- PHPの比較演算子については弊社のブログでも深掘りしておりますので、ご興味のある方はぜひ御覧ください
解消例
===
に比較演算子を変更します。
これにより、左辺・右辺の値が等しく、かつデータ型も一致する場合のみtrue
を返すため、データ型の不一致による予期しない結果を避けることができます。
<?php if ($input === 0) { echo('0です!'); }
中級
Sample 4
BADコード
<?php /** * お店が営業日かをチェックする * * @param $day 曜日の文字列 * @return bool */ function isShopOpen($day): bool { if ($day) { if (is_string($day)) { $day = strtolower($day); if ($day === 'friday') { return true; } elseif ($day === 'saturday') { return true; } elseif ($day === 'sunday') { return true; } return false; } return false; } return false; }
良くない理由
このコードについて良くない理由として、以下の2点が考えられます。
どういうチェックが必要なのかがわかりにくい
- 分岐をすべて読まないとチェックしたい内容がわからない
- $day が以下の場合
true
- friday
- saturday
- sunday
- $day が以下の場合
- 分岐が進むにつれて、該当処理が実行される条件の数が多くなる
- これまでの分岐条件を記憶していかないといけない
- 分岐をすべて読まないとチェックしたい内容がわからない
引数に型指定されていない
- メソッド内で型チェックが必要
- ロジックを読むまで$day が String 型であることがわからない
- 引数名からDateTimeクラスのインスタンスとも捉えられる
- だが実際はString型で受け取ることを前提とした処理になっている
参加者からは次のようなコメントが寄せられました。
- 「if-elseで条件分岐を作る際は、単純化できないか考えるチャンス」
その他、参加者からは「ネストがV字に広がっている様子がまるで波動拳のようだ」というコメント から有名なミーム画像が紹介され、「本格的な波動拳ですね」、「今度コードレビューで使いたい!」などコメントが寄せられ、かなり盛り上がりを見せていました。
解消例
解消例は以下になります。
<?php /** * お店が営業日かをチェックする * * @param string $day 曜日の文字列 * @return bool */ function isShopOpen(string $day): bool { // 値がセットされているか(これがガード節) if (empty($day)) { return false; } $openingDays = ['friday', 'saturday', 'sunday']; return in_array(strtolower($day), $openingDays, true); }
主な修正点は以下のとおりです。
- $day の引数型をString型で宣言することで、データ型のミスリードをなくす
- 前提部分のチェックをガード節で対応することで、余計なネストを生まないようにする
- 営業日を$openingDays に入れることでいつが営業日かわかりやすくなり、変数名が説明変数を担っている
こちらに関して、参加者からは次のようなコメントを頂きました。
- match文でもかけそう
- empty() はないほうが良い
- ※empty() の場合、変数が存在し、かつ値が空でない場合のみ
false
が返されます。ここで実施したいのは空文字チェックですが、empty() では仕様上、空文字でもtrue
が返却されるため、空文字チェックとしてempty()を使用するのは適切ではありません。
- ※empty() の場合、変数が存在し、かつ値が空でない場合のみ
- 列挙型(enum)も活用できそう
Sample 5
BADコード
このケースは少し特殊で、修正前のコードを間違えて修正後のコードに直してしまった、というケースを想定して作られています。
修正前
<?php public function getRecipeListString():string { $recipeList = getRecipeList(); // なにかレシピのリストを配列で取得するもの return implode(",", recipeList); }
修正後
<?php public function getRecipeListString(bool $isSpace):string { $recipeList = getRecipeList(); //何かレシピのリストを配列で取得するもの if ($isSpace) { return implode(“ ”, $recipeList); } else { return implode(“,”, $recipeList); } }
良くない理由
- 違う要件が来たときにまたif文が増える
- 仮に「レシピのリストをハイフン区切りで表示する」といった仕様になった場合、新たに分岐を追加する必要がある
解消例
解消例として以下の2パターンが挙がりました。
パターン1:関数ごとに処理を分割
「カンマ区切り」・「スペース区切り」と、要件ごとに同じ処理を関数に区切ったパターンです。
<?php public function getRecipeListStringWithComma():string { $recipeList = getRecipeList(); // なにかレシピのリストを配列で取得するもの return implode(",", $recipeList); } public function getRecipeListStringWithSpace():string { $recipeList = getRecipeList(); // なにかレシピのリストを配列で取得するもの return implode(" ", $recipeList); }
パターン2:デリミタ(区切り文字)を引数にする
<?php public function getRecipeListString(string $delimiter):string { $recipeList = getRecipeList(); // なにかレシピのリストを配列で取得するもの return implode($delimiter, $recipeList); }
またパターン2について、参加者からは「$delimiter に初期値を入れてはどうか?」という意見がありましたが、こちらについて以下のような反応がありました。
- 確実に必要ではない引数にはじめからデフォルト値をセットしないほうが良いと思う
- 既存のものを移行するなどのケースであれば、オプショナルにするのもあり
- データが収束しているように見えるからメソッドが気になる
Sample 6
BADコード
<?php //メルマガ購買顧客リストまたはYoutubeチャンネル登録会員リストを更新する function updateMailMagazineListOrYoutubeList($accountId, $MailMagazine, $Youtube, $isMailMagazine, $isYoutube) { if ($isMailMagazine) { # code… } elseif($isYoutube) { #code… } if ($isMailMagazine) { $sql = “update mailMagazineList set …”; } elseif ($isYoutube) { $sql = “update youtubeList set…”; } }
良くない理由
- 別のビジネス概念を無理やり1つの関数の処理にまとめている
- フラグを引数で渡しているのでビジネス概念が増えるほど引数が増える
- 今後さらにビジネス概念が増えた場合、より分岐が複雑化する
- 実装者は共通化したいという意図を持っていたと思われるが、結局共通化できていない
参加者の方々からも次のようなコメントが寄せられました。
- 割りとよく見る
- 既存のものに焼き増しで追加した結果こうなってしまった?
- なかなかリアルなケース
等、現場でも見覚えのある方が多くいらっしゃったようです。
解消例
異なるビジネス概念を扱うなら、関数・クラスは分けるべき
<?php function updateMailMagazineList($accountId, $Mail Magazine) { #code… $sql = “update mailMagazineList set,,,” #code… } function updateYoutubeList($accountId, $Youtube) { #code… $sql = “update mailMagazineList set…” #code… }
上級
Sample 7
BADコード
<?php /** * PHPによる形態素解析処理 * * * @param string $code 文章 */ function parseBetterPHPAlternative(string $code): void { $regexes = [ //… ]; $statements = explode(‘ ‘ , $code); $token = [ ]; foreach ($regexes as $regex) { foreach($statements as $statement) { //… } } $ast = [ ]; foreach ($tokens as $token) { //lex… } foreach ($ast as $node) { //parse… } }
良くない理由
- 1つの関数で複数の処理を行っている
- 修正時の影響が大きくなる
- 修正箇所の後続処理への影響を考えて修正しなければならない
- 修正時の影響が大きくなる
- 不具合が発生した際の問題箇所の特定が困難
- 複数の処理が組み込まれていると処理の前後関係を理解する必要がある
- 処理が分割していれば問題箇所を特定しやすくなる
- 複数の処理が組み込まれていると処理の前後関係を理解する必要がある
- 内部処理の再利用ができない
- 処理の中に組み込まれてしまうと特定処理だけを他の処理でも利用したくても利用できない
- テストが書きにくい
- 処理が分割されていれば細かな条件のテストコードが書ける
参加者からは次のようなコメントを頂きました。
- 一度に一つ以上のことをやらないでほしい...
- サブルーチンに切り出してほしい
等の意見が挙がっており、やはり1つの関数で複数の処理を実行することに否定的な意見が多く寄せられました。
解消例
parseBetterPHPAlternative
を3つのクラスに分割することで、可読性を高めつつ、各処理が依存していない状態に修正しました。これならばユニットテストも書けそうですね。
<?php /** * トークナイザ */ class Tokenizer { /** * 文章を単語に分解する * * @param string $code 文章 * @return array $tokens 単語のリスト */ public function tokenize(string $code): array { $regexes = [ // ... ]; $statements = explode(' ', $code); $tokens = []; foreach ($regexes as $regex) { foreach ($statements as $statement) { $tokens[] = /* ... */; } } return $tokens; } } /** * 字句解析器 */ class Lexer { /** * 単語の解析処理 * * @param string $tokens 単語のリスト * @return array $ast 解析結果のリスト */ public function lexify(array $tokens): array { $ast = []; foreach ($tokens as $token) { $ast[] = /* ... */; } return $ast; } } /** * PHPによる形態素解析処理 * * @param string $code 文章 */ class BetterPHPAlternative { /** @var Tokenizer */ private $tokenizer; /** @var Lexer */ private $lexer; public function __construct(Tokenizer $tokenizer, Lexer $lexer) { $this->tokenizer = $tokenizer; $this->lexer = $lexer; } /** * 形態素解析 * * @param string $code 文章 */ public function parse(string $code): void { $tokens = $this->tokenizer->tokenize($code); $ast = $this->lexer->lexify($tokens); foreach ($ast as $node) { // parse... } } }
Sample 8
BADコード
- FavoriteRecipe クラス
<?php class FavoriteRecipe { public function getRecipe(string $name):void { $limit = 10; $recipeRepository = new RecipeRepository(); $recipes = $recipeRepository.findByName($name, $limit); foreach ($recipes as $recipe) { var_export($recipe); } } }
- RecipeRepository クラス
<?php class RecipeRepository { // Cookpadからレシピを取得する function findByName(string $name, int $limit):Recipe { $cookPad = new CookPad(new \PHPHtmlParser\Dom); $result = $cookpad->search($name, 1, $limit, false); return $result; } }
良くない理由
- FavoriteRecipeクラスがRecipeRepositoryクラスの実装に依存している
- RecipeRepository::findByName に何かしら影響が発生した場合、FavoriteRecipeクラスにも影響が及ぶ
- 例. findByNameメソッドの引数を増やした場合
- RecipeRepository::findByName に何かしら影響が発生した場合、FavoriteRecipeクラスにも影響が及ぶ
<?php class RecipeRepository { // 引数を修正 - function findByName(string $name, int $limit):Recipe { + function findByName(string $name,int $pageNum, int $limit, bool $isRandom):Recipe[] { // ... } } class FavoriteRecipe { public function getRecipe(string $name):void { // ... // reposiotoryの修正を受けて引数を修正 - $recipes = $recipeRepository.findByName($name, $limit); + $recipes = $recipeRepository.findByName($name, $pageNum, $limit, $isRandom); } }
解消例
SOLID の原則の D:依存性逆転の原則(DIP)を活用して、上位モジュールのFavoriteRecipeクラスが下位モジュールのRecipeRepositoryクラスに依存しなくなるようにクラス構成を修正します。
RecipeRepositoryインターフェースを作成
<?php interface RecipeRepository { public function findByName(string $name, $pageNum = 10, $limit = 10, $isRandom = false):Recipe[]; }
RecipeRepositoryクラスはRecipeRepositoryインターフェースを実装する
<?php class RecipeRepositoryImpl implements RecipeRepository { public function findByName(string $name, $pageNum = 10, $limit = 10, $isRandom = false): Recipe[] { $cookpad = new Cookpad(new \PHPHtmlParser\Dom); $result = $cookpad->search($name, $pageNum, $limit, $isRandom); return $result; } }
FavoriteRecipeクラスはRecipeRepositoryインターフェースを参照する
<?php class FavoriteRecipe { private RecipeRepository $recipeRepository; public function __construct(RecipeRepository $recipeRepository) { $this->recipeRepository = $recipeRepository;[f:id:d-t-kong:20230530144526j:plain][f:id:d-t-kong:20230530144526j:plain] } public function getRecipe(string $name):void { $pageNum = 10; $limit = 10; $isRandom = false; $recipes = $this->recipeRepository.findByName($name, $pageNum, $limit, $isRandom); foreach ($recipes as $recipe) { var_export($recipe); } } }
こうすることで、FavoriteRecipeクラスがFavoriteRecipeクラスに依存しないクラス構成になりました。
こちらの解消例について、参加者の方から次のようなご指摘をいただきました。
ユニットテスト時には、外部APIへのHTTPリクエストを避ける必要があります。参加者の間では以下の改善案が提案されました。
編集後記
以上、PHPのリーダブルなコードについてまとめました。 今回提示したアンチパターンを業務でやってしまった!という方も中にはいらっしゃったのではないでしょうか?
「PHP TechCafe」では今後もPHPに関する様々なテーマのイベントを企画していきます。 皆さまのご参加をお待ちしております!