はじめに

この記事の対象はこちら↓

Ruby on Rails初心者です！な方
Ruby on Railsを使っているけど、リファクタリングって何してるの？という方
良いコードを書きなさいと言われたけど改善方法がわからない！という方
コードを整理したい！と言う方

環境

Ruby ver_2.6
Ruby on Rails ver_5.1.7

リファクタリングって何？

一言で言うと、「外部から見た時の挙動は変えずに、プログラムの内部構造を整理・改善する」ことです。
「外部から見た時の挙動は変えずに」と記述があるように、ユーザー側からすれば大きな変化は起こりません。

ではなぜリファクタをするのか？

「ただ整理するなら急いでやる必要ないじゃん！」と思った方、私は声を大にして伝えたい。

私はこのリファクタを通して処理の高速化を実現させたり、コードへの理解も高まりました！
今まで気にしていなかった「動くものをとりあえず作る」という思考から、「不具合を未然に防ぐリーダブルコードの書き方」を知ることとなったのです！🔥

リファクタリングでやること、そのメリット

リファクタリングの意味を理解したところで、ここからは「リファクタリングって結局何をするの？」「どんなメリットがあるの？」を端的にまとめていきます！
今回は、私が実務の中で教わったことをまとめていますので、こんなのもあるよ！というものがあれば是非コメントください！

やること１：コードの整理整頓をする

メリット

理解しやすくなる
修正・拡張性の高さを考慮したコードになると開発がしやすくなる

やること２：　コードの品質を上げる

メリット

パフォーマンスの向上（処理の軽量化、高速化）

どのタイミングでリファクタするの？

結論：自分がこのコード読みづらい！わかりづらい！と感じた時

「そもそも何がわかりづらいのかがわからない、、」
という方はぜひ、師匠と呼べる方とペアプロできる環境があると大きく進化できるかもしれません。ピチューからライチュウくらいの勢いで進化できます。

私は師匠に、「ここの処理、もっとよくできるよね〜」と目を凝らしていただきながらリファクタ箇所を発見し、都度修正しています。

実例を見ながらリファクタリングしてみる

やること１：コードの整理整頓をする

モデルにparamsをそのまま渡していないかチェック
マジックナンバーを多用していないかチェック
条件分岐を3回以上ネストしていないかチェック

1. モデルにparamsをそのまま渡していないかチェック

📃理由：悪意のあるユーザーがSQLインジェクションを起こす可能性がある

※SQLインジェクション：アプリケーションの脆弱性を攻撃し、個人情報漏えい事故につながる可能性があるサイバー攻撃の1つです。

例えば以下のようなパターン。

# controllers/users_controller.rb
def create
 User.new(params[:user])
end

Userモデルには氏名やメールアドレス、パスワード情報などが含まれていると想定してください。
めちゃくちゃ個人情報ですよね。この処理の実行時、もし本来の意図ではない不当な「SQL」文が作成されると、データベースのデータを不正に登録できます。
modelはDBと接続する箇所なので、外から来た生データをそのまま渡してしまうと、情報漏洩や改ざんに使われてしまうのです。。怖い。。。

📝対応：「Strong Parameters」を追記

「でもparamsのデータを使いたい！」というときにはRailsが用意してくれてる便利な「Strong Parameters」を追記しましょう。
このとき一点注意したいのが、Strong Parametersを書く場所です。外部から変更されたくないのでprivateメソッドに書きます。

# controllers/users_controller.rb
def create
 User.new(params[:user])
end

# Strong Parametersの追記
private
  def user_params
    params.require(:users).permit(:name, :email)
  end

コードの記述としては間違いなく動くものですが、セキュリティ観点でコードを見直すことはとても大切なことですね。

2.マジックナンバーを多用していないかチェック

こちらは大きく二つの理由があります。順番にご紹介していきます！

📃理由1 ：ナンバーが結局何を意味しているのかが解らない！

例えば下記のような「Meat(お肉)モデル」があるとします。

# models/meat.rb
# Table name : meats
#  id　   　　　　　　　　　 :integer     not null, primary key
#  type  :integer     default("chiken"), not null
MEAT_TYPES = { :chiken => 1, :beef => 2, :pork => 3, :lamb => 4, :other => 5 }

上記のモデル下部に指定している定数を無視した記述でMeat.newのとき、typeカラムに「lamb(子羊肉)」を指定してみます。

# controllers/meats_controller.rb
 meat = Meat.new
 meat.type = 4

初めてみた人はmeat.type = 4って何やねん...となるかと思います。
モデルを見ればわかることではありますが、人間が直感的にわかるのは文字です。
本来の値を指定したつもりでも、マジックナンバーによって別のものが入ってしまう可能性もあります。

このように、後で他の人（本人が見ても）それが何を表すのかが分からなかったり間違えたりすることがあります。
読みやすいコードを意識するために、定数を活用していきましょう！

📃理由２：変更が生じた時、数値を参照している箇所を書き換えなければならない！

もし、Meat.typeのlambが不要になったので削除するとします。

# models/meat.rb
# Table name : meats
#  id　   　　　　　　　　　 :integer     not null, primary key
#  type  :integer     default("chiken"), not null
MEAT_TYPES = { :chiken => 1, :beef => 2, :pork => 3, :other => 4 }

この定数の変更後、meat.type = 4の記述をそのままにしてしまうと、、、

# controllers/meats_controller.rb
 meat = Meat.new
 meat.type = 4
# => otherが返ります

先ほどと想定したものではなく、別の値を取っていますね。

📝対応：コード中には定数名などの形で値を参照する

モデルにはすでに定数を指定しているのでこちらを活用します！

#↓ この定数を使っていく！！
MEAT_TYPES = { :chiken => 1, :beef => 2, :pork => 3, :other => 5 }

# controllers/meats_controller.rb
 meat = Meat.new
 meat.type = :lamb

定数にすることで他の人が見ても何が返ってくるのかが明確ですし、自分も記述ミスを起こす確率は減らせます！

3.条件分岐を3回以上ネストしていないかチェック

📃理由：処理が解りづらい

ここでもMeatモデルを使っていきます。

# models/meat.rb
# Table name : meats
#  id　   　　　　　　　　　 :integer     not null, primary key
#  type       :integer     default("chiken"), not null

MEAT_TYPES = { :chiken => 1, :beef => 2, :pork => 3, :lamb => 4, :other => 5 }

ここでいう「条件分岐をネストしている状態」とは下記のような状態です。

# controllers/meats_controller.rb
if #処理
  if meat.type == 'chiken'
    puts "鶏肉"
  elsif meat.type == 'beef'
    puts "牛肉"
  elsif meat.type == 'pork'
    puts "豚肉"
  elsif meat.type == 'other'
    puts "その他"
  end
end

ではこの条件分岐にもっとネストの階層を増やすとどうなるでしょう？

# controllers/meats_controller.rb
if # 条件
  if # 条件
    if # 条件
      if # 条件
        if # 条件
          if meat.type == 'chiken'
            puts "鶏肉" 
          elsif meat.type == 'beef'
            puts "牛肉"
          elsif meat.type == 'pork'
            puts "豚肉"
         　　elsif meat.type == 'other'
　　 　   　      puts "その他"
          end
　　　　　　　　    elsif # 条件
          # 処理
        else
          # 処理
        end
      end
    end
  end
end

書いた本人ですら頭がこんがらがりそうですね。脳内メモリに保っておくべき情報が増し増しで増えてショートします。

📝対応：条件自体の見直しをし、場合によっては後置ifを活用する

後置ifとは、条件分岐のネストを深くしないための技法の一つです。
例外的な条件で値を返すことがわかっていれば、処理の冒頭部分でreturnし例外処理を完結させちゃいます。メインの処理に影響を与えないようにする手段です！
要は初めから例外になるなら、冗長に書くより「結論。例外です！」と書いてしまうわけです。

# 先頭に後置ifを持ってくることで、その下に長い処理があっても読まなくていい可能性がある
return [] if hoge.nil?

あえて注意点を告げるとすると、「returnの内容が文字数的に長すぎる時は記述を工夫した方が良い」ということです。
上記のように英語を読む感覚で「 []を返す、hogeがnilだったら」などとスラッと読めればgoodなのですが。

後置ifの使用感に関して詳しく知りたい方はこちら↓（とても解りやすく記述されてますのでご参考に）
【Ruby】乱用厳禁！？後置ifで書くとかえって読みづらくなるケース

やること２：　コードの品質を上げる

処理の軽量化、高速化できる箇所をリファクタする

処理の軽量化、高速化できる箇所をリファクタする

📃理由： 504エラーなどを極力避けるため

504エラーとは、サーバーからの応答を長く待ち続け、「タイムアウト」になることです。
処理が重すぎた結果サーバー側が処理できずにエラーが返ってしまうことがあります。
非同期処理にしたり、サーバーの強化などもできますが、コードの見直しだけでも改善が見込めることも！

そもそもなぜ処理が重くなるの？

自分が対応したものだと、ほとんどの場合が「N+1問題」によるものでした。

「N+1問題」とは、ループ処理の中で都度SQLを発行し、大量のSQLが発行されてパフォーマンスが低下してしまう問題です。

処理が重たい = SQLの発行が無駄に多い

これをいかに減らすことができるかを考えると軽量化・高速化に繋げられます。
「SQLで複数同じSELECT文が発行され、WHERE条件のidだけ変わってるなぁ...」というログを見つけたらリファクタのチャンスかもしれません！

では実際にN+1のコードを見ていきます！
こんな感じの中間テーブルを含めた3つのモデルがあるとします。

そして以下の例はCSVのデータを作成するコードです。

    csv = CSV.generate do |csv|
      csv << ["ユーザーID", "メールアドレス", "姓", "名", "セイ", "メイ",　"レシピ投稿状況",
      users.each do |user|
        row = [
          user.id,
          user.email
          user.last_name,
          user.first_name,
          user.last_name_kana,
          user.first_name_kana
        ]

        posted_recipe_ids = RecipeRelation.where(user: user).pluck(:recipe_id)
        posted_recipe_ids.find_each do |posted_recipe_id|
        # 中間テーブルのRecipeRelationをSELECTし、真偽判定後rowに記述する値を決める
          if RecipeRelation.exists?(posted_recipe_id)
            is_saved = "投稿済み"
          else
            is_saved = "未投稿"
          end
          row.push(is_saved)
        end
        csv << row
      end
    end.windows31j_safe_encode
    send_data(csv, filename: "recipe.csv")
  end

「N+1問題」になっていそうな箇所を探ると、、繰り返し処理の中にありましたね。

posted_recipe_ids = RecipeRelation.where(user: user).pluck(:recipe_id)

こちらは繰り返す意味がなさそうです。

もう1箇所。

          if RecipeRelation.exists?(posted_recipe_id)
            is_saved = "投稿済み"
          else
            is_saved = "未投稿"
          end

こちらはRecipeRelationモデルに指定のIDがあるかないかの判定を下しています。
これもSELECTが都度発行され、「繰り返し処理の中にある繰り返し処理」で二重構造になっています。これはパフォーマンスが下がる。。。

上記2点を解消していきます！

📝対応1 ：繰り返し処理の外に出して問題ないものは外に出す

繰り返しが不要ならものは外でお願いして不要なSQLを発行させないようにしましょう！

📝対応2 ：モデルからSELECTせず、配列内で完結させる

モデルから取得するより、範囲が定まってる配列内で処理を実行すればそれだけで処理の軽量化につながります！

　　　　　　　　# 対応1 ： 繰り返し処理の外に出して問題ないものは外に出す
    posted_recipe_ids = RecipeRelation.where(user: user).pluck(:recipe_id)

    csv = CSV.generate do |csv|
      csv << ["ユーザーID", "メールアドレス", "姓", "名", "セイ", "メイ",　"レシピ投稿状況",
      users.each do |user|
        row = [
          user.id,
          user.email
          user.last_name,
          user.first_name,
          user.last_name_kana,
          user.first_name_kana
        ]
        # 配列user_posted_recipe_idsを生成
        user_posted_recipe_ids = RecipeRelation.where(user: user).pluck(:recipe_id)
        posted_recipe_ids.find_each do |posted_recipe_id|
          # 対応2 ： モデルからSELECTせず、配列の中に指定のIDがあるかチェック
          if user_posted_recipe_ids.include?(posted_recipe_id)
            is_saved = "投稿済み"
          else
            is_saved = "未投稿"
          end
          row.push(is_saved)
        end
        csv << row
      end
    end.windows31j_safe_encode
    send_data(csv, filename: "recipe.csv")
  end

ちなみにこちらのリファクタ、実務の中で実行した際はCSV生成完了まで243202msだったものが5291msまで抑えることができました！

まとめ

今回はリファクタについてまとめてみました。

「自分が読みづらいな」、「理解しづらいな」と思ったコードは、結構みんなもわかりづらいことが多いようです。笑
自分のためにも、そしてそのシステムに携わる人のためにも、良いコードへしていけたらみんなでハッピーになれますね☺️

最後までお読みいただきありがとうございました！

参考

https://www.shadan-kun.com/waf/sql_injection/
https://railsguides.jp/action_controller_overview.html#strong-parameters

はじめに

環境

リファクタリングって何？

ではなぜリファクタをするのか？

リファクタリングでやること、そのメリット

やること１ ： コードの整理整頓をする

メリット

やること２： コードの品質を上げる

メリット

どのタイミングでリファクタするの？

結論 ： 自分がこのコード読みづらい！わかりづらい！と感じた時

実例を見ながらリファクタリングしてみる

やること１ ： コードの整理整頓をする

1. モデルにparamsをそのまま渡していないかチェック

📃理由 ： 悪意のあるユーザーがSQLインジェクションを起こす可能性がある

📝対応 ： 「Strong Parameters」を追記

2.マジックナンバーを多用していないかチェック

📃理由1 ： ナンバーが結局何を意味しているのかが解らない！

📃理由２ ： 変更が生じた時、数値を参照している箇所を書き換えなければならない！

📝対応 ： コード中には定数名などの形で値を参照する

3.条件分岐を3回以上ネストしていないかチェック

📃理由 ： 処理が解りづらい

📝対応 ： 条件自体の見直しをし、場合によっては後置ifを活用する

やること２： コードの品質を上げる

処理の軽量化、高速化できる箇所をリファクタする

📃理由 ： 504エラーなどを極力避けるため

そもそもなぜ処理が重くなるの？

処理が重たい = SQLの発行が無駄に多い

📝対応1 ： 繰り返し処理の外に出して問題ないものは外に出す

📝対応2 ： モデルからSELECTせず、配列内で完結させる

まとめ

参考

やること１：コードの整理整頓をする

やること２：　コードの品質を上げる

結論：自分がこのコード読みづらい！わかりづらい！と感じた時

やること１：コードの整理整頓をする

📃理由：悪意のあるユーザーがSQLインジェクションを起こす可能性がある

📝対応：「Strong Parameters」を追記

📃理由1 ：ナンバーが結局何を意味しているのかが解らない！

📃理由２：変更が生じた時、数値を参照している箇所を書き換えなければならない！

📝対応：コード中には定数名などの形で値を参照する

📃理由：処理が解りづらい

📝対応：条件自体の見直しをし、場合によっては後置ifを活用する

やること２：　コードの品質を上げる

📃理由： 504エラーなどを極力避けるため

📝対応1 ：繰り返し処理の外に出して問題ないものは外に出す

📝対応2 ：モデルからSELECTせず、配列内で完結させる