Rails でやったらアカン事が大量に詰まったプロジェクトとの戦いを晒す

少し前の話だけど途中からプロジェクトにジョインして色んな意味でヤバい目にあった経験を晒す。
今思うとよくあのプロジェクトの火消しをしてリリースしたなと思う。

※ 当時のプロジェクトの Rails バージョンは 5.1.x です。

開発メンバー

Rails を書くエンジニアは 5 名くらい？いたが、そもそも Rails またはプログラミング自体の知識不足の状態だった。また 1 名を除いてはほぼ新卒orジョブチェンをした若手メンバーで構成されていた。

その 1 名のシニアなメンバーも本業が Android エンジニアだったり、もう一人は個人開発で多くの経験を積んでいたが業務プログラミングが超絶初心者で好き勝手「とりあえず動く (It works)」コードが書き散らされていた。

これだけの人数がいるにも関わらずまともに Rails を理解しており指導できる人が居なかったというのが全てだったと思うし、そのために自分がアサインされた。

まず最初に目についたこと

何と言っても最初コードを見た時にものすごい吐き気と共に冷や汗をかいたのを今でも覚えている。

全てのコードフォーマットに統一性が無くインデントの数が 2スペース, 4スペース, 8スペース, タブ, etc. が書いた人や同じ人でもバラバラ

で、インデントの展覧会状態になっていた。

まずは、 rubocop や slim-lint を導入して全コードフォーマットを統一したのが最初の仕事になった。

rubocop によって発掘・修正されたコードの中には

• SyntaxError
  • せめて動かせよ！
• 古い Rails の記法、非推奨になったものがザクザク
  • ググりながら古い Rails バージョンのコードをコピペしたのだろうな
• 後置 if のつもりが if が無く単に判定結果を返す君
  • return request.path.start_with? '/v1/auth/' => return if request.path.start_with? '/v1/auth/' と書きたかった
  • 動かせx2
• etc.

書ききれない、、

まずは、現在の実装の確認

rubocop を導入したことでとりあえずは「読める」コードになったので現在の実装状況を確認し始めた。 Rails プロジェクトを最初に見る時はだいたい、

• Gemfile
• config/routes.rb
• schema.rb
• app/models
• app/controllers
• app/spec

の順番で読み進める。
Gemfile や config はあまりカスタマイズされていなかったので schema.rb を読み進めようとすると次なる吐き気に襲われた。

DB 設計がヤバい

まずは、全体像を把握するためにも annotate を導入してモデルごとに使用を把握しつつマイグレーションにメスを入れていく。

ここで発掘されたのは

• 外部キー項目が t.references を使わず t.integer を使って書かれている
  • ときには t.string になっている箇所も、、、
  • 当然外部キー制約も index も入っていない
• 必須なカラムなのに null: false が入っていない
  • ここまで来たら驚きもしない

勘の良い人なら気づいているかも知れないが、 Rails 5.1 から id のタイプは bigint が標準になっているので、

• 主キー (bigint タイプ) と外部キー (integer タイプ) が混在している

の対応を行った。

このプロジェクトは開発開始から数ヶ月経っていたので、未リリースにも関わらずかなりのテーブル数になっていた。

モデルを見始めて、、

ここに来て、ようやく Ruby らしいコードを見始める。当然ながらここまでの対応をするにあたってモデルも非常に ヤバい というのは理解していたのでここからが地獄の始まりだった。

ここでの発掘は初心者で良くあるものなので、やっぱりかというものばかりだった。

• そもそもモデルの中身がほとんど書かれていない
  • ビジネスロジックがほぼコントローラwww
• バリデーションが奇跡的に書かれていても、全て validate :custom_validater 形式のメソッドでガッツリ if 文を使って記述
• belongs_to has_many が書かれていない箇所も、、
  • ので a.hoge と本来なら出来る箇所も Hoge.find(a.hoge_id) と書かれていたり、、
• 唯一書かれているバリデーションも i18n を一切使っていないのでエラーメッセージが直書き ( ｰ`дｰ´)ｷﾘｯ

モデルの記述量が無さ過ぎてやることがほぼ無かった。

コントローラの前に routes.rb の見直し

当然このファイルもひどいひどい

• resources は使っているものの get, post のカスタムアクションが大量に
  • 出来る限り resources を利用してデフォルトのアクション以外を使わないようにコントローラも分割
• resource を使うべきところが resources だったり
• post :delete というカスタムアクションがあったりw
  • もう何でもあり

コントローラの鬼リファクタ

ここまで来てようやく大量にビジネスロジックが詰まったコントローラのリファクタに着手。

発掘されたもの、やったこと

• 大量に書かれているバリデーションをモデルへ移す
• 大量に書かれているビジネスロジックをモデルへ移す
  • プロジェクトは Web ページとネイティブアプリ (iOS, Android) を提供するように開発されていたので、Web 用のコントローラと API とで重複したビジネスロジックが個別に存在していた
• ユーザに関するレコードの作成や更新なのにアソシエーションベースのクエリがされていない
  • 大量のセキュリティホール
    • => Hoge.find(params[:id)) を current_customers.hoges.find(params[:id]) に書き換え
  • ほぼ全てのエンドポイントにおいてセキュリティホールが発見できましたw
  • 他の人のファイルの参照・作成・変更・削除は余裕です！
• find find_by がそれほど区別なく利用されている
  • find はレコードがなければ ActiveRecord::RecordNotFound が raise され find_by は nil が返ることを理解出来ていない
  • なので URL 直打ちで id に存在しないものを入れると nil エラーで 500 で落ちること山のごとし
• save と save! も区別なく利用されている、統一感も無し
  • if hoge.save! があったり
  • save! なのに ActiveRecord::RecordInvalid が rescue されていないので余裕の 500 エラー
• N + 1 なんて当然意識されていない
  • eager_load と preload をガシガシ追加
• view で使わないのに @インスタンス変数 が大量に
• 自前ページネーション limit(30).offset((@page-1)*30)
  • 漢は黙って kaminari
• 大量の生 where .where("starttime > now()")
  • モデルの scope なんてものは当然ほぼ書かれていない
• Time.now Time.zone.now Datetime.now 等が全く意識されずに混在してタイムゾーンバグが大量

当然 test/rspec は書かれていない現状で動いているか動いていないかも微妙なものを仕様を想像しながら・動かしながらの鬼リファクタ

最後に view 側のリファクタ

• html 的にヤバいもの
  • 全く構造化文書になっていない
  • id 属性が大量に
  • scss も id 属性ベースで大量にネストして書かれているので手がつけられない
• scss 使っているのに css で書かれている
• jbuilder には json.set! が何故か大量
• Bootstrap を使っているけど適当に Bootstrap class を利用していたり
• eslint も使われていない無法地帯
  • Ruby 側と一緒なので驚きもしない
• 一部 React.js が使って頑張っていたが、1 ファイルが 1000 行オーバー
  • もちろん js のやったらアカンが大量にw

ようやく

ここまで来てようやく最初に吐き気を催した部分は一通り修正が完了し、リリースの目処が立ち始めた。

ここまで大量の一般的にはやったらアカンと言われているものが詰まったプロジェクトのリリースはとても勉強になったし、何故やったらアカンのかを深く理解が出来た気がする。

まだまだ細かくはあった気はするけどもう数年も前の話なので覚えていないが、「Rails のやったらアカン」　= 「Rails way から外れる行為」は現在でも通用するはずなので誰かの参考になると嬉しいです。