ウホーイが気になったところ

uhooi commented 4 years ago

ウホーイが気になったところを書いていきます！

uhooi commented 4 years ago

継承されていないクラスには何も考えずに final を付けたい https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/ViewController.swift#L11

sachiko-kame commented 4 years ago

『ウホーイが気になったところを書いていきます！』がとてもチャーミングで素敵です！

修正します！

uhooi commented 4 years ago

チャーミング頂き光栄です✨

sachiko-kame commented 4 years ago

面白くもありつい笑ってました！✨

uhooi commented 4 years ago

変更しないプロパティは var でなく let にしましょう！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/ViewController.swift#L13

uhooi commented 4 years ago

ついでにUI部品はできる限りInitialization Closureを使って初期化したい（これは好み）

    private let tableView: UITableView = {
        let tableView = UITableView()
        tableView.frame = UIScreen.main.bounds
        tableView.register(cellType: TableViewCell.self)
        return tableView
    }()

sachiko-kame commented 4 years ago

変更しないプロパティは var でなく let にしましょう！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/ViewController.swift#L13

はい！すみません。気をつけます！修正します！

uhooi commented 4 years ago

setup() メソッドで「テーブルビューの初期化」と「アイテムの取得」の2つのことを行っているので、メソッドを分けるといいと思います！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/ViewController.swift#L25-L32

自分ならこう書きます↓ あと self の有無は統一したい！笑

    override func viewDidLoad() {
        super.viewDidLoad()

        configureTableView()
        self.presenter.itemsGet()
    }

    private func configureTableView() {
        self.tableView.delegate = self
        self.tableView.dataSource = self
        self.view.addSubview(tableView)
    }

uhooi commented 4 years ago

@sachiko-kame こんな感じの内容が続きますが、大丈夫でしょうか…？

一旦休憩で、続きは後ほど見ます！

sachiko-kame commented 4 years ago

ついでにUI部品はできる限りInitialization Closureを使って初期化したい（これは好み）
    private let tableView: UITableView = {
        let tableView = UITableView()
        tableView.frame = UIScreen.main.bounds
        tableView.register(cellType: TableViewCell.self)
        return tableView
    }()

（これは好み)ってくれていますが結構こっちの方が好まれる印象です。なかなかなれなくてこの書き方いつもできていないです。修正します！

sachiko-kame commented 4 years ago

setup() メソッドで「テーブルビューの初期化」と「アイテムの取得」の2つのことを行っているので、メソッドを分けるといいと思います！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/ViewController.swift#L25-L32

自分ならこう書きます↓ あと self の有無は統一したい！笑
    override func viewDidLoad() {
        super.viewDidLoad()

        configureTableView()
        self.presenter.itemsGet()
    }

    private func configureTableView() {
        self.tableView.delegate = self
        self.tableView.dataSource = self
        self.view.addSubview(tableView)
    }

確かに！おっしゃる通りです！沢山ありがとうございます！修正します！

uhooi commented 4 years ago

Initialization Closureを使うと、例えばTableViewをCollectionViewに変えたくなったときに、そこだけ変えればいいのでわかりやすいです！あとメソッド化されないので、初期化処理が一度しか呼ばれないことが保証できます。（普通はやらないですが、 setup() メソッドを2回呼ぶことも理論上はできてしまうので）

sachiko-kame commented 4 years ago

@sachiko-kame こんな感じの内容が続きますが、大丈夫でしょうか…？

一旦休憩で、続きは後ほど見ます！

はい！とても助かります！ありがとうございます！！✨

sachiko-kame commented 4 years ago

Initialization Closureを使うと、例えば�TableViewをCollectionViewに変えたくなったときに、そこだけ変えればいいのでわかりやすいです！あとメソッド化されないので、一度しか呼ばれないことが保証できます。

確かに！利点の方が大きいですね！ありがとうございます！

uhooi commented 4 years ago

です！あとちょっとコメント編集しましたー

sachiko-kame commented 4 years ago

です！あとちょっとコメント編集しましたー

はい！！実は変更後すぐにみて確かに！となっていました。ありがとうございます！🙇‍♀️

sachiko-kame commented 4 years ago

@uhooi

現時点で指摘頂けたところ修正しました！本当にありがとうございます！勉強になります！🙇‍♀️

https://github.com/sachiko-kame/architecture_iOS/commit/76a8d89a6c35becdda37c5d697521765cf6b92b1

uhooi commented 4 years ago

すぐに対応していただけると、こちらもレビューしたかいがあって嬉しいです✨

sachiko-kame commented 4 years ago

@uhooi

レビューがとても分かりやすかったのが大きかったかと思います！

自分であやふやだったり悩んでいたことなどもuhooiさんのおかげで解消できたので本当に助かりました！✨

本当にありがとうございます！🙇‍♀️✨

uhooi commented 4 years ago

自分はViewに単体テストを書きたくなく、できる限りViewに分岐（if, switch）を入れたくないので、 presenter.itemGet() メソッドの戻り値はオプショナル型じゃない方がいいです！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/ViewController.swift#L59

分岐はPresenter側のメソッドに入れて、あとTableViewのデータソースで取得するのではなく、 viewDidLoad() などで予め取得するといいと思います！

uhooi commented 4 years ago

MVPだったらView以外で import UIKit するのはよくないです。 import Foundation で済ませましょう！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Presenter.swift#L9

uhooi commented 4 years ago

変数名で、 numberOfItems より普通に itemCount でいいと思いました笑 https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Presenter.swift#L12

sachiko-kame commented 4 years ago

自分はViewに単体テストを書きたくなく、できる限りViewに分岐（if, switch）を入れたくないので、 presenter.itemGet() メソッドの戻り値はオプショナル型じゃない方がいいです！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/ViewController.swift#L59

分岐はPresenter側のメソッドに入れて、あとTableViewのデータソースで取得するのではなく、 viewDidLoad() などで予め取得するといいと思います！

データソース以外の取得方法がそもそもわかっていなかったので考えます！

sachiko-kame commented 4 years ago

変数名で、 numberOfItems より普通に itemCount でいいと思いました笑 https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Presenter.swift#L12

そこまで配慮できていなかったです！汗ありがとうございます！以後気をつけます！

uhooi commented 4 years ago

個人的には itemGet() でなく itemGeted() だとアイテムをゲットした後の処理だとわかって好みです！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Presenter.swift#L43

uhooi commented 4 years ago

DispatchQueue.main.async はViewでやればいいと思います！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Presenter.swift#L46

uhooi commented 4 years ago

ModelもUIKitのインポート禁止〜！笑 https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Model.swift#L9

sachiko-kame commented 4 years ago

変数名で、 numberOfItems より普通に itemCount でいいと思いました笑 https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Presenter.swift#L12

これは私も実は『itemCount派』です。どっちも良いと今英和辞書で調べてはっきりしたので『itemCount』に変更したいと思います！

ちなみに、『number』の方がいいのか調べて『number』は『数』とでるのに対して、『Count』は『数える』で出てきたので『number』の方がいいのかな？と『numberOfItems』にしてしまいました汗

sachiko-kame commented 4 years ago

個人的には itemGet() でなく itemGeted() だとアイテムをゲットした後の処理だとわかって好みです！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Presenter.swift#L43

確かに！私はゲットしにいくって感じでとらえてしましました！『アイテムを取得した』の中でアイテム取得の処理を書くというイメージの方がしっくりくる感じですかね？

好み

の問題と言ってくれているので少し悩みたいと思います！

uhooi commented 4 years ago

これは私も実は『itemCount派』です。どっちも良いと今英和辞書で調べてはっきりしたので『itemCount』に変更したいと思います！

そもそも items.count を返してるので、素直に同じ名前を使っていいと思いました笑

uhooi commented 4 years ago

『アイテムを取得した』の中でアイテム取得の処理を書くというイメージの方がしっくりくる感じですかね？

すみません勘違いしました、、これはViewから呼ばれているメソッドですね。そしたら基本的にメソッドは動詞始まりがいいと思うので、 getItem() がいいと思います！

uhooi commented 4 years ago

このあたりもclassにはすべて final を付けるクセを付けるといいと思います！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Parts/Cell/TableViewCell.swift#L15

継承したくなったら外せばいいので笑

uhooi commented 4 years ago

好みかもしれませんが、cellにqiitaを持たせる必要はないと思います！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Parts/Cell/TableViewCell.swift#L17

uhooi commented 4 years ago

@IBOutlet や @IBAction には基本 private を付けて、外部からのアクセスを防ぐと、UIが外から変更されないことが保証されます！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Parts/Cell/TableViewCell.swift#L20-L22

uhooi commented 4 years ago

ここでcellに分岐が入るのが気になります。 setRead() と setUnread() とセットのメソッドを2つ作って呼び分けると分岐をなくせます！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Parts/Cell/TableViewCell.swift#L38

uhooi commented 4 years ago

とりあえず以上です！

あとは単体テストを書いてみるのがいいと思います。自分はアーキテクチャを「テストを書きやすくするため」にあると思っていて、「あ、ここテスト書きづらいな」と思ったら設計が悪いことが多く、設計の良し悪しに気づきやすくなります！ Viewは手動でテストすることが多いので、まずPresenterからテストを書くのがオススメです〜

uhooi commented 4 years ago

そしたら基本的にメソッドは動詞始まりがいいと思うので、 getItem() がいいと思います！

すみません、 get○○() はゲッターにしか付けたくないので、 loadItem() や fetchItem() などのほうがいいですね〜

sachiko-kame commented 4 years ago

DispatchQueue.main.async はViewでやればいいと思います！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Presenter.swift#L46

viewの処理開始一番最初に書くか、実際に描画している所で書くかの違いなのですかね？

非同期の後に処理が遅くなるということでここに入れているのかな？と思いました。

正直今回の場合描画が遅いとかはなさそうなので『DispatchQueue.main.async』を書く必要がないのかもしれないと今更ながらに思ったりもしました。

非同期の中で描画が遅い時に出すでもいいのかとも思いました、、、

参考: https://teratail.com/questions/52000

ちょっと悩みます汗

sachiko-kame commented 4 years ago

ModelもUIKitのインポート禁止〜！笑 https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Model.swift#L9

すみません！

sachiko-kame commented 4 years ago

これは私も実は『itemCount派』です。どっちも良いと今英和辞書で調べてはっきりしたので『itemCount』に変更したいと思います！

そもそも items.count を返してるので、素直に同じ名前を使っていいと思いました笑

確かに！同じ名前の方がしっくりきますよね！

sachiko-kame commented 4 years ago

『アイテムを取得した』の中でアイテム取得の処理を書くというイメージの方がしっくりくる感じですかね？

すみません勘違いしました、、これはViewから呼ばれているメソッドですね。そしたら基本的にメソッドは動詞始まりがいいと思うので、 getItem() がいいと思います！

英語構文的に i get item(SVO)ですもんね汗修正します！

sachiko-kame commented 4 years ago

このあたりもclassにはすべて final を付けるクセを付けるといいと思います！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Parts/Cell/TableViewCell.swift#L15

継承したくなったら外せばいいので笑

ありがとうございます！そういって頂けるとfinalつけやすくなります。

sachiko-kame commented 4 years ago

https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Parts/Cell/TableViewCell.swift#L17

入れざるを得なかったという感じでした汗 cell内のボタンタップ時に値を送りたくて、、、 modelの方で変えるように実装した方がいいですかね？すごく悩みます汗

sachiko-kame commented 4 years ago

@IBOutlet や @IBAction には基本 private を付けて、外部からのアクセスを防ぐと、UIが外から変更されないことが保証されます！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Parts/Cell/TableViewCell.swift#L20-L22

ありがとうございます！修正します！

sachiko-kame commented 4 years ago

ここでcellに分岐が入るのが気になります。 setRead() と setUnread() とセットのメソッドを2つ作って呼び分けると分岐をなくせます！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Parts/Cell/TableViewCell.swift#L38

確かにそうですよね。viewの方で分岐する方が良いって感じですかね？ viewでの分岐ではアイテムが『あり』『なし』で行っていたのでcellのセットならコードもそんなに多くないしとつい入れてしまいました。

sachiko-kame commented 4 years ago

そしたら基本的にメソッドは動詞始まりがいいと思うので、 getItem() がいいと思います！

すみません、 get○○() はゲッターにしか付けたくないので、 loadItem() や fetchItem() などのほうがいいですね〜

勉強になります！変更します！✨

sachiko-kame commented 4 years ago

とりあえず以上です！

あとは単体テストを書いてみるのがいいと思います。自分はアーキテクチャを「テストを書きやすくするため」にあると思っていて、「あ、ここテスト書きづらいな」と思ったら設計が悪いことが多く、設計の良し悪しに気づきやすくなります！ Viewは手動でテストすることが多いので、まずPresenterからテストを書くのがオススメです〜

ありがとうございます！色々試行錯誤しながら試して考え反映していきたいと思います！本当にありがとうございました！！✨

uhooi commented 4 years ago

DispatchQueue.main.async はViewでやればいいと思います！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Presenter.swift#L46

viewの処理開始一番最初に書くか、実際に描画している所で書くかの違いなのですかね？

はい！描画の直前に書くと、そのメソッドを呼ぶたびに書かなくて済むので👍 あとは好みかもしれませんが、描画しないPresenterにこの処理があると違和感を感じます。

非同期の後に処理が遅くなるということでここに入れているのかな？と思いました。

正直今回の場合描画が遅いとかはなさそうなので『DispatchQueue.main.async』を書く必要がないのかもしれないと今更ながらに思ったりもしました。

非同期の中で描画が遅い時に出すでもいいのかとも思いました、、、

いや、そもそもiOSはメインスレッドでしかUIを更新できないので、データを非同期で取得するなら必須です！試しに外してデバックすると、紫のアイコンで警告が出ますよ〜

uhooi commented 4 years ago

https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Parts/Cell/TableViewCell.swift#L17

入れざるを得なかったという感じでした汗 cell内のボタンタップ時に値を送りたくて、、、 modelの方で変えるように実装した方がいいですかね？すごく悩みます汗

タップしたセルのインデックスをキーに、Qiitaのリストから取得するようにすれば持つ必要がなくなる気がします！手を動かさないとわからないので、できなかったらすみません💦

uhooi commented 4 years ago

ここでcellに分岐が入るのが気になります。 setRead() と setUnread() とセットのメソッドを2つ作って呼び分けると分岐をなくせます！ https://github.com/sachiko-kame/architecture_iOS/blob/feature/MVP/sample/sample/Parts/Cell/TableViewCell.swift#L38

確かにそうですよね。viewの方で分岐する方が良いって感じですかね？ viewでの分岐ではアイテムが『あり』『なし』で行っていたのでcellのセットならコードもそんなに多くないしとつい入れてしまいました。

分岐はできる限り親側（うまく言えない…呼び出し元側、ですかね）で行う派です〜今回は呼び出し元がViewなのでテストしにくいですが、テストしやすくなることが多いので！

sachiko-kame / architecture_iOS

ウホーイが気になったところ #2