概要
「自信が無いエンジニアこそコードレビューをするべき」なんて偉そうな記事を書いてから
半年が過ぎた今日この頃、コードレビューやリリースを繰り返し、これはレビュー時に確認しておいた方が良いな、と思った項目をまとめます。
こういうポイントも見たほうがいい！などあれば是非コメントでご意見頂きたいです！
レビューポイントは自分の中で大まかに分けて以下の2つにあります。
- クラッシュ、不具合系
- リファクタリング系
この枠に沿って、ポイントをまとめていきたいと思います。
コードレビュー時に確認しているポイントまとめ
クラッシュ、不具合系
- 外部からのAPIを使う時にレスポンス内容がおかしいときを考慮してハンドリング出来ているか？nilが許容出来ない作りになっていないか？
- 配列操作で配列の操作外を指すようなコードはないか？
- OSVersionの違いで落ちたり、画面が崩れるようなコードは無いか？
- バックグラウンドから復帰した時に期待する動作になるか？
- UserDefalutsに新しい値を追加しているとき、アップデートユーザーも期待する動作をするようになっているか？
- CoreDataなどのDBに新しくカラムを追加したときにアップデートユーザーも期待する動作をするようになっているか？
- 通信状況が途中で悪くなったときに画面が固まるような設計になっていないか？
- UITableViewでCellを使い回すときに、値やViewがしっかりリフレッシュされるようになっているか？値が無いときにそのハンドリングがされているか？
- 既存の処理を消している箇所がある場合、代用のコード、あるいは消した理由が明確に分かるようになっているか？
リファクタリング系
- 何回も重複して書かれている処理がないか？
- 循環参照していそうな箇所は無いか？クラスでdelegateを持っているときや、closureの中でSelfのプロパティをキャプチャしているときに大丈夫か？
- 今後の拡張性を考えた作りになっているか？使い回しが効かないような引数や設計になっていないか？
- クラスごとの責任範囲が明確になっているか？決められたフレームワークに沿ってかかれているか？
- 既存クラスでよく使うような関数などはプロトコルに切り出して綺麗にかけるようにならないか？
- クラスに役割がわからないような名前をつけていないか？
- いらないコメントやTODOが残っていないか？
- コーディング規約に沿ってコードが書かれているか？
- Storyboardでwarningが出ていないか？
- 変数や関数のアクセス修飾子が適切につけられているか？
- 何度も使っているのに定数化していないような文字列や数字はないか？
- viewDidLoadなどのdelegateの中身が肥大化していないか？細かく関数に分けられないか？
- if文などのネストが深くなりすぎていないか？早期return出来ないか？guard文がかけないか？
- xxな状態、などの条件をswitch文で分岐している時に、xxしている状態などをenumなどで表せないか？
- 強制アンラップしている箇所はないか？guard文などを使って綺麗に書けないか？
それぞれ軽く解説
クラッシュ、不具合系
1. 外部からのAPIを使う時にレスポンス内容がおかしいときを考慮してハンドリング出来ているか？nilが許容出来ない作りになっていないか？
APIの仕様を変えたらいきなりクラッシュが増えた、なんてことよくありますよね。
APIのレスポンスが追加されたときや、あるいは減ったとき、レスポンスのkey-valueの形が変わったときに、
アプリ側でしっかり吸収出来ていないと落ちてしまいます。
APIのレスポンスを受け取るときに、強制アンラップなどをしている箇所があれば要注意です。
2. 配列操作で配列の操作外を指すようなコードはないか？
Array index out of range.
というやつですね。
これもAPIの内容やUITableViewCellを生成するときなど、配列の内容をindexで操作するときに特に気をつけなければいけません。
以下のようなextensionもあるので、こういったものを使うととても安心出来るのでぜひ活用すると良いかと思います。
swiftでArrayの範囲外をよんだときにエラーにならずnilを返すextension
3. OSVersionの違いで落ちたり、画面が崩れるようなコードは無いか？
特にiOS7をサポートしている場合、特別に処理をする必要がある場合がよくあります。
Pushの許諾周りもそうですし、UIViewControllerの背景を透過させるときだったり…etc
レビューする側もiOS7の動作はしっかりと理解し、PRを投げた側にはiOS7でもテストをしっかりしておかないと泣きをみることがあります。
4. バックグラウンドから復帰した時に期待する動作になるか？
Alert系やTouchIDなど、通常のViewとは少し違うものを表示しているときにバッググラウンドにいったり、
通信中にバックグラウンドに行き復帰したときに期待する動作になるかは確認した方が良いです。
5. UserDefalutsに新しい値を追加しているとき、アップデートユーザーも期待する動作をするようになっているか？
アップデートユーザーの値はxxである、という処理が書かれていないと、新規ユーザーもアップデートユーザーも
同じ動きをしてしまうようなコードになってしまいます。
新規ユーザーだけこうしたい、のような動きを期待する場合、しっかりと確認する必要が有ります。
6. CoreDataなどのDBに対して変更を加えた時にアップデートユーザーも期待する動作をするようになっているか？
5.と少し似ていますが、DB系は値が参照出来ないとクラッシュしてしまうので、内容を変える場合
しっかりとアップデートユーザーに対してマイグレーションをする必要があります。
これでアップデートユーザーが全て落ちるという最悪の事態に陥ることにもなるので、DB系は特に慎重に見る必要があります。
7. 通信状況が途中で悪くなったときに画面が固まるような設計になっていないか？
通信エラーの際のハンドリングやユーザーに対しての通知(通信出来ませんでした)などがしっかり書かれているかチェックする必要が有ります。
これもあるあるですね。
8. UITableViewでCellを使い回すときに、値やViewがしっかりリフレッシュされるようになっているか？値が無いときにそのハンドリングがされているか？
UITableViewCellを使い回している時に、非同期で画像を取得してはめ込む、といった処理がある場合、
通信中、あるいはCellが使いまわせる時に一度nilを差し込んだり、リフレッシュをかけないとそのまま前の画像が出てしまったりする不具合が発生する可能性があります。
cellのプロパティに対して値を代入している場合、使いまわされる時に問題ないかはしっかりチェックしましょう。
9. 既存の処理を消している箇所がある場合、代用のコード、あるいは消した理由が明確に分かるようになっているか？
どういうときに、というのではなく幅広いのであれなのですが、既存の処理が消えている時は結構怖いと自分では思っています。
消してあるけど大丈夫そう、自分ではいらないなと思ったコードが実は無いと、こういう条件の時に限って落ちてしまうとか実際よくありました。
何故この処理を消して大丈夫だったか、OS違いやライフサイクルの都合上だったのか、不安なら必ずコメントで聞くようにしています。
リファクタリング系
1. 何回も重複して書かれている処理がないか？
関数化出来ないか、継承出来ないか、プロトコルにかけないか考えます。
2. 循環参照していそうな箇所は無いか？クラスでdelegateを持っているときや、closureの中でSelfのプロパティをキャプチャしているときに大丈夫か？
weakを適切に付けれていない場合をチェックします。
3. 今後の拡張性を考えた作りになっているか？使い回しが効かないような引数や設計になっていないか？
今回だけしか使わないようなクラスであれば良いのですが、今後も使い回すだろうと想定できるものに対しては、
なるべく拡張性を持たせておくのが良いと思います。
ただ、例えば関数に対してこの引数は使いそうだ、のように使ってもいないのに足す、のような考えではなく、
特定のクラスから呼び出すときにしか使えないように、呼び出される側のクラスが呼び出し元に依存しすぎているような
処理になっているときは、うまくClosureを使って使う側が処理をかけないか、などを考えるような感じですかね。
4. クラスごとの責任範囲が明確になっているか？決められたフレームワークに沿ってかかれているか？
MVC、MVVMなどプロジェクトごとにフレームワークが決まっていると思いますので、それに沿ったクラスの責任範囲が振られているかです。
ViewControllerが肥大化しすぎていないか、ViewがModelを操作してしまっていないか、などなど…
5. 既存クラスでよく使うような関数などはプロトコルやextensionに切り出して綺麗にかけるようにならないか？
Storyboardをコードから生成したり、xibファイルの登録など毎回毎回書くようなものは
もうextensionなどにまとめてしまいましょう。
以下の記事はとても参考になるのでぜひ！(会社の後輩が書いた素晴らしい記事です！!)
使うと手放せなくなるSwift Extension集
6. クラス名や関数名に役割がわからないような名前をつけていないか？
命名規則的なところですね。神クラス的な名前になっていないかなどなど。
7. いらないコメントやTODOが残っていないか？
デフォルトでついてしまっているコメントや、コードと同じ意味のコメントは消してしまいましょう。
TODOなどはswiftLintでもチェック出来るので導入をおすすめします！
8. コーディング規約に沿ってコードが書かれているか？
こちらもswiftLintである程度吸収できるのでそれで自己チェックできるようになっていると幸せですね！
9. Storyboardでwarningが出ていないか？
Storyboardの差分は正直見るのしんどいですが、最低限「misspect」がないかだけは確認します。
これは制約でwaringがでていないかをチェックできます。
10. 変数や関数のアクセス修飾子が適切につけられているか？
何が他のクラスから触れて何が触れないのかはしっかりとつけましょう。
このあたりは基本中の基本ですが、しっかり出来ていないと一気に保守性も可読性も下がるのでしっかりやるべきだと思います。
11. 何度も使っているのに定数化していないような文字列や数字はないか？
structやenumの出番です。マジックナンバーは極力減らしましょう。保守性も高くなります。
12. viewDidLoadなどのdelegateの中身が肥大化していないか？細かく関数に分けられないか？
だらだらといろんな初期化処理を書いてしまうと一気にコードの見通しが悪くなってしまいます。
-(void)viewDidLoad {
[super viewDidLoad]
[self p_setupTutorial];
[self p_setup...];
[self p_hogehoge];
...
}
このように、役割ごとに関数を分けるととても見やすいですし、バグが起きたときにもみつけやすくなります。
13. if文などのネストが深くなりすぎていないか？早期return出来ないか？guard文がかけないか？
循環的複雑度の部分ですね。
if elseが長いとほんと追うのが辛いので、なるべくgurad文などをうまく使いこなしたほうが個人的にはいいと思います。
14. xxな状態、などの条件をswitch文で分岐している時に、xxしている状態などをenumなどで表せないか？
switch + enumは網羅性があるのでとても良いです。
詳しくは以下の記事を参考にしてみてください！
Swiftの列挙型、switch文、網羅性チェックが素晴らしい！
15. 強制アンラップしている箇所はないか？guard文などを使って綺麗に書けないか？
強制アンラップしたくなるときもありますが、なるべく減らしたほうが変更に強いコードになると思います。
swiftLinstだと強制アンラップを使っているとエラーを吐くようなものがあるのでそれでチェックするのもありです。
終わりに
色々書いたのですが、PJによってどこまで修正するかや何をマストで見るべきかはしっかりと決めるべきだと思います。
全部が全部見ていると実業務だとやはり時間が足りなくて直せない部分も出てくると思うので、、
直すなら直すでやりきる、直さないなら直さないでいつまでにやるのか、はたまたやらないのかも目処を立てて、
日々コードを改善するサイクルをまわしていけるような体制にすると、いわゆる技術的負債が少ないいいコードになると思います。
また、コードレビューはいかに想定外を想定出来るかが大事だと思っていまして、こういうときにこうなるよね、の知見を増やすことがレビュアーのレベルをアップさせることだと実感しております。
そこで、冒頭にも書いたのですが、是非自分はこんなところよく注意しているよ！といった部分があれば是非コメントに書いて頂けると嬉しいです！