RuboCopのMetrics/AbcSizeで時間を浪費するのそろそろやめませんか？
はじめに
RuboCopはRubyのコードの静的解析を行ってコードをきれいに保つために大変便利なツールです。
しかし、うまく運用ルールを決めないとRuboCopの警告だらけのコードになったり、警告を無視するために# rubocop:disableコメントが乱立したりと逆にストレスを生み出す原因にもなってしまいます。
-
Metrics/AbcSizeの基準値はいくつに設定したらいいんだろう・・・ - 警告が出たからリファクタリングしないといけないっぽいけど、これ以上削るところがない・・・
- 仕方ないからひとまずdisableコメントつけちゃえ
今回は、おそらくRailsを触る多くの人が1度は悩んだことがあるMetrics系の設定についての考えを記事にします。
これが絶対的な正解というわけではなく、あくまで1意見として捉えていただけると幸いです。
最初に結論：Metrics系は思い切ってOFFにしよう
最初に結論をいうと、Metrics系はOFFにした方が開発体験がいいと考えています。
.rubocop.ymlに以下を記述するだけです。
Metrics:
Enabled: false
いろいろ反論もありそうなので、以下でこの考えについてもう少し深掘りします。
RuboCopの警告を大きく2種類に分けてみる
RuboCopの警告を大きく2種類に分けると、以下に分けられると考えています。
- 誰が見ても直すべきもの
-
a=b+cよりもa = b + c - インデントを揃えるとか
-
- 直すべきかどうかがケースバイケースなもの
- まさに
Metrics系の警告が出るもの
- まさに
このうち、誰が見ても直すべきものはRuboCopが活躍します。
インデントが揃っていなかったときにその指摘を人間がするのはナンセンスなので、RuboCopに任せるべきです。
しかし、Metrics系は直すべきかどうかがケースバイケースです。
ロジックが複雑になっていてリファクタリングすべきケースもあれば、それ以上シンプルにできないケースもあります。
なので、結局その警告にしたがってコードを修正するか無視するかは最終的に人間の判断になります。
具体例：Metrics/AbcSizeを守ることで逆に可読性が落ちるケース
具体例として、Metrics/AbcSizeを守ることで逆に可読性が落ちるケースを挙げてみます。
適当なコードですが、値が存在するものだけのハッシュを返すメソッドがあるとします。
（本来ならcompactメソッドを使えば済みますがあくまで例として）
class User
# 省略
def user_params
result = {}
result[:first_name] = first_name if first_name.present?
result[:last_name] = last_name if last_name.present?
result[:email] = email if email.present?
result[:password] = password if password.present?
result
end
end
実はこのコード、メソッド内で何回かif文があるせいでRuboCopのMetrics/AbcSizeの警告が出ます。
警告が出ないように、if文をなるべく書かないようにリファクタリングしてみます。
これで無事に警告が出なくなりました。めでたしめでたし・・・でしょうか？
値が存在するときだけハッシュに含めて存在しないときは含めないという簡単なメソッドなのにも関わらず、なんだか回りくどいことをしています。
RuboCopの警告を守ったために逆に読みにくくなってしまいました。
結局、Metricsの警告が出たとしてもそのコードを読みやすいものにするかどうかは開発者のスキル次第になります。
リファクタリングのきっかけに気付けるというメリットもあるのですが、disableコメントが乱立したりMetricsについての設定をどうするかの方針を話し合うコミュニケーションコストによるデメリットの方が大きいと個人的には感じています。
であれば、MetricsはOFFにしておいてその部分はコードレビューで補った方が考えることが減ってシンプルなのではないでしょうか。
さいごに
というわけで、RuboCopのMetricsはOFFにした方がいいのではという個人的な意見でした。
RuboCop自体はとてもいいツールなのでいい感じに付き合っていきたいですね。
何か少しでも参考になれば幸いです。
Discussion