RAKUS Developers Blog | ラクス エンジニアブログ

株式会社ラクスのITエンジニアによる技術ブログです。

PHPerのための「PHPのリーダブルなコード」を語り合う【PHP TechCafe イベントレポート】

弊社で毎月開催し、PHPエンジニアの間で好評いただいているPHP TechCafe。

2022年10月のイベントでは「PHPのリーダブルなコード」について語り合いました。

弊社のメンバーが事前にまとめてきたコードの書き方の事例にしたがって、他の参加者に意見を頂いて語り合いながら学びました。

今回はその内容についてレポートします。

rakus.connpass.com

特集: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の比較演算子については弊社のブログでも深掘りしておりますので、ご興味のある方はぜひ御覧ください

tech-blog.rakus.co.jp

解消例

===に比較演算子を変更します。 これにより、左辺・右辺の値が等しく、かつデータ型も一致する場合のみ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点が考えられます。

  1. どういうチェックが必要なのかがわかりにくい

    • 分岐をすべて読まないとチェックしたい内容がわからない
      • $day が以下の場合true
        • friday
        • saturday
        • sunday
    • 分岐が進むにつれて、該当処理が実行される条件の数が多くなる
      • これまでの分岐条件を記憶していかないといけない
  2. 引数に型指定されていない

    • メソッド内で型チェックが必要
    • ロジックを読むまで$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()を使用するのは適切ではありません。
  • 列挙型(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つの関数で複数の処理を行っている
    • 修正時の影響が大きくなる
      • 修正箇所の後続処理への影響を考えて修正しなければならない
  2. 不具合が発生した際の問題箇所の特定が困難
    • 複数の処理が組み込まれていると処理の前後関係を理解する必要がある
      • 処理が分割していれば問題箇所を特定しやすくなる
  3. 内部処理の再利用ができない
    • 処理の中に組み込まれてしまうと特定処理だけを他の処理でも利用したくても利用できない
  4. テストが書きにくい
    • 処理が分割されていれば細かな条件のテストコードが書ける

参加者からは次のようなコメントを頂きました。

  • 一度に一つ以上のことをやらないでほしい...
  • サブルーチンに切り出してほしい

等の意見が挙がっており、やはり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メソッドの引数を増やした場合
<?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リクエストを避ける必要があります。参加者の間では以下の改善案が提案されました。

  • テストモックを準備する
  • Cookpadインスタンス生成部分をもう一段階DI
  • CookpadPSR-18に準拠している場合、HTTP通信部分のみを外部から切り出す

編集後記

以上、PHPのリーダブルなコードについてまとめました。 今回提示したアンチパターンを業務でやってしまった!という方も中にはいらっしゃったのではないでしょうか?

PHP TechCafe」では今後もPHPに関する様々なテーマのイベントを企画していきます。 皆さまのご参加をお待ちしております!

Copyright © RAKUS Co., Ltd. All rights reserved.