WEBサービス開発グループ２年目の松居と、同グループ１年目の久保田です。２人共@nifty不動産の開発を担当しています。
本記事は２人の共同執筆です。
以前、社内コードレビュー実施についての記事（ニフティでもコードレビュー会をやっています）を公開しました。
今回は、実際にどのようなコードレビューが行われているか、iOSアプリ開発での例を紹介していこうと思います！
長文にはなりますが、お付き合いいただけますと幸いです！
①コミットメッセージは「なにをどのように変更したのか」を書こう
初歩的なことですが、非常に重要なことだと思います。が、1年目の私はそれすらも意識せずに雑なコミットメッセージを書いていました。
こちらです。
ひどいですね。 このコミットでは何を修正したんでしょうか？
これに対していただいたレビューがこちら。
「打ち消すことを考えた粒度」で、「伝わるコミットメッセージ」をしっかり意識してコミットメッセージを残していきましょう。
②メソッド名は「どのような処理をするか理解できる」ようにしよう
メソッド名や変数名などは人それぞれにつけ方があるかと思います。その際、そのメソッドを使う人が名前からどのような処理をするか理解できるようにすることはみなさん意識していると思います。
が、それもできていなかったのです。
|
1 2 3 4 5 6 7 8 |
private changeFormat(num: Int) -> String? { let formatter = NumberFormatter() formatter.numberStyle = NumberFormatter.Style.decimal formatter.groupingSeparator = "," formatter.groupingSize = 3 return formatter.string(from: NSNumber(value: num)) } |
メソッド名は”changeFormat”。メソッド名だけ見てどのような処理をしているのかわかりますか？
フォーマットが変わるみたいだけど、何が変わるのだろうか。となりませんか？
ここでは数字の3桁ごとに「,」を付与して返すという処理をしています。
この場合、”addSeparator”、”addThousandsSeparator”、“separatedDigits”など、桁区切りを行うことが伝わる命名がいいですよね。
③UIViewでのframe指定は”layoutSubviews()”で行おう
layout系に関する失敗事例を紹介します。
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 |
class UIView: UIView { private let searchButton: UIButton override init(frame: CGRect) { self.searchButton = UIButton(type: .custom) super.init(frame: frame) self.searchButton.frame = CGRect(x:self.bounds.size.width / 3, y:self.bounds.size.height * 1 / 10, width:(self.bounds.size.width - 10.0 * 2) * 2 / 3, height:self.bounds.size.height * 8 / 10) self.searchButton.setTitle("検索する", for: UIControlState.normal) self.addSubview(self.searchButton) } //以下略 } |
ここでは、検索ボタンのViewを作成しています。
その中で、frameをinitで指定しています。
このとき、initの時点で”self.view”のサイズは確定していることが保証されていません。
この場合、例えば画面回転を行った場合に表示崩れが起こる恐れがあります。
UIViewではlayoutSubviewsでframeを取得しましょう。（UIViewControllerではviewDidLayoutSubviews以降にframeを取得しましょう）
そのため、”layoutSubviews()”にframeに関する部分を以下のように書き出しました。
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 |
class UIView: UIView { private let searchButton: UIButton override init(frame: CGRect) { self.searchButton = UIButton(type: .custom) super.init(frame: frame) self.searchButton.setTitle("検索する", for: UIControlState.normal) self.addSubview(self.searchButton) } override func layoutSubviews() { self.searchButton.frame = CGRect(x:self.bounds.size.width / 3, y:self.bounds.size.height * 1 / 10, width:(self.bounds.size.width - 10.0 * 2) * 2 / 3, height:self.bounds.size.height * 8 / 10) } //以下略 } |
④ControllerはModelとViewの”参照”と”操作”を行う
Controllerで扱うべきでない処理をControllerで扱ってしまったという失敗事例です。
今回「検索を行い、アニメーションを使ってHITした件数を表示する」プログラムを記述しました。
このとき”アニメーションを使ってHITした件数を表示する”部分をControllerに記述していました。
本来、Controllerの役割はModelとViewの参照と操作を行う部分になります。
（参照：やはりお前らのMVCは間違っている）
今回の「検索を行い、アニメーションを使ってHITした件数を表示する」処理をMVCの役割に当てはめると、
Model：”検索ロジック”
View：”アニメーションを使ってHITした件数を表示する”
Controller：”Modelの検索ロジックの結果を参照し、Viewを操作する”
となるのが正解となります。
ですので、”アニメーションを使ってHITした件数を表示する”処理はViewに任せ、その操作をControllerが担います。
MVCを理解し、扱いにくいコードを生み出さないようにしたいものです。
⑤アクセスコントロールの使い分けをしっかり考えましょう
クラスやメソッド、変数につける public や private などのアクセスコントロールがSwift3から変更されました。
open: 別のモジュール（プロジェクト）からでもアクセスでき、別モジュールで継承またはオーバーライドができるpublic: 別のモジュールからでもアクセスできるが、別モジュールでは継承、オーバーライドはできないinternal: 同じモジュール内からのみアクセスできる。定義時に何もつけないとinternalになるfileprivate: 同じソースファイル内からのみアクセスできるprivate: クラスなどの定義した中でのみアクセスできる
例としてはこんな感じになります。
|
1 2 3 4 5 6 7 8 9 |
class Say { // privateなのでSayクラス内でしかmessageは使えない private let message = "こんにちは" // 定義なしなのでinternal。同じプロジェクト内であれば呼び出せる func hello() { print(message) } } |
変数などの宣言に何もつけないとクラス外からも操作できてしまうため、知らないうちにクラス外から変数の値が書き換わっている･･･という可能性が発生してしまいます。
クラスの外から見える必要があるかどうか？を考え、基本見える必要はないと思うものにはprivateで宣言しましょう。
⑥extension（拡張）を使ってクラスを見やすくしましょう
ニフティ不動産のアプリは、UITableViewDelegateをはじめとして、さまざまなプロトコルを一つのクラスに実装しています。
その際に一つのクラスの中にいろいろなプロトコルを実装すると、どれがどのデリゲートメソッドなんだ？と可読性が下がってしまいます。
そんなときはextensionを使いましょう！
extensionとは、既存のクラスや構造体、列挙型に新たに機能を追加することができる機能です。
継承と違って、クラスなどの名前はそのままで、メソッドやプロパティを追加できます。
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 |
class Say { func hello() { let message = "こんにちは" print(message) } } extension Say: ThankyouProtocol { func thankyou() { let message = "ありがとう" print(message) } } extension Say: SorryProtocol { func sorry() { let message = "ごめんなさい" print(message) } } |
extensionを使ってプロトコルごとに分けて実装すると、機能ごとにまとまっているので可読性が上がります。
⑦エラーを握りつぶさないこと
Swiftにはオプショナル型というものがあり、nil（値が無い）状態を保持できる変数を宣言することができます。
また、if let 文や guard let 文を使って、変数にnilが入っているかを判定し、エラー処理をすることができます。
この処理を入れれば、変数にnilが入ってアプリがクラッシュするということがなくなるため、安全なコードを書くことができます！
|
1 2 3 4 5 6 7 8 9 |
func openBrowser(urlString: String) { guard let url = URL(string: urlString) else { // URLを生成できなかったときはここでメソッドから抜ける return } // 正しく生成できたURLでブラウザを開く処理 // ・・・・・・・ } |
しかし、「とりあえずguard letでnilを避けよう」と安易にこの処理を使いすぎてしまうと良くないこともあると指摘をもらったことがあります。
想定外のエラーが発生しているにもかかわらずアプリがクラッシュしないという状態になってしまうので、バグを握りつぶしてしまい、原因の特定が遅くなってしまうという可能性が発生します。
クラッシュを発生させる方法は以下の方法があります。
assert：デバッグビルド時のみ実行される。実装の確認やデバッグ時に役立つprecondition：デバッグビルド・リリースビルド時で実行される。事前条件を設定し、それに合わない条件だった場合にクラッシュさせるfatalError：デバッグビルド・リリースビルド時で実行される。致命的エラーでこれ以上実行できない場合に使う
想定外のエラーは勇気を持ってわざとクラッシュさせて、クラッシュのログを貯めるようにしましょう！
いかがでしたでしょうか。
基礎中の基礎から、少し難しいエラーの扱いなど、WEBサービス開発グループのメンバー間で盛んにコードレビューが行われています。
機会があれば、他のコードレビューも紹介できればと思います！
(^ v ^)/ < Let’s code review !!
