🐸

レビュー指摘事項からふりかえる、良くない設計の傾向と対策

まさき。

2024/09/10に公開

 はじめにこんにちは、まさき。です。

最近、句点の使い方で自分だと特定されるようになりました（笑）。
先日、インボイス対応の請求書出力機能を改修した際に、社内のつよいエンジニアにコードレビューを受ける機会がありました。エンジニアとしてさらに成長したいと感じ、PRで指摘された具体的な問題を振り返り、どのようにすれば自分でも早期に改善点に気づけるかを考えました。
!この記事は株式会社GoQSystemさんとの合同勉強会でのLTの完全版です。

 背景
 ことのはじまり2023年10月に施行されたインボイス制度に対応するため、NE株式会社のプロダクトであるネクストエンジンでも対応を進めました。その中で、請求書出力機能のインボイス対応を行い、いくつかの課題をクリアして無事にインボイス対応の請求書をダウンロードできるようにしました。
そのタイミングで、ユーザーからインボイス対応の請求書のテンプレートを増やしてほしいという要望が届きました。
実際にダウンロードできるインボイス対応の請求書の例
この請求書は大きく分けると以下の3つのブロックに分けることができます。
ヘッダーブロック
明細ブロック
インボイス項目ブロック
ヘッダーブロックは、請求金額や振込先といった請求書の大事な情報が記載されていて、

明細ブロックやインボイス項目ブロックは、購入した商品の金額や税率ごとの金額などが記載されています。
今回はちょっと見た目を変えた請求書を作って欲しいということでした。

一見簡単そうに思えました。既存のテンプレートをコピーし、必要な部分を変更するだけで対応できると考えていましたが、将来的なメンテナンス性を考慮すると課題が浮かび上がりました。

 リファクタする経緯しかし、インボイス対応のテンプレートだけでなく、今後別のテンプレート（例：新たな税制対応など）が追加される可能性が十分にあります。単にテンプレートをコピーするだけでは、今後のメンテナンスが複雑化し、新しい税制や取引ルールに対応するたびに、コードの重複や冗長性が生まれてしまうリスクがありました。

このような状況に備えて、テンプレートの構成要素を独立したクラスとして定義し、それらを組み合わせる形にリファクタリングすることにしました。
10年経ったらまだ新たな税制や取引ルールに対応するためのテンプレートが増えているかもしれません

 レビューでもらった指摘事項について振り返るリファクタしている中でいくつか指摘をもらったので、その指摘事項についてふりかえろうと思います！

 指摘事項
 ヘッダーと明細という分割で本当にいいのか？
 どんな指摘だったのかリファクタで、請求書を「ヘッダー」と「明細」に分割したのですが、本当にその2つに分割するのでいいんだっけ？と指摘をいただきました。「ヘッダー」と「明細」の間になにか別なものを追加したくなったらどうする？「フッター」を追加したくなったら改修は大変じゃない？といった旨の意見でした。
たしかに、新たにブロックが増えることはありえないとは言えないのでその通りだなと思いました。
また、HeaderのInterfaceと明細のInterfaceを実装していたのですが、以下のようなものでした。
interface Header {
    public function build($a, $b, $c , ...); 
}
この実装では、そもそもヘッダーが必ず$a, $b, $c といった要素を所持していることが要求されており、もし新たに出てきたHeaderが $b を必要とせず、$z を必要としていたらなど、必要なものが増えたらどうするんだ？という指摘でもありました。

 じゃあどうすれば既存の設計では、ヘッダーと明細の処理が密接に結びついており、テンプレートの拡張が難しくなっていました。そこで、共通部分を抽象化し、柔軟な構成を実現するためにInvoicePdfインターフェースを導入しました。

このインターフェースを考えるにあたり、ヘッダーでも明細でも変わらないものと考えたら、描画開始位置くらいだったので、最終的には以下のようになりました。
interface InvoicePdf {
    public function build(int $y, $pdf): int; 
}
ひとことでHeaderと言ってもどの要素を必要とするかはまた違う問題なので、

必要な要素は実装クラスのコンストラクタで持たせるようにすることで、変更に強い実装になりました。
$header = new InvoiceHeader($a, $b, $c, ... );
$y = $header->build($y, $pdf);
これからどんな拡張が考えられそうか、その拡張は簡単にできそうか？といったことから、

OCP（Open-Closed Principle：拡張には開かれ、変更には閉じられている設計）を守れているか？

という視点で実装したり設計を考えると良いと思いました。

 プロパティのないクラス
 どんな指摘だったのか「プロパティのないクラスだね」。この指摘を受けたクラスでは、すべてのデータがメソッドの引数として渡されており、何がそのクラスで必要な情報なのかが一目でわかりにくい状態でした。メソッド呼び出し時に、引数に何を渡すべきかも把握しづらく、メソッドの再利用性が低くなる問題もあります。
プロパティを持たないクラスは、実質的にクラスではなく、関数置き場のようになってしまいます。これでは、オブジェクト指向の設計原則に反し、責務の分離が不十分になります。

class Hoge
{
   public function methodA($a, $b, $c)
   {
      return $a + $b + $c; 
   }

   public function methodB($a, $b, $c, $d)
   {
      return $a + $b + $c + $d;
   }
} 

$hoge = new Hoge();
$result_a = $hoge->methodA($a, $b, $c);
$result_b = $hoge->methodB($a, $b, $c, $d);


 じゃあどうすれば各メソッドで共通で必要となる変数をコンストラクタから渡してあげて、プロパティにセットして呼び出してあげることで、

インスタンス生成時にこのクラスが何を要求しているのかがわかりやすくなります。
LCOM（Lack of Cohesion in Methods）は、クラスのメソッドがどのくらい同じインスタンス変数(プロパティ)を共有しているかを測る指標です。低凝集なクラスは、メソッド間であまり関係のないインスタンス変数(プロパティ)を使い、再利用性が低くなる傾向があります。
LCOM はメソッド内で参照しているインスタンス変数（と他のメソッド）に着目します。直感的には、2つのメソッドが互いに関連しているならば、きっと同じインスタンス変数を参照しているはずです。

https://qiita.com/fujiharuka/items/65125592bd31e2a1c16d
今回のように修正することで、同じインスタンス変数(プロパティ)を共有するようになるので、凝集度の高いクラスになります。
class RefactoredClass {
   protected $a;
   protected $b;
   protected $c;
   protected $d;

   public function __construct($a, $b, $c, $d) {
      $this->a = $a;
      $this->b = $b;
      $this->c = $c;
      $this->d = $d;
   }

   public function methodA() {
      return $this->a + $this->b + $this->c;
   }

   public function methodB() {
      return $this->a + $this->b + $this->c + $this->d;
   }
}
というわけで、プロパティのないクラス、コンストラクタのないクラスに遭遇したら、低凝集なクラスになってないかな？と問いかけてみるのはいかがでしょうか？

 テンプレートが増えたらifが増えて複雑になりそうだな
 どんな指摘だったのか今回みたいにまたテンプレートが増えたらテンプレートごとにインスタンスの生成をするからif文が増えて複雑になりそうですね、というご指摘をもらいました。いや至極当然の指摘ですね。テンプレートがもう増えないわけがない。

public function hogeAction()
{
    if (  ... ) {
       // パターンAのテンプレートのインスタンス 
    }
    if ( ... ) {
       // パターンBのテンプレートのインスタンス
    }
    // パターンC, Dが増えたらここがまた複雑になりそう
　　 // 書き込み処理など
}

 じゃあどうすればこのような場合、Factoryパターンを使ってインスタンス生成の責務を分離するのが良い解決策です。Factoryパターンは、複雑なインスタンス生成ロジックを一箇所に集約できるため、クライアントコードが簡潔になり、コードの保守性が向上します。また、新しいテンプレートを追加する際の修正箇所を最小限に抑え、拡張に強い設計となります。

public function hogeAction()
{  
   $template = TemplateFactory::createTemplate($a, $b, ...);
   $template->write(); // 書き込み処理など
}

もしもパターンが増えたら複雑になるかもしれない、と気づいたらこのことを思い出していただけると嬉しいです。

 privateメソッドなのにテスト書いてるね
 どんな指摘だったのか「privateメソッドなのにテストを書きたくなってしまっているということは、本当は1クラスに切り出せるほどの内容なのかもね」と、多くの責務を抱えてしまっているのかもしれない。という指摘をもらいました。
実際、privateメソッドはテストを書かない方がいいというのは割と有名な話で私もそれ自体は知っています。

知っているけど、書いてしまっているのはなんでなんだろうね、をふりかえろうと思います。

privateメソッドのテストって書かない方がいいんだっけ？

 どうしてまずいのか？そもそもprivateメソッドなのにテストを書きたくなってしまうのは、そのメソッドの挙動に不安があるからです。本来、privateメソッドは内部的な実装に過ぎず、本当にテストすべきなのは「publicメソッドを通じて機能が正しく動作するか」ということです。
でも、privateメソッドであまりにも複雑な分岐などを実装していると、public メソッドからの呼び出しだけでは不安になり、テストを書いてしまうことになります。
今回の場合は、PDFに描画する draw()メソッドで呼び出していた描画するものを組み立てるbuildメソッドのテストを書いていました。このメソッドでは、以下の3つのことをやっていました。
データを取得する
データから計算する
表示用に整形する
データの取得結果によって計算結果も変わってしまうため、テストもしづらいものでした。

class Hoge
{
  /**
   * 描画する
   */
  public function draw()
  {
    $this->build();
    // 書き込み処理
  }
  /**
   * DBからデータを取得して画面表示用に整形する
   */
  private function build()
  {
    // データを取得する処理
    // 取ってきたデータから計算する処理
    // 計算したものを表示用に整形する処理
  }
}

 じゃあどうすればそもそもprivateメソッドの中にある処理自体が別のクラスに切り分け可能なものでした。

そこで新たに、取得したデータを表示用に整形するクラスとメソッドを切り出すように変更し、データ整形だけを責務としました。

これにより、データ整形だけでテストでき、DBなどからのデータ取得や計算結果などについては影響を抑えることができました。もちろん、まだDBからデータを取得する処理と計算する処理が同一クラスのメソッドに実装されているのでここも分離したほうがより良いコードになるのは間違いないです。
<?php

class Builder 
{
    public function build(array $params): array
    {
        // 受け取った値を表示用に組み立てるだけ
    }
}

class Hoge
{
    /**
     * 描画する
     */
     public function draw()
     {
         $params = $this->getParams();
         $this->builder->build($params);
         
         // 書き込み処理
     }
     
     /**
      * DBから取得したデータをもとに計算した結果を返す
      */
      private function getParams(): array
      {
          // DBからデータを取得する
          // 計算してから返す
          // TODO: DBからの取得を分離する
      }
}

1クラスに様々な責務を持たせてしまうと、部分的にテストができずつらくなるので、privateメソッドなのにテストを書きたい！と思ったらクラス分割ができないか考えるのがいいのかなと思いました。

 メソッドの中でクラスをnewして使ってる
 どんな指摘だったのかメソッドの中でクラスをnewして使っているよね、これだとインスタンス生成とその使用を同時に行っていることになる。複数の責務を持ってしまっているとの指摘でした。

class Hoge
{
   public function __construct()
   {
       $this->instance = new Fuga();
    }

    public function do()
    { 
       $this->instance->do();
    }
} 

$hoge = new Hoge();
$hoge->do();

このクラスをテストすると、内部で生成されるインスタンスの影響なのか、クラスのメソッドの影響なのかを区別することができない。

 じゃあどうすればインスタンス生成と使用は別クラスで行うべきでした。生成されたインスタンスをコンストラクタで受け取って、それを使うだけにすることで、責務を分けることができ、テストもしやすくなります。
インスタンスの取得と利用を同じクラスで行っているときは、単一責任の原則に違反しているかもしれないので処理を見直しましょう。
class Hoge
{
   public function __construct(Fuga $fuga)
   {
       $this->instance = $fuga;
    }

    public function do()
    { 
       $this->instance->do();
    }
} 

$fuga = new Fuga();
$hoge = new Hoge($fuga);
$hoge->do();


 まとめレビューで指摘される事項の多くは、SOLID原則に基づいています。これらの原則をしっかり理解し、実践することで、柔軟でメンテナンス性の高いコードを書くことができます。私自身も、書籍で学んで理解したつもりでも、実務では活かしきれていない部分がありました。しかし、指摘を受け、それを振り返ることで、コード設計の本質が理解できるようになりました。
コードレビューを通じて、自身の成長の機会を見つけることができます。次のプロジェクトや機能追加の際にも、このような観点を取り入れてみてください。小さな改善の積み重ねが、大きな成長につながります。

NE株式会社の開発ブログPublication

NE株式会社のエンジニアを中心に更新していくPublicationです。 NEでは、「コマースに熱狂を。」をパーパスに掲げ、ECやその周辺領域の事業に取り組んでいます。 Homepage: ne-inc.jp/

はじめに

背景

ことのはじまり

リファクタする経緯

レビューでもらった指摘事項について振り返る

指摘事項

ヘッダーと明細という分割で本当にいいのか？

どんな指摘だったのか

じゃあどうすれば

プロパティのないクラス

どんな指摘だったのか

じゃあどうすれば

テンプレートが増えたらifが増えて複雑になりそうだな

どんな指摘だったのか

じゃあどうすれば

privateメソッドなのにテスト書いてるね

どんな指摘だったのか

どうしてまずいのか？

じゃあどうすれば

メソッドの中でクラスをnewして使ってる

どんな指摘だったのか

じゃあどうすれば

まとめ

Discussion