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

uhooi · 2020-04-17T09:53:36Z

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

uhooi · 2020-04-17T09:54:32Z

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

sachiko-kame · 2020-04-17T09:56:07Z

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

修正します！

uhooi · 2020-04-17T09:57:51Z

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

sachiko-kame · 2020-04-17T09:59:43Z

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

uhooi · 2020-04-17T10:00:12Z

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

uhooi · 2020-04-17T10:01:38Z

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

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

sachiko-kame · 2020-04-17T10:04:33Z

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

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

uhooi · 2020-04-17T10:05:58Z

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 · 2020-04-17T10:06:49Z

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

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

sachiko-kame · 2020-04-17T10:07:55Z

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

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

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

sachiko-kame · 2020-04-17T10:09:05Z

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 · 2020-04-17T10:09:37Z

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

sachiko-kame · 2020-04-17T10:09:38Z

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

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

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

sachiko-kame · 2020-04-17T10:10:40Z

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

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

uhooi · 2020-04-17T10:11:10Z

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

sachiko-kame · 2020-04-17T10:12:27Z

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

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

#2 (comment) #2 (comment) #2 (comment) #2 (comment)

sachiko-kame · 2020-04-17T10:36:30Z

@uhooi

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

76a8d89

uhooi · 2020-04-17T15:09:27Z

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

sachiko-kame · 2020-04-17T15:17:02Z

@uhooi

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

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

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

uhooi · 2020-04-17T15:17:56Z

自分は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 · 2020-04-17T15:20:02Z

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

uhooi · 2020-04-17T15:21:27Z

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

sachiko-kame · 2020-04-17T15:22:52Z

自分は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 · 2020-04-17T15:23:53Z

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

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

uhooi · 2020-04-17T15:27:05Z

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

sachiko-kame · 2020-04-17T16:26:18Z

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

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

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

sachiko-kame · 2020-04-17T16:32:32Z

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

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

sachiko-kame · 2020-04-17T16:34:54Z

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

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

sachiko-kame · 2020-04-17T16:49:22Z

ここで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 · 2020-04-17T17:01:27Z

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

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

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

sachiko-kame · 2020-04-17T17:02:38Z

とりあえず以上です！

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

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

uhooi · 2020-04-18T01:19:19Z

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 · 2020-04-18T01:22:17Z

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

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

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

uhooi · 2020-04-18T01:24:35Z

ここで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なのでテストしにくいですが、テストしやすくなることが多いので！

uhooi · 2020-04-18T01:26:47Z

正直テストがあるだけで、「少なくともテストが書きやすい設計になってるんだな」とわかるので、それだけでも書くべきです！
逆にテストを書かずに良い設計で実装できる人すごいと思います…

sachiko-kame · 2020-04-18T03:28:39Z

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を更新できないので、データを非同期で取得するなら必須です！
試しに外してデバックすると、紫のアイコンで警告が出ますよ〜

確かに処理2回も書くのナンセンスですよね！
具体的に教えて頂いたのでとてもスッキリしました！
ありがとうございます！

sachiko-kame · 2020-04-18T03:37:43Z

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

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

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

cell内では今の所自分の力ではわからなかったのでmodel内で値変更を行うようにしました！
cellのボタンタップ　→　modelのisReadボタンを逆にして登録
という感じです。

sachiko-kame · 2020-04-18T03:43:45Z

ここで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 · 2020-04-18T03:55:45Z

正直テストがあるだけで、「少なくともテストが書きやすい設計になってるんだな」とわかるので、それだけでも書くべきです！
逆にテストを書かずに良い設計で実装できる人すごいと思います…

そうですよね、お恥ずかしながらテスト自体をしっかり理解していないので勉強します！
すぐには出来ませんがより良いコードを書けるようになるために頑張っていきたいと思います！
なかなかこんなに丁寧に指摘して頂けることはなかったので本当に感謝です！
自分は凄い優しい方に恵まれたなと強く感じました！
本当にありがとうございました！
また修正確認後コミットしたいと思います！✨

uhooi · 2020-04-18T04:07:40Z

なんか楽しくなってきたので、自分でも手元でリファクタリングしてますー笑
ただ完全に自分の好みで失礼なレベルでガンガン書き換えているので、PR出すか迷います…

sachiko-kame · 2020-04-18T04:10:09Z

なんか楽しくなってきたので、自分でも手元でリファクタリングしてますー笑

そう言ってもらえると嬉しいです！時間を奪っちゃっているような気がして感謝と一緒に罪悪感もあったので、、

ただ完全に自分の好みで失礼なレベルでガンガン書き換えているので、PR出すか迷います…

意見を聞けるのなら聞きたい願望です。なのでもしよければお願いしたいです！！

uhooi · 2020-04-18T04:12:07Z

自分が勝手にやってるので罪悪感は持たないでくださいｗ
そしたら完成したら上げますね！

sachiko-kame · 2020-04-18T04:24:19Z

ありがとうございます！！✨

修正完了 #2 (comment) #2 (comment) #2 (comment) #2 (comment) #2 (comment) #2 (comment) #2 (comment) #2 (comment) 現在できていない #2 (comment)

uhooi · 2020-04-18T06:35:48Z

超ウホーイの好みにリファクタリングしてみました笑https://github.com/uhooi/architecture_iOS/tree/feature/fix_mvp/sample/sample

diffが出過ぎると思うので、Xcodeを2つ起動してソースを見比べるといいかもしれませんｗ
あくまで参考で、間違っていることもあると思うので鵜呑みにしないでください〜

uhooi · 2020-04-18T06:38:10Z

気になったらここでもTwitterでもどこでもいいので、何でも聞いてください〜✨

sachiko-kame · 2020-04-18T06:42:35Z

超ウホーイの好みにリファクタリングしてみました笑https://github.com/uhooi/architecture_iOS/tree/feature/fix_mvp/sample/sample

diffが出過ぎると思うので、Xcodeを2つ起動してソースを見比べるといいかもしれませんｗ
あくまで参考で、間違っていることもあると思うので鵜呑みにしないでください〜

ありがとうございます！！🙇‍♀️
色々見比べたりして勉強させていただきたいともいます！！

sachiko-kame · 2020-04-18T06:43:21Z

気になったらここでもTwitterでもどこでもいいので、何でも聞いてください〜✨

嬉しいです！
設計強い方が側にいるの心強いです！！
本当にありがとうございます！✨✨✨✨✨

uhooi · 2020-04-18T06:49:37Z

設計強いかはわかりません笑
実はそんなに実務経験がないので、他で通用するのかわかんないんですよね…

sachiko-kame · 2020-04-18T06:59:05Z

強いと思います！
理由がはっきりしているので通用すると思っています。
難しいとすぐ簡単な方に逃げてしまう私とは大違いで反省しています！
ウホーイさんを見習って勉強に励んでいきたいと思います！！✨

uhooi · 2020-04-18T07:00:50Z

ありがとうございますｗ
簡単な方に逃げる考えは間違っていないと思います！
逃げ方ですよねｗ

uhooi · 2020-04-18T07:02:49Z

あ、 setRead() は get○○() と同様の理由でよくないです！
read というプロパティにセットするセッターに見えるので、 setupForRead() のようにするといいです！

sachiko-kame · 2020-04-18T07:06:06Z

あ、 setRead() は get○○() と同様の理由でよくないです！
read というプロパティにセットするセッターに見えるので、 setupForRead() のようにするといいです！

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

ありがとうございますｗ
簡単な方に逃げる考えは間違っていないと思います！
逃げ方ですよねｗ

ありがとうございます！
うまく出来るよう試行錯誤重ねていきたいと思います！✨

uhooi · 2020-04-18T07:24:26Z

あと if isRead == true {} は if isRead {} のように書くのが好みです。
前者は isRead が Bool? 型の場合に使うと、オプショナルかどうかがすぐにわかって可読性が上がります！
（ Bool? 型は if isRead {} と書くとビルドエラーになる）

sachiko-kame · 2020-04-18T08:19:01Z

あと if isRead == true {} は if isRead {} のように書くのが好みです。
前者は isRead が Bool? 型の場合に使うと、オプショナルかどうかがすぐにわかって可読性が上がります！
（ Bool? 型は if isRead {} と書くとビルドエラーになる）

確かにです！！
そう言った見方までできていなかったので非常にためになります！
ありがとうございます！✨

sachiko-kame added the swift swiftについて label Apr 17, 2020

sachiko-kame assigned sachiko-kame and uhooi Apr 17, 2020

sachiko-kame added a commit that referenced this issue Apr 17, 2020

指摘部分の修正

76a8d89

#2 (comment) #2 (comment) #2 (comment) #2 (comment)

sachiko-kame added the MVP MVPのプロジェクトについて label Apr 17, 2020

sachiko-kame added a commit that referenced this issue Apr 18, 2020

指摘部分の修正

7b097e8

修正完了 #2 (comment) #2 (comment) #2 (comment) #2 (comment) #2 (comment) #2 (comment) #2 (comment) #2 (comment) 現在できていない #2 (comment)

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

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

Comments

uhooi commented Apr 17, 2020

uhooi commented Apr 17, 2020

sachiko-kame commented Apr 17, 2020

uhooi commented Apr 17, 2020

sachiko-kame commented Apr 17, 2020

uhooi commented Apr 17, 2020

uhooi commented Apr 17, 2020

sachiko-kame commented Apr 17, 2020 • edited Loading

uhooi commented Apr 17, 2020

uhooi commented Apr 17, 2020

sachiko-kame commented Apr 17, 2020

sachiko-kame commented Apr 17, 2020

uhooi commented Apr 17, 2020 • edited Loading

sachiko-kame commented Apr 17, 2020

sachiko-kame commented Apr 17, 2020

uhooi commented Apr 17, 2020

sachiko-kame commented Apr 17, 2020

sachiko-kame commented Apr 17, 2020

uhooi commented Apr 17, 2020

sachiko-kame commented Apr 17, 2020

uhooi commented Apr 17, 2020

uhooi commented Apr 17, 2020

uhooi commented Apr 17, 2020

sachiko-kame commented Apr 17, 2020

sachiko-kame commented Apr 17, 2020

uhooi commented Apr 17, 2020

sachiko-kame commented Apr 17, 2020

sachiko-kame commented Apr 17, 2020

sachiko-kame commented Apr 17, 2020

sachiko-kame commented Apr 17, 2020 • edited Loading

sachiko-kame commented Apr 17, 2020

sachiko-kame commented Apr 17, 2020

uhooi commented Apr 18, 2020

uhooi commented Apr 18, 2020

uhooi commented Apr 18, 2020

uhooi commented Apr 18, 2020

sachiko-kame commented Apr 18, 2020

sachiko-kame commented Apr 18, 2020

sachiko-kame commented Apr 18, 2020 • edited Loading

sachiko-kame commented Apr 18, 2020

uhooi commented Apr 18, 2020

sachiko-kame commented Apr 18, 2020 • edited Loading

uhooi commented Apr 18, 2020

sachiko-kame commented Apr 18, 2020

uhooi commented Apr 18, 2020

uhooi commented Apr 18, 2020

sachiko-kame commented Apr 18, 2020

sachiko-kame commented Apr 18, 2020

uhooi commented Apr 18, 2020

sachiko-kame commented Apr 18, 2020

uhooi commented Apr 18, 2020

uhooi commented Apr 18, 2020

sachiko-kame commented Apr 18, 2020

uhooi commented Apr 18, 2020

sachiko-kame commented Apr 18, 2020

sachiko-kame commented Apr 17, 2020 •

edited

Loading

uhooi commented Apr 17, 2020 •

edited

Loading

sachiko-kame commented Apr 17, 2020 •

edited

Loading

sachiko-kame commented Apr 18, 2020 •

edited

Loading

sachiko-kame commented Apr 18, 2020 •

edited

Loading