【Ruby】リファクタリングに挑戦してみよう！

最近現役のエンジニアの方々にコードレビューをしていただく機会がありました。
大変参考になりましたので、シェアしたいと思います。

演習

sales.csvを読み込んで出力結果通りに出力するプログラムを書く。
いきなりですが以下のRubyのコードにはいくつかの改善点があります。
改善点を指摘して、リファクタリングしてみましょう！

# Time.parseメソッドを使うためにtimeクラスを読み込む
require "time"
# csvを扱うライブラリを読み込む
require 'csv'

class User
  attr_reader :name, :birthday
  def initialize(name, birthday)
    @name = name
    @birthday = birthday
  end
end

class Product
  attr_reader :name, :quantity, :price
  def initialize(name, quantity, price)
    @name = name
    @quantity = quantity
    @price = price
  end
end

# CSVファイルを1行ずつ取り出す
CSV.foreach("sales.csv", headers: true) do |row|
  user = User.new(row[0], row[1])
  now_date = Time.now
  # 文字列から時刻オブジェクトに変換する
  birthday = Time.parse(user.birthday)
  # 秒を分、時間、日数、年数に直す
  age = sprintf("%d",((now_date - birthday) / 60 / 60 / 24)/365)
  product = Product.new(row[2], row[3], row[4])

  puts "#{user.name}(#{age})は#{product.name}を#{ product.quantity.to_i * product.price.to_i}円お買い上げ。"
end

sales.csv

名前,誕生日,買上品目,数量,単価
つんく♂,1968/10/29,チロルチョコ,13,10
はたけ,1968/8/17,雪見だいふく,3,100
まこと,1968/12/31,チョコモナカ,3,140
たいせい,1969/11/29,フルーツコッペ,4,300

# 出力結果
つんく♂(52)はチロルチョコを130円お買い上げ。
はたけ(52)は雪見だいふくを300円お買い上げ。
まこと(52)はチョコモナカを420円お買い上げ。
たいせい(51)はフルーツコッペを1200円お買い上げ。

---

以下解答

---

解答

• 改善点
  • 変数名が適切ではない(Time型に〜dateという変数名は不適切)→適切な変数名に修正する
    • そもそも今回の場合は変数を定義すること自体必要ないので削除
  • 引数row[添字]が何を表しているのか分かりづらい→キーワード引数を使う、csvファイルのヘッダーを添字に使う
  • 年齢計算処理が複雑かつ唐突に出てきている→メソッドに切り出す
  • 合計金額計算処理が少しわかりづらい→メソッドに切り出す
  • row[3]とrow[4]の型変換を最後にしている(日時を文字列のまま引きずり回している)→オブジェクト生成時に型変換しておく

require "time"
require 'date'
require 'csv'

class User
  attr_reader :name, :birthday: 

  def initialize(name:, birthday:)
    @name = name
    @birthday = birthday
  end

  def age
    ((Date.today - @birthday) / 365).to_i
  end
end

class Product
  attr_reader :name, :quantity, :price

  def initialize(name:, quantity:, price:)
    @name = name
    @quantity = quantity
    @price = price
  end

  def sum
    @quantity * @price
  end
end

CSV.foreach("sales.csv", headers: true) do |row|
  user = User.new(name: row['名前'], birthday: Date.parse(row['誕生日']))
  product = Product.new(name: row['買上品目'], quantity: row['数量'].to_i, price: row['単価'].to_i)

  puts "#{user.name}(#{user.age})は#{product.name}を#{product.sum}円お買い上げ。"
end

これまであまり機会がなかったのですが、コードレビューをしていただくことの重要性を実感しました。
今までいかに読みやすいコードを書くことを考えられていなかったか...
他にも良い書き方があれば教えてください！