この頃のプルリクに対するレビュー

この記事はアピリッツの技術ブログ「DoRuby」から移行した記事です。情報が古い可能性がありますのでご注意ください。

What is the difference between pull request and merge request?

自分がされたり、自分がしたり

そのクエリ参照する変数宣言、一定の時しか必要ないよね？

def create_current_banner_info(user,  now = Time.now)
  user_items = user.items.pluck(:item_code, :quantity).to_h
  BannerMaster.holdings(now).each_with_object([]) do |banner, res|
  if banner.user_condition_need?
    case banner.condition_type
    when :fulfill_item
      next unless user_items[banner.neccesary_item_code].to_i >= banner.nessesary_quantity
    when ...
      ...
    end
  end
  res << banner.as_json(:code, :banner_type, :banner_icon, ...)
end

アイテムを保持しているかで表示するかを分けるバナーって複数あったりもするだろうけど、いつもある訳じゃないよね？
だったら一番上のuser_itemsはnilで宣言しちゃって、 when :fulfill_item の中に入ったところで||=で宣言すれば無駄ないと思うよ。
いやそれよりも、もっと良い方法あるな。
そもそもさ、複数あるって言っても、pluckして必要なデータの取得を最低限にしてると言ってもさ、全部のアイテム参照してるのかなり無駄じゃん。
バナーを表示するかとレスポンス作っているところを一緒くたにしないでさ、先にアイテムが必要なバナーだけを抽出してしまえば、その参照すべきアイテムコード群を取れるよね。そうすれば本当に必要最低限しか取らずに済むよ。
で、今すぐマージ必要？　必要じゃないか、分かった。じゃあお願いします。

先にグルーピングしましょう

def past_events_user_data(user, now = Time.now)
  events = EventMaster.ended(now)
  user_events = user.user_events.where(event_code: events.pluck(:code))
  events.map do |event|
    user_event = user_events.find{ |user_event| user_event.event_code == event.code}
    create_past_event_response(event, user_event)
  end
end

ソースコードの重みってクエリ参照が一番でかいけど、コード自体の重みも結構馬鹿にならないんだよ。
ここだと、eventsの数だけfindの線形探索が走っちゃうから計算量がO(n² ) であんまりよろしくない。
なのでuser_eventsを参照しているところで、index_by(&:event_code)をしておけば、後のfindを消せて、user_event = user_events[event.code] と綺麗に済ませられる。ハッシュ検索はO(1)だから、これなら計算量をO(n)まで落とせた。

混乱するかも

def lottery_apple(user, event_code, index)
  user_received_apples = user.user_apples.where.not(received_at: nil).where(event_code: event_code, apple_index: index)
  AppleMaster.lottery_no_duplicate(event_code, index, user_received_apples)
end

...

def lottery_orange(user, event_code, index)
  user_received_oranges = user.user_oranges.where.not(received_at: nil).where(event_code: event_code, orange_index: index)
  OrangeMaster.lottery_no_duplicate(event_code, index, user_received_oranges)
end

……あ、これ上のコードのindexと下のコードのindexの意味合い違うのね。
同じファイル内にあるソースならちゃんと変数名変えておいた方が良いと思うよ。

消すよりコメントアウトの方が時には良い

enum 1,  :new
enum 2,  :intensive
enum 3,  :course
enum 4,  :obliteration
enum 5,  :lenthal
enum 6,  :environmental
enum 8,  :search
enum 10, :end
enum 11, :alternate
enum 12, :dead_s

これ、7と9が抜けたの後から見たら理由が良く分からないしログ追うハメになるから、コメントアウトの方が良いかな。

後置ifはほどほどに

return long_method(argument1, argument2) unless too_long_long_methooooooooooooooooooooooood?(argument3, argument4)

流石にこれだと長いから、後置ifじゃなくて普通にif~endで囲もうか。その方が直感的な理解が早い。
というかrubocop怒られなかった？　……あら、無いのか。
付けようとすると……あー、駄目だ、案の定たっぷり出たわ出たわ。
issue化しておこうか(そして誰もやらない)。