コピーアプリ１：ヤフオク！のレビューを受けて

前回完成した！と書いたコピーアプリのレビューで、目からうろこ感の指摘が色々あってとっても勉強になった話。

前回の記事：

mpiyok.hatenablog.com

一番悩んでた「終了状態を表すフラグをどうやって設定するか」という問題、そもそもそんな必要なかったという・・・

解決したかった問題

「オークション締切日に締め処理させたい」・・・だったんだけど・・

この時想定してた「締め処理」は２つ

オークションが終了した、という状態をフラグとして保存
入札があった場合、最高金額を「落札」として扱いたい

（あとで落札者は評価＆コメント記入できる）

最初の実装では、 closed, successful_bid_id をカラムとしてモデルに追加していた。

successful_bid_idはわざわざ

class Auction < ActiveRecord::Base
  # ...
  has_one :successful_bid, class_name: "Bid", primary_key: :successful_bid_id, foreign_key: :id 
end

とかやって、関連を定義したりしてたし；；

実は両方共関連から辿れる・・というのがポイント

オークションが終了した状態を取得する方法

class Auction < ActiveRecord::Base
  # ...
  def closed?
    self.deadline_date < Time.current
  end
end

単純に、オークション締切日と現在時刻を比較すれば、close or notの判定はできる。（・・た・・確かに！！）

「落札」した入札を取得する方法

class Auction < ActiveRecord::Base
  # ...
  has_many :bids

  scope :close_items, -> { where("deadline_date < ?", Time.current).order(deadline_date: :desc) }

  def successful_bid
     self.bids.successful.order(price: :desc).first
   end
end

class Bid < ActiveRecord::Base
  # ...
  belongs_to :auction

  scope :successful, -> { joins(:auction).merge(Auction.close_items) }
end

落札も関連から取得できる。

辿りすぎで後々パフォーマンス上問題になるかも？なアプローチだけど、締め処理で値を設定してしまうより確実。（データの整合性上）

整合性を保とうとして、Auctionモデルの before_save とかのコールバックで、一生懸命フラグのメンテナンスしようとしてたからこっちのほうがかなりシンプルになっていい感じです！

その他指摘

Rubyっぽくない冗長なコードの書き方

[before]

def max_bid_price
  max_bid = self.bids.max_by { |b| b.price }
  if max_bid.present?
    max_bid.price
  else
    0
  end
end

↓

[after]

def max_bid_price
  self.bids.maximum(:price) || 0
end

・・・・改めて見ると、最初のやつヒドイ状態；；

6行も書いてる時点でなんか気づいても良さそうなもんだよな・・・（反省）

まとめ

改めて見なおしてみると、指摘をもらった箇所はなんか明らかにコードがゴチャゴチャしていた箇所。。「なんかおかしい」フラグ立ってるよなぁ；；

コード全体

github.com

おまけ

この指摘箇所とは別で、自分でコードを見直してた時に気づいた修正点をQiitaに書いてみた。

qiita.com

まりぴよこのブログ

日々の日記。技術ネタでまとまりきってないものの記録、伝わる文章の書き方を練習とか。