Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: StaffDataRepository.staff getter returns new instance each calls #674

Merged
merged 2 commits into from
May 23, 2018

Conversation

amay077
Copy link
Contributor

@amay077 amay077 commented May 23, 2018

Overview (Required)

ブログを書いていたら、StaffDataRepository に2つの問題があるコードを見つけたので報告しておきます。
アプリの動作には問題となって表れない issue です。

1. loadStaff() を呼ばなくても staff の getter で検索処理が実行される

loadStaff() で検索処理が実行され、staff as Flowable を通じて結果が通じると期待しましたが、loadStaff() を呼ばずに staff プロパティを取得するだけで検索処理が実行されてしまいます。
なので、スタッフ画面の起動時に、スタッフ検索処理(getStaff())が2回実行されていました。

2. staff プロパティは get する度に新しい Flowable のインスタンスを生成してしまう

staff の getter や、 loadStaff() を呼ぶ度に Flowable インスタンスが再生成されています。
このため、 StaffViewModel 側の受信処理で

val staff: LiveData<Result<List<Staff>>> by lazy {
    repository.staff
            .toResult(schedulerProvider)
            .toLiveData()
}

と定義しても、その時の repository.staff は次回の loadStaff() 呼び出しで使われなくなってしまうので、結果として、loadStaff() を2度目以降の呼び出しには StaffViewModel.staff は無反応になります。アプリのスタッフ画面は loadStaff() が意図して複数回呼び出されるケースがないので、こちらも顕在化しません。

override val staff: Flowable<List<Staff>>
get() = getStaff().toFlowable().subscribeOn(schedulerProvider.io())
private val sender = BehaviorSubject.createDefault<List<Staff>>(emptyList())
override val staff : Flowable<List<Staff>> = sender.toFlowable(BackpressureStrategy.LATEST)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Unexpected spacing before “:”

@takahirom
Copy link
Member

👀

@takahirom
Copy link
Member

takahirom commented May 23, 2018

Thanks!
I didnt notice it🙏
LGTM

@takahirom takahirom merged commit a98b363 into DroidKaigi:master May 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants