「リファクタリング専門チームによるお金周りリファクタリング」を斜め読みして思ったこと

engineer.crowdworks.jp

これ。すごく読みやすいし良い記事だった。

再現性のある取り組みだと思うのだけど、微妙に補足しといたほうが良いのかなーと思うところもあり、メモがてら書いておく。

専任が引っ張る体制

属人化を嫌って、「技術的負債はみんなで対処するぞ!」「そのためのコーディング規約とかを整備するぞ!」とかとかやりだすと、続かない。

2016年頃に、自分がまだRails初心者だったころにこんなことがあった。

class HogeHogeFeedbacksController < SecureApplicationController

  def create
    feedback_params = params.require(:feedback).permit(:content, :score, :version, :device_name)
    feedback = HogeHogeFeedback.new(feedback_params)
    feedback.user = current_user
    if feedback.save
      ...
      redirect_to ...
    else
      render :new
    end
  end
end

っていう感じの、ほぼほぼRails Wayに乗っかるようなコードをPull Request出したときのことだ。

レビュー時に「サービスクラス作れ」「サービスハンドラークラス作れ」とかとか言われた。「コード量が増えるだけじゃん?」と初心者ながらに反論したものの、受け入れられず、無駄に記述量だけが多いコードを書くことになった。

class HogeHogeFeedbacksController < SecureApplicationController

  def create
    HogeHogeServiceHandler.new(
      content: params[:feedback][:content]
      score: params[:feedback][:score]
      version: params[:feedback][:version]
      device_name: params[:feedback][:device_name]
    ).handle

    ...

    redirect_to ...
  rescue HogeHogeServiceCreationError => e
    render :new
  end
end
class HogeHogeServiceHandler
  def initialize(content, score, version, device_name)
    @content = content
    @score = score
    @version = version
    @device_name = device_name
  end

  def handle
    HogeHogeFeedbackCreateService.new(
      content: @content,
      score: @score,
      version: @version,
      device_name: @device_name
    ).perform
  end
end


class HogeHogeFeedbackCreateService
  def initialize(content, score, version, device_name, current_user)
    @content = content
    @score = score
    @version = version
    @device_name = device_name
    @current_user = current_user
  end

  def perform
    feedback = HogeHogeFeedback.new
    feedback.content = @content
    feedback.score = @score
    feedback.version = @version
    feedback.user = @current_user
    feedback.save!
  rescue ActiveRecord::RecordNotFound
    ...
  end
end

あまり覚えてないけど、たしかこんな感じの構成に書き換えたと思う。そしてレビューが通ってリリースされた。(そして、3ヶ月後くらいに変数名のタイポを発見したw)

Railsを普通に使ってる人が10人いたら、おそらく10人全員が変更前のコードを好むだろう。なのになぜタイポのリスクが高い後者のようなコードが受け入れられたのか?

 

そこに「ルール」があったからだ。

Railsをあまり知らない人でもとりあえずルールに従っておけば間違いない」みたいな解釈がされたルールがあったんだと思う。

 

課題のないところをルールで縛るとか、冷静に考えるとありえない話。なんだけど、 属人化を排除しないといけない!って思うと、「なるべくルールの例外はつくるべからず」みたいな判断が働いてしまう のだろう。しらんけど。

 

ともかく、今回MinoDrivenさん(とkinakoboさん?)がかなり引っ張っていろいろやってるのが大きいんだろうなーーーーーってブログ記事読んで思った。

すごく大変だと思うのだけど、こだわり持ってる人がとにかく突き進めていくのがこういう成功事例につながってるんだと思う。

間違っても「全員がこういう美しいコード書けるように整備しよう」とか考えないでほしいなって思った。

現状理解と斬新的な改善

現状を納得する必要はないけど、少なくとも動いてしまってるコードである以上は理解する必要がある。

  • 誰がどういう期待をもって使うものか
  • かりに全部作り変えたとして、残さないといけないものはどれか、捨ててもいいものはどれか

などなど、いくら現状がクソコードで納得しがたいものであっても、理解はしておかないといけない。

  たしかMinoDrivenさんと一緒にリファクタリングやってるkinakobo氏は、「実は使われていないメソッド」みたいなのを地道に消す取り組みとかもやってた気がする。

今回の記事には関係がないけど

qiita.com

あたりを読んだときも、似たようなことを感じた。

 

「こんなの1から作り直してやる!」から始めてはいけないと改めて思った。

「DDDで」とか「クリーンアーキテクチャで」とかは解決策ではない

自分がうっっっっっっすら記憶してる限りだと、お金周りのコードはモデルがファットなだけじゃなくて、コントローラもそこそこボリューミーだし、サービスクラスもいくつか絡んでいる、みたいな構造だったはず。(自分みたいな素人がなんとなくリファクタやってくれっていわれたら、多分コントローラとかサービスクラスを分解して、闇雲に何かをやる気がする)

そんななかで、モデルの役割整理にフォーカスを絞っているのが、成功の鍵なんだろうなーってうっすら思った。

・・・てか

リファクタリング専門チームによるお金周りリファクタリング」に書かれてないような前提の補足をして、より理解を深めて読んでもらいたいなーから書き始めたんだけど、なにも貢献できてない気がする。

やはり自分には文章力が足りない。