メンバー全員でリファクタリング戦略会議をした話 #Laravel

はじめに

この記事は自分がリードエンジニアを務めた新規案件プロジェクトでリファクタリングした際のルールを書き留めたものです。

どうやってコードの品質を向上させるのか？という部分について実践した事を具体的にまとめたものになりますので一度読んでみて頂ければ嬉しいと思いました

どうか優しい目線で見て頂ければ嬉しいです！

リファクタ戦略

では実際にリファクタリングを実施していくのですが
まずリファクタリング対象のコードを明確にする必要があります

ただそれだけだと、どの様にリファクタリングするかチームメンバーの間でブレが出てしまうので

修正対象のコードがどれか見分けられる基準を作る
またそのコードをどの様に修正するのか、修正後の状態を明確にする

まで決めました。これは後ほど紹介させて頂きます

目指しているソースコード

チームでどの様なソースコードを目指すのか目標を決めました

何を実施しているかすぐに理解できるソースコード(可読性に優れている) ※1番大事
各クラス、各レイヤーで責務が守られているソースコード
セキュリティ要件が守られているソースコード
処理速度に対して考慮されているソースコード(N+1、適切なindexが貼られている)

この4つに絞ったんですが、特に1番大事と定めたのが可読性です。

リファクタリング前のコードですが、典型的なFatControllerになっており
コードの見通しが悪く、理解するのに時間がかかるのが1番の課題でした。

実際にリファクタリングした内容

1.自動テストの拡充

まず自分が担当になっているエンドポイントで書かれている自動テストを読む
その上で、テストケースを拡充した方が良さそうであれば、テストを拡充する
- 現状ほとんどモデルにメソッドがないのでまずはFeatureテストに正常系と異常系がある事を確認する。その上で足りていないテストがあった場合は拡充する
観点としては、異常系が確実に書かれているか（特にクリティカルなもの）を基準に確認して足りていなければ書く
- 認証401のテストケースがないので、各自タスクが振られた場合、存在しなければ書く
- 認可のテストに関しても、403のテストケースがまだ存在しないので、必要な場合は書く
- ユーザー画面の認可は、404のステータスコードが返却されているか確認する(ユーザーの存在が推測されない様にするため)

2.変数とメソッド名をわかりやすい命名に変更

コードで使用されている変数名、メソッド名を確認する。
まずはそのままメソッドが処理している内容を命名する。そのままつけた結果明らかに長くなったり、2つ以上の事を実施していたら、メソッド分割して責務を一つにする。
コレクションやarrayが返却されるなら、複数形、その他は単数系で命名する
クラス名、メンバー変数名が主語、メソッド名が動詞という関係で読む事ができるか確認する
それ以外のケースはデイリーで相談して決める

複数形の例

$users = User::all();
$user = User::first();

3.ロジックはモデルに書く

データとロジックの乖離を防ぐ様にする。該当モデルの属性を使って何か関数を定義したい場合は、そのモデルに記載する
load、withなどのメソッドを使って、リレーション先の情報を取得する記述はコントローラーに書く

3.クラスメソッド、インスタンスメソッドの使い分けをして下さい

クラスメソッド
- テーブルに対しての操作。つまりモデル全体に対して実行したい場合はクラスメソッドを使う
インスタンスメソッド
- レコード1つに対して、つまり個別のインスタンスで動的に処理を変えたい場合はインスタンスメソッドを使う

例

usersテーブルの内容をソートして取得するメソッド

class User
{
    public static function orderByName()
    {
        return self::orderBy('name')
    }
}

User::orderByName()

ユーザーが管理者か確認するメソッド


class User
{
    public function isAdmin()
    {
        return $this->role === 'admin';
    }
}

$user = User::find(1)
if ($user->isAdmin()) {
    logger->info('彼は管理者です');
}

4.リクエストに依存するものをコントローラーに書く

auth('api')->user()などのリクエストに依存する値をモデルで直接使用せずに引数で渡す
$requestオブジェクトをそのままモデルに引数で渡さない

5.バリデーションはrequestクラスに書く

基本的にリクエストの内容のバリデーションはrequestsクラスに書く
リクエストに依存しないビジネスロジックの前提条件の検証など、モデルに書いた方が良いと判断した場合は、モデルに書いても問題なし、ここは柔軟にレビューで会話をして結論を出す
https://readouble.com/laravel/8.x/ja/requests.html

6. レスポンスの定義はresourcesクラスに書く

APIのレスポンス定義は、resourcesクラスに書く
極端に簡易なレスポンス例えば属性を一つだけ返却するなどの場合は、コントローラーやapi.phpなどに記述しても良い
https://readouble.com/laravel/8.x/ja/eloquent-resources.html

7.ロジックをモデルに書いた後は、テストを書く

featureテストでのテストの観点
- リクエストの対して正しいステータスコードとレスポンスボディが返却されているか確認する
- 複数の状態で挙動が変わる事が想定される場合は、すべてのケースを書く
- DBの状態が想定される値になっているかを検証する
Unitテストでのテストの観点
- 正常系が正しく動作するか確認する
  - 返り値が存在する場合は、返り値の値を検証する
  - DBに変更を与える場合は、DBの値が正しい事を検証する
- 異常系が想定している動作をするか確認する
  - 想定している例外が正しくthrowされるか検証する
  - 独自のハンドリングでエラーメッセージが意味を持つ場合はそれも検証する
- 境界値のテストを実施する
  - 例えば、18歳以上の場合trueを返却するメソッドがある場合は
    - 18歳の場合
    - 19歳の場合
  - この場合上記の2つの値の場合の挙動を検証する

8.必要なコメントは容赦無く書く！

仕様が複雑なもの、処理が複雑なものに関しては、なぜ実施しているか説明しているコメントを書く(極力命名で表したいですが、特に複雑な点はわからないより良いので絶対に書く)
複雑なものは処理の説明を文章として書く
仕様の背景が難解、直感的ではない場合は、詳しくその仕様についてコメントしても良い

9.機能として実装が必要など発見した場合は、必ず別タスクを切る(重要)

リファクタリングのタスクとして切っているので、それ以外の事は実施する
とはいえ完全にルールで縛るとスピードが落ちるので、気軽にチームメンバーに相談する

終わりに

実際にリファクタリングで実施した内容を書いてみました
当たり前の事ばっかりやないかい！と思われた方もたくさんいると思うのですが
短時間でリファクタリングをして効果を出すという制約がある中で、今回は基本的な部分を徹底することで可読性を担保しようと思いました

またこのプロジェクトはこのリファクタリングを実施した後は大きなバグもなく順調に機能改修が進んでいます(まだ運用して10ヶ月ほどですが)

このリファクタリングの効果かどうかは分かりませんが、これからも常にコードの品質を向上させていく姿勢を持っていきたいと思った経験でした