概要
自分のチームはiOS開発をGitHub Flowで行っていて、各自がPRを作成し日々コードレビューをしている。
しかし最近業務量が増えてきたためレビューも効率化できないかと考えるようになった。
そこで今までのレビュー内容を振り返った時、レビューで出てくる修正点のうち一部は次のような特徴を持つことに気づいた。
- 高頻度で現れる
- テンプレ的に修正できる
- 動作確認の前で直せる
こういった修正を毎回のレビューで見つけて修正依頼するのは 非常に開発効率が悪いことに気づいた 
できれば上記のようなケースは、極力自動(SwiftLintやテスト等の仕組み)で検知・修正されるのが理想である 
ただ自分のチームでは、自動的にレビューできる仕組みが整っていなかった。
またコーディング規約等によっては、直接の自動化が難しいような項目もある。
そこで、ひとまずレビュイーがレビューを受ける前にセルフチェックできるようリストに起こしてみることにした 
事前にレビューを出す側がセルフレビューすることで、レビューする側の負荷が下がり、ひいては開発効率の向上につながるのではないかと思う 
以下のリストは汎用的で他のチームでも応用できそうな項目を挙げてみたので、チームごとのルール(「変数が命名規則に則っているか」とか)に絡むようなものや見解が分かれそうな内容(「selfをつける」とか)は載せていない。
コードレビューを受ける前のセルフチェック項目
不要になったView Elementは消す
新規のView(Controller)作成では、試行錯誤的にView Element(UIViewだったりUILabelだったり)を配置してレイアウトを調整することがあるが、最終的には不要なView Elementは削除しておく 
理由
複雑なViewであればあるほど、Interface Builder上では各View Elementの役割が見えづらくなる。
従って必要な要素以外はすべて消すのは当然のこと、AutoLayoutで制約が付いている場合は制約の依存関係も考慮しなければならず、1つのViewを消すのにも作業負荷が増大する(制約のエラーも依存や原因が見えにくい)。
不要なViewに紐付いた不要な制約は、後々の開発での画面変更を難しくする負債にもなるので、必要なもの以外は残さないのがベスト
不要になったIBOutletやIBActionは消す
SwiftファイルにIBOutletやIBActionで紐付けされているものの、コードで操作が不要なView Elementは消す
理由
Viewに限らず変数が多いほどコードの状態を把握するのが難しくなり、ロジックを追うのが難しくなる。
さらに紐づけされたViewは 必ず表示の状態を持っている。
つまり、コードレビュー時にレビュアーは表示の状態を想像しながらロジックを考えなければならない。
よって無駄なViewの状態を考えさせることは、レビュアーの負担を無駄に増やすことになるので予め消しておく
3. UIView/UIViewControllerのライフサイクル上適切なタイミングに処理を書く
初期化(及び破棄)時に必要となるデータフェッチや表示初期化などの処理は、適切なタイミングで呼ぶ
理由
UIViewControllerにはタイミングに応じて呼ばれるライフサイクルメソッドがある。
自分のチームで特によく使用するのは以下の4つ。
-
viewDidLoad内 : ViewController初期化時に1度だけやれば良い処理- delegateの設定
- ボタン等へのアクションの設定
- privateなプロパティの初期化
-
viewWillAppear内 : ViewControllerは破棄されないが非表示と再表示のタイミングで呼び出さねばならない処理- viewの初期化
- DataSourceの初期化
-
viewDidLayoutSubviews内 : 描画時の計算でサイズ決定が必要な処理- 角丸の設定
- グラデーションレイヤーの追加
- マスキング
-
viewDidAppear: 表示直後にユーザーに見える効果- 遷移直後に走るアニメーション
コードのイメージはこんな感じ。
override func viewDidLoad() {
super.viewDidLoad()
// delegateの設定
self.collectionView.delegate = self
// ViewやButtoｎへのアクション追加
let tapTargetView = UITapGestureRecognizer(target: self,
action: #selector(AnotherViewController.didTapTargetView))
self.view.addGestureRecognizer(tapTargetView)
}
override func viewWillAppear() {
super.viewWillAppear()
// 情報更新があるかの確認
self.presenter.checkUpdate()
}
override func viewDidLayoutSubviews() {
super.viewDidLayoutSubviews()
// 高さに依存する角丸処理
self.roundView.layer.cornerRadius = self.roundView.frame.height() / 2.0
}
override func viewDidAppear(_ animated: Bool) {
super.viewDidAppear(animated)
// アニメーション
self.animateSomething()
}
例えば、本来1度しか呼ばなくて良い処理をviewDidLayoutSubviews内で複数回呼んでしまったとする。
すると、単にパフォーマンスに影響があるだけではなく、状態の更新タイミングが保証されず非同期処理などと絡んでバグの温床になる。
逆にContainerViewControllerを使った場合等で インスタンスが破棄されない場合は、viewDidLoadではなくviewWillAppearに処理を書く必要がある。
よって、ライフサイクルを理解して実装するということは、パフォーマンスを最適化しかつ予期せぬ挙動を防ぐことと同義である。
Viewの初期化処理等を Storyboard と Swiftコード のどちらかに寄せる
チームのポリシーによって寄せる対象は変わるものの、基本的には処理をどちらかに寄せる
理由
これはロジックと状態が分散するのを防ぐためである。
Viewの状態を管理する場所が分散すると、レビューやデバッグ時に原因の特定が難しくなることは自明である。
逆に処理を一箇所に寄せることで、レビューで集中して見るべき場所を集約可能である。
なお自分のチームでは、静的なViewの設定はStoryboardに寄せ、動的に変更されるプロパティ(グラデーション、角丸の描画)等でのやむを得ない場合のみSwiftに書くのがルールとなっている。
delegateメソッドはprotocol extensionを使い記述場所を分離する
protocolを実装する場合はprotocol extensionを使いブロックを分けて記述する。
理由
UICollectionViewやUITableViewなどのprotocolを実装する場合に
class MyViewController: UIViewController, UITableViewDataSource, UITableViewDelegate {
// methods...
}
のように UIViewController を直にextendしてしまうと、一つのclassブロックに沢山のメソッドがあるのは可読性が下がってしまう。
またdelegate間で似たような名前のメソッドが定義されていることがあり、新たにメソッド追加する際に補完で出るメソッドが多くなってしまう。
例えばUITableViewDelegateにもUITableViewDataSourceにも tableView という名前でたくさんのメソッドがオーバーロードされている。
特にDelegateメソッドは、役割ごとにブロックに分けるだけで、レビューしたい対象がどこにあるか分かりやすくなり集中して見ることができる(「UITableView周りを見たいならUITableViewDelegateのブロックを見る」とか)。
例えば上述のUITableViewDelegate及びUITableViewDataSourceは、protocol extensionで次のように記述場所を分離するのが良い
class MyViewController: UIViewController {
// methods of UIViewController...
}
extension MyViewController: UITableViewDataSource {
// methods of UITableViewDataSource...
}
extension MyViewController: UITableViewDelegate {
// methods of UITableViewDelegate...
}