【Code Review Challenge】解説１問目：Task と enum には要注意 #try! Swift Tokyo 2024

2024-04-09

こんにちは。iOSアプリエンジニアのKiichiです。

先日開催されたtry! Swift Tokyo 2024のLINEヤフー企業ブースでは、Code Review Challengeを行いました。Code Review Challengeとは、Bad CodeをGood Codeにするための公開コードレビューです。参加者に技術への関心を持ってもらうこと、外部の方々からのレビューにより社員の学びにつなげることを目的に、過去のイベントで行っていた取り組みを今回のイベントでも実施しました。

本記事では、今回出題したCode Review Challengeの1問目の解説をします。

出題コード

import SwiftUI

struct TODO: Hashable {
    let text: String
    let status: TODOStatus?
}

enum TODOStatus {
    case completed
    case inProgress
}

struct TODOListView: View {
    @State var todos: [TODO] = [] {
        didSet {
            print("todos changed")
        }
    }

    @State var addingTask: Task<Void, Error>? = nil

    var body: some View {
        NavigationView {
            List(todos, id: \.self) { todo in
                HStack {
                    Text(todo.text)

                    switch todo.status {
                    case .completed:
                        Image(systemName: "checkmark.circle.fill").foregroundColor(.green)
                    case .inProgress:
                        Circle().fill(.yellow).frame(width: 8)
                    default:
                        EmptyView()
                    }
                }
            }
            .toolbar {
                ToolbarItem(placement: .topBarTrailing) {
                    if let addingTask = addingTask {
                        Button("Cancel") {
                            addingTask.cancel()
                        }
                    } else {
                        Button("Add") {
                            addTODO()
                        }
                    }
                }
            }
        }
        .onAppear {
            Task {
                do {
                    todos = try await TODOService.shared.fetch()
                } catch {
                    showToast(message: "Failed to load TODOs.")
                }
            }
        }
    }

    func addTODO() {
        addingTask = Task.detached {
            try await TODOService.shared.add(todo: TODO(text: "", status: nil))
            showToast(message: "Added successfully.")
        }
    }

    func showToast(message: String) { /* ... */ }
}

// Connect to server
class TODOService {
    static let shared = TODOService()

    func fetch() async throws -> [TODO] {　/* ... */ }
    func add(todo: TODO) async throws { /* ... */ }
}

これは、TODOリストアプリのコードです。
サーバー上にあるTODOの一覧表示と、TODOの追加機能が実装されています。

このコードには複数の問題点がありますが、その中でも特に重要かつやってしまいがちな以下2点について解説します。

不十分なTaskの扱い
可読性・保守性の悪いenum設計

1. 不十分なTaskの扱い

このコードでは、サーバーとの通信を行う際にTaskを用いています。Taskは簡潔に非同期処理を書けるため便利ですが、キャンセル処理やエラーハンドリング・スレッドの扱いには注意が必要です。

まずは以下のTODO一覧取得箇所を見てみましょう。

.onAppear {
    Task {
        do {
            todos = try await TODOService.shared.fetch()
        } catch {
            showToast(message: "Failed to load TODOs.")
        }
    }
}

画面が表示されたタイミングで取得処理を行っていますが、処理中に画面が非表示になっても処理は継続されます。そのまま画面が再表示されると、処理結果が重複してしまう可能性があります。onAppearの代わりにtaskを用いるか、onAppearが必要な場合は同時にonDisappearでキャンセル処理をするのがベターです。

続いて、新たなTODOを追加する処理です。

func addTODO() {
    addingTask = Task.detached {
        try await TODOService.shared.add(todo: TODO(text: "", status: nil))
        showToast(message: "Added successfully.")
    }
}

Task.detached は処理をメインスレッドから逃すために使われることがありますが、今回のケースではdetachedをつける必要はありません。というのも、@MainActorでないasync関数は、detachedかどうかによらずバックグラウンドで実行されるからです。さらに、後続のUI更新処理であるshowToastはメインスレッド上で動作すべき関数なので、なおさらdetachedは不要です。

また、エラーハンドリングが行われておらず、ユーザー視点では処理中なのか失敗したのか判断できません。

そして、別の部分にキャンセル処理が記述されているにもかかわらず、ここではキャンセルされた場合の挙動が定義されていません。
TODOService.shared.addの実装によってはshowToastまで実行され、キャンセルしているのに “Added successfully” と表示されてしまいます。awaitした後は Task.isCancelledをチェックし、キャンセルされている場合は処理を中断するようにしましょう。

さらに、今のままでは処理が終了したことが別の箇所から判別できません。Taskの最後に “addingTask = nil” の1行を挿入し、終了を表現すると良いでしょう。

2. 可読性・保守性の悪いenum設計

まずは以下をご覧ください。

struct TODO: Hashable {
    let text: String
    let status: TODOStatus?
}

enum TODOStatus {
    case completed
    case inProgress
}

TODO.statusが取り得る値は.completed、.inProgress、nilの3種類あります。ユーザー視点でTODOが進行中のときはstatus: .inProgress、完了済みのときはstatus: .completedだと分かります。では、nilは何を表しているのでしょうか？　まだTODOを開始していない状態なのかもしれませんし、途中でキャンセルした状態を表している可能性もあり、確かなことは言えません。このような状況を回避するためには、”意味のある nil” を使用しないことが重要です。

struct TODO: Hashable {
    let text: String
    let status: TODOStatus
}

enum TODOStatus {
    case completed
    case inProgress
    case notStarted
}

のように、statusをnon-optionalにしつつ具体的なcaseを追加すると、status が何を表しているか分かりやすくなります。

続いて、このenumを使用している部分も見てみましょう。

switch todo.status {
case .completed: /* ... */
case .inProgress: /* ... */
default: /* ... */
}

今はdefaultに当たるのはnilの場合だけですが、将来enumのcaseが追加される可能性もあります。その都度使用部分でもcaseを追加すれば良いのですが、そのままでもコンパイルエラーにならないため、見落としが発生しやすいです。defaultの代わりに具体的なcaseを記述するようにしましょう。

その他の改善点

その他、以下のような改善点が考えられます。

コーディングスタイル

外部から使用しない変数や関数にはprivate修飾子をつけます。特に@Stateプロパティは外部から隠した方が良いです。
protocolを型として用いる（存在型の）場合、anyキーワードをつけます。（例：Task<void, error=""> → Task<void, any="" error="">）</void,></void,>
optional型の宣言に “= nil” は不要です。
foregroundColor(:)は将来的に非推奨になるため、foregroundStyle(:)を用います。
if letの省略記法を用いて簡潔に記述できます。（例：if let addingTask = addingTask {} → if let addingTask {}）

アーキテクチャと設計

TODOStatusはTODOの状態を表す用途しかないため、以下のようにTODO名前空間を活用すると分かりやすくなります。
```
struct TODO {
  // ...
  enum Status {
      // ...
  }
}
```
シングルトンを使用せず、TODOServiceを依存関係として注入できるようにすることで、テスト性向上を図れます。
Image(systemName:)は再利用する可能性が高いため、以下のようにまとめておくと良いです。ライブラリや自動生成も役に立つでしょう。
```
extension Image {
  static let checkmarkCircleFill = Image(systemName: "checkmark.circle.fill")
}
```
実装にもよりますが、現状ではTODOServiceはclassである必要がないため、structにすると良いです。classが必要な場合はfinal修飾子をつけるべきです。

UX向上/バグ回避

@StateプロパティのdidSetは、Binding型と併用した場合などにうまく機能しないことがあるため、onChangeを用いた方が良いです。（ただしEquatableへの準拠が必要）
TODOの中身が同じ場合にListのidが重複してしまうので、TODOをIdentifiableに準拠させ、UUIDのような一意なidを用いるべきです。
addingTaskの最後にTODOService.shared.fetch()を行わないと、TODO追加時にリストが更新されません。

想定解答

上記の問題点をすべて解決した想定解答は以下です。

import SwiftUI

struct TODO: Identifiable, Equatable {
    let id = UUID()
    let text: String
    let status: Status

    enum Status {
        case completed
        case inProgress
        case notStarted
    }
}

struct TODOListView: View {
    @State private var todos: [TODO] = []
    @State private var addingTask: Task<Void, any Error>?

    let todoService: any TODOServiceProtocol

    init(todoService: some TODOServiceProtocol) {
        self.todoService = todoService
    }

    var body: some View {
        NavigationView {
            List(todos) { todo in
                HStack {
                    Text(todo.text)

                    switch todo.status {
                    case .completed:
                        Image.checkmarkCircleFill.foregroundStyle(.green)
                    case .inProgress:
                        Circle().fill(.yellow).frame(width: 8)
                    case .notStarted:
                        EmptyView()
                    }
                }
            }
            .toolbar {
                ToolbarItem(placement: .topBarTrailing) {
                    if let addingTask {
                        Button("Cancel") {
                            addingTask.cancel()
                        }
                    } else {
                        Button("Add") {
                            addTODO()
                        }
                    }
                }
            }
        }
        .task {
            do {
                todos = try await todoService.fetch()
            } catch {
                showToast(message: "Failed to load TODOs.")
            }
        }
        .onChange(of: todos) { _, _ in
            print("todos changed")
        }
    }

    private func addTODO() {
        addingTask = Task {
            do {
                try await todoService.add(todo: TODO(text: "", status: .notStarted))
            } catch {
                guard !Task.isCancelled else {
                    showToast(message: "Cancelled.")
                    addingTask = nil
                    return
                }

                showToast(message: "Failed to add TODO.")
                addingTask = nil
                return
            }
            guard !Task.isCancelled else {
                showToast(message: "Cancelled.")
                addingTask = nil
                return
            }

            showToast(message: "Added successfully.")

            do {
                todos = try await todoService.fetch()
            } catch {
                showToast(message: "Failed to load TODOs.")
            }
            addingTask = nil
        }
    }

    private func showToast(message: String) { /* ... */ }
}

// Connect to server
protocol TODOServiceProtocol {
    func fetch() async throws -> [TODO]
    func add(todo: TODO) async throws
}

struct TODOService: TODOServiceProtocol {
    func fetch() async throws -> [TODO] { /* ... */ }
    func add(todo: TODO) async throws { /* ... */ }
}

extension Image {
    static let checkmarkCircleFill = Image(systemName: "checkmark.circle.fill")
}

まとめ

この記事では、Code Review Challengeの1問目の解説として、主に以下の点について説明しました。

Taskを使って非同期処理する際のキャンセル処理・エラーハンドリング・スレッドの扱いに注意して行う必要がある
特にenumを使う際には “意味のある nil” を避け、switch文ではdefaultの代わりに具体的なcaseを記述した方が良い

入社1年目で新たにチームに参加したエンジニアとして、チーム開発における可読性や保守性の重要性を強く感じています。コードレビューによって健全なコードを後世に残していけたらと思います。

また、今回はコードを見て指摘する問題でしたが、ユニットテストをしっかり書けば気づけるような点、linterやformatterが自動で指摘してくれるような点も多くありました。それらも活用し、効率的にコードレビューできるようになると良いと思っています。