自分のホームページのコードをリファクタリングしたので感想
ホームページはこちら
コード概要
エディタは emacs で solargraph + robe で開発しています
リファクタ方針
- ハッシュベースのルーティング定義をクラス化
- rubocop の導入
- TwitterAPI v2 への対応
クラス化について
routing = {
'/about': {
tpl: :about
},
'/apps': {
tpl: :apps
}
}
...
routing.each do |path, info|
get path do
erb info[:tpl]
end
end
こんな感じでハッシュで定義してループで動的にルーティングを定義していたのを
class AboutPage < Sinatra::Base
def initialize
@path = '/about'
@tpl = :about
end
def routing
AboutPage.get @path do
erb @tpl
end
end
end
こんな感じでクラスベースにして Rack::URLMap.new に設定します
メリット
- ルーティングごとに分岐しなければいけない処理が複雑化しない
- xxxPage クラスの親クラスを作って共通処理を抽象化できる
- もしくは Mix-in を使って処理を共通化し特定のページでのみ使い回せる
デメリット
- ルーティングを追加する際にクラスを追加しなければならない
- ファイルが増える
rubocop について
基本は rubocop が出す警告などは全部対応します
基本辛いが特に辛い点
- Metrics/MethodLength
- Metrics/AbcSize
Method に限らず Block, Class, Module が長すぎるという警告に対応するのは大変です
個人的には以下のように対応しています
- メソッドに分割
- 抽象クラスの作成
- One line 記述を使う
- ハッシュなどのデータは外部ファイルに移動して
JSON.parse(File.read('xx.json')) する
rubocop っぽいコードになる
特に以下のような記述が増えます
- ガード
- 細かいメソッドやモジュール、クラスに分割される
- クエスチョンマークを使ったメソッドが増える
- get set などを使ったメソッド名が消える
- freeze がよく出る
__dir__ がよく出る- クラスやモジュールの先頭に必ずコメントが出てくる
do .. end or {} の使い方が明確になる- ハッシュは
{key: 'value'} のような記述になる
- ダブルクォートは使わなくなる
- リテラルが増える
特に2つ目の細かいメソッドやモジュールへの分割ですがこれが個人的に何ともというか本当に良いコードになっているのか微妙でコードを読むときにいろいろなところにジャンプしなければいけないのが辛いかなと思っています
行は増えますが単純に上から処理を辿って行くだけで読み解くことができるコードのほうが自分は見やすいかなと思っています
rubocop は他の言語の linter に比べてかなり厳しい印象です
古いアプリが pending され使えなくなっていたので新しく作り直したのと新しいアプリは v2 を使うようにしました (参考)
gem によっては v1.1 しか使っていないので v2 化していない部分は自作しました
全体通して
個人的にリファクタリングはテストを書くくらい嫌いです
理由はリファクタリングが直接利益につながることが少ないからです
また際限がなくどこまでもできてしまうのと人による好み (バイアス) により記述が変わるので正解がありません
今回のリファクタリングの目的はコードをキレイにしてメンテナンスしやすいコードにするというよりかは Ruby 力を高めるためにやりました
コードフォーマットについて
外部に公開しているコードは linter などを適用したほうがいいと思うのですが Ruby の場合いろいろな linter や formatter があるのでなかなか万人に共通するコードにするのは難しいかなと思います
また Sorbet や Tapioca など静的解析ツールがまだまだどうなるかわからないのでそれに追従すると特殊な記述がコード内にいろいろと現れます
lsp の登場による影響なのかもと思ったりしていますが最近は Type hint がやたら流行っている気がします
型があったほうが堅牢なコードにはなりやすいとは思います
ただ Ruby の場合 lsp のために (エディタのために) コード内に記述しなければいけないコードがあったりするのでそこは少し本質とは異なるので歯がゆいかなと思っています (実際になくても動くのであればないほうがいいのかなと)
Ruby の場合は無理して Type hint しなくてもいいのかなと思っています
Try & Error や Spec テストで補えばいいのかもしれません
テストについて
大規模リファクタリングをするときにテストがあるかないかで負荷がだいぶ変わってくる気がします
特にユニットテストではなく外部からテストするような運用テストなどがしっかり備わっているかどうかで大規模リファクタリングに踏み出せるかどうかが決まると言ってもおかしくないかなと思います
