ネストが深いリレーションの為、eachのネストが発生するスクリプト（本文中にあり）がでます、こういうのは設計を見直すべきなのでしょうか？

Question

Userに紐づく、店舗の従業員の給与合計を出力するスクリプトを書きたいです。

以下のようなのを作成したのですが、each がネストしているのがすごく違和感を感じます。

私自身の経験が浅いための勘違いかもしれませんが、
eachがネストしている事自体はさほど問題ないのでしょうか？

sum = 0
User.first.shops.each do |shop|
  shop.employments.each do |employment|
    sum += employment.price * employment.hour
  end
end

今後、このデータの数も増えてくるため、
今のうちに設計を見直すべきなのかと思っております。

ご助言頂けると幸いです。
よろしくお願いします。

Answer 1

SQLで条件を定義するのが困難な場合を除いて、集計はDB側でやるのがセオリーです。集計のためにアプリケーション(Rails)側でループを回すのは典型的なアンチパターンです。

モデルの構造がわからないので想像ですが、

User.first.shops.select('SUM(employments.price * employments.hour) AS total').joins(:employments).first.total

こんな感じで行けるんじゃないでしょうか。

Comments

[qaz3330]

こういうのはアンチパターンなんですね。
勉強になりました。

店舗の従業員の給与合計という風に記載しましたが、実際に運用するのは、もう少し違っていて、合計の金額が数分毎に変わります。

そのような中、今回の仕様上、できれば、ユーザーがアクセスした際に、そのユーザーに紐づく合計金額を出したいです。

もし上記要件を満たそうとした場合に私が考えたのは

・5分毎にDB側で集計をするバッチ処理を書く(5分かどうかは未定ですが)
・今回の使用を考慮し、includeなどでeager_loadingさせる方法で対応させる

のがいいのかなと思いました。

やはりこういう場合ですと、バッチ処理のほうがいいのでしょうか？

[suzukis]

随時計算するかバッチにするかは状況次第ですので、どちらが良い悪いはありません。たとえば、全結果の大半が利用されるのか少数しか利用されないのか、などで判断します

Answer 2

私だったら、それぞれのモデルクラスに集計用のメソッドを書いて、それを内部で呼び出す形式にします。
で、後々パフォーマンス等々の問題が出たときには、それぞれのメソッドでがさっと取ってくるように変更してゆきます。
まあ、中間のメソッドが必要ないのであれば、最上位のuserに一括でemploymentを取得して集計するようにしますが。

class User < ActiveRecord::Base
  has_many :shops
  # 普通に使うとき
  def total_payment
    payment = 0
    self.shops.each {|s| payment += s.total_payment }
    return payment
  end

  # 高速化してみる
  def hi_speed_total_payment
    user_table = User.arel_table
    shop_table = Shop.arel_table
    emp_table = Employment.arel_table
    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
    total = 0
    Employment.joins(join_s).where(user_table[:id].eq(self.id).each do |emp|
      total += emp.total_payment
    end
    return total
  end
end

class Shop < ActiveRecord::Base
  belongs_to :user
  has_many :employments
  
  def total_payment
    payment = 0
    self.employments.each {|emp| payment += emp.total_payment }
    return payment
  end
end

class Employment < ActiveRecord::Base
  belongs_to :shop
  
  def total_payment
    return self.price * self.hour
  end
end

Railsを使っていて、パフォーマンスの問題が出るのは、ネストの深さというよりは、DBアクセスのオーバーヘッドにボトルネックがあるので、(eachで回してると、それぞれのeachの中でDBアクセスが発生する)パフォーマンスの問題を考えるなら、どこでDBアクセスが発生しうるのかを考えるのがポイントかと。
eager_loadingとか使ってて、あらかじめ関連インスタンスをメモリにロードしてる場合はループで回しても同じとか、その辺を考える分けです。

可読性の問題とか、その辺を考えるなら、モデルクラスのメソッドをコールして、その内部でループを回す方法が良いかと思います。

Comments

[qaz3330]

ご回答ありがとうございます！

可読性の面やパフォーマンスの面からアドバイスを頂き、大変勉強になりました。

回答を読んでいる中高速化について質問があります。

表現があってるかわかりませんが、
Controllerのところで、includeして、viewに渡します。
(質問文にそこまで書くべきでした・・・。すみません。)

@users = User.include(:shops, : employments)

このControllerのところでincludeしておけば、高速化したhi_speed_total_paymentメソッドを使用する必要がないのでしょうか？

気になる点としましては、Controllerでincludeしてさらにhi_speed_total_paymentをつかうと、高速化のつもりがさらに遅くなるのかなという点です。

Controllerでincludeするため、total_paymentメソッドを使用するのが今回のようなケースではあっているのかなと素人ながらに思ったのですが、いかがでしょうか？

[rifuch]

おっしゃるとおり、Controllerでincludeしていれば、サンプルの高速化は意味が無いですね。
ちょっと詳しく調べてないので、あやふやですが、Arelが採用された後のRaills(確か3.0以降)は、.eachとか、.allとか、.countなどのメソッドがコールされるまでDBアクセスは発生しません。
で、あらかじめincludesをしてあれば、モデルのメソッドで.eachがコールされたときに、関連モデルも含めて一気に取ってくる仕様になっているはずです。
なので、あらかじめincludesしてあるのであれば、通常のメソッドを使っても大丈夫なはず。
なお、高速化の方は通常の.eachアクセスとは全く別にDBアクセスを発行するので、どっちにしても変わらない、という事になります。(高速化のメソッドコールで余計に1回DBアクセスが走るかな）
実際にDBアクセスが発生するかは、ログを読んで確認するのが一番手っ取り早いかと思います。

[qaz3330]

ありがとうございます。
DBの高速化などの経験がこれまであまりなかったので、大変参考になりました。
ログを読んで確認するというのに、尽きるのですね。
頑張ってログをみて、高速化対応やってみます。

Answer 3

他の方がきっとしてくれるであろう回答が勉強になりそうで楽しみです。

私としてはネストの深さより、スクロースせずに見える範囲にループが収まるか否か、という方を重要視する場合が多いと思います。ループが 3 重くらいでも素直な構造のもの (たとえば x, y, z 軸についてのループとか) でループ全体が 20 行で収まっているなら特におかしいとは思いません。実際にはネストの深さというか、条件分岐なども含んだインデントの深さで考えますが。

他のところの処理にもよりますが、今回のようなケースで、クラスを用いてさらにコードのネストを減らすことは有用なことも少なくないと思います。ネストを減らすことが目的ではなく、コードの見通しをよくし、仕様変更などに強くするのが目的になりますが。
具体的には、Shop クラスを作って totalSalary という getter を用意し、Employment クラスを作って salary という getter を用意する、という感じでしょうか。たとえば、支店ごとの合計の他に、支店内の部署ごとの合計も出したい、とかなったときに Employment クラスには手を入れなくても対応できるようになります。