目次
リファクタリングに着手するまでの経緯
初めまして！楽楽販売新卒エンジニアのudonrmです。
本記事では、配属後初めての業務として経験した既存コードのリファクタリングについて過程と学びを伝えていきます。
弊社では、3ヶ月間の新卒合同研修を経た後にそれぞれの部署に配属され、そこで配属後研修が行われます。私の所属された楽楽販売では、配属後に検証環境を使用した実践的な開発演習や商材理解を中心に2ヶ月半ほどの研修がありました。9月中旬ごろには配属後研修が終了し、そこから実業務を開始しました。
初めての業務として取り組んだタスクが、コード品質の改善を目的とした既存コードのリファクタリングです。 このリファクタリングは、既存のバリデーションアーキテクチャの改修やクラスの責務の見直し、可読性の向上がメインテーマになります。
バリデーションアーキテクチャ変更の経緯については、弊社過去ブログをご参照ください。 tech-blog.rakus.co.jp
苦労した点や学び
ここからは、リファクタリングに着手する際に特に苦労した点や学びのあった点を観点別に紹介していきます。
仕様を理解する
まず苦労した点に、仕様理解を挙げたいと思います。
楽楽販売は自社の業務フローに合わせて表示項目や操作メニューをカスタマイズできることが特徴の一つで、長所でもあります。ただ、その分どうしても機能数が増えてしまう傾向にあり、仕様理解が難しいことが業務を進めるうえで自分の課題になっていました。
今回のタスクは、配属後研修で軽く触れた程度でほとんど知見のない機能を対象としたリファクタリングでした。そのため、当然ですが仕様の理解にも苦労がありました。
当初の考えとしては、仕様理解はリファクタリングの方針検討やレビュワーとの認識合わせを経て自然と理解していくものくらいの認識を持っていました。
実装の序盤ごろまでは仕様の理解不足による影響は特になかったのですが、時間がたつほどじわじわと仕様の理解不足による影響を感じ始めます。
詳細は省きますが、画面上で使われているとある単語の意味を若干勘違いしていたり、細かい挙動の把握漏れがあったりしました。誤った前提知識を持ったままコードリーディングを進めるので、細部を読み間違えてしまったり、最悪の場合はデグレが起きていることに気が付かなかったりしてしまいます。
結果的に、今回のタスクでは仕様理解に対して以下の学びを得ることができました。
- 適切な仕様理解はコードリーディングや実装の精度向上に役立つ
- 「ながらの仕様理解」には限界がある
- 進捗がストップするのを恐れずに積極的に時間を作るべき
- 違和感を持った仕様は問題ないはずと思いこまずに確認する
- これについては、実装のことで頭がいっぱいで確認を怠ってしまった経緯があります。 正しい仕様を理解しないと実装する必要のない箇所を実装してしまったり、考慮漏れが生まれたりしてしまう可能性があるので特に強く意識していきたいと感じた部分でした。
既存コードを読み解く
リファクタリング着手に当たり、既存コードのロジック把握にもかなり苦労しました。
当然ですが、簡単にスラスラと読めるものではありません。
機能理解が不十分なことや、処理の流れが脳に収まらないことなど原因はいくつもありますが、今振り返るとコードを読む際の取り組み方に問題があったと思います。 ここからは問題点を3点に絞ってふりかえってみたいと思います。
①目的や仮定を持たずに一気に全体を追ってしまう
業務のコードは単に量が多いだけでなく、複雑な仕様が影響してどうしても複雑になってしまいます。楽楽販売のソースコードも同様でした。
当初は、右も左もわからず目についた処理をとりあえず追ってしまっていました。上から下に流れるように読んでいくだけなので、修正箇所の特定にも時間はかかるし、何より脳が疲弊して効率がどんどん下がっていきます。ある日、そのモヤモヤを日報に書いてみたら先輩からこんなフィードバックが来ました。
後日実際にペアプロをしていただき、以下の学びを得ました。
- 命名から役割が自明な関数に対して仮説を立てること
例えば、以下のようなメソッドがあります。ここで命名と返り値だけに注目すると、「ユーザーIDを受け取ってHogeArrayを返すんだな」ということが自明だと思います。
次に、getHogeArrayByUserid() を呼び出す際は以下のようになると思います。
命名から役割が推測できて返ってくる値も把握できている場合には、わざわざgetHogeArrayByUserid() の中身の処理まで追いに行く必要はなさそうです。$hogeArrayの正体に対して仮説を立てる→デバッグを使って検証の繰り返しでコードリーディングが進みます。
当初、目についたメソッドをただひたすら読みに行ってしまっていたので、このアドバイスを受けてコードリーディングが少し楽になったのを覚えています。
こうして書き出してみると当たり前のことだなとは思いますが、当時はコードリーディングがまったく進まないことに焦り、ヒントになりそうなロジックがないかを探そうとして本来読む必要がない箇所まで読んでしまっていたので悪循環になってしまっていました。
②コメントに惑わされてしまう
命名と返り値以外にも、コードリーディングの理解を助ける要素としてコメントがあります。 先輩とのペアプロから得た学びを頼りに、ロジックを深追いしない代わりにコメントを意識して積極的に読むようにしてみました。
結論としては、「仕様理解、機能理解がままならない場合はコメントを読むとかえって混乱する場合がある」という学びを得られました。
実際に経験した例では、実態に即したコメントでなかったり、表現があいまいでなにを指しているのかがわからなかったりするケースがありました。
詳細は省きますが以下のようなコメントがありました。
コメントではターゲットのリストを取得という補足がされていますが、返り値の型がない(リリース当時は型宣言がサポートされていませんでした)ことやターゲットがさす内容がロジックを追わないとわからないことがコードリーディング時の負担になっていました。
実際には中身のロジックを追っていけば取得できるリストは確認できるのですが、仕様が複雑なためターゲットの実態をうまくつかめず不安を抱えたまま読み進めていきました。仕様理解や機能理解があれば、このメソッドの出力結果が使われる場面や使われ方が具体的に想像しやすくなるはずです。
実際には、このターゲットの正体は仕様をしっかり理解していれば特に苦労することなく理解できるものでした。仕様理解や機能理解に不安がある場合は、コメントで感じた違和感を放置せずにひとつひとつ確認していくことが大切だなと感じた場面でした。
③効果的な作業メモを取らない
既存コードを読み解くうえで、作業メモの取り方に関しても課題感を感じていました。
「①目的や仮定を持たずに一気に全体を追ってしまう」の課題と類似しますが、作業メモも自分の思考の流れを一気に書き出しているだけだったので、読み返すのが辛いだけでなく、作業の進捗状況がわかりづらいアウトプットになってしまっていました。
当時の作業メモを見返してみると問題点は以下の2つが考えられます。
- 事実や気づきが入り混じっている
- メモを書いているときは問題ないが、あとから見返す際に信頼すべき情報かどうかの判断にリソースが使われてしまう
- フォーマットや粒度がそろっていない
- メモを追記していく際にどこに追記すればいいか迷う→フォーマットや粒度がさらに崩れてどんどん読みづらくなる
- メモを読み返す際にどこを読めばいいか探すのに時間がかかる
当時のメモをそのまま紹介することは難しいので、動物園に行くためのメモというテーマに変えて当時のメモを再現してみました。(GPT4o生成)
このメモは極端な例ですが、
- 事実や気づきが入り混じっている
駐車場はあるけど使うかわからない。料金高めだった気がする（曖昧）。この書き方は、書いている最中は思考が整理されて問題はないはずですが、量が増えれば増えるほど自分のメモに信頼が持てなくなってしまいます。気づきとして書いたつもりのメモを事実として誤読してしまうと、作業が直感的に進まずもはやメモを読まないほうがスムーズに進みます。これは実際に体験したので少し痛い目を見ました。
- フォーマットや粒度がそろっていない
- 「目的」と「見たい動物」の見出しの粒度が明らかに揃っていません。適切な階層構造になっていないとメモを読み返す際の障害になってしまいます。メモを追記していく際にどこの階層に記述していくかの判断を誤ってしまうかもしれません。実際に当時のメモを読み返してみると階層構造が全体的に統一されていないため、読むのに時間がかかってしまいました。
せっかくメモを取ったのに、メモを見ないほうが進捗がいい気がするという本末転倒な状況になってしまったのでメモの取り方を見直してみました。
改善後のメモはこんな感じです。
まずは、どんなタスクをするとしても共通のフォーマットを使用するように変更しました。タスク単位でissueを+1していけばいいだけなので追記も楽ですし、読み返す機会も圧倒的に増えました。
メモには気づきベースのメモを書き、補足事項には事実ベースのメモを書くことで情報が一気に整理されて読みやすくなったのを実感できました。
このフォーマットが最適解と言い切れるわけではありませんが、無理なくメモを取る習慣を続けられているので今のところうまくいっている感じがします。
適切な命名
命名に関しても、コードリーディング時と実装時双方の場面で何度も頭を悩ませました。
*1リーダブルコードの購読や配属後の開発演習などで、可読性の低い命名がもたらす弊害については認識していたつもりでしたが、修正方法や考え方についてソースコードベースで学べたのは本当にいい経験だったと思います。ここではコードリーディング時と実装時にわけて適切な命名について考えたいと思います。
①コードリーディング時
今回の既存実装は、汎用的な命名や抽象的な命名をしている箇所が多くあったため、直感的にコードリーディングをするのが厳しいなと感じる場面が多くありました。
以下のコードは一例です。
どの変数も命名が抽象的です。例えばこのメソッドの再利用性が高く、汎用的なロジックを提供している場合は具体的な文脈に依存しない抽象的な命名でも問題はないと思います。
ただ、用途が限定されたメソッドで呼び出し箇所が1か所しかないケースなどにおいてはコードリーディングの障害になってしまいそうです。
実体験ですが、中身にどんな情報が入っているかの把握や推測が難しくなってしまいました。デバッグで何とか情報を得ようとしましたが、クラス単位で命名が不適切な箇所が多くあったので全体の構造理解にはかなり苦労したことを覚えています。
不適切な命名の変数を放置したまま実装を進めても、コードリーディング時に無駄なリソースが割かれてしまうのでリファクタリング着手の最初のステップとして全体的に変数名を見直しました。
変数名見直しは主に以下の2点を意識しました。
1.地道にデバッグして変数に格納されているデータの中身を確認する
格納されているデータの把握ができないと方針が立てられないため、デバッグを通じてデータの中身を確認していきます。変更しなくてもよさそうな変数や、不足した情報がある変数をリストアップして修正可能な箇所を洗い出しました。
2.命名規則を決めて粒度をそろえる
一例ですが、以下のような命名規則を決めました。
- リストを扱う配列にはListを付けて
$hogeListのような形にする
返り値がリストであることが自明であるにもかかわらず、変数名からそれが読み取れないケースが多くありました。こういったケースは機械的に修正する方針に決めました。
- 情報量を増やせそうな場合は増やす
デバッグした結果、用途が限定的かつ情報量を足せそうな変数に対しては情報量を増やす方針を決めました。メソッドの呼び出し階層が深いケースでも、呼び出し元までたどる必要がなくなるので以降のコードリーディングに大いに役立ちました。
②実装時
新規で追加したメソッドやクラスの命名も、コードレビューのやり取りを通して何度も修正していきました。
PRの指摘などでいただいたものの中で今後も意識しておきたいと思ったものをまとめたいと思います。
- 変数名を変に省略しようとしない
変数名もそうですが変に省略しようとしない方が大体上手くいきますね。今はIDEが優秀なので長くても別に困りませんし。
下記は簡単な例ですがクラスの命名ではできるだけ汎用的な命名は避けたほうが無難という学びを改めて認識しました。
変に省略するよりも役割を適切に表現する命名にすることで可読性は上がる場合が多そうです。
- 対称性を意識する
*2書籍プリンシプルオブプログラミングの中で対称原理について触れられている箇所があり、以下のようなメリットが紹介されていました。
コードの形に対称性があると、予測がつきやすく、コード理解のスピードが速まります。また、視覚的にも美しいコードになるので、これもコードの理解のスピードを速めてくれます。
対称性を全く意識しない命名にすると、例えば以下のようなコード例になります。
命名に対称性がなく、可読性が低いです。
例えば、insertとdeleteLastは命名の粒度がそろっていません。
insertAnimalはリスト全体に関する操作が想定されますが、deleteLastAnimalは「最後の動物だけ」という限定的な操作が想定されます。このままだと、コードリーディング時に命名から直感的に処理を把握していくことが難しくなってしまいそうです。
次に、ロジックはそのままで命名が対称になるように修正されたコードの例を示します。
命名の粒度をそろえ、add⇔removeとopen⇔closeのようにごく一般的な対称性を意識しました。
命名から役割も推測でき、「登録削除」「動物園の開閉」と観点が限定的になるのでコード理解のスピードが速くなります。
対称性は命名だけでなく設計全般にも役立ちそうな基本的な考え方なので、今後も意識しておきたいです。
まとめ
苦労の多かったリファクタリングですが、チーム内外の様々な方の手厚いサポートのおかげもあっておよそ1か月半ほどで実装~受け入れが無事完了しました。 今回紹介したもの以外にもたくさんの学びがあったので、今後の業務に積極的に役立てていきたいと思います。
最後までご覧いただきありがとうございました。
