コードオーナーがよくレビューする事5選

Last updated at 2023-12-14Posted at 2023-12-14

はじめに

Hubble Advent Calendar 2023の11日目¹です！

Hubbleでバックエンドエンジニアをしている @power3812 です。オブジェクト指向大好きマンで、神クラスを作れないかと模索の日々です

Hubbleのバックエンドチームはコードオーナー制度を導入しており、PRを出す際にコードオーナー1人以上のapproveがないとマージすることが出来ません。
その際にJoinしたての方によく自分がレビューする内容5選を紹介しようと思います

1. ActiveRecordでidを事前に配列で取ってwhereの条件にする

こちらが一番多いレビューになります。コードを見ると理解できると思います。

hoge_ids = Hoge.pluck(:id)
fugas = Fuga.where(hoge_id: hoge_ids)

これによって発行される実際のSQLは以下の通りです。

SELECT `fugas`.* FROM `fugas` WHERE `fugas`.`hoge_id` IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10...)

一見よくあるActiveRecordのロジックだと思われますが、hoge_idsが大量に存在する場合、実際のSQLのクエリ長が長くなりすぎてMySQL側でmax_allowed_packetの制限に引っかかってエラーになる可能性があります。
whereの条件にはサブクエリ(ActiveRecord_Relation)を指定する事ができるので、こういう場合はpluckではなくて、selectでidを指定してサブクエリにします。

hoge_id_query = Hoge.select(:id)
fugas = Fuga.where(hoge_id: hoge_id_query)

そうすると発行されるSQLは以下の通りです。

SELECT `fugas`.* FROM `fugas` WHERE `fugas`.`hoge_id` IN (SELECT `hoges`.`id` FROM `hoges`)

サブクエリになったことによりクエリ長が短くなり、max_allowed_packetの制限に引っかかる可能性がかなり低くなります。
しかし、重いサブクエリに関しては、あえて先にpluckしてidを取得している実装等もあります。(Hubbleにもあえてそうしている実装があります)
もちろん、JOINで条件を絞れるならそれが良いですが、ここらへんは実行計画を見ながらという形になります。

2. クラスのプロパティに非必須なプロパティを設定する

こちらもコードを見ると理解しやすいと思います。

class Validator
  private attr_reader :rules
  private attr_reader :inputs
  private attr_reader :errors

  def initialize(rules, inputs)
    set_rules(rules)
    set_inputs(inputs)
    clear_errors
  end

  def validate
    # バリデーションロジックを実装
    # 最後にエラーをreturnする
    @errors
  end
end

validator = Validator.new(rules, inputs)
errors = validator.validate

validator2 = Validator.new(rules, inputs2)
errors2 = validator.validate

上記のように実際のバリデーションで必要な変数をすべてプロパティで持ってしまうと、Validatorインスタンスを使い回すことができず、バリデーションが必要なたびにそのバリデーション専用のインスタンスを生成することになってしまいます。
そのため、原則としてクラスのプロパティはそのクラスが成り立つために必要最低限に抑える必要があります。

class Validator
  private attr_reader :rules

  def initialize(rules)
    set_rules(rules)
  end

  def validate(inputs)
    errors = []
    # バリデーションロジックを実装
    # 最後にエラーをreturnする
    errors
  end
end

validator = Validator.new(rules)

errors = validator.validate(inputs)
errors2 = validator.validate(inputs2)

ここで、rulesもプロパティから外すかどうかは設計によります。(Validatorを継承したHogeValidator等でrulesを隠蔽するときなど)

3. 例外処理で例外をcatchした際にそのままエラー文をフロントに返却する

Hubbleではバックエンドとフロントエンドが完全に別れていてAPI通信をしています。そのため、バックエンド側でエラーが発生した場合、そのメッセージをフロントに返す必要がありますが、それが例外で起きた場合、そのままエラーメッセージを返してはいけません。
具体例としては下記です。

def update
  ActiveRecord::Base.transaction do
    # 色々な処理
    @item.save!
  end
  
  render json: { message: "success" }, status: :ok
rescue StandardError => e
  render json: { message: e.message }, status: :bad_request
end

これだと、例外が起きた際にActiveRecord::RecordInvalid: Validation failed: Name can't be blank等のメッセージが返ります。これだとユーザー側はどうinputが間違っているのかわかりづらいのとなりより、内部エラーをそのまま返しているのでセキュリティリスクが上がります。今回の場合だと、Railsを使用していることと、nameというカラムが存在していることがバレます。

特にinternal errorの場合にその内容を返すとセキュリティリスクがかなり高まります。
APIのI/Oはユーザーフレンドリーであることと、セキュリティリスクが少ないことが重要です。

def update
  ActiveRecord::Base.transaction do
    # 色々な処理
    @item.save!
  end
  
  render json: { message: "success" }, status: :ok
rescue ActiveRecord::RecordInvalid => e
 # 本来はbefore_action等のバリデーションで弾くべき
 render json: { message: "record invalid" }, status: :bad_request
rescue StandardError => e
　　render json: { message: "internal server error" }, status: :internal_server_error
end

4. 変数名に型情報をつける(いわゆるハンガリアン記法)

これはチームのコード規約によるので一概に違うとは言えないですが、少なくともHubbleでは変数名に型を付けるハンガリアン記法は取り入れていないため、レビューの対象となります。

# bad
item_array

# good
items

# bad
item_json
item_hash

# good
item

これが見られる傾向としては、すでに変数として使用している単語が衝突するため、回避的に使われることがあります。(Rubyではprefixに_を使用できないため_itemなどに出来ないのも影響していると思います。)
そういう場合は実際にした処理を変数名につけると良いです。

# bad
item_array

# good
excluded_hoge_in_items

# bad
item_json
item_hash

# good
parsed_item

5. 複雑なクエリをORMで書く

ときにORM(Hubbleの場合はActiveRecord)で複雑なクエリを組む場面があると思います。その場合にクエリビルダがサポートしていないSQLの関数などを使用するときに生のSQLを組むときがあると思いますが、クエリビルダと生のSQLが入り混じったコードは書くのも読むのも時間がかかってしまいます。
例えば、従業員が所属する部署ごとに、最も給与が高い従業員を取得するクエリを考えます。
Rubyで考えると以下になります。

max_salary_query = Employee.joins(:department)
                           .where('employees.department_id = departments.id')
                           .select('MAX(salary)')

max_salary_employees = Employee.joins(:department)
                               .where('employees.department_id = departments.id')
                               .group('employees.department_id, department_info, employees.employee_id, employee_name')
                               .having('MAX(salary) = ?', max_salary_query)                                                           
                               .select('employees.department_id, CONCAT(departments.department_name, " (ID: ", departments.department_id, ")") AS department, employees.employee_id, CONCAT(first_name, " ", last_name) AS employee_name, MAX(salary) AS max_salary')

SQLだと以下になります。

SELECT
    employees.department_id,
    CONCAT(departments.department_name, ' (ID: ', departments.department_id, ')') AS department,
    employees.employee_id,
    CONCAT(employees.first_name, ' ', employees.last_name) AS employee_name,
    MAX(employees.salary) AS max_salary
FROM
    employees
JOIN
    departments ON employees.department_id = departments.department_id
GROUP BY
    employees.department_id, department_info, employees.employee_id, employee_name
HAVING
    MAX(employees.salary) = (
        SELECT
            MAX(salary)
        FROM
            employees
        WHERE
            department_id = employees.department_id
    );

ここまでだとまだActiveRecordの方がわかりやすいという方もいるかと思いますが、ここにCASE文などが混ざってくると難読になってくると思います。
これと言った明確な基準はないですが、複雑なクエリを組むときはORMよりもSQLを素直に書いたほうがいい場合もあります。

余談ですがORMのあるモデルがリレーション先をJOINした時点でJOIN先のテーブルのプロパティも所持してしまうので、そのテーブルクラスの役割を逸脱しているのでそのモデルでクエリを組むんじゃなくて生のSQLで組んだ方が良いとされる場合もあります。

終わりに

今回はHubbleでよくするコードレビュー5選について書いてみました！あくまでHubble流なので、個々のチームのコード規約をベースに参考にして頂けたら嬉しいです！

次回はアドカレ投稿2回目²の @KOHETs さんです！

平日のみの投稿なので15日ですが11日目の記事としています。 ↩
1回目の投稿はこちら ↩

You get articles that match your needs
You can efficiently read back useful information
You can use dark theme

What you can do with signing up