新卒iOSエンジニアがコードレビューを受けて気づいたポイント4選

はじめに

DeNA 20 新卒 Advent Calendar 2020の21日目の記事です。
こんにちは、@yayamochiです。業務ではiOSアプリの開発を、プライベートではOS問わずアプリ開発を行っています。
私は入社前に長期のインターンを体験した経験がなく、アプリ開発の知識は独学で学んだものしかありませんでした。
この記事では、業務でのチーム開発未経験の私がコードレビューを受けて、開発する上で気をつけておきたいと感じたiOSのコーディングのポイントをある例をもとに共有したいと思います。

対象読者📖

• チームでの開発をし始めた方
• iOSアプリの開発者

例として扱う仕様 : 生年月日の登録🗓

今回はアプリ上での生年月日の登録をサンプルとして、コーディングのポイントを振り返っていきます。
仕様は以下の通りです。

仕様1. アプリ上で生年月日を入力させる
仕様2. 入力後、OKボタンを押した後に生年月日が有効な日付(1900年~現在の年まで)でなければバリデーションエラーのアラートを表示
仕様3. バリデーションが通っていれば、サーバーに生年月日の登録のリクエストを送る
仕様4. リクエストが成功したら登録完了のダイアログを表示、エラーであればリクエスト失敗のアラートを表示
仕様5. 年齢が20歳未満であれば、未成年向けの登録完了ダイアログを表示

1. 言語の機能を活用し、バグが起こりづらい記述をする🚫

仕様2の生年月日が有効な日付かどうかのバリデーションについてみていきます。

今回の条件はif文を用いると以下のように記述できます。

let calendar = Calendar(identifier: .gregorian)
// 確実に存在する日付なので強制アンラップ
let startDate = calendar.date(from: DateComponents(year: 1900, month: 1, day: 1))! 
let endDate = Date()
let validationTerm = startDate...endDate

// birthdayはアプリのDataPicker上で入力した生年月日    
if !validationTerm.contains(birthday) {
    showValidationErrorAlert()
    return
}
// バリデーション後の処理

ここでよく起こるのが、returnを書き忘れてしまい、条件を満たしていないのに処理が進んでしまうパターンです。

一方Swiftのguard文を使うと次のように記述できます。

guard validationTerm.contains(birthday) else {
    showValidationErrorAlert()
    return // returnを忘れるとコンパイルエラーになる
}
// バリデーション後の処理

guard文とは、条件を満たしていなければスコープを抜ける という処理が簡潔に書くことのできる構文です。
guard文を使うことでreturnの書き忘れを防ぐことができます。
そしてif文よりも、「この条件を満たすことを強制しているよ」という意図が受け取りやすくなります。

この他の例としては、「複数の条件によって処理を分ける際に、enum型を定義し、switch文を用いて、条件分岐の漏れの可能性をなくす」などといったものが挙げられると思います。
言語仕様を完璧に覚えておく必要はありませんが、このように言語の機能を活用することでバグを起こしにくく、よりわかりやすくする意識は重要です。

2. 名前が本当にその処理を表しているのかを考える🤔

では次に、仕様3~5の生年月日登録の一連のリクエストの処理について見ていきます。

以下のregisterBirthdayは文字通り生年月日を登録する処理です。

// OKボタンを押下した際に生年月日登録のリクエストを送る
func didTapOKButton() {
    // バリデーション処理を行う

    // birthdayはアプリのDataPicker上で入力した生年月日    
    registerBirthday(birthday)
}

func registerBirthday(_ birthday: Date) {
    // WebAPIはリクエストを送るためのクラス
    WebAPI.request(url, method: .post, parameters: ["birthday": birthday]).response { [weak self] response in
        switch response.result {
        case .success(_):
           if isAdult(birthday) {
               self?.showRequestCompleteDialog()
           } else {
               self?.showRequestCompleteDialogForUnderage()
           }
        case .failure(error):
            self?.showRequestErrorAlert(error)
        }
    }
}

// 実際はレスポンスで判定すべきだが、今回は説明の都合上導入
func isAdult(_ birthay: Date) -> Bool {
    return age(from: birthday) >= 20
}

func age(from birthday: Date) -> Int {
    let calendar = Calendar(identifier: .gregorian)
    return calendar.dateComponents([.year], from: birthday, to: Date()).year!
}

// viewの描画処理
func showRequestCompleteDialog() {}
func showRequestCompleteDialogForUnderage() {}
func showRequestErrorAlert(_ error: Error) {}

コードを読んでみると、エンドポイントにリクエストを送るだけではなく、メソッド名からは読み取れないUIの動作(未成年かどうかを判定し、アラートを表示しわける)が実行されています。
これでは他のチームメンバーがこの処理を使いたい場合に予想外の処理が実行されてしまいバグの原因になってしまったり、テストができなくなるといったことが考えられます。

このメソッドの中身を正しく名付けるならどうなるでしょうか？
直訳するならばregisterBirthdayAndShowDialogといった長い名前になってしまいます。
このメソッドには複数の処理が入り混じっていて、分離したほうが良いという判断がつきそうです。

改善の一例として、以下のように処理を分割してみました。

// OKボタンを押下した際に生年月日登録のリクエストを送る
func didTapOKButton() {
    // バリデーション処理を行う

    // UIの処理はcompletionを外部から渡すことで責務を分離する
    registerBirthday(birthday) { [weak self] result in
        switch result {
        case .success(_):
           if isAdult(birthday) {
               self?.showRequestCompleteDialog()
           } else {
               self?.showRequestCompleteDialogForUnderage()
           }
        case .failure(error):
           self?.showRequestErrorAlert(error)
        }
    }
}

// リクエストを送るだけの処理
func registerBirthday(_ birthday: Date, completion: @escaping(Result<Data, Error> -> Void)) {
    WebAPI.request(url, method: .post, parameters: ["birthday": birthday]).response { [weak self] response in
        switch response.result {
        case .success(_):
           completion(.success(response.result.data))
        case .failure(_):
           completion(.failure(error))
        }
    }
}

func isAdult(_ birthay: Date) -> Bool {
    return age(from: birthday) >= 20
}

func age(from birthday: Date) -> Int {
    let calendar = Calendar(identifier: .gregorian)
    return calendar.dateComponents([.year], from: birthday, to: Date()).year!
}

// viewの描画処理
func showRequestCompleteDialog() {}
func showRequestCompleteDialogForUnderage() {}
func showRequestErrorAlert(_ error: Error) {}

まとまっていた処理を、通信部分とViewの操作に分離することができました。

命名に関しては以下の記事が参考になると思うのでぜひ読んでみて下さい。
よりよいネーミングを目指して

3. 依存性を外部から注入する👶

仕様5の年齢の計算処理について見ていきます。

以下のメソッドでは引数を受け取り、誕生日から今日の日付までの年数を計算しています。

let calendar = Calendar(identifier: .gregorian)

func age(from birthday: Date) -> Int {
    return calendar.dateComponents([.year], from: birthday, to: Date()).year!
}

let birthday = calendar.date(from: DateComponents(year: 2000, month: 12, day: 21))!
print(age(from: birthday)) // 20

正しく計算はできているし、一見良さそうです。
ではここで、年齢のテストを書いてみましょう。

let calendar = Calendar(identifier: .gregorian)
let birthday = calendar.date(from: DateComponents(year: 2000, month: 12, day: 21))!
expect(age(from: birthday)).to(equal(20))

本日2020年12月21日に実行すればこのテストは通りますが、2021年12月21日以降に実行すると年齢が21歳という結果になり、このテストは通りません。
比較対象の今日の日付を関数の内部に依存させてしまっているのが原因です。

ではどうすれば良かったのでしょうか？

let calendar = Calendar(identifier: .gregorian)

// 今日の日付を外部から注入する形に変更。デフォルトの引数には現在の時刻を入れる。
func age(from birthday: Date, at now: Date = Date()) -> Int {
    return calendar.dateComponents([.year], from: birthday, to: now).year!
}

let birthday = calendar.date(from: DateComponents(year: 2000, month: 12, day: 21))!
print(age(from: birthday)) // 20

外部から今日の日付を注入できるように修正しました。
テストの際はat:引数にダミーの日付を入れることで、いつでも正しく検証することが可能になりました。

let calendar = Calendar(identifier: .gregorian)
let birthday = calendar.date(from: DateComponents(year: 2000, month: 12, day: 21))!
let now = calendar.date(from: DateComponents(year: 2020, month: 12, day: 21))!
expect(age(from: birthday, at: now)).to(equal(20))

私は個人のアプリでテストを書く習慣がほぼなく、この実装をテストできるか？という観点でコードを振り返ることがありませんでした。
内部に処理を依存させないことで誰でも扱いやすいメソッドに変更することができました。
今回の現在の時刻が関わる年齢計算の例だけではなく、本番のデータの代わりにモックを外部から注入する場面などでも使うかと思うので、ぜひ意識してみて下さい。

以下参考サイトです
Swiftにおける現実的なモック

4. 命名に困ったらAppleの方針を参考に🍎

例えば先ほど説明した年齢計算のためのageメソッドは以前の私であれば以下のように記述していたと思います。

func age(birthday: Date) -> Int {
    let calendar = Calendar(identifier: .gregorian)
    return calendar.dateComponents([.year], from: birthday, to: Date()).year!
}

もちろんこれでも問題なく読めますが、API Design Guidelineに従うとよりわかりやすく改善できそうです。
この記事にはStrive for Fluent Usageという項目があり、次のように書かれています。

Prefer method and function names that make use sites form grammatical English phrases.

つまり純粋な英語としても解釈できるように命名しましょうということです。
3. での改善では

func age(from birthday: Date, at now: Date = Date())

と表現しており、引数ラベルを活用することで、誕生日から現在時点での年齢を計算しているという内容がより読み取れるような命名になったと思います。

また、メソッド名の方針を決める際にもこの方法は役立ちそうです。
例えばボタンをタップしたときの処理をdelegateで実装する場合、以下のようなパターンが考えられそうです。

func didTapOKButton() // メソッド名が動作
func didTapOKButton(viewController: XXViewController) //メソッド名は動作 + 引数にView
func xxViewControllerDidTapOKButton() // メソッド名がView名 + 動作

ここでCoding Guidelines for Cocoaを読んでみるとDelegate Methodsの項目にて以下のような記述がされています。

Use “did” or “will” for methods that are invoked to notify the delegate that something has happened or is about to happen.

- (void)browserDidScroll:(NSBrowser *)sender;
- (NSUndoManager *)windowWillReturnUndoManager:(NSWindow *)window;

このような例に従うと、「View名+動作」といった命名にするといった方針が立てられそうです。(あくまで自分が命名に悩んだ際に参考にするもので、他の案が悪いという意味ではありません。)

まとめ

この記事では、多くのコードレビューを受け、意識すべきと思った点を4点まとめました。
このような内容を本や記事で何度も読み、「そりゃそうでしょ！」と思ってはいたものの、自分の実装を振り返ってみると、意外とできていないことに気づけました。

この記事を読んで「面白かった」「学びがあった」と思っていただけた方、よろしければ Twitter や facebook、はてなブックマークにてコメントをお願いします！
また DeNA 公式 Twitter アカウント @DeNAxTech では、 Blog記事だけでなく色々な勉強会での登壇資料も発信してます。ぜひフォローして下さい！