Wansuko-cmd commented 2 years ago

概要

リファクタリングとして指摘していただいたところを修正しました 👍 すべてやると長いので、下のコメントのところまで修正しています https://github.com/Wansuko-cmd/Passon/issues/32#issuecomment-1066029468

実装したところ

coreモジュールを中心としたリファクタリング

ktlintの導入（これのせいで結構な範囲のファイルが変更されています 😇 別プルリクに分けるべきでした）

懸念点

それぞれのコメントに返信欄があればいいのに・・・

https://github.com/Wansuko-cmd/Passon/issues/32#issuecomment-1066020320 constructorからの実行を許さないためにdata classのconstructorをprivateにしようと考えていますただ、data classですので警告がうるさいです普通だと、このまま無視するのでしょうか？それとも普通のclassにするのでしょうか？

https://github.com/Wansuko-cmd/Passon/issues/32#issuecomment-1066022484 KMMでデスクトップ組んでみたいので残しておきたいです逆に、こういうテスト用のリポジトリなどはこんな書き方でいいのでしょうか

https://github.com/Wansuko-cmd/Passon/issues/32#issuecomment-1066023517 データベースに登録等するのではなく、あくまでインスタンスを作成するだけだということがいいたいですどういう名前が多いのでしょうか？

https://github.com/Wansuko-cmd/Passon/issues/32#issuecomment-1066024704 UseCaseの中身を知らないと実行できないような形にしたくないです今回の場合だと、setup -> fetchの順で実行しないと落ちるというのは（同じプロジェクト内にあるViewModel - Viewならともかく）違和感を感じます関数名をfetchに変えるだけでもよさげな気もしますが、他のUseCaseの関数名がfetchにはならないので、ここだけ命名を外すのも・・・という感じです getAllByPasswordGroupId -> getAllとかにするのも考えられますが、引数の型がプリミティブになっている以上これも微妙だと考えています

https://github.com/Wansuko-cmd/Passon/issues/32#issuecomment-1066025073 runCatching関数が書けない？書くとすれば

inline fun <T, U: Throwable> runCatching(block: () -> T): State<T, U> =
    try {
        State.Success(block())
    } catch (e: U) {
        State.Failure(e)
    }

となりそうですが、 catch文の中で型パラメータは使えないです（Why?）ですので、エラーの型がThrowableにならざるを得なくなります。これではエラーの型を持っている恩恵を受けれないのであまりうれしくないです参考：https://github.com/michaelbull/kotlin-result/blob/master/kotlin-result/src/commonMain/kotlin/com/github/michaelbull/result/Factory.kt#L11

的外れな点があれば申し訳ございません 🙇

Index197511 commented 2 years ago

それぞれのコメントに返信欄があればいいのに・・・

めっちゃわかります... 毎回引用しないといけないの大変だよね

https://github.com/Wansuko-cmd/Passon/issues/32#issuecomment-1066020320 constructorからの実行を許さないためにdata classのconstructorをprivateにしようと考えていますただ、data classですので警告がうるさいです普通だと、このまま無視するのでしょうか？それとも普通のclassにするのでしょうか？

constructorをprivateにすると確かに警告出るよね.... 僕はprivateにまでしなくてもよいかなと思っています。それぞれのクラスにしっかりと生成用のメソッドが用意されておりそれらを呼んでオブジェクトを作成する文化がプロジェクト内に整うことが大事なので、privateにして呼ぶことを強制する必要はないかなと思います！

https://github.com/Wansuko-cmd/Passon/issues/32#issuecomment-1066022484 KMMでデスクトップ組んでみたいので残しておきたいです逆に、こういうテスト用のリポジトリなどはこんな書き方でいいのでしょうか

意図があって残しているのであれば問題ないです！～といった意図で残しています！みたいなことをコード内にコメントとして残しておくとほかの人にも意図が伝わるのでよりよいかもです！

https://github.com/Wansuko-cmd/Passon/issues/32#issuecomment-1066023517 データベースに登録等するのではなく、あくまでインスタンスを作成するだけだということがいいたいですどういう名前が多いのでしょうか？

createInstanceという名前がUsecase内の関数として使われていると、PasswordのオブジェクトよりもUsecase自体のインスタンスを作成する意味合いに見えかねないのが今回の懸念点だと思っています..... ちょっといい命名思いつかないので他の人にも聞いてみてください:bow:

Index197511 commented 2 years ago

https://github.com/Wansuko-cmd/Passon/issues/32#issuecomment-1066024704 UseCaseの中身を知らないと実行できないような形にしたくないです今回の場合だと、setup -> fetchの順で実行しないと落ちるというのは（同じプロジェクト内にあるViewModel - Viewならともかく）違和感を感じます関数名をfetchに変えるだけでもよさげな気もしますが、他のUseCaseの関数名がfetchにはならないので、ここだけ命名を外すのも・・・という感じです getAllByPasswordGroupId -> getAllとかにするのも考えられますが、引数の型がプリミティブになっている以上これも微妙だと考えています

この辺は好みですが、せっかく操作とクエリを分けているのでCQSライクな感じにしてもよいのかなと思っています！何かを取得(監視)するUseCase

interface xxxWatchUseCase {
    fun setup(id: ID)
    fun fetch()
}

何かに対して操作をかけるUseCase

interface xxxCommandUseCase {
    fun insert()
    fun delete()
}

みたいな感じを個人開発ではやったりしています！この辺はお任せします！

Index197511 commented 2 years ago

https://github.com/Wansuko-cmd/Passon/issues/32#issuecomment-1066025073 runCatching関数が書けない？

これは僕の勘違いです.... もうしわけない:bow:

blackbracken commented 2 years ago

そもそも論にはなるんですが、data class は単にデータを保持することを目的とするクラスであって、あるデータを変換して保持するものではないです(言語側としても、copy問題や主コンストラクタを隠すと分解構文を使うハードルが上がる)。元来こういったファクトリメソッドを提供する手法はjava時代から受け継がれたものですが、of や from はあるオブジェクトから生成するための(あくまでも)ファクトリーのユーティリティを提供してあげるという意識でいると良いのかなと思っています 💭

blackbracken commented 2 years ago

あとファクトリメソッドについて加えて気になったんですが、from や of には一般的な命名パターンがあって、そういった名前を使うなら意味合いもそれに沿った命名にしても良いかもしれません 💭 https://qiita.com/nyandora/items/3e5ec76ca3881bc17924#%E7%9F%AD%E6%89%80

blackbracken commented 2 years ago

{ } のような何もしないラムダ式は、Unit を返すようにするか、/* do nothing */ のようなコメントを書いて、敢えて何もしていないことを明示すると分かりやすくて良いかもしれません ✉️

Wansuko-cmd commented 2 years ago

https://github.com/Wansuko-cmd/Passon/pull/34#issuecomment-1068664579

この件についてじっくり考えたのですが、時間がないのと、現状のレベルで十分良さそうなので、変更せずにやらせてください 🙇

ただ、この方式自体はめちゃくちゃ興味ある（正直UseCaseのできることがCRUDなのが変な気がしてた）ので、今度の夜にでもよければもう少し教えていただけると嬉しいです！

Wansuko-cmd / Passon

リファクタリング案の適用その1 #34

概要

実装したところ

懸念点