Ubiregi Advent Calendar 2018 の20日目です。
@katorieの 第1回ユビテック「ユビレジアプリの起動処理のリファクタリング」 を受けて、ユビレジで行なっているリファクタリングに関して書こうと思います。
リファクタリングの動機
iOSのアプリ開発をしている方ならお馴染みのFat View Controllerと呼ばれる、ViewControllerの責務を超えた処理が大量に書かれているViewControllerが、ユビレジのアプリにも存在しています。
今、作業をしている「起動処理のリファクタリング」というのも、ViewControllerにViewとは関係のないロジックなどの処理がたくさん書かれてしまっているのを整理したい、というのが発端となっています。
「起動処理のリファクタリング」で当面やりたいこととしては…
- ViewControllerからViewに関係のない起動ロジックを別のクラスに切り分けたい
- テストが書きやすくなったロジック処理にユニットテストを充実させたい
- あちこちのメソッドを行ったり来たりしている処理をステートマシンで整理したい
という感じです。もちろん、元の動作と変わらないように作業を行うことが前提です。
ステートマシンを導入する是非
ステートマシンで整理するというのはコードの見た目的には元の状態より複雑になってしまうので、やらなくて済むならやらない方が良いのですが、まず「複雑な状態を複雑なままで全体像を捉える」のに必要ということで進めています。
それ以外にも、「ある処理が実行されてはいけないタイミングで確実に実行させないようにする」という利点もあります。単にコードの整理というだけではなく、より動作を安定させるという意味でも導入する価値があると感じています。
とはいえ、一旦リファクタリングを終えて全体像が見えてより良いアイデアが浮かんだら、ステートマシンを捨ててリファクタリングし直すかもしれません。
レビューするつらさ
さて、ここからはちょっと話を変えて、リファクタリングの作業をどうやって確実に進めるかということを書いていこうと思います。
リファクタリングをするにしても、変更を全部やった後でドンッと一個の大きなPull Requestを出してしまうと、レビュアーの負担がものすごく大きくなってしまいます。しかもリファクタリングですから、必ずしも急いでやらないといけないことでもありません。
例えば巨大なPull Requestを出した場合、
慎重派のレビュアーA「（うーん…変更が多すぎるし、やっていることが複雑で理解できないなぁ…）すみません、ギブアップします…」
と言われてマージできなかったり
イケイケなレビュアーB「（うーん…この Pull Request もう2週間くらい放置しちゃってるなぁ…コードはよくわからないけど動作は問題なさそうだし、まぁいいかなぁ。よし！）LGTM！」
という感じで十分にレビューされずにマージされてしまったりといったことが起きてしまいがちです。
マージ戦略
そのような経験から、1つのフィーチャーが大きい場合には、レビューしやすい単位でPull Requestを作るように心がけています。
今回の「起動処理のリファクタリング」で言うと、ViewControllerの処理を別のクラスへ移動する作業が終わったところなのですが、ここまですでに10個くらいのPull Requestに分けて出しています。全体では20以上のPull Requestを出すことになるのではないかと思います。
どれくらい細かいかというと、「処理を移す先のクラスを空で作ってViewControllerに保持する」くらいで1つ、「ViewControllerにあったプロパティを移動先のクラスに移す」で1つ、「1つのメソッドを移動先のクラスに移す」で1つ、くらいの粒度です。移動したいメソッドは5〜6個あったので、単にメソッドを移動するだけで5〜6個のPull Requestに分けているということになります。
細かすぎると思われるかもしれませんが、レビューをする難易度からすると細かいに越したことはないと思っています。特に今回は、移動元のクラスがObjective-Cで移動先のクラスがSwiftということもあり、単に同じ処理をそのまま移動したことを確認するのも注意が必要なので、1メソッド単位にしたという感じです。1メソッドがそれなりに大きかったというのも一つの理由です。
少し話はそれますが、例えば単なる関数名のリネームという作業をするにも、その関数を利用しているファイルが100個あるとしたら、作業自体はXcodeの機能で一瞬で終わりますが、（たまに動作が怪しい時もありますし）真面目に確認するなら100箇所以上見なければいけないわけで、それは相当な負担になります。
そのような場合でも、一旦リネーム後の関数でリネーム前の関数をそのまま呼び出すだけの状態でラップして、少しずつリネーム後の関数を使うように置き換えていき、全部置き換えが終わったら元の関数を置き換えるという手順で行えば、1回の分量を減らすことができます。時間はかかりますが、変更箇所を少なくすることで確実に作業を進めることができます。
ただし、細かくPull Requestを分けたことによって、マージしたらアプリがリリースできない状態になるのは避けなければいけないと考えています。うまく分けられないからと言ってフィーチャーブランチで進めると、masterブランチと乖離してきてマージが難しくなったり、途中で中断してしまうと作業を続けること自体が曖昧になったりします。
最終的には無駄になる変更が含まれるとしても、masterブランチにマージしてリリースされても良いという状態でPull Requestを出すようにして、確実に進めるように気をつけています（関数のリネームの例であれば、無駄に関数が2つある状態でも気にしない）。
作業内容の共有
そのような感じで1個1個のPull Requestを細かくすると、今度は1つのPull Requestを見ただけでは細かすぎて何のためにその変更をしているのか伝わりにくくなる可能性があります。
GitHubのIssueに全体像をちゃんとまとめてリンクしておくというのは当然必要ですが、ある程度大きな変更をするなら説明会を開いて口頭で説明すると他のメンバーにもより理解を得て協力してもらいやすくなります。
とはいえ、いきなり「説明会するよ！」と言って人を集めるのはちょっとハードルが高いので、ユビレジでは「ユビテック」というエンジニア同士で技術情報を共有する時間を定期的に開催するようにしました。この時間を使って他のエンジニアにやりたいことを共有した上で作業を進めています。
以前はiOSに一番詳しい1人のエンジニアだけがレビューをしがちだったのですが、説明会を開きPull Requestを細かく分割することで難易度が下がり、そこまでiOS開発が得意ではないエンジニアからもレビューをしてもらえるようになりました。今の所はうまく回っているので、しばらくこの方法でリファクタリングを進めていこうと思っています。
まとめ
リファクタリングを進めるにあたって気をつけているのは、まとめると以下の３点です。
- 説明を十分に行う
- レビューの単位を細かくして難易度を下げる
- 常にリリース可能な状態にする