リファクタリング案

[Index197511]

リファクタリングの余地がある部分をここにぶらさげていきます！

[Index197511]

Modelの作成は何かしらのメソッドを介して行う形にしてもよいかもしれません:eyes: よく見るのはof、from、toですね、これらを介すことによって何から作成できるのかがわかりやすいといった点やモデルの生成に対してのテストが書けるようになる利点があります！ また、data classのプロパティ自体にデフォルト引数はあまり書かない方がよいかもしれません。あくまでデータを保有するだけのクラスなので、データを保有する以外の操作(引数がなかった時にいい感じの値で埋める)は持たない方がよいかもしれないです！(言語化が下手で伝わりにくいかもです....) https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/core/domain/src/main/kotlin/com/wsr/password/Password.kt#L5-L10

[Index197511]

value classを利用してもよいかもしれないかも.....? https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/core/domain/src/main/kotlin/com/wsr/user/User.kt#L11-L13

[Index197511]

Roomを使って保存するように今のPRで対応しているので削除してしまってよさそうですね:eyes: https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/core/repository/src/main/kotlin/com/wsr/password/TestPasswordRepositoryImpl.kt#L6

[Index197511]

companion objectがクラス上部にあったり下部にあったり、2行の空白だったり1行の空白だったりするので統一したいですね:eyes: ktlintとか導入してもよさそう！ https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/core/repository/src/main/kotlin/com/wsr/password/TestPasswordRepositoryImpl.kt#L8

[Index197511]

createInstanceという名前が気になりますね:eyes: https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/core/usecase/src/main/kotlin/com/wsr/password/create/CreatePasswordUseCase.kt#L6

[Index197511]

UseCaseの名前からすべてのパスワードを取得してくることは予測ができるので、もう少し関数は単純な名前でもよいかも？ いっそのことこんな風にしてもいいかも？

class GetAllPasswordUseCaseImpl() {
    private lateinit var passwordGroupId: String

    fun setup(passwordGroupId: String) {
        this.passwordGroupId = passwordGroupId
    }

    fun fetch() {
        try {
            _data.emit(State.Loading)
            val id = UniqueId(value = passwordGroupId)
            val passwords = passwordRepository
                .getAllByPasswordGroupId(id)
                .map { it.toUseCaseModel() }

            _data.emit(State.Success(passwords))
        } catch (e: GetAllDataFailedException) {
            _data.emit(State.Failure(e))
        }
    }
}

https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/core/usecase/src/main/kotlin/com/wsr/password/getall/GetAllPasswordUseCase.kt#L8-L12

[Index197511]

runCatching使ってもよいかも？ https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/core/usecase/src/main/kotlin/com/wsr/password/getall/GetAllPasswordUseCaseImpl.kt#L22-L31

[Index197511]

trailing comma(末尾カンマ)があったりなかったりするので統一したいですね:eyes: https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/core/usecase/src/main/kotlin/com/wsr/password/upsert/UpsertPasswordUseCaseImpl.kt#L12

[Index197511]

すべての型がStringなので誤ったデータの詰め替えを防ぐ意味でも名前付き引数を利用したほうがよいと思います！ 僕はよくこういうミスをするので..... またそういったミスを検知する意味合いでもデータの詰め替え部にもテストを書いておくとよいです！ https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/core/usecase/src/main/kotlin/com/wsr/password/PasswordUseCaseModel.kt#L10-L15

[Index197511]

どっちもupdate処理をする部分ではあると思っているんですが、片方はUnitが返ってきてもう片方はUsecaseModelが返ってくるのが気になっています:eyes: 特別な意図がないのであれば統一していきたいですね！ https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/core/usecase/src/main/kotlin/com/wsr/password/upsert/UpsertPasswordUseCase.kt#L7-L14 https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/core/usecase/src/main/kotlin/com/wsr/passwordgroup/update/UpdatePasswordGroupUseCase.kt#L6-L12

[Index197511]

ここもう少し見栄えよくできたりしないかな....？ kotlin in actionもこんな感じの実装だったらこれが正解なのかも....？ https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/utils/src/main/kotlin/com/wsr/state/State.kt#L45-L48

[Index197511]

ここたくさん拡張関数が定義されているのでテスト書きたいですね:eyes: https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/utils/src/main/kotlin/com/wsr/state/State.kt#L3-L7

[Index197511]

ここSharedFlowでイベントを所持していると再講読時にRefreshEventが流れてきてしまいませんか？replay=0で対応できそう:eyes: https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/android/app/src/main/java/com/wsr/index/IndexViewModel.kt#L30-L31

[Index197511]

ここイベントをまとめすぎかもしれませんね.... イベントは可能な限りプリミティブにしていいと思います！ 例えばこの画面で起こるイベントはRefreshを行う、編集画面への遷移を行うの２つだと思います、これらをそれぞれイベントとして作成してあげるのがよいと思います！

class IndexViewModel() {
  val navigateToEditEvent: Event<Unit>
  val refreshEvent: Event<Unit>
}

https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/android/app/src/main/java/com/wsr/index/IndexRefreshEvent.kt#L3-L10

[Index197511]

ここの改行気になります！ https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/android/app/src/main/java/com/wsr/index/IndexEpoxyController.kt#L11

[Index197511]

EpoxyのController内で表示する内容を外から渡すのもいいんですが、渡すアイテムが多くなってくると引数がその分増えて大変なのでresources: Resourcesを渡す形にしてもよいかもしれません！

class IndexEpoxyController(
    private val onClick: (String) -> Unit,
    private val resources: android.content.res.Resources
) : TypedEpoxyController<List<PasswordGroupIndexUiState>>()

https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/android/app/src/main/java/com/wsr/index/IndexEpoxyController.kt#L7-L10

[Index197511]

Fragmentのbinding初期化はonViewCreated内で行うことができるのでお勧めです！ onViewCreated内で初期化を行うことでFragment内にプロパティとしてbindingを持つ必要がなくなる気がします！ https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/android/app/src/main/java/com/wsr/index/IndexFragment.kt#L35

[Index197511]

空行気になります:eyes: https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/android/app/src/main/java/com/wsr/index/IndexFragment.kt#L53-L62

[Index197511]

同じく空行気になります:eyes: https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/android/app/src/main/java/com/wsr/index/IndexFragment.kt#L71-L74

[Index197511]

onViewCreatedはでかくなりがちなので、細かく関数に分けるといいですね:+1: 例えばですが、ClickListener系統をまとめてセットアップする関数、値の監視をまとめて行う関数などにまとめるのがあると思います！ https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/android/app/src/main/java/com/wsr/index/IndexFragment.kt#L43-L48

[Index197511]

changeよりもswitchの方が二値を切り替えるという意図がわかりやすそう？ https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/android/app/src/main/java/com/wsr/index/dialog/IndexCreatePasswordGroupDialogViewModel.kt#L13

[Index197511]

ここの部分、親のViewModelを介して動かす方がよさそうな感じがしますね:eyes: あまりこういう手法を見ないのもそうですが、今回は親のFragmentから遷移する処理や親のFragmentをRefershする処理といった親に依存した操作を行うことが予想されるので猶更そういったアプローチがよい感じがします！ https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/android/app/src/main/java/com/wsr/index/dialog/IndexCreatePasswordGroupDialogFragment.kt#L21-L23

[Index197511]

空行が気になります:eyes: https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/android/app/src/main/java/com/wsr/infra/PassonDatabase.kt#L11-L15

[Index197511]

単純な操作ですがテストを書きたいですね:eyes: https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/android/app/src/main/java/com/wsr/edit/EditUiState.kt#L7-L18

[Index197511]

ここ使われてなさそう？ https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/android/app/src/main/java/com/wsr/layout/LayoutTextField.kt#L66-L74

[Index197511]

やりたい操作は特定のプロパティを更新した新しいモデルを返すことだと思うんですが、命名に統一感がないので統一してもよいかも！ https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/android/app/src/main/java/com/wsr/edit/EditUiState.kt#L12-L13 https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/android/app/src/main/java/com/wsr/show/ShowUiState.kt#L13

[Index197511]

ShowFragmentに限った話ではないんですが、Fragmentの方がViewよりも生存期間が長いのでonDestroyViewで値をクリアするようにしてあげてください！ autoClearedValueというのを作成するのもありだと思います！ https://developer.android.com/topic/libraries/view-binding#fragments https://satoshun.github.io/2020/01/fragment-view-memory-leak/ https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/android/app/src/main/java/com/wsr/show/ShowFragment.kt#L26-L27

[Index197511]

requireContextを使うとよいです！ https://github.com/Wansuko-cmd/Passon/blob/417117c31bf9790e0fded6abd6a5bc4454058eca/android/app/src/main/java/com/wsr/show/ShowFragment.kt#L97

[Index197511]

ざっとしかまだ確認できていないですが、とりあえずxml以外は見ました！ 修正するかどうかはお任せします！ もし修正したor修正しない場合はその旨を残していただけると！