こんにちは！フリーランスエンジニアの曽根田です。
本日は「Let'sリファクタリング！！」ということで、以下の内容でお届けさせていただきます。
はじめに
スマートキャンプさんには 2020 年 4 月から業務委託としてBOXIL開発チームの一員としてジョインさせていただきました。 スマートキャンプさんでは、前向きに意見交換し、問題解決・プロダクト改善に取り組むことのできる文化があります。
スマートキャンプさん主催のB2B SaaS エンジニア Meetupイベントに登壇させていただき、それをきっかけに交流が始まりました。
素晴らしい文化を持つ開発チームの一員として、実際に一緒にお仕事をすることができるのを大変嬉しく楽しく思っています。
(受け身のような働き方になるのかなと思っていたのですが全く違った!) tech.smartcamp.co.jp
ボクシル開発チームで取り組んできたこと
さて、参戦させていただいてから今までの半年間で、ボクシル開発チームでは様々なことに取り組んできました。 大きく分けると以下に分類されます。
- 新機能の開発
- デザインリニューアル
- インフラのアップデート
- 日々の改善タスク
今回は「日々の改善タスク」の中で取り組んできたリファクタリングについてご紹介したいと思います。
(※実際のコードではなくサンプルコードを用いて紹介します)
また今回紹介させていただくリファクタリングのスコープは、アプリケーションコードの改善を中心とします。
- 不要ファイルの削除
- view のコンポーネント化
- パーフォーマンスの改善(主に N+1)
- DB のリファクタリング(小規模)
- ドメインの責任範囲の整理
- test コードを充実させる
次項の技術スタック概要紹介の画像にもありますが、 現在 BOXIL 本体で採用されている言語はRubyであるため、サンプルのコードはRubyで記載します。 (フレームワークはRuby on Rails)
中上級者の方にとってはあたりまえに思える内容かもしれません。また、派手な話ではないですがご了承いただけると嬉しいです。
リファクタリングに取り組むにあたっての背景
BOXIL のサービスは 2015 年から運営されており、すでに 5 年の歴史があります。 以下の画像は現在 BOXIL 開発チームで運用している技術スタックの概要になります。
BOXIL の PM である笹原さんの記事にもありますが、最近ではエンジニアの人数が今までの 3 倍となり、 改善の規模や内容が濃くなってきております。
エンジニアとしては面白い時期なのではないでしょうか？
新言語の採用なども検討中のようです。
とはいえ、
- 現行動いているアプリケーションが少なくともしばらくは稼働すること
- アプリケーションが整理されている状態が今後の濃い改善への足がかりになること
を考えると日々のリファクタリングは非常に重要な取り組みになります。
また日々のリファクタリングをするにあたって、BOXIL 開発チームでは特に以下を意識して取り組んでいます。
- 不明なドメインを明確化させていくこと
- 可能な範囲で制約を取り入れていこと
- 可読性が良く、パフォーマンスが極端に悪くないこと
リファクタリングコードのサンプル
こちらにサンプルのコードを用意しました。 特にアプリケーションの仕様に意味はありません。 課題をわかりやすく、少しダメなコードを書いています。
以下の順番でリファクタリングを進めていきます！
- DB への問い合わせの削減(N+1 対策)
- 対策 1: categories と category_setting_items を予め読み込んでおき、キャッシュさせておく
- 対策 2: product_category_settings を予め読み込んでおき、キャッシュさせておく/ view での SQL の発行を止める
- 改善結果
- view のコンポーネント化
- 未使用の関数の削除
- 外部キー制約をつける
- NOT NULL 制約をつける
- test コードを書く
以下はそれぞれリファクタリング前後を比較するためのブランチと差分です。
リファクタリング前のブランチ(before_refactor)
リファクタリング前のブランチとリファクタリング後のブランチの差分
Let's リファクタリング
DB への問い合わせの削減(N+1 対策)
http://127.0.0.1:3000/productsへアクセスし、rails serverの log を覗いてみましょう。
なにやら大量の SQL が発行されています。
対策 1: categories と category_setting_items を予め読み込んでおき、キャッシュさせておく
[対応 PR]DB への問い合わせの削減(N+1 対策)-NO1
https://github.com/soneda-yuya/refactoring_sample/pull/1
ActiveRecord の性質上、ActiveRecord に実装されてあるincludesやpreloadを使わないと簡単に N+1 のデータベースアクセスが発生しています。
かといって何でもかんでもキャッシュするのはパフォーマンス上良くなく、基本的には使用するもののみキャッシュするのが良いです。
対策 2: product_category_settings を予め読み込んでおき、キャッシュさせておく/ view での SQL の発行を止める
[対応 PR]DB への問い合わせの削減(N+1 対策)-NO2
https://github.com/soneda-yuya/refactoring_sample/pull/2
よく見かけるのが、Model 内に定義された便利関数を view 内で多用しているケースです。データベースにアクセスしないものであれば良いのですが、関数内でデータベースへのアクセスを行っている関数も時々みられます。(データ有無のチェックなど)
実際に使用する際は、関数内の定義がどのようなものなのか、しっかり把握した上で view 内で関数を呼び出しましょう。
データベースへのアクセスを行っている関数を view 内で呼び出さないといけないケースが出てきた際は、他に良い実装方法がないかを検討することをおすすめします。
また余裕があれば、その関数を使用している箇所をリファクタリングしてあげてください。
改善結果
かなりパフォーマンス改善が行えました。
あえてダメなコードを書いているサンプルコードなので、これだけの差が出ているのですが、 実際のプロダクトでも DB への問い合わせの削減(N+1 対策)を意識することで、大幅なパフォーマンス改善に繋がることもあります。 ぜひ意識してみてください。
view のコンポーネント化
http://127.0.0.1:3000/productsとhttp://127.0.0.1:3000/categoriesにアクセスしてみてください。
http://127.0.0.1:3000/products
http://127.0.0.1:3000/categories
よく見ると(よく見なくても)赤枠の部分が同じ内容ですね。
view をコンポーネント化し、共通で使用することによって、修正漏れを防げたり、管理が楽になるメリットがあります。
[対応 PR]view のコンポーネント化
https://github.com/soneda-yuya/refactoring_sample/pull/3
スマートキャンプさんではデザイナーとエンジニアはすごく寄り添いながらプロジェクトを進めています。
あるプロジェクトでは Atomic Design と Figma を組み合わせ、プロダクト開発しやすいデザインシステムを採用している事例もあります。
React や Vue が登場しだしてからは、コンポーネント化が当たり前の時代になってきているので、デザイナーさんと今まで以上に結束力を高めましょう！
そうすればより実装しやすいデザインになり実装スピードも上がるはずです。
未使用の関数の削除
おや、viewのコンポーネント化の作業の際に使用しなくなった関数がありました。
またコードを読んでいると使ってなさそうな関数があります。
どうやら修正や機能開発の際に、未使用になった関数がそのままになっていることがよくあるようです。
[対応 PR]未使用の関数の削除
https://github.com/soneda-yuya/refactoring_sample/pull/4
新たな機能を開発する際や、リファクタリング際に未使用の関数が残っていると影響範囲の調査に一手間かかってしまうことがあります。 未使用な関数は積極的に消していきましょう。
外部キー制約をつける
おっと。開発中に不具合が発生しました。
調査を進めると Products テーブルの以下のデータが原因であることがわかりました。
Products テーブル
| id | category_id | name |
|---|---|---|
| 1 | 8000 | Borvo the Hutt |
どうやら存在していないカテゴリーが登録されているようで、product.categoryでカテゴリーを参照しようとした際にNULLになっていそうです。
カテゴリーの削除機能もないので、存在しないカテゴリーが設定できるはずがありません。おそらく開発中に意図せず値を変更してしまったのかもしれません。
意図しない値は許容しないように、関連するテーブル同士に外部キー制約を貼ることで、今後このようなことが起きないようにしましょう。
[対応 PR]外部キー制約をつける
https://github.com/soneda-yuya/refactoring_sample/pull/5
今回は開発中に気づいたので、再度 seed でデータを作成し事なきをえました。
既に本番データで不整合が発生してしまった場合は、その状態で外部キー制約を貼ろうとしてもデータベース側のエラーで実行できません。 そのため、データの整合性を整えてから実行する必要があります。
外部キー制約については貼る貼らないの宗派がありますが、個人的にはデータの整合性を担保するために積極的に外部キー制約を貼る事をおすすめします。
NOT NULL 制約をつける
ふとデータベースをみていると見慣れないレコードを発見しました。
NULLが存在するはずのないテーブルにNULLが登録されてしまっています。 おそらく開発中に意図せず値を変更してしまったのでしょう。
意図しないデータが登録できることは良くないので、こういったカラムにはNOT NULL 制約を付けましょう。
[対応 PR]NOT NULL 制約をつける
https://github.com/soneda-yuya/refactoring_sample/pull/6
もちろん全てのカラムにNOT NULL 制約をつければ良いのかというとそういうわけではありません。 NULLを許容するかどうかをよく吟味し制約をつけていきましょう。
test コードを書く
開発をしていて、そういえば test コードどうなってるんだろ...みてみるとない!....みたいなことは時々あるかと思います。
その時の状況でどうしても時間がなかったなど理由があったのだと思います。
test コードがないコードを見つけた時は、積極的に楽しんで test コードを追加していきましょう！
[対応 PR]test コードの追加
https://github.com/soneda-yuya/refactoring_sample/pull/7
慣れてくると開発時に test コードを書く方が効率が良くなってきます。
test コードは通常のコードの記法と少し違い、学習コストが少しかかるかもしれません。
ただし、それ以上の費用対効果を得ることができるので、早いうちに慣れることをおすすめします。
まとめ
上記のサンプルもまだまだ改善の余地がありますが、いかがでしたでしょうか？
BOXIL 開発チームでは新しい技術への挑戦も積極的ですが、上記のような改善にも日々取り組んでいます。
短期的にみると早くコードを書くことが優先されますが、長期的な目線で見ると品質の高いコードがより生産性をあげてくれます。
Let's リファクタリング！！
最後まで読んでいただきありがとうございました！
