現在時刻を扱うメソッドはデフォルト引数を使いましょうという話

GMOペパボ1ヶ月記念に、おそらく一番最初にコードレビューで伝えたことを書いておこうと思う。

まとめ

現在時刻を扱うようなメソッドには、デフォルト引数で Time.now を使って、外からも時刻を渡せるようにしましょう。

テストのため

2014年の今となっては、テストのために時刻の扱いを気をつけましょうという必要もない気がするけど、CodeIQの和田さんの解説にもあるように、システム全体の時刻をスタブしたりするのは多少のリスクが伴うと思う。

実行中に現在時刻が変わってしまう

問題は 複数の現在時刻を扱うロジックを連続で実行すると現在時刻が変わってしまう ということである。

1つのコード例は以下の通り。

class Article < ActiveRecord::Base
  scope :recents, -> { where('created_at > ?', 1.week.ago) }
end

class HogeController
  def summary
    @articles =  Article.recents.count
    @comments = Comment.recents.count
  end
end

大体の場合は問題無いだろうけど、例えば日またぎの場合や、ビューに｢集計日時｣みたいなのがあったことを想像してみてほしい。

別のケースとしてはこんなものもある。

class Article
  def recent?
    created_at > 1.week.ago
  end
end

%ul
  - articles.each do |article|
    %li= "#{article.title} #{article.recent?}"

これは、回しているうちに同じ結果を期待する複数のオブジェクト間で結果が変わってしまう可能性がある。

｢そうなったときにリファクタリングすればいい｣というのはそうなんだけど、大抵の場合はこういう使われ方をするので最初からデフォルト引数を使うようにしておいたほうがいいんじゃないかというのが実感としてある。

メソッド名はもっとよいものがある気がするけど、私がざっと書くなら最初からこんな感じ。

class Article < ActiveRecord::Base
  scope :recents, ->(now = Time.now) { where('created_at > ?', 1.week.ago(now)) }

  def recent?(now = Time.now)
    created_at > 1.week.ago(now)
  end
end