うんこーどを直したい

はじめに

筆者転職活動中に久々に書かないとなと思うコードに遭遇してしまい筆を取る次第。

この記事のうんこーどがやりたいことを簡単に言うとINPUTに対して該当するプランがあるのか？他に予約がないか？をチェックして通ったら合計金額を返すだけのコードである。皆で仲良く直しましょう。

お題

皆さんはうんこーどに遭遇したことはありますか？私はあります。
とりあえず何も考えずに以下のコードを御覧ください。

class HogeController < ApplicationController
  def index
    room = Room.find(params[:room_id])

    plans = Plan.where(room_id: params[:room_id], status: true)
    if plans.count == 0 || params[:started_at].nil? || params[:ended_at].nil?
      return render json: {}
    end
    started_at = Time.zone.parse(params[:started_at].to_s)
    ended_at = Time.zone.parse(params[:ended_at].to_s)

    available_plans = []
    plans.each do |plan|
      calendar_cells = []
      current = started_at
      while current < ended_at
        calendar_status = nil
        hourly_price = nil

        plan_hour = plan.plan_hours.find_by(weekday: current.wday)
        if plan_hour.present?
          if plan_hour.start_hour <= current.hour && current.hour < plan_hour.end_hour
            calendar_status = 'available'
            hourly_price = plan.hourly_price
          end
        end

        room.reservations.each do |reservation|
          if reservation.started_at <= current && current < reservation.ended_at
            calendar_status = 'is_reserved'
          end
        end

        calendar_status = 'not_available' if calendar_status.nil?

        calendar_cells << CalendarCell.new(
          year: current.year,
          month: current.month,
          day: current.day,
          hour: current.hour,
          status: calendar_status,
          hourly_price: hourly_price,
        )

        current += 1.hour
      end

      is_all_available = calendar_cells.all? {|c| c.status == 'available' }
      is_all_available = false if calendar_cells.length == 0
      if is_all_available
        calendar_plan = CalendarPlan.new(plan: plan, is_available: is_all_available)
        calendar_plan.set_sum_price(calendar_cells)
        available_plans << calendar_plan
      end
    end

    render json: available_plans
  end

end

INPUT

room_id: 1
started_at: 2020/04/01 13:00:00
ended_at: 2020/04/01 15:00:00

OUTPUT

[
    {
        "plan": {
            "id": 1,
            "room_id": 1,
            "name": "パーティープラン",
            "status": true,
            "hourly_price": 2000,
        },
        "is_available": true,
        "plan_sum_price": 4000
    },
    {
        "plan": {
            "id": 2,
            "room_id": 1,
            "name": "会議プラン",
            "status": true,
            "hourly_price": 1000,
        },
        "is_available": true,
        "plan_sum_price": 2000
    }
]

で、こんな感じのコードを修正する要件があったのだが皆さんこのコード見て何を思いました？
ぶっちゃけ汚いと思うのが当たり前なので改修する前にリファクタリングしちゃいましょう。

このコードの注目ポイントはこれ

      is_all_available = calendar_cells.all? {|c| c.status == 'available' }
      is_all_available = false if calendar_cells.length == 0
      if is_all_available
        calendar_plan = CalendarPlan.new(plan: plan, is_available: is_all_available)
        calendar_plan.set_sum_price(calendar_cells)
        available_plans << calendar_plan
      end

statusを注目してほしいんですが、このコードstatusが全てavailableでないとレスポンスを返さないんですよね。こういううんこーどは早期リターンするに限ります。

言葉でいうと
・room.reservationsがstarted_atとended_atと被ってないこと
・plan.plan_hourが存在していない場合
・plan_hour.start_hourとplan_hour.end_hour内にcurrent.hourが無い
上記はそうそうにレスポンスしないのでリターン

さらに、Planループ内のwhile文で1時間毎にさらにroom.reservationsをループしているがそりゃこんなコードで5万レコードとか扱うと秒で死ぬ。特に理由がないなら予約の確認と該当プランがあるかはPlanループ初めにしたい

    available_plans = plans.map { |plan|
      # プラン基本情報
      plan_hour = plan.plan_hours.find_by(weekday: started_at.wday)
      next unless plan_hour.present?
      next unless plan_hour.start_hour <= started_at.hour && ended_at.hour <= plan_hour.end_hour
      next if room.reservations.any? { |reservation|
        reservation.ended_at　> started_at && ended_at > reservation.started_at
      }
...

こうでしょ？（※記事用に雑に描き下ろして条件ミスってる場合があります。いいたい事がわかってくれたらいいので・・・）
そうするとあとに残るのは以下みたいな感じである。

      while current < ended_at
        calendar_status = nil
        hourly_price = nil

        calendar_cells << CalendarCell.new(
          year: current.year,
          month: current.month,
          day: current.day,
          hour: current.hour,
          status: calendar_status,
          hourly_price: hourly_price,
        )

        current += 1.hour
      end

ここに到達するコードは予約の被ってない、該当プランがあるコードなのでcalendar_status及びhourly_priceはavailableとプランの値段で固定です。ここでは2000円にしときましょうか。

で結局ここって予約開始時間と終了時間だけcalendar_cellsというよくわからない物体を生み出すうんこーどなんです。注目はここ

      is_all_available = calendar_cells.all? {|c| c.status == 'available' }
      is_all_available = false if calendar_cells.length == 0
      if is_all_available
        calendar_plan = CalendarPlan.new(plan: plan, is_available: is_all_available)
        calendar_plan.set_sum_price(calendar_cells)
        available_plans << calendar_plan
      end

リファクタリングに伴いis_all_availableはtrue固定なので条件文共々不要です。
calendar_plan.set_sum_priceの中身ですが。

  def set_sum_price(calendar_cells)
    calendar_cells.each do |cell|
      self.plan_sum_price += cell.hourly_price || 0
    end
  end

hourly_priceを足してるだけ・・・しかも早期リターンしてないからhourly_priceが空の場合があるため|| 0とかやっちゃってる始末。お前に入れてるyearとかdayはどこにやったんだ。なんでループしてたの馬鹿なの？

リファクタリングするとこうなります

plan_sum_price = ((ended_at.to_i - started_at.to_i) / 60 / 60 * plan.hourly_price).floor

calendar_plan = CalendarPlan.new(plan: plan, is_available: true)
calendar_plan.plan_sum_price = plan_sum_price
calendar_plan

※ 消費税の切り捨て切り上げはここでは適当とします。
結局やりたいのは予約出来た期間の合計金額を出したいだけなのでこんな感じになります。

ここまでを踏まえたコード

class ReservationAvailablePlansController < ApplicationController
  def index
    room = Room.includes(:reservations).find(params[:room_id])

    plans = Plan.includes(:plan_hours).where(room_id: params[:room_id], status: true)
    if plans.count == 0 || params[:started_at].nil? || params[:ended_at].nil?
      return render json: []
    end

    started_at = Time.zone.parse(params[:started_at].to_s)
    ended_at = Time.zone.parse(params[:ended_at].to_s)

    available_plans = plans.map { |plan|
      # プラン基本情報
      plan_hour = plan.plan_hours.find_by(weekday: started_at.wday)
      next unless plan_hour.present?
      next unless plan_hour.start_hour <= started_at.hour && ended_at.hour <= plan_hour.end_hour

      # 別の予約がある場合は不可
      next if room.reservations.any? { |reservation|
        reservation.ended_at　> started_at && ended_at > reservation.started_at
      }

      plan_sum_price = ((ended_at.to_i - started_at.to_i) / 60 / 60 * plan.hourly_price).floor

      calendar_plan = CalendarPlan.new(plan: plan, is_available: true)
      calendar_plan.plan_sum_price = plan_sum_price
      calendar_plan
    }.compact

    render json: available_plans
  end
end

はい半分消し飛びました。ようやく本題にとりかかれそうですね。
この段階で1時間単位とか関係なく何分のインプットでも大丈夫になってます。

本題はここから分単位でも予約出来るようにプランにminuteをはやしてちょちょっと調整すれば要件完了です。そこは割愛です。

終わり

なんでこの記事を書いたかというと、転職活動中こんな感じの課題を頂いて上記のような実装した所理解出来なかったらしくCalendarCellの実装がないね実力不足なのでごめんなさいと言われたのである。自分の考え方や働き方が組織に合わなくてごめんなさいなら理解出来るけどCalendarCellの実装が駄目ですって言われてカチンときた。CalendarCellが何やってるか理解出来てないでしょ！！！！足し算してるだけなんだよ！！！

現在転職活動中なので私のやってることが理解できる企業様のスカウト待ってます

結局そこかーい