Payload Logo

コードレビュー

Date Published

参考URL

効果的なコードレビューの実践

https://zenn.dev/superstudio/articles/9975ba58a51384


コードレビューのイロハをまとめました

https://qiita.com/hariNEzuMI928/items/5e3d96a069e075e37720


リーダブルコードの要点整理と活用法をまとめた

https://qiita.com/KNR109/items/3b14e2e8f89a33c0f959


コードレビュー観点表を作った話

https://zenn.dev/castingone_dev/articles/981e1ae03a70e5



観点


・可読性

・保守性

・堅牢性

・正しさ

・セキュリティ

・パフォーマンス



Linterの警告対応



原則

YAGNI(You Ain't Gonna Need It)

今必要のない機能や汎用性を先回りして実装しない


KISS(Keep It Simple, Stupid)

誰が見ても一瞬で理解できる最も単純な構造にする


SLAP(Single Level of Abstraction Principle)

抽象化レベルの統一

1つの関数の中に、「高レベルなビジネスロジック」と「低レベルな細かいデータ処理」を混ぜて書かない


SOLID原則

S:単一責任の原則 (Single Responsibility Principle)

1つのクラス(関数)は、1つの役割だけを持たせる

Userクラスが、ユーザー情報の管理だけでなく「CSV出力」や「データベース保存」の処理まで持っている状態。


O:開放閉鎖の原則 (Open-Closed Principle)

機能追加にはオープン(開いている)、既存コードの修正にはクローズ(閉じている)

新しい機能を追加するときに、既存のソースコードを書き換えるのではなく、「新しくコードを追加するだけ」で済むようにする(インターフェースなどを活用する)

→新しい支払い方法(PayPayなど)を増やすたびに、既存の決済関数の if/switch 文を書き換えている状態。

→今後も支払い方法が増えそうですね。if文を増やすのではなく、決済用の共通インターフェースを作って拡張できるようにしませんか?


L:リスコフの置換原則 (Liskov Substitution Principle)

親クラスは、いつでも子クラスに置き換え可能でなければならない

継承(extends)を使うときのルールです。親クラスができることは、子クラスでも「全く同じ期待通りの挙動」をしなければなりません。

→「鳥」という親クラスに fly() メソッドがあるのに、飛べない「ペンギン」クラスがそれを継承し、fly() の中で「エラーを投げる」ように実装されている状態

→ペンギンクラスを鳥として扱うとバグになります。継承関係が間違っているか、共通メソッドの定義が不適切です


I:インターフェース分離の原則 (Interface Segregation Principle)

使わないメソッドを強制(依存)させるな

大きすぎるインターフェース(仕様の詰め合わせ)を作らず、必要最小限のサイズに細かく分割します。

→「多機能プリンター」という大きなインターフェースのせいで、ただの「印刷機」クラスを作りたいだけなのに、使わない「FAX送信」「スキャン」といったメソッドまで実装を強制されている状態。

→このクラスではFAX機能は使いません。インターフェースを『印刷用』と『FAX用』に切り離しましょう


D:依存性逆転の原則 (Dependency Inversion Principle)

具体的なもの(詳細)ではなく、抽象的なもの(ルール)に依存せよ

上位のビジネスロジックが、下位の具体的なツール(特定のデータベースや外部APIなど)に直接依存しないようにします。間にインターフェース(抽象)を挟むことで、ツールをいつでも差し替え可能にします。



POINT

現場のレビューでSOLID原則を出すときは、「今のコードだと、将来〇〇という変更が入ったときに、関係ない場所まで巻き添えで修正することになってバグを生みますよ」という文脈でアドバイスする