Userに紐づく、店舗の従業員の給与合計を出力するスクリプトを書きたいです。
以下のようなのを作成したのですが、each がネストしているのがすごく違和感を感じます。
私自身の経験が浅いための勘違いかもしれませんが、
eachがネストしている事自体はさほど問題ないのでしょうか?
sum = 0 User.first.shops.each do |shop| shop.employments.each do |employment| sum += employment.price * employment.hour end end
今後、このデータの数も増えてくるため、
今のうちに設計を見直すべきなのかと思っております。
ご助言頂けると幸いです。
よろしくお願いします。
気になる質問をクリップする
クリップした質問は、後からいつでもMYページで確認できます。
またクリップした質問に回答があった際、通知やメールを受け取ることができます。
バッドをするには、ログインかつ
こちらの条件を満たす必要があります。
回答3件
0
SQLで条件を定義するのが困難な場合を除いて、集計はDB側でやるのがセオリーです。集計のためにアプリケーション(Rails)側でループを回すのは典型的なアンチパターンです。
モデルの構造がわからないので想像ですが、
User.first.shops.select('SUM(employments.price * employments.hour) AS total').joins(:employments).first.total
こんな感じで行けるんじゃないでしょうか。
投稿2016/05/18 13:53
総合スコア1449
0
私だったら、それぞれのモデルクラスに集計用のメソッドを書いて、それを内部で呼び出す形式にします。
で、後々パフォーマンス等々の問題が出たときには、それぞれのメソッドでがさっと取ってくるように変更してゆきます。
まあ、中間のメソッドが必要ないのであれば、最上位のuserに一括でemploymentを取得して集計するようにしますが。
Ruby
1class User < ActiveRecord::Base 2 has_many :shops 3 # 普通に使うとき 4 def total_payment 5 payment = 0 6 self.shops.each {|s| payment += s.total_payment } 7 return payment 8 end 9 10 # 高速化してみる 11 def hi_speed_total_payment 12 user_table = User.arel_table 13 shop_table = Shop.arel_table 14 emp_table = Employment.arel_table 15 join_s = emp_table.join(shop_table, Arel::Nodes::OuterJoin).on(emp_table[:shop_id].eq(shop_table[:id]).join(user_table, Arel::Nodes::OuterJoin).on(shop_table[:user_id].eq(user_table[:id]).join_source 16 total = 0 17 Employment.joins(join_s).where(user_table[:id].eq(self.id).each do |emp| 18 total += emp.total_payment 19 end 20 return total 21 end 22end 23 24class Shop < ActiveRecord::Base 25 belongs_to :user 26 has_many :employments 27 28 def total_payment 29 payment = 0 30 self.employments.each {|emp| payment += emp.total_payment } 31 return payment 32 end 33end 34 35class Employment < ActiveRecord::Base 36 belongs_to :shop 37 38 def total_payment 39 return self.price * self.hour 40 end 41end
Railsを使っていて、パフォーマンスの問題が出るのは、ネストの深さというよりは、DBアクセスのオーバーヘッドにボトルネックがあるので、(eachで回してると、それぞれのeachの中でDBアクセスが発生する)パフォーマンスの問題を考えるなら、どこでDBアクセスが発生しうるのかを考えるのがポイントかと。
eager_loadingとか使ってて、あらかじめ関連インスタンスをメモリにロードしてる場合はループで回しても同じとか、その辺を考える分けです。
可読性の問題とか、その辺を考えるなら、モデルクラスのメソッドをコールして、その内部でループを回す方法が良いかと思います。
投稿2016/05/18 07:26
総合スコア1901
バッドをするには、ログインかつ
こちらの条件を満たす必要があります。
2016/05/18 09:29
2016/05/18 14:25
0
他の方がきっとしてくれるであろう回答が勉強になりそうで楽しみです。
私としてはネストの深さより、スクロースせずに見える範囲にループが収まるか否か、という方を重要視する場合が多いと思います。ループが 3 重くらいでも素直な構造のもの (たとえば x, y, z 軸についてのループとか) でループ全体が 20 行で収まっているなら特におかしいとは思いません。実際にはネストの深さというか、条件分岐なども含んだインデントの深さで考えますが。
他のところの処理にもよりますが、今回のようなケースで、クラスを用いてさらにコードのネストを減らすことは有用なことも少なくないと思います。ネストを減らすことが目的ではなく、コードの見通しをよくし、仕様変更などに強くするのが目的になりますが。
具体的には、Shop
クラスを作って totalSalary
という getter を用意し、Employment
クラスを作って salary
という getter を用意する、という感じでしょうか。たとえば、支店ごとの合計の他に、支店内の部署ごとの合計も出したい、とかなったときに Employment
クラスには手を入れなくても対応できるようになります。
投稿2016/05/17 22:23
総合スコア2468
バッドをするには、ログインかつ
こちらの条件を満たす必要があります。
あなたの回答
tips
太字
斜体
打ち消し線
見出し
引用テキストの挿入
コードの挿入
リンクの挿入
リストの挿入
番号リストの挿入
表の挿入
水平線の挿入
プレビュー
質問の解決につながる回答をしましょう。 サンプルコードなど、より具体的な説明があると質問者の理解の助けになります。 また、読む側のことを考えた、分かりやすい文章を心がけましょう。
バッドをするには、ログインかつ
こちらの条件を満たす必要があります。
2016/05/18 14:24
2016/05/19 00:34