rails RuboCop: Combine this loop with the previous loop. [Style/CombinableLoops]のエラーがでないようにしたい

Question

前提・実現したいこと

現在Rails開発に携わっています。
CSVの一括登録・更新機能を実装しています。
以下のコードがRuboCop: Combine this loop with the previous loop. [Style/CombinableLoops]となっているため、のエラーがでないように修正したいです。

value = row[:value]
# 先に更新かどうかを確認し、そうでなければ登録する
# 更新の場合
(1..5).each do |i|
  if fuga.id == hoge.public_send("fuga_#{i}_id")
    updated_indexs << i
    return {
      "hoge_#{i}_id": fuga.id,
      "hoge_value_#{i}": value,
    }
  end
end
# 登録の場合
(1..5).each do |i|
  unless updated_indexs.include?(i)
    return {
      "hoge_#{i}_id": fuga.id,
      "hoge_value_#{i}": value,
    }
  end
end

rubocopの修正方針は一つのループの中に入れろということだとは認識しています。

ただ、仕様的には、分ける必要があるのかなと思っており、rubocopエラーが出ないようにするにはどうしたらいいかアドバイスを頂きたいです。

以下仕様です。

---

①アップロードするCSVには複数行があり、それをアップロードしたらhogeテーブルの値（fuga_N_id,fuga_value_N）が更新されます。（一つのhoge.idに対して最大５行）
CSVの列にはid,fuga_id,fuga_valueがあります。

②hogeテーブルにはid,name,fuga_1_id ~ fuga_5_id, fuga_value_1 ~ fuga_value_5などがあり、
このCSVアップロードによってfuga_1_id ~ fuga_5_id, fuga_value_1 ~ fuga_value_5を登録・更新できます。
（hogeテーブルにおいてfuga_N_id,fuga_value_Nは横持ち。過去の経験からこの仕様に落ち着いた。）

③登録行か更新行はCSVに入力したfuga_idが、すでにhogeテーブルのfuga_1_id ~ fuga_5_idに存在していれば（一致していれば）更新、そうでなければ登録です。

---

①〜③のように、まず更新かどうかを確認してから、そうでなければ登録したいので、上記のコードのようにまず５回転してfuga_idがどれも一致しなければ、登録するようにしています。

それも踏まえてどのように修正すればいいかアドバイス頂けますと助かりますmm

試したこと

・RuboCop: Combine this loop with the previous loop. [Style/CombinableLoops]でググる
・他に参考がないか調べている

Comments

[maisumakun]

＞ 仕様的には、分ける必要があるのかなと思っており

どうしてそう考えていますでしょうか？

[k499778]

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

CSVのアップロードですでに更新された行を上書きしないようにしたいからです。

データが横持ちなので、更新行か判断するためには、一度 「hogeテーブルのfuga_1_id ~ fuga_5_id」 と「csvで入力されたfuga_id」が一致しているかどうかを確認する必要があると思っており、その後、登録するようにしています。更新の際に、どのカラム（fuga_N_id）を更新したかを保持しておき、それ以外のカラムを登録するようにしたいという意図でした。

[maisumakun]

> 更新の際に、どのカラム（fuga_N_id）を更新したかを保持しておき、それ以外のカラムを登録するようにしたいという意図でした。

元のコードがreturnばかりで書かれていて、「1行でもマッチしたら即脱出」となっていますが、それは意図通りではないということでしょうか？

[k499778]

意図通りです！

CSVの一行に対して、もし更新であれば即脱出するように意図しています。
更新でなければ登録するという意図です。

[k499778]

CSVの次の２行目に対して、１行目で更新したカラムに対して登録しないようにしています。

[maisumakun]

> 更新でなければ登録するという意図です。

えっと、returnすると、書かれているメソッド全体を抜けてしまうかと思うのですが、「更新でなければ登録する」という動作になるのでしょうか？

[k499778]

例えば以下のようにDBの「fuga_2_id」に値がある場合

--------------------------------------------
CSVのデータ
id | fuga_id | fuga_value
1 | 101 | 1000

hogeテーブル（DB）
id | name | fuga_1_id | fuga_value_1 | fuga_2_id | fuga_value_2 | fuga_3_id | fuga_value_3 | fuga_4_id | fuga_value_4 | fuga_5_id | fuga_value_5
1 | 名前１| | | 101 | 0 | | |
| | |
--------------------------------------------

まずは更新行かどうか判断してから、そのカラムを（fuga_N_id）保持しておき、登録するようにその保持していたカラムを避けて登録するようにしています。

OKパターン（期待値）
--------------------------------------------
hogeテーブル（DB）
id | name | fuga_1_id | fuga_value_1 | fuga_2_id | fuga_value_2 | fuga_3_id | fuga_value_3 | fuga_4_id | fuga_value_4 | fuga_5_id | fuga_value_5
1 | 名前１| | | 101 | 1000 | | |
| | |
--------------------------------------------

もし更新行か確認せずに登録を行おうとすると結果以下となってしまう認識です。

NGパターン
--------------------------------------------
hogeテーブル（DB）
id | name | fuga_1_id | fuga_value_1 | fuga_2_id | fuga_value_2 | fuga_3_id | fuga_value_3 | fuga_4_id | fuga_value_4 | fuga_5_id | fuga_value_5
1 | 名前１| 101 | 1000 | 101 | 1000 | | |
| | |
--------------------------------------------

[k499778]

>「更新でなければ登録する」という動作になるのでしょうか？

CSVの入力データ「fuga_id」の値がhogeテーブルに設定されていなければ、更新処理内でreturnで抜けず、登録される認識です。

[k499778]

例えば以下のパターン。hogeテーブルにfuga_idが設定されていない
--------------------------------------------
CSVのデータ
id | fuga_id | fuga_value
1 | 101 | 1000

hogeテーブル（DB）
id | name | fuga_1_id | fuga_value_1 | fuga_2_id | fuga_value_2 | fuga_3_id | fuga_value_3 | fuga_4_id | fuga_value_4 | fuga_5_id | fuga_value_5
1 | 名前１| 　|　 |　 |　 |　 |　 |　 |　 |　 |
--------------------------------------------

[k499778]

if fuga.id == hoge.public_send("fuga_#{i}_id")
に合致しないときに登録処理になる認識です

Answer 1

単にCopを無視させる、でいいのではないでしょうか？

公式にも、「書き換えれば意味が変わってくる恐れがある」と書いてあるようなCopです。

Comments

[maisumakun]

コメント形式で、「ここだけ」Style/CombinableLoopsを止める、というような書き方ができます。

[k499778]

>Safety
The cop is unsafe, because the first loop might modify state that the second loop depends on; these two aren’t combinable.

こちらですね！ありがとうございます。無視するという選択肢を今までしたことがあまりなかったので、参考になりましたmm

[k499778]

# rubocop:disable Style/CombinableLoops
を処理に上部にコメントするよう対応しました