去年の夏頃から株式会社LITALICOでエンジニアをやっています。@ti_aiutoです。
この記事は「LITALICO Engineers Advent Calendar 2019」の17日目の記事です。
LITALICO Engineers Advent Calendar 2019 - Qiita
https://qiita.com/advent-calendar/2019/litalico
はじめに
背景
下半期から「改善大臣」に任命されて、開発環境の整備やリファクタリングを推進しています。（弊事業部のエンジニア間では、お互いへの敬意を込めて？「○○係」とかではなく「○○大臣」と呼び合う慣習？があります。）
チームに加わってまだ長くはないのですが、だからこそ気づくことや気になることもあるだあろうということで、そういう視点から、少しずつ課題を探したり、ディスカッションしたり、変更を加えたりしています。カマス実験みたいな感じです。
この記事では、その中で考えたこと、実際に取り組んだことや取り組んでいることについてご紹介します。
技術書を何冊か読んだ感想
上半期から業務時間の1割ちょっと＋プライベートの時間を使って、古典？の技術書を読み込む運動をしていました。取り組みについて紹介する前に、この記事に関連する箇所で気に入った部分を紹介します。
Refactoring - Improving the Design of Existing Code(2nd Edition)
「既存の構造を先に変更したほうが作業を効率的に進められることがあり、その下準備こそが『リファクタリング』なんだ」という点が強調されていたと思います。リファクタリングはプログラミングと分けて考えるものではないし（もちろん通常の開発と分けて長期的に進めるべきこともある）、コードの見た目が醜いからやるというものでもない（きれいなコードでもリファクタリングは必要）、という話もありました。
作業の進め方については、細かくコミットすることと、変更を加えたらすぐにテストを回すこと、壊してしまったら作業を破棄して最後の作業からより細かい粒度でやり直すことなどがアドバイスされていました。
『エリック・エヴァンスのドメイン駆動設計』
この本は原著と邦訳で合わせて二周読みましたが、全て理解するにはあと何周読めば良いのかという感じです。
リファクタリングに言及している箇所でいうと、エンジニアにリファクタリングで工数を使わせないことについて、「変更を完全に正当化できるまで待つのは、待ちすぎというものである。」「ソフトウェア開発は、変更することで得られる利益や、変更しないことで生じるコストを正確に割り出せるような、予測可能なプロセスではない。」と書かれている箇所があります。エンジニア自身に向けたメッセージというよりは、それを取り巻く環境について言っている感じがします。何にしても、手遅れになる前に（ぎこちないコードが拡散する前に）早めに手を付けよう、という話です。
それよりも印象に残っているのは、本文を通して何度も登場する「高凝集・低結合」の考え方と、次の箇所です。
ある開発者があるコンポーネントを使用するために、その実装についてじっくり考えなければならないのであれば、カプセル化の価値は失われる。もともとそのコンポーネントを開発した人とは別の人がオブジェクトや操作の目的を推測する上で、実装を確認しなければならないとしたら、その新しい開発者は、操作やクラスが偶然満たしているだけのものを目的と思ってしまうかもしれない。そうして推測された目的が本来の意図と異なっていたら、コードはさしあたり動くかもしれないが、設計の概念的な基盤が崩壊し、2人の開発者は互いに矛盾した目的に向けて仕事をすることになるだろう。
手の込んだ仕組みはすべて抽象的なインターフェースの背後にカプセル化し、またそうしたインターフェースには、手段ではなく目的の観点から語らせなければならない。
（邦訳版p.251)
そもそもカプセル化にどんな意義があるのか、それが欠けると何が困るのか、何を目指してカプセル化を行うべきかが、この段落を読むだけでよく分かります。「意図の明白なインタフェース」(intention revealing interfaces)という表現も度々登場していました。
コードに語らせるべきことについては、次のような記述もありました。
優れたオブジェクト設計の本質は、各オブジェクトに明白で限定された責務を与え、相互依存関係を最小限に減らすことである。だが時には、チームでの交流を、ソフトウェアにおいてあるべき姿と同じくらい整理しようとしてしまうことがある。だが、うまくいっているプロジェクトには、他人のことに首を突っ込む人々が多い。開発者がフレームワークを試し、アーキテクトがアプリケーションコードを書く。全員が全員と話をする。これは効果的な混沌だ。オブジェクトをスペシャリストに仕立てて、開発者はジェネラリストにすること。
（邦訳版p.502)
「開発者が過度な役割分担で専門分化するのではなく、ジェネラリストとして他の役割の開発者やビジネス側のメンバーとも関わるようにすることで、全体でよりよい設計にたどり着ける。その分、オブジェクト（設計されたもの）を分かりやすく、知識を濃く反映したものにすれば良いんだ。」というふうに読みました。
あと、この本だったか自信がありませんが、「どんな設計も作ったときにはそれが一番だったんだ」みたいな話もありました。これも重要な視点です。
（これか『レガシーコード改善ガイド』か『パターン指向リファクタリング入門』のどれかのはずです。）
『レガシーコード改善ガイド』
この本は一言でいうと「コードベースにどうやってテストコードをねじ込むか」の話と言っていいと思うのですが、どうしてテストが必要なのかという点で、序盤の「編集して祈る」か「保護して変更する」かの話がわかりやすかったです。
テストコードがなければ、既存のコードを壊してしまっても、元々の振る舞いが維持されているかどうかは動かすまでわからないので、何も壊れていないことを「祈って」人力で動作を確認するしかありません。
一方、テストコードで既存の振る舞いを明示していれば、それが壊れたときにすぐに気づくことができます。この「既存の振る舞いを固定しながら変更を加えられる」という点を「ソフトウェア万力」と表現しています。動いてほしくないものを固定して作業する様子は確かに想像しやすいです。
作業方針
大まかな方針は、先ほどの技術書の感想に沿っているつもりです。
なお、リプレイスやマイクロサービス化のような大きな（中長期的な）変更についてはまた別で話が上がっているので、その範囲までは考えていません。
今いる場所から漸進的に
どんなコードも、書かれた当時には総合的に判断してそれがベストだったからこそ、そう書かれているわけです。そうして積み上げられてきた資産を活かしつつ、これからの変更を進めやすくするためにはどうしたら良いのか、どんな点が開発スピードを落としそうなのか、という点から考えています。
大まかな指針としては「高凝集・低結合」
「本来関係がないように見えるところが実はつながっていた」とか、「本当はあまり関係ない機能の細かい仕様まで気にしながら開発をしないといけない」とかといった状態だと、大きく分けて二点の問題があると思います。
- 目の前の開発以外と関係が薄いことまで気にする必要があるため、必要以上に認知資源を消費して、作業効率が下がる
- 変更の影響範囲が広がりやすくなるため、変更漏れ、変更ミス、不具合や誤作動が起きやすくなる
この状態を改善するために、主に次のような方針で作業を行っています。
- 関連の薄い機能同士、開発スピードに大きな差のある機能同士を分離する
- コード同士の依存関係を分かりやすくする
- 処理の詳細はメソッドやクラスの中に隠して、それを使う側は抽象的に、目的中心で呼び出せるようにする
コードの振る舞いを明示する
後からコードを変更した開発者（未来の自分自身も含めて）に、「どうしてそんなことしたんだ！」と文句を言うのは簡単ですが、テストコードや静的型付けによってコードの期待されるふるまいが明示・固定されていれば、そもそもそういうことにはならないはずです。
プルリクエストやコメントも活用しつつ、コードそのものやテストコードでそのへんを表現していくのが重要だと思います。
開発環境
具体的には次の技術を使って開発しています。
- Rails 5.x(Ruby)
- Sprockets + webpack
- SCSS
- jQuery(JS)
- Vue.js 2.x(JS, TS) + Jest(JS, TS)
（※この記事を読む未来の後輩？に向けて念のため補足をすると、jQueryのコードは保守がメインで、新規に書くことはほとんどありません。）
Railsアプリのリファクタと言いつつも
タイトルは「Railsアプリのリファクタ」としていますが、以下の内容はフロントエンドの内容が多くなっています。
困ったところから作業した結果そうなったのですが、この背景としては、サーバサイドの実装で機能ごとにModel, layouts, Helperの実装が分かれているために、関連の薄い機能同士のコードが混ざることが少ない、というのがあると思います。
活動報告・活動予定
Jestの導入（完了）
（これは改善大臣に任命される前にやったことですが、今の開発の下地になっているのこれも入れておきます。）
課題と目的
テストコードの重要性については技術書紹介のところに書いたとおりです。
Vue.jsのコードが増えてきて、しかも複雑になってきたところで、JSのテストフレームワークのJestを導入しました。
作業内容
テストフレームワークを導入したら、あとはひたすらテストを追加していくのみです。
主に次のようなところをテストしています。
-
methodsの単体テスト（引数の値を変えながら） -
computedの単体テスト（Vueインスタンスの状態を変えながら） -
createdやmountedの処理について期待した振る舞いのテスト - 外部APIの呼び出しなど副作用のある処理のモック・検証
-
v-ifの出し分けの中でも重要な箇所について、DOM要素の表示非表示が切り替わっていることのテスト
テストに使うVue.jsのインスタンスをどうやって初期化するのか、propsに渡す値のダミーデータをどう用意するのかなど、まだ方法を試行錯誤している箇所もいくつかありますが、ひとまず意味のあるテストは書けていると思います。
JS, CSSファイルを機能ごとに切り出す（一部完了、保留中）
課題と目的
これまで、一部の機能で新しいVue.jsのライブラリを導入したり更新したりしたときに、その影響が他の機能にまで広がることがあったので、新規のライブラリの導入を必要以上に控えているところがありました。特にUI関連のフレームワークやライブラリは影響が広がりやすいです。
ほとんど変更しない機能を壊さないために、開発スピードの速い機能の作業が滞るのは困るので、JSとCSSを別のファイルに分けてしまうことにしました。
作業内容
次のような手順で行いました。
- JSファイル(A)の中で、各機能で共通して使われている部分と機能固有の部分を特定し、コメントなどで整理しておく
- 新しいJSファイル(B)を作成する
- webpackの
entryを変更して、共通(A)＋固有の入力(B)からそれぞれJS, CSSが出力がされるよう変更する -
layoutsを編集して、(3)で新しく設定したファイルが読み込まれるようにする - 動作を確認しながら、(A)の機能固有の部分を(B)に移す
単純といえば単純ですが、（主に精神的な面で）効果は大きかったと思います。
ただ、ファイルを分割するとその分リクエスト数が増えてパフォーマンスに影響する可能性があるので、最低限の作業ができたところでいったん保留にしています。
グローバル変数・メソッドを減らす（進行中）
課題と目的
だいぶ前に書かれたコードの中には、グローバルに定義されたjQueryやVue.jsのコードを使って、グローバルに定義された関数を起動して何かする、というコードが数多くあります。かつてのJSは依存関係や可視性の管理は全てユーザ任せだったので、そういうコードになるのももっともかもしれません。
ただ、グローバル関数や変数というのはどこからでも変更できてしまうので、間違えて変更したとか二重に定義したとかいうときに、想定外の挙動する可能性があります。また、参照している側から見れば処理がどこに定義されているのかわかりにくくなりますし、コードを変更したときの影響範囲もわかりにくくなります。
幸いなことに、今のフロントのコードにはSprocketsに加えて、既にwebpackが導入されています。どちらもJSファイルの下処理をやってくれますが、webpackを使うと何がいいかというと、モジュール管理の機能が使えるようになるところです。これにより、使いたいコードを、使いたい場所だけに持ってきて利用することが可能になり、グローバル空間を利用する必要がなくなります。
作業内容
次のような手順で作業を進めているところです。
- グローバルから除きたいオブジェクト（関数や変数）を一つ決める（検証が大変なので一つだけにします）
- Sprockets側の対象から除いてwebpack側の対象に加える（よほどタイミングにシビアな処理でない限りそのまま動きます）
- グローバルに定義されていたオブジェクトを、
exportするコードを準備する（または準備されていることを確認する） - (2)で移動した各ファイルで、(3)のコードを
importするよう変更する - グローバルに定義されていたオブジェクトを削除する
まだ一つしかできていませんが、これも検証が大変なのでまだ時間がかかると思います。
JSファイルのTS化（進行中）
課題と目的
開発を進めやすくするためにも、メソッドや関数の使い方を分かりやすくするためにも、コードの振る舞いを固定するためにも、TS化は役に立ちます。
先輩がTypeScriptの導入はしてくださっていたのですが、実際問題コードが書かれてはいなかったので、Vue.jsのコンポーネント定義を中心に徐々にTSにしています。
作業内容
これはひたすら変更していくのみです。テストがあればなおよし。
ただし、TS化が完了する前にeslintのTSプラグインを入れると全JSにWARNINGが出るので要注意です。
一部のモデル更新のAPI化（着手前）
課題と目的
一部の共通機能のモデルの操作（updateやupdate_allなど）が、複数のModel, Controllerなど様々な場所から呼ばれているところがあります。item.update(hoge_flag: true, fuga_flag: false)のような感じです。
これはこれで動いていますし、慣れれば特に困ることもないのかもしれませんが、特定の機能のモデルやコントローラが、関係の弱い共通機能のモデルの詳細なデータ構造について知っていないといけないというのは、利用する側としては少し荷が重くなります。また、不変条件（値同士の関係、片方が○○ならもう片方□□であることのような）が維持される保証もありません。
実際にこれらのコードをコピペした場面が何回かあったのですが、色々調べるのが大変でした。
これらの処理をAPIとして適切に隠蔽することで、上記の問題が緩和されますし、将来マイクロサービス化のような話が出たときにも、変更が進めやすくなります。
作業内容
まだ未着手なので具体的なことは決まっていませんが、次のような手順になると思います。
- 既存のコードをよく調べる
- テストコードが準備されていることを確認する（なければ準備する）
- データ更新・読み出しの目的ごとに該当箇所を分類する
- (3)ごとにメソッドを定義する
- (4)のメソッドを使うように変更する
- 何も壊していないことを確認する
ただ、update_allを使う操作については、処理をどこに書けば良いのか悩ましいです。ActiveRecord::Relationのクラスに直接定義するわけにもいきませんし、かといってクラスメソッドに引数で対象を渡すのも使いやすいと言えるのか怪しいですし、まだ検討が必要そうです。
データウェアハウスのDB分割（一部完了、保留中）
課題と目的
Railsアプリケーションからデータウェアハウス(BigQuery)に格納する際に、全機能のデータが一緒に格納されています。
基本BigQueryは処理対象のデータ量で課金されるので、比較的リクエストの少ない機能のログをみたいときに、全機能のログを一気に処理にかけるのは無駄が多すぎます。また、同じ一行でも、読み出すカラムが軽いほど安くなるので、一つのカラムにJSONなどで値を格納するよりは、複数のカラムにはじめから分けたほうが得策です。
作業内容
アクセスは比較的少ないが走らせるクエリが多い機能について、テーブルを分割しました。また、JSON形式でデータを保持しているカラムについては、よく使う値を別のカラムに展開するようにしました。
これにより、処理対象が70GBから300MBまで減ったクエリもありました。
Vue.js関連のリファクタ（進行中）
課題と目的
Vue.jsは（Angularと比較した印象では）自由度が高いと思うのですが、その分自力で設計を工夫・整理しないといけない部分が多いと思います。
API呼び出しの処理の書き方がコンポーネントごとに違うとか、共通処理の書き方が場所によって違うとか、コンポーネント同士の連携が交錯していてスパゲッティ直前になっている箇所があったりとかで、開発が進めにくくなっています。
なんとかリリースまでこぎつけたところで、少し時間をかけてリファクタしていこうという話が出ています。
作業内容
コンポーネントの責務を、大きく通信やアプリケーションロジックを含むものと、UI関連に特化するものに分け、後者の中でもレイヤを2つに分割して再利用しやすくしているところです。（先輩の発案を話し合って設計して、実装を進めているという感じです。）
一部のコンポーネントで実装を進めているところですが、最上位のコンポーネントはテストが薄いorないこともあって、作業がかなりゆっくりになっています。
あとは、通信の処理の書き方もコンポーネントによってバラバラなので、永続化（通信）の処理を、DDDの本のような形でRepositoryに切り出して、そのRepositoryの生成もFactoryに任せる、というような形で責務を分けて、統一した形で書けるように直す予定です。（これも先輩の発案ですが。。）
この辺はある程度進んだらまだ別で記事にしたいところです。
結論
ということで、上に書いたようなことを少しずつ進めています。もちろんメインのプロダクトの開発もあるのでなかなか時間がとれないときもありますが、自分含めて全体の開発の効率アップにつながったり、新しいメンバーの学習コスト削減につながったりする部分については、もっと積極的に進めていきたい思っています。
それにより、先輩方々から受け継いできたコード（によるサービス）で、少しでも長くユーザの皆さんに価値を届けられたらなと思います。
明日も私@ti_aiutoがアドベントカレンダーを担当します。
久々にSQLについてがっつり勉強＆練習したので、それについて記事を書きます。