やわらかテック

興味のあること。業務を通して得られた発見。個人的に試してみたことをアウトプットしています🍵

「Aを変更したらBも変更してね」コメントはいらない

いつものようにプロダクトのコードを読んでいた時のことです。
コード内に「Aを変更したらBも変更してね」という旨のコメントがいくつか記載されていることに気づきました。

def do_something
  # ここを変更したらlibのdo_somethingも変更すること
  :
end

このコメントに対して個人的にモヤッ...と感じる部分があるので、自分の考えをまとめてみたいと思います。

設計を疑うべし

そもそも、なぜ「Aを変更したらBも変更してね」というコメントを書いておく必要があるのでしょうか。
多くのケースでは、モジュールやクラスの設計が良くないからだと思われます。今回、自分が観測したケースだと、ステータス一覧に新しい定義を追加後、ステータスを英語表記から画面表示用の日本語表記に変換する関数に対して、このコメントが記載されていました。

class Status
  # STATUSESを追加したらto_textも変更すること
  STATUSES = {
    disabled: 0,
    enabled: 1,
  }
  
  def self.to_text(status_id)
    texts = { STATUSES[:disabled] => '無効', STATUSES[:enabled] => '有効' }
    texts[status_id]
  end
end

このようなコメントを残すという選択をするのであれば、現状の設計を疑ってほしいです。
その上で今は変更ができない・規模的に厳しいのなら、その理由も合わせて追記しておくべきだと思いますし、後にリファクタリングしたい部分としてタスク化しておく・チケットを作成するというのも大切なアクションです。

# STATUSESを追加したらto_textへ変更すること
# 今の設計だとto_textへの変更は必須。リファクタリングのタスクを作成済み
STATUSES = {
  disabled: 0,
  enabled: 1,
}

コメントで期待するのは危険

おそらくコメントを残した人は「このコメントを見て対応してくれるはず...」と思っての判断だったのでしょう。
「対応してくれるはず...」と淡い期待をするのは良いですが、対応してくれるかどうかは当事者次第です。仮に対応してもらえなかった・対応が漏れてしまった場合、どうなってしまうのでしょうか。

重要度が高い処理であればなおさら、コメントで何とかしようという判断は控えるべきです。
対応がされていない場合に何らかの方法で気づくことが出来る状態を用意しておくのが重要になります。
CI/CDが用意されているという前提上ですが、自分だったらAにのみ変更があるけどBに変更がない場合に必ず失敗する単体テストをBの方に実装しておきます。そうすれば、CIがコケて調査の結果、Bの対応が不足していたということが分かりますし、CIがコケる以上、対応不足のコードがリリースされる心配はありません。

context 'to_text' do
  it '定義されたステータス数と日本語表記のステータス数が一致すること' do
    :
  end
end

最後に

珍しく突発的な内容で記事を書いてしまいました。
ただ「コメントを記載すれば何とかなる」という判断がされたことが、個人的には違うんじゃないのかな...と思った次第です。
コメントを記載する際は「本当に書くべきコメントなのかを考えろ」とよく言われるものですが、そもそもコメントを書かないといけない設計・状況になっていること自体を疑う視点を持ちたいですし、持ってほしいです。

少しでも「ええな〜」と思ったらイイネ!・シェア!・はてなブックマークを頂けると励みになります。