読者です 読者をやめる 読者になる 読者になる

まりぴよこのブログ

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

コピーアプリ1:ヤフオク!のレビューを受けて

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

前回の記事:

mpiyok.hatenablog.com

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

解決したかった問題

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

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

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

(あとで落札者は評価&コメント記入できる)

最初の実装では、 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