こんにちは。 @starmaine777と申します。
現在、株式会社Diverseにて、PoiboyというマッチングアプリのAndroidアプリを作っています！
Diverse Advent Calendar 2018の14日目ということで、
長くやっているアプリにはつきものである、リファクタリングをゴリゴリ泥臭くしたお話を書かせていただきます。
リファクタリング対象
男性が女性にアピールする画面(以下アピール画面)のカードがスワイプする動きをリファクタリングしました
※こちらはデモなので私が撮影した我が家のかわいいネコチャンが表示されています。実際のアプリではスワイプするたびにかわいい女性が表示されます。
そもそものアピール画面の状態
男性アピール画面が独自のAdapterとContainerViewで死ぬほどゴリゴリやってた
- 先頭カードをview.getAt(0)で取るとか、現在何番目のカードを表示しているかのpositionを5,6箇所で更新していたため整合性とはなんぞやみたいな状態に
- ↑だったので、バグが頻出、何かしらViewが重なったタイミングで連打すると先頭カードがうまく取得できず、先頭カードが最前に残り続けるというやばめのバグが出ていた。
FragmentがFatFragment 1206行
- カードめくったあとにAPI通信してその戻り値からいろんな判断するロジック部分がここに搭載されていた
- もちろんテストなんてない そもそもこのFragmentで叩いてるAPIがとても多いのでMockでテストしようにも前提条件作るだけですごいコード数になる
こうして行きたい 理想形
RecyclerViewとかのAdapterとかでよきにやってほしい
position管理を自前でやっていくとかやってられないので、RecyclerViewのAdapterでお願いしたい！！Tinder系のCardUIなんてどうせ誰か作ってるでしょ！？（身も蓋もない表現
ロジック部分を切り出してテストちゃんと書きたい！！
少なくともCardの増減とかそのへんFragmentで書きたくない！！
Fragmentで！！！ロジックを書くな！！Fragmentは考えずに取って出しみたいなのに徹して！！！！
基本方針
カードをSwipeできるRecyclerViewのAdapter探し
https://github.com/nihad92/SwipeableCards
キミにきめた。意外と Tinder系のSwipeライブラリ+RecyclerViewのAdapterがあんまりなかった。
ロジック部分を完全に切り出し & MVVMに軟着陸させたい
名前だけはMVP+CleanArchitectureみたいな状態で、Presenter〜UsecaseはほぼAPI通信の土管をするだけでさらにFragmentにもロジックを書いていた、みたいな混沌とした状態だったので、
FragmentでやっていたロジックをAndroidArchitectureのViewModelに置き換え。
そのAPI通信ロジック部分もただAPIの結果をそのまま渡すだけだったので、ViewModelの段階でGsonでとったそのままのクラスがやってくる状態で、クラスの階層がやばくテスト時にMock作るのが地獄すぎたので、ViewModelの前の段階で濾過してフラットなObjectに入れ替える作業もついでに行いました。
VM、ならDataBindingを使ったほうがいいのかもですが、個人的にはFragmentのコード内でViewをどう処理するか書いたほうがわかりやすいなーと思っているので、ViewModelがEventを投げてFragmentはそのEventに沿って表示部分を変更する、という方式をとりました。
各種EventはViewModelでSubjectでガンガン出していく
最終的には以下のもののSubjectが作成された
- Dialog(ErrorHandling,SnackBarも含む)
- CardListの変更
- ボタン制御
- tutorialStatusの変更
- ログ送信(ログにcontextが必要だったので)
これを全部Fragment側でsubscribeしてるんですけど、何も考えずにSchedulers.io()でsubscribeOnしてるんだけどこれ良いんだろうか……この辺のSchedulerはどうするのが適任なのかちゃんと理解をしていないですね……
5つもsubscribeするのなかなかだるいですが、なんとかならないかなあ……
リファクタ最中に起きた辛かったあれこれ
カード復活Revive機能
1つ前のカードを復活するというReviveという機能。
Reviveするとスワイプした方から戻ってくるアニメーションがある。通常の画面初期化等だとアニメーションなし。
どう解決したか
スワイプしたタイミングで「1つ前のObject」を保存しておく。そのObjectに「スワイプした方向」の情報を残しておく。
ReviveがされたタイミングでAdapterにわたすリストの先頭に「1つ前のObject」を追加、「1つ前のObject」として保管してあったものはnullに。notifyDataSetChange()する。
Adapter側のonBindViewHolder()で、もしも「スワイプした方向」の情報が入っていた場合にのみその方向からアニメーションするようにすると、通常の追加時にはアニメーションしないがrevive時にはするようになる
EmptyStateを忘れていた
カードがめくりきって0件になる処理と検索した直後に0件だから表示できないよの処理を変える必要があった。(前者は別Fragment呼び出し、後者はEmptyState表示が必要)
どう解決したか
BehaviorSubjectでカードのArrayListをわたしていたのを、Listをプロパティに持つsealed classのStateを渡すように変更
Swipeのタイミングについて
例えば右Swipeする動きでも3種類あった(やめてほしい)
- カード持って右Swipe(アピール)
- アピールボタン押下で右Swipe(アピール)
- スーパーアピールボタン押下でスーパーアピールしたよアニメーション後に右Swipe
で、今回のライブラリだとItemTouchHelperだと1.はonSwipeRight()でしか拾えないし、2,3はswipe()メソッドを呼び出したら1.のonSwipeRight()が呼び出されてしまう……
どう解決したか
Swipe処理指示前にJudgeStateを入れておいて、Swipe後の処理内でそのJudgeStateによって動きを変える
3.の場合はswipeの後に処理が必要だったので、onItemSwipe()時に必ず呼び出すメソッドを作成、JudgeStateがスーパーアピールのときのみに処理するとかしました。
もっと綺麗なやりかたあれば知りたいです。
ボタンでSwipeアニメーションが行われている間にカード触ってるとキャンセルされる問題
ボタンアピールで右にスワイプするタイミングで左にスワイプすると上書きされて左スワイプ扱いになる
どう解決したか
RecyclerViewのタッチ自体をできないようにする。isLayoutFrozenというメソッドを使った。
ボタンタップでisLayoutFrozen=true, ItemTouchHelperのonItemSwiped()でisLayoutFrozen=falesにするようにしたらいい感じになった。
isLayoutFrozenはViewの更新自体もFreezeさせてしまうので、ViewHolderの更新を行うタイミングでtrueだといろいろ問題が出る(itemViewがnullだよっていうのでめっちゃ問題出て辛かった)ので、View更新を行うタイミングでは外してください。notifyDataSetChanged前が正しいかな。
他にRecyclerViewとViewHolder全体を動かなくするメソッドあったら教えてください……
スワイプした瞬間にボタンタップされたらスワイプ重なってぬるぽで落ちる
まんま。いろんな方法でアピールさせたいがためにこんなえぐいことに。
どう解決したか
RecyclerViewのsetOnTouchListenerでDOWNのときはボタンをdisable/UPしたときはenableにする。
このとき離した直後にボタン押されると結局同じことになるので、enableタイミングを500secほど後になるようにする。ボタンでタップした分受け入れてなんてやるものか。
5系でぐりぐりと動かすたびに画像が変になる
レンダリング関連でおかしい……？なにこれ……しかもリリース直前に見つかりよった……
どう解決したか
問題の特定をするために同じライブラリを使った簡単なアプリを作成。動きを見る。サンプルアプリだとまともに動く……
結局画像の角丸clippingのためにそれまで使っていたImageViewのWrapperクラスが問題だったことが判明。
API21でClippingのためのAPIが追加された＋もう4系は切る！！と宣言済みだったので普通のImageView＋OutlineProviderで角丸対応。
リリースぎりぎりで発覚したためにマジで辛くて落ち込みまくったのであっさり修正方法が判明してよかった。
https://qiita.com/ushi3_jp/items/c568e7be76efafbe7de9
最終的にどうなった？
男性アピール画面が独自のAdapterとContainerViewで死ぬほどゴリゴリやってた
→RecycelerViewとSwipeableCardsでスワイプをゴリゴリアニメーションする処理等は書かなくて済んだ！！！
Position地獄からも開放された気がする！あとやばめのバグがだいぶ減った！（いつも実機でバグを発見してくれるお姉さんに「Androidめっちゃよくなりましたよ！」って言われた
FragmentがFatFragment 1206行
→1084行！！そんなに減ってないな！！！388行増、510行減！
多分Dialogの出し分けを一つのSubjectで行っている＆渡ってきたresultのclassが何かのWhen文で出し分けしているので結構ひどいことになっている
(DialogのSubscribe関連だけで50行はあった)
でもロジック部分は全部ViewModelに切り出してテストしたからまだましな状態になっているはず……！
全体のPullRequestでの行数の変化
3,603 additions and 648 deletions.
めっちゃ増えとるやないか。
原因としては
- 今までなかったテストを書いた 結構書いた
- 消してやろうと思っていたCardContainer(820行)＋CardContainer用Adapter(98行)＋CardContainer用Adapterを更にラッパーしたAdapter(90行)がチュートリアル関連で使われていた。そちらもがっつりfragmentにロジックが書いてあったので正直修正する気力がなかった。1000行くらい消せたのか……
- subscribeで結構行数必要だった。.subscribeOn(Schedulers.io())
.observeOn(AndroidSchedulers.mainThread())
.subscribあたりのおまじないがいくつか並んだ
まとめ
やっぱりテストがあるときもちいい
ViewModelくんでContextを排除してJUnitのテストを書いてやったらだいぶ楽になった。逆に、この戻り値使いづらいな！みたいなのもわかった。
ViewModelくんにデータを渡す前のUseCaseで、API叩いた直後の状態のObjectが渡ってきていたのを使うObjectだけに置き換えしたらめっちゃすっきりした。
今後はテストどうやったらやりやすいか考えて行きたいし色んな所のリファクタしていきたい！！
プログラム的にこれでいいのかな？感はある
特にイベント関連。いちいちSubject作ると結構たいへんだし、Dialogの当たりはめっちゃwhen文で分岐した。
めっちゃSubscribeしてるけどメモリとかそのへん大丈夫かわりと不安
条件が複雑すぎる
API通信してからのロジックでの分岐がエグい。ことになった。
条件が難解過ぎだしDialog出しすぎ。今後整理したい……
Sealed Class便利すぎ
実は初めて使ったんですけどめっちゃ便利ねこの子。Stateだらけのマッチングアプリだと使う機会多そう。
想像より時間かかった
2週間でいければいいなと思ったけど1ヶ月以上かかった！！Libraryの使い方とテストをどう動かしていくかみたいなやり方でまずつまづき、テストでつまづき、
あとやっぱりUI関連だと難しい！！連打とかスワイプした直後にボタン押下とか、そのへんの最後の調整で2週間くらいかかった気がします。
新しいライブラリで困ったときはサンプルアプリ
リリース直前で5系がおかしいってなったときにめちゃくちゃ絶望したんですが、簡易サンプルアプリで問題を切り分けたら意外とあっさりと解決しました。
まあ悪かったのは自分側のアプリのWrapperクラスだったので、それを使わない方向で修正。
これでちゃんと切り分けができなかったら無駄にさまよいあるくことになっていたと思うので、ちゃんと落ち着いて使い方あってるかな、みたいなのは確認したほうがいいですね。
（あと最初からminSdkVersionで確認したほうがよかったのかもね……
今後もリファクタリングやっていきしたいと思います！！
リファクタリングは大変ではありますが、終わるとスッキリとしますし、多分今後追加実装等が入ったときに効力を発揮するのかなー？と思っています！
今後もがんばっていくぞ！！！
次回は @nog さんです！