session 9 の実装

[askldfghj]

概要

session 9 の課題を実装しました。

セッションリンク

https://github.com/yumemi-inc/ios-training/blob/main/Documentation/ThreadBlock.md

変更点

• UIActivityIndicatorView の表示

スクリーンショット

[askldfghj]

データロード中はタップを防ぐ

[novr]

[imo] MVPはPassive Viewパターンなので、Passive ViewはViewが受動的（Passive）で能動的ではない
あくまでViewは受け身でありPresenterがViewに値をセットしたりする、自分で値をセットしたりしない方針かと思います
ただ、研修としての課題ではないので対応自体はどちらでもOKです 🙇

[askldfghj]

@novr
なるほど。 View だからと言って View を操作してもいいということではないのですね。
d83e59d
上記のコミットで output の関数を追加して、 prsenter で View の表示を操作する方向で修正してみました。いかがでしょうか。

[es-kumagai]

インジケーターの表示開始が HomePresenter から HomePresenterOutput に showLoadingUI で直接的に指示されるのに対して、その非表示が HomePresenterOutput の updateInfoDisplay と showAlertControllerByError 内で自発的に（感じる）タイミングで実施されるのが、なんとなく気になりました。

上記で登場したどのメソッドも HomePresenterOutput に規定されているものであるのを踏まえると、現状の動きも自分がただ違和感を覚えるだけで妥当なのかもしれないですが、自分なりに考えてみると —

たとえば HomePresenterOutput の定義を、次のようなものにしてみる手もあるかもしれません。こうすると対がわかりやすくなりそうなのと、どこか iOS でいう Delegate パターンで馴染みある見た目にも近づきます。

protocol HomePresenterOutput {
   func weatherDidStartLoading()
   func weatherDidFinishLoading(_ result: Result<WeatherResponse, Error>)
}

ただ、MVP として見たときには View に対する指示が不明瞭な感じもするので、上記も少しばかり考慮しつつ、適切な方法を模索してみても良いかもしれない印象でした。現状と同じように HomePresenterOutput に hideLoadingUI を定義してそれを HomePresenter から適切なタイミングで指示する方法もあるかもしれないです。

[ykws]

テストパスすることの確認をお願いします！
（すみません、課題の内容にその旨追記するようにしました）

• スレッドブロックのセッションに非同期テスト対応を明記 ios-training#61

[askldfghj]

@ykws
コメントありがとうございます。
確かに HomePresenterOutput に関数を追加したのが影響し、テストのビルドが通らなくなっていました。
f71f09c
上記のコミットで対応しました。
非同期の対応は元々実装を非同期を想定していたので、 session 8 でそれに基づいた実装を行いました。

また、テストを毎回確認するより CI がいた方が良さそうだったので、 actions を使うことをやってみました。
#11

https://zenn.dev/yumemi_inc/articles/xctest-github-actions
ちょうどこの記事を参考にしていましたが、ローカルではプロジェクトが見つからないというのと、マージされてないので actions タブで結果が見れなく困ってました。
時間あるときレビューしていただけると助かります。

[ykws]

LGTM 👍

[es-kumagai]

非同期を先取りして実装されているため無意味な指摘になりますけれど、HomePresenter の loadWeatherData で使う API を処理の重たいものに変えても何も影響を受けないところが面白かったです。

ただし仮に「スレッドブロック」の課題に純粋に取り組んだとしたとき、WeatherModelInput の fetchWeatherData(request:) async throws が「async」指定で非同期呼出を求めてしまうため、ここでスレッドブロックの API が実質的に非同期 API として吸収されてしまうのが気になりました。

問題はないのですが、一般的な視点で「非同期処理を必要としない YumemiWeather.syncFetchWeather(json:) throws をほぼ単純にラップする WeatherModel.fetchWeatherData(request:) async throws で非同期メソッドとして備えるところが、何も間違いはないのですが、些か過剰な強化のようにも感じます。ここの async をなくしてコードの変化を見てみても面白いかもしれないです。

[askldfghj]

@es-kumagai
コメントありがとうございます。
確かに課題としては過剰かもしれないですが、これでメインスレッド止まったらダメだろ。。という気持ちでした。（そのためあらかじめ非同期の仕組みを見ていました）
そしてローカルで async を削除して動きを見てみました。予想通り何もタップできない状態で、ロード中にタップした close が通信終了後に反応して、なるほど、と思いました。

[askldfghj]

@novr @ykws @es-kumagai
レビューありがとうございます。こちらマージします。