比較的大きめな規模のiOSアプリのコードレビュー をする機会がよくあるため、 どういった点に注意を向けてレビューしているかまとめておこうと思う。 以下は主にGitHubのPull Requestベースでレビューをする場合を想定している。
非コード編
1. 該当のissueがリンクされているか
Pull Requestを出した背景となるissueがあればそのリンクがDescription欄に貼られているか
2. 変更点が簡潔に書かれているか
コードを見ればいいのだけど、変更した内容を簡潔に書かれているとコードレビュー する前にある程度把握することができるので書きましょう
3. スクリーンショットが添付されているか
UIの変更を伴う修正の場合はスクリーンショットを付けた方が良い。 スクリーンショットに関してはいくつか見る箇所があって、
- レイアウトが崩れていないか
- 該当のissueの要件になっているか
4. CIが通っているか
CIが導入されているのであれば、CIのステータスがGreenになっているか
コード編
5. 新しくファイルを追加する場合、適切なGroupに配置されているか
比較的規模の大きいアプリであれば、暗黙的なGroup分けの仕方が存在していると思うので、適切なGroupに配置するか、新しくGroupを切りましょう
6. 目的の修正と関係ないファイルが変更されていないか
よくあるのが、CocoaPodsの Podfile.lcok がなぜか更新されていたり、
関係ないStoryboardで差分が出てしまってコミットされているケース。
後から変更を履歴を追いたい時に不要な変更があると原因の特定が難しくなったりするため、そもそも意図しない変更以外はコミットしない方が良い。 使っているXcodeのバージョンがチームで違っていたりすると、Storyboardを開いただけでdiffが発生したりするので注意。
7. ファイル名、クラス名が適切か
そのファイル名やクラス名を見ただけで役割が推測できたら良い。 できなかったら不適切な名前を付けている可能性が高い。
8. インデントが崩れていないか
プロジェクトが採用しているインデントのルールに沿って改行やインデントされているか。
9. 変数名が適切か
その変数の型と役割を表す名前になっているか。
let image: UIImageView
例えば上の例だと、宣言している箇所ではまあわかるが、他の箇所で image が参照されていると、 UIImage の変数なのかと思えてしまう。
let imageView: UIImageView
とした方が理解しやすい。
10. 循環参照していないか
特にクロージャーを使っている箇所で self が循環参照していないか。
11. 小さい画面の端末が考慮されているか
iPhone SEなど解像度が低い端末で実行した時に崩れたり、意図しない表示にならないか。
12. Forced Unwrapping（強制Unwrap）を使っていないか
実行時にクラッシュしてしまうためOptionalを使いましょう。
13. ライブラリを導入する際は、導入目的が明確か、メンテナンスされているか
UIKitで目的が達成できるのであればライブラリは使う必要はないと考えています。 また似たようなライブラリがあった際は、なぜそのライブラリである必要か説明があった方がいいです。
14. Dark Themeを導入していれば、Themeの切り替えで意図しない表示になっていないか
Storyboard上で容易に切り替えて確認できるので確認しましょう
15. print文などをDEBUGフラグ確認なしに使って、不必要なログを出力していないか
APIのレスポンスなど不要なログは出力しないほうが懸命です。 SwiftyBeaverなどのライブラリを使うのも手です。
まだまだレビュー観点はたくさんあるんですが、 他にもこういう観点でコードレビュー しているなどあれば、コメントもらえると嬉しいです。
![Google Pixelbook (i5, 8 GB RAM, 256GB) [並行輸入]](https://images-fe.ssl-images-amazon.com/images/I/51TxM4dNYhL._SL160_.jpg "Google Pixelbook (i5, 8 GB RAM, 256GB) [並行輸入]")
