Button("Reload") {
    guard viewState != .loading else { reutrn }
    viewState = .loading
    Task {
        // ...
    }
}

UIWindowSceneの取得方法

UIWindowSceneの取得方法として以下のようなコードが書かれていました。

let windowScene = UIApplication.shared.connectedScenes.first as! UIWindowScene

会場でのコードレビューのコメントにはここのas!を心配するコメントが多数ついていました。 そのコメントは正しく、connectedScenesにはUIWindowScene以外の UISceneが入る可能性があります。 UISceneのサブクラスとして圧倒的に目にするのはUIWindowSceneではあるものの、CarPlayのためのCPTemplateApplicationSceneといったサブクラスが渡される可能性もCarPlayのサポートをしているとあります。

では、以下のようにforce unwrappingを避ければ良いのでしょうか？

let windowScene = UIApplication.shared.connectedScenes.compactMap { $0 as? UIWindowScene }.first

まだ安心できないです。iPadアプリの場合マルチウインドウの考慮が必要です。
マルチウインドウの場合、各ウインドウに対応するUIWindowScene がこのconnectedScenesに入っています。
今回の場合、エラーを表示するためのUIWindowSceneなのでユーザーが操作したUIWindowSceneと同じ UIWindowSceneを取得する必要がありますが、この方法ではそのUIWindowSceneを取得できているとは限りません。

また、iPhoneであっても安心はできません。
iPhoneを外部ディスプレイに繋いでいる場合、iPhoneのウインドウとは別に繋がっている外部ディスプレイもUIWindowSceneとして扱われます。

ではどうするのがいいのかですが、もともとのコードはSwiftUIなのでSceneDelegateからUIWindowSceneをEnvironmentに流すのが良さそうです。

その他の問題点

・taskの中でTaskの発行をしていて、重複しています。
・ApiClientはDIした方がいいです
・ApiClientではなくて、APIClient にした方が命名として自然です
・ClientErrorのlocalizedDescriptionが使われていますが、使用用途がUIのアラートなのでLocalizedErrorに準拠しても良さそうです
・その他、細かい問題 (アクセス修飾子、classよりstructやenumの方が良い、命名などなど)

問題1のまとめ

UIと非同期にまつわる問題のコードが抱えている課題を紹介しました。 大きな改善として、以下のようなことが考えられます。

・Viewの状態をenumで管理する
・ボタンのタップなど、ユーザーの動作をトリガーにした非同期処理をする場合、非同期処理が重複して呼ばれないようにする
・UIWindowSceneの取得にconnectedScenesからとる方法は避ける

どれも実際によく出てくるパターンだと思いますので、みなさまの日々の開発でも気をつけてみてください。

問題2

Core DataのNSManagedObjectの変更を検知して、最新のNSManagedObjectをViewに表示させています。

class ManagedPerson: NSManagedObject {
    @NSManaged var name: String
    @NSManaged var postCount: Int // 投稿の数
}
 
class ManagedObjectObserver {
    var object: NSManagedObject!
    func observe(_ handler: @escaping (NSManagedObject?) -> Void) {
        Task {
            let notifications = NotificationCenter.default.notifications(
                named: .NSManagedObjectContextObjectsDidChange,
                object: nil
            )
            for await notification in notifications {
                let userInfo = notification.userInfo!
                let inserted_objects = userInfo[NSInsertedObjectsKey] as! Set<NSManagedObject>
                let updated_objects = userInfo[NSUpdatedObjectsKey] as! Set<NSManagedObject>
                let refreshed_objects = userInfo[NSRefreshedObjectsKey] as! Set<NSManagedObject>
                let deleted_objects = userInfo[NSInsertedObjectsKey] as! Set<NSManagedObject>
 
                if inserted_objects.contains(object)
                    || updated_objects.contains(object)
                    || refreshed_objects.contains(object) {
                    handler(object)
                } else if deleted_objects.contains(object) {
                    handler(nil)
                }
            }
        }
    }
}
 
class PersonViewModel: ObservableObject {
    @Published var person: ManagedPerson
    private var Observer: ManagedObjectObserver
 
    init(person: ManagedPerson) {
        self.person = person
        Observer = ManagedObjectObserver()
        Observer.object = person
        Observer.observe { [weak self] object in
            self?.person = object as! ManagedPerson
        }
    }
}
 
struct PersonView: View {
    @ObservedObject var viewModel: PersonViewModel
 
    init(person: ManagedPerson) {
        self.viewModel = PersonViewModel(person: person)
    }
 
    var body: some View {
        VStack {
            Text(viewModel.person.name)
            Text("\(viewModel.person.postCount)") + Text("Posts")
        }
    }
}

このコードが抱えている問題

今回のユースケースのように、SwiftUIのViewに最新のNSManagedObjectを表示したいだけなら、実はManagedObjectObserverというクラスは必要ありません。
NSManagedObjectはObservableObjectなのでSwiftUIのViewから監視できるので、出題されたコードのうち、ほとんどが実は不要なコードでした。

ただ、そこに目を瞑ってもまだまだ致命的な問題を抱えています。
ここから先は「何かしらの事情で、ManagedObjectObserverが必要」という前提で解説をしていきます。

このコードが抱えている致命的な課題としては以下があります。

・NSManagedObjectがスレッドを跨いで共有されている
・Taskの中が無限ループしている

NSManagedObjectがスレッドを跨いでいる

NSManagedObjectはスレッドセーフではないです。 このコードは、外から渡されたNSManagedObjectをTaskを超えて渡しています。 スレッドを超えて、NSManagedObjectを共有してしまうと、意図しない挙動をしたり最悪の場合クラッシュしたりします。 ここでは、NSManagedObjectを共有するのではなくNSManagedObjectIDを代わりに共有するべきです。

Taskが無限ループしている

次にTaskの中でNotificationCenterを監視していますが、このfor awaitが終わることはありません。
外から適切なタイミングでTaskをキャンセルしてこの監視を終了する必要があります。

ちなみに、このTaskの中でobjectを参照していますが、これはManagedObjectObserverのプロパティです。
Taskのクロージャーの中でプロパティを参照した際selfを暗黙的に強参照するので、Taskが解放されるまで、このManagedObjectObserverが解放されず、メモリーリークが発生してしまう、というバグもここにはありました。

その他の問題点

その他にも命名やforce unwrap、キャストなど、アクセス修飾子など様々な改善の余地を抱えています。
ちょっとした意地悪としてNSDeletedObjectsKeyを使うべきところを、NSInsertedObjectsKeyにしたのですが、会場でそれを見抜いた人がいてとてもびっくりしました。

問題2のまとめ

Core DataとSwift Concurrencyを併用した際に発生しやすい間違いを問題に含めてみました。
特にfor awaitが出てきた時には、そのループが終了するかどうかを意識してコードを書く必要があります。
もし無限ループするものであれば、どこかでTaskをキャンセルすることを考えてみてください。

問題3

ToDoの一覧を表示する画面です。 ナビゲーションバーのボタンを押すことで「未完了」だけ表示するか、全てのToDoを表示するか変更できます。

struct CellInformation: Hashable, Equatable {
    let id = UUID()
    let title: String
}
 
protocol ToDoRepository {
    func fetchUnfinishedToDo() -> [ToDo]
    func fetchAllToDo() -> [ToDo]
}
 
class ToDoListViewModel: ObservableObject {
    let repository: any ToDoRepository
    @Published var isFiltering = true
    @Published var cellInfos: [CellInformation] = []
 
    init(repository: any ToDoRepository) {
        self.repository = repository
        reload()
    }
 
    func reload() {
        let todos = isFiltering ? repository.fetchUnfinishedToDo() : repository.fetchAllToDo()
        cellInfos = todos.map { todo in CellInformation(title: todo.title) }
    }
}
 
class TODOListViewController: UIViewController {
    typealias DataSource = UITableViewDiffableDataSource<Int, CellInformation>
    let tableView = UITableView()
    let viewModel: ToDoListViewModel
    var dataSource: DataSource?
    var cancellable: [AnyCancellable] = []
    // initは省略
    override func viewDidLoad() {
        super.viewDidLoad()
        setUpLayout() // <- tableViewをaddしたり、auto layoutを貼ったり(省略)
        tableView.register(ToDoCell.self, forCellReuseIdentifier: "Cell")
        dataSource = DataSource(tableView: tableView) { tableView, _, item in
            let cell = tableView.dequeueReusableCell(withIdentifier: "Cell") as! ToDoCell
            cell.apply(info: item)
            return cell
        }
        viewModel.$cellInfos.sink { [weak self] _ in self?.reload() }.store(in: &cancellable)
        viewModel.$isFiltering.sink { [weak self] isFiltering in
            let action = UIAction({ _ in
                self?.viewModel.isFiltering = !isFiltering
                self?.viewModel.reload()
            })
            let item = UIBarButtonItem(title: isFiltering ? "未完了" : "全て", primaryAction: action)
            self?.navigationItem.rightBarButtonItem =  item
        }.store(in: &cancellable)
    }
 
    private func reload() {
        var snapshot = NSDiffableDataSourceSnapshot<Int, CellInformation>()
        snapshot.appendSections([0])
        snapshot.appendItems(viewModel.cellInfos)
        dataSource?.apply(snapshot)
    }
}

このコードが抱えている問題

このコードが抱えている主要な問題として以下がありました。

・TableViewの更新タイミング
・cellの同一性

TableViewの更新タイミング

問題となっているのはこのコードです。

viewModel.$cellInfos.sink { [weak self] _ in
    self?.reload()
}.store(in: &cancellable)

cellInfoの値が更新されたら tableViewを更新する処理です。

一見正しそうに見えますが、実はこの実装では正しく動きません。
cellInfosは@Publishedが添えて宣言してありますが、Publishedは値が更新される直前にイベントが発行されるのでsinkが呼ばれた時点では、まだcellInfosの値は更新されていません。
更新された値は、sinkに渡されますが、reload関数内では渡された値ではなくviewModelの値を直接参照して更新しています。
その結果、古い値をもとにtableViewを更新してしまっています。

ここでは、以下のようにsinkに渡される値を使って更新する必要がありました。

viewModel.$cellInfos.sink { [weak self] cellInfos in
    self?.reload(cellInfos) // クロージャーに渡されたcellInfosを用いてtableViewを更新する
}.store(in: &cancellable)

cellの同一性

UITableViewDiffableDataSourceでは前後のitemが同一の場合更新しない、いわゆる差分更新をサポートしています。
CellInformationの中でUUIDを発行する実装になっていますが、そのような実装ではtableViewを更新するたび全てのセルが削除されて、挿入されるアニメーションになってしまいます。

この実装を修正するにはいくつかの修正案が考えられます。
例えば、ToDoに識別子になるような値があればそれを使う方法が考えられます。
ただ、そういった実装にしてしまうと、セルの識別子が同じ時に中身が更新されないという別の問題も発生させてしまいます。
どういった方法が良いのかはケースバイケースですが、iOS16以上をサポートしている場合は、UIHostingConfigurationとの組み合わせが良いかも知れません。
詳しくはWWDCのセッション動画Use SwiftUI with UIKitをご確認ください。

その他の問題点

その他にもこのコードにはたくさんの問題点があります。

・ToDoRepositoryがどのような方法で値を取得するのかはこのコードからはわかりませんが、一般的にはAPI呼び出しやDBアクセスがあるのならasync interfaceが良さそうに思います
・全体的にアクセス修飾子がないです
・ViewModelのinitではsomeが使えそうです
・DataSectionのSectionがIntなのは少し雑に感じます。適切なenumを定義するのが良さそうです
・UITableViewを使っていますが、最近ではUICollectionViewでも同様のことができそうです
・dataSourceの型が DataSource?になっていますが、lazy varを使うことで、DataSourceにできそうです

lazy var dataSource =  DataSource(tableView: tableView) { tableView, _, item in
    let cell = tableView.dequeueReusableCell(withIdentifier: "Cell") as! ToDoCell
    cell.apply(info: item)
    return cell
}

・cellのregistationのidentifierを毎回ハードコーディングしています。どこかで定数として宣言した方が良いでしょう

問題3のまとめ

UIKitとCombineの組み合わせで陥りやすいバグを問題に仕込んでみました。 @Publishedは便利ですが、イベントが発火するタイミングはwillSetなので、注意しながら実装する必要があります。 @Publishedに対してsinkしているのに、その値を使っていないコードがあったら、少し注意が必要そうです。

最後に

イベント当日は多くの方に参加していただき、多くのレビューコメントが寄せられました。その中には予想してないかったアイデアや意外な視点からのコメントもあり、その度参加者、出題者ともに盛り上がりました。 参加者との意見交換を通じて、どのような実装がベストであるか、こういうケースにはどう対処するのかなど、どうしても会社の中で閉じてしまいがちな議論をiOSDCの参加者とオープンに議論できました。 このようなフィードバックや議論は出題者側であった私たちもにとっても非常に価値のある学びとなりました。 今回の「Code Review Challenge」に参加してくださった全ての方に心より感謝しています。