🍣
コードコメントにはwhy notを書こう
モチベーション
- Takuto Wada先生のツイートに対しての記事がネットで探してもあまりなかったので
TL;DR
- 「あえてやらなかったこと」をコメントしないと、レビュワーからは「知らなくてやった」と認識される
- 「あえてやらなかったこと」は、ほとんどの場合、ちゃんとコメントしておかないと理解してもらえない
- 「あえてやらなかった」ことを記載すると、レビュワーのコードリーディングの負担が減る
背景
- 新規機能を実装した際に、既存のDBの数値を更新する一時的に利用するスクリプトを実装する必要があった。
- PRには以下のようなコードが上がっていた。
/scripts/onetime/modifiy-db-issue-111.rb
# コードは適当です
Medium.all.each do |medium|
medium.articles.each do |article |
Conversion.where(article: article).update_all(article: aritlce)
end
end
実装者の気持ち
- それぞれのモデルのレコード数は、300, 300, 300。データ量は少ない。
- かつ今後のデータに関しては、新規の実装で整合性が合うはずなので、スピード改善やろうと思えばできるけど、やる必要ないかな。
- そこに時間をかけるくらいなら、他のタスクに時間をかけたいな。
- 多分意図汲んでくれるだろうから、コミット、、と。
レビュワーの気持ち
- 「2重loopしているので、計算量気になるな」
- 「しかも計算量マシマシなのにSQLでN+1が発生しているな」
- 「コード気持ち悪いな、レビューしたろｗ」
レビュワーのコメント
- 2重ループになっているので修正してください
- N+1が発生しているので、includesを使って先読みしてください
レビュイーのコメント
この実装は1回しか使わないのと、データ数が300くらいなので、
頑張って綺麗に描く必要がないのでこのままにさせてください!
何が問題か
- 「あえてやらなかったこと」 =
why notは、言語化しない限りは「知らなくてやらなかったこと」として認識される。 - レビュワーは「あえてやらなかったこと」だとわかっていれば指摘しなくても済んだ内容を指摘せざるを得なかったこと
- 後でコードを読んだ人も、そのコードにならってコピペでコード実装する可能性があり、パフォーマンス低下の温床になる可能性があること
どうしたらいいか
- コードコメントには、「あえてやらなかったこと」 = why notを書こう
例
- こんな感じで書くと、レビュワーとしてもそこを突っ込まなくて済む
- あとから参加する開発者も、ここのコードを参考にせずに済む
# 2重ループやN+1などの問題があり、パフォーマンス改善をしようと思えばできたけど、
# それぞれのDBのレコード数は300 / 300 / 300 だったため、
# あえて対応しなかった。（ここwhy not)
# また、新規の実装に関しては、同じようなDBの整合性を合わせる必要がないため、
# 再利用されることを想定していないです
Medium.all.each do |medium|
medium.articles.each do |article |
Conversion.where(article: article).update_all(article: aritlce)
end
end
その他
- 今後技術的負債になると認識した上で、やむを得ない事情で実装する必要がある場合は必ず
FIXMEコメントを入れておきたい
例）
# FIXME
# モデルのリファクタリングをしないと対応不可
# 本当はcontroller側で対応したくなかったけど、モデルいじるとなると間に合わないので一時的に負債として積みます
def index
...
end
補足
- そもそも負債になるような書き方をしなければよいのでは？という意見はいったん割愛
Discussion