この記事は公開されてから1年以上経過しています。情報が古い可能性がありますので、ご注意ください。
はじめに
CX事業本部の中安です。まいどです。
現在絶賛開催中の Developers.IO 2020 CONNECT で、自分も先日 iOSの設計パターンである VIPER について発表をさせていただきました。
ブログはこちら
公開まではドキドキとしていましたが、いざ公開してみると社内外から色々反応をいただけて嬉しいかぎりですね。
その中で YouTubeコメントで1件、Uhooi THE さんからお褒めとご指摘のコメントをいただきました。 すごく細かく動画の内容を見ていただいていてありがたいかぎりですし、YouTubeコメントで細かなコードレビューをいただくなんて不思議かつ斬新な感覚でした。
今回、投稿者の Uhooi THE さんにもYouTubeコメント上で許可いただいたので、 いただいたコードレビューの回答をブログにしたためていこうと思います。
ご指摘と回答
SceneDelegateとその設定(Info.plist)について
13:28 `Info.plist` に `UISceneStoryboardFile` と `UIMainStoryboardFile` のキーが残っていると使っていると思うので、キーごと削除するのもありです。 ref: https://sarunw.com/posts/how-to-create-new-xcode-project-without-storyboard/
ありがとうございます。
今回の動画はプロジェクトを新規作成して、そこからスタートするという前提でお話させていただきました。 シングルページアプリケーションのテンプレートとしてはSceneDelegateが置かれた状態からスタートすると思いますので、そこは変更なく進めさせていただきました。
シーンを意識しないアプリであれば、従来のAppDelegateから始まるように Info.plist上のSceneの設定と、SceneDelegate自体を削除してしまってもいいかと思います。
動画時間の都合上、そのあたりの説明を入れると長くなってしまうと思い、テンプレートのデフォルトのまま進めさせていただきました。 (「最初にSceneDelegateは削っておきますね」という進め方でもよかったかもしれませんね)
ArticleDetailViewControllerのセル分岐について
15:04 せっかくenumで定義しているので、 `row` はifでなくswitchで分岐すべきです。
ありがとうございます。 たしかにご指摘どおりですね。
// Before
func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
let row = Row.rows[indexPath.row]
let cell = tableView.dequeueReusableCell(withIdentifier: row.rawValue, for: indexPath)
if row == .title {
cell.textLabel?.text = "タイトル"
cell.detailTextLabel?.text = articleEntity.title
}
if row == .body {
cell.textLabel?.text = articleEntity.body
cell.detailTextLabel?.text = nil
}
return cell
}// After
func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
let row = Row.rows[indexPath.row]
let cell = tableView.dequeueReusableCell(withIdentifier: row.rawValue, for: indexPath)
switch row {
case .title:
cell.textLabel?.text = "タイトル"
cell.detailTextLabel?.text = articleEntity.title
case .body:
cell.textLabel?.text = articleEntity.body
cell.detailTextLabel?.text = nil
}
return cell
}このほうがよいと思います。
弱参照と未所有参照について
23:55 Routerで `viewController` をイニシャライザで保持する場合、`weak` でなく `unowned` で定義すれば、 `let` にできて `!` 型を外せます。 `unowned` は `nil` が入らないことを前提としているので、もし `nil` が入る可能性があるなら使うのを避けるべきです。 25:27 のPresenterも同様です。
ありかがとうございます。
Swift - Resolving Strong Reference Cycles Between Class Instancesを改めて参考にし、弱参照と未所有参照の理解を深めました。
Weak References
A weak reference is a reference that does not keep a strong hold on the instance it refers to, and so does not stop ARC from disposing of the referenced instance. This behavior prevents the reference from becoming part of a strong reference cycle. You indicate a weak reference by placing the weak keyword before a property or variable declaration.
Because a weak reference does not keep a strong hold on the instance it refers to, it’s possible for that instance to be deallocated while the weak reference is still referring to it. Therefore, ARC automatically sets a weak reference to nil when the instance that it refers to is deallocated. And, because weak references need to allow their value to be changed to nil at runtime, they are always declared as variables, rather than constants, of an optional type.
弱参照
弱い参照とは、参照しているインスタンスを強く保持していない参照のことで、ARCが参照しているインスタンスを破棄することを止めません。この動作により、参照が強力な参照サイクルの一部になるのを防ぎます。この振る舞いにより参照が強参照サイクルの一部になるのを防ぎます。プロパティまたは変数の宣言の前に weak キーワードを置くことで弱参照を示します。
弱参照は参照しているインスタンスを強く保持していないため、弱い参照がまだ参照している間にそのインスタンスが解放される可能性があります。そのため、ARCは参照しているインスタンスが解放されると自動的に弱い参照をnilに設定します。また、弱参照は実行時にその値を nil に変更できるようにする必要があるため、常に定数ではなくオプショナル型変数として宣言されます。
Unowned References
Like a weak reference, an unowned reference does not keep a strong hold on the instance it refers to. Unlike a weak reference, however, an unowned reference is used when the other instance has the same lifetime or a longer lifetime. You indicate an unowned reference by placing the unowned keyword before a property or variable declaration.
Unlike a weak reference, an unowned reference is expected to always have a value. As a result, marking a value as unowned doesn’t make it optional, and ARC never sets an unowned reference’s value to nil.
IMPORTANT: Use an unowned reference only when you are sure that the reference always refers to an instance that has not been deallocated. If you try to access the value of an unowned reference after that instance has been deallocated, you’ll get a runtime error.
未所有参照
弱参照と同様に、未所有参照は参照しているインスタンスを強く保持しません。ただし、弱参照とは異なり、未所有参照は他のインスタンスが同じライフタイムまたはそれより長いライフタイムを持つ場合に使用されます。未所有参照はプロパティまたは変数宣言の前に unowned キーワードを置くことで指定できます。
弱参照とは異なり、未所有参照は常に値を持つことが期待されます。その結果、値を unowned として記述してもオプショナルにはならず、ARCが未所有参照の値を nil に設定することはありません。
重要 ： 所有されていない参照は、その参照が常に解放されていないインスタンスを参照していることが確実な場合にのみ使用してください。インスタンスが解放された後に所有していない参照の値にアクセスしようとすると、実行時エラーが発生します。
今回の Router ではルータが生きている限りは ViewController の参照を見続けることがわかっている状態なので、
指摘いただいたとおり unowned で宣言するほうが正しいと思います。
ありがとうございます。
// Before
class ArticleListRouter: ArticleListRouterProtocol {
weak var view: UIViewController!
init(view: UIViewController) {
self.view = view
}
// (以下略)// After
class ArticleListRouter: ArticleListRouterProtocol {
unowned let view: UIViewController
init(view: UIViewController) {
self.view = view
}
// (以下略)クラスとプロトコルの命名について
24:55 `○○PresenterProtocol` という名前にすると、「View→Presenter」or「Interactor→Presenter」のどちらで使うプロトコルかわかりづらいです。 私は「View→Presenter」で使うプロトコルを `○○EventHandler` 、「Interactor→Presenter」で使うプロトコルを `○○InteractorOutput` という名前にしています。 ただ、今回は「Interactor→Presenter」をすべてコールバックで処理していてプロトコルを噛ませなかったので、このままでもわかりづらくなかったです。
動画でも少し言及しましたが、世の中にはVIPERのサンプルコードは有名・無名なもの含め沢山あります。そして、それぞれに少しずつ構成や命名が異なるところがあります。
Interactorについては、抽象をあらわすプロトコル名として「〜Input」「〜Output」と名付けている有名どころのサンプルを自分自身も見たことがあります。
しかしながら、この命名規則ではなく今回の動画のように命名したのは「初めて VIPER に触れる方にとって、使用する側・使用される側を意識した命名であると逆に混乱を生むのではないか」と思ったからです。 Presenterには「PresenterProtocol」Routerには「RouterProtocol」のように、まず分かりやすい名前であったほうが意味が伝わりやすいのではないかと思うに至りました。
しかし、指摘いただいたように「要素同士の方向が分かりづらくなる」という意見が出るということは、まだまだ一考の余地はあるはずですし、 もしも実際にチームで開発するとなると、議論をしてみる価値のあるところかと考えます。(このあたりは動画の最後に触れたところになります)
今回は「Interactor→Presenter」をすべてコールバックで処理していてプロトコルを噛ませなかった
コメントでこのようにおっしゃっていただいたように、 今回のサンプルでは UseCaseというものを使い プロトコル指向プログラミングからは外れたコーディングとなっています。 これをプロトコル指向プログラミングで書いていくとなると、他の要素の命名規則も含め、方向性を考慮した命名にする必要があるかなと思いました。
Dependency型内の宣言について
31:55 Presenterが依存するものを構造体として定義して注入するの、わかりやすくて勉強になりました！ ただ、 `Dependency` 構造体の `router` は `!` 型にする必要がないと思います（試していないので違ったらすみません）
ご指摘いただいとおりですね。見落としかと思います。ありがとうございます。
// Before
class ArticleListPresenter {
struct Dependency {
let router: ArticleListRouterProtocol!
let getArticlesArrayUseCase: UseCase<Void, [ArticleEntity], Error>
}
// (以下略)// After
class ArticleListPresenter {
struct Dependency {
let router: ArticleListRouterProtocol
let getArticlesArrayUseCase: UseCase<Void, [ArticleEntity], Error>
}
// (以下略)RouterのDIについて
36:15 遷移元のRouterで遷移先のDIを行っていますが、遷移元によってDIするインスタンスが変わることはあまりないので、遷移先のRouterに `assembleModule()` のようなインスタンスを生成するstaticメソッドを持たせるのもいいと思います。 ただ単一責任原則に反するので、例えばインスタンス生成のみを担う `○○Builder` クラスを作るのもありです。 （こちらは最後に触れていましたね、、）
ありがとうございます。
実は自分もアプリを実際に開発する際には「インスタンスを作るクラス」と「画面遷移をするクラス」を分けることが多いです。
今回は時間の都合やアプリの規模などを加味して、一緒くたの状態でコーディングをしています。
（こちらは最後に触れていましたね、、）とおっしゃっていただいた通り、補足として後半の深堀り部分でクラス分割の話をさせていただきました。
もちろん、これも規模やスケジュール、設計思想に基づいてどこまで細分化するかの検討は必要だと思っています。
小さいアプリなのにクラスがいくつも分かれていると逆にメンテナンスが苦しくなるという事態も起きうるでしょうから、 最後には「これが正しい」というよりは「TPOによる」という曖昧な帰結になってしまうのですが。。。
ちなみに途中で登場した参考書籍では、AppDependencyというDIを包括するようなクラスを登場させるという方法が採られているようです。
Codableについて
36:45 デコードするだけなら `Codable` でなく `Decodable` のみに準拠させたほうが意図がわかりやすいです。 `Codable` に準拠していると「エンコードもするのかな？」と考えてしまいます。
ありがとうございます。こちら素晴らしいご指摘だと思います。
これは既存アプリからソースコードを引っ張ってきた際に「RestAPIのGET以外を叩く」という部分を持ってきていたためにCodableとして残ってしまったものをそのまま残してしまっていたようです。(動画作成のときにも気づけていませんでした)
おっしゃるとおりに、この箇所は意図を明確にするために「Decodable」に準拠させたほうがよいと思います。
// Before
struct ArticleEntity: Codable {
let id: Int
let userId: Int
let title: String
let body: String
}// After
struct ArticleEntity: Decodable {
let id: Int
let userId: Int
let title: String
let body: String
}スレッドの責務について
38:00 メインスレッドで実行必須なのはUIの更新なので、Interactorで行う強い理由がなければView側でやるのがいいと思います。 そのほうが `DispatchQueue.main.async` で括る回数を減らせることが多いです。
ありがとうございます。
ここは「たしかに・・」と思う反面、少し迷いもあります。
メインスレッドは描画スレッドであるため、Viewの責務に感じるところではありますが、 スレッドの処理自体はロジカルな部分であるかもしれないなぁと思うところです。
データが別スレッドで生成(または取得)・運搬されてViewに到達するときに、見た目に集中したいViewがスレッドを気にするべきなのか？という迷いです。
しかしながら「では、それはInteractorの仕事か?」と言われると、 指摘されて気付きましたが、Interactorが描画のことに関心を持ってしまっている問題があるのですね。 (正しくは「描画スレッドのこと」)
あるいは、ビューロジックを担当し、旗振り役でもある Presenter にその責務をもたせるという可能性もあるのではないかという個人的な見解になります。
Presenterは仕様書のイメージ
42:27 「Presenterは仕様書のイメージ」はそのように考えたことがなく、なるほどと思いました！
ここは過去にMVPパターンを教えてもらったときに同時に教わったことでして、自分も腑に落ちたところなので紹介させていただきました。
「どのソースを見ればこのアプリの仕様が分かるんだ?」という事態も、完全にではないですが解消できるアプローチだと思います。
ユースケースのファイルについて
43:53 個人的には、VIPERはただでさえファイル数が増えがちで管理が大変なので、1ユースケースに1つのメソッドしか持たせないのはエグいと思いますｗ 私はPresenterと同名のInteractorクラスを1つ作り、1:1の関係にすることが多いです。 APIクライアントはライブラリを使うことが多いので、Interactorとは別にAPIクライアントを実装し、Interactorからはそれを呼び出して使います。 そうするとInteractorが外部ライブラリに依存することを防げます。
ファイル数が増えがちで管理が大変なので、1ユースケースに1つのメソッドしか持たせないのはエグいと思いますｗ
たしかに "エグい" ですね (笑)
単一責任の原則は理想通りにすると、なかなかなファイル数と手間がかかるという前提は理解しております。 (他の要素も単一責任の原則を適用したら、もう無理…って感じですね)
繰り返しになりますが、原則に苦しめられるならば原則を変える(もちろん破綻しないように)という作業は常に考えていくべきですね。
私はPresenterと同名のInteractorクラスを1つ作り、1:1の関係にすることが多いです。
こういうパターンもありますね(むしろ多数派な気がします)。 自分はもしInteractorを束ねるとすれば、画面ごとではなく近しいドメイン同士で集めるという手を使います。
たとえば Twitterのようなアプリを作るとして「一覧からユーザをフォローできる」「詳細からもユーザをフォローできる」「〜画面からもユーザをフォローできる」という仕様だったとしたとき、 PresenterとInteractorが1:1だった場合に、同じ仕事だが原則通りにすると別々の場所に実装する必要がでてきてしまいます。
その解決策はいくつもありそうですが「ユーザをフォローできるInteractor」が1つあることで、解決できるのではないかと考えました。 (これもまたもちろん「コレが正解である」と言いきりたいわけではありません)
Interactorとは別にAPIクライアントを実装し、Interactorからはそれを呼び出して使います。
はい。まさにそのとおりでして、今回API呼び出し部分は動画時間の都合上、愚直な組み方をしたと思っています。
本来はネットワーキングのライブラリを採用した上で、おっしゃるとおりにAPIクライアント的なクラスを定義し、そのクラスとも疎結合な関係で呼び出せるようにするのが正しいと思います。
最後に
動画を公開したときに、これほどのフィードバックをいただけるとは思ってもいませんでした。
あらためて Uhooi THE さんに感謝です。
自分のソースコードの拙さを実感するとともに、知見を得ながらソースコードのアップデートとそのアウトプットができてよかったです。
なお、命名規則とスレッド責務については、ご指摘もらったことに対して明確なお答えがてきたかどうか、 ちょっと不安ではあります。(色々ツッコミどころありそうだなと)
今後もすべてについて回答できるわけではありませんが、 動画アップロードの際には、温かいご支援(コメント)と、建設的なお話がまたできればと思います。
ではまた。
6
