1

私は本当に不十分に書かれたメソッドをリファクタリングしようとしています。このメソッドは、次のようなバランス ハッシュを含む配列をスキャンすることを目的としています。

{ amount: $123, month_end: '2013-01-31' }

同じ月末のすべての残高を返し、金額を合計します。

def monthly_total(month)
  balances = [ balance_1, balance_2 ]
  # get and array of balances
  balances_for_month = balances.select do |balance|
    balance.month_end == month
  end
  # grab only the balances for the desired month
  balance_amounts = balances_for_month.map do |balance|
    balance.amount
  end
  #take all the balances for the month and sum them.
  balance_amounts.inject{|sum,x| sum + x }
end

しかし、これを行うためのより洗練された方法が必要です。新しい配列を作成してそれらをループするのではなく、元の配列を 1 回ループするようにこのメソッドを再構築するにはどうすればよいですか?

4

1 に答える 1

1

それは良いコードです。変数名は適切で、意図は明らかです。

配列を 2 回通過しても問題はありません。Ruby は、可能な限り高速なコードよりも、意図を明確にすることを目的としています。コードが十分に高速でないことがわかっている場合にのみ最適化してから、実際に高速化されていることを確認するために測定する必要があります。Ruby は、速くなると思うことをすると遅くなって驚くことがよくあります。

2 番目のループ (balance を balance.amount に変換) は次のように短縮できます。

balances_amounts = balances_for_month.map(&:amount)

合計は次のように短縮できます。

balance_amounts.inject(&:+)

to_proc メソッドの意味を参照してください。これらがどのように機能するかを説明します。

一時的にコードがわかりやすくなる場合があります。時々そうではありません。上記の手法を使用すると、一時的なものを取り除くことができます。

balances.select do |balance|
  balance.month_end == month
end.map(&:amount).inject(&:+)

これは最初は少し難解に思えるかもしれませんが、これらのイディオムに慣れると明らかになります。

于 2013-10-14T02:49:29.567 に答える