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

# はじめに
[Hubble Advent Calendar 2023](https://qiita.com/advent-calendar/2023/hubble)の11日目[^1]です！
[^1]:平日のみの投稿なので15日ですが11日目の記事としています。

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

Hubbleのバックエンドチームは[コードオーナー](https://docs.github.com/ja/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners)制度を導入しており、PRを出す際にコードオーナー1人以上のapproveがないとマージすることが出来ません。
その際にJoinしたての方によく自分がレビューする内容5選を紹介しようと思います:sunglasses:

## 1. ActiveRecordでidを事前に配列で取ってwhereの条件にする
こちらが一番多いレビューになります。コードを見ると理解できると思います。

```Ruby
hoge_ids = Hoge.pluck(:id)
fugas = Fuga.where(hoge_id: hoge_ids)
```
これによって発行される実際のSQLは以下の通りです。
```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の制限に引っかかって[エラー](https://dev.mysql.com/doc/refman/8.0/ja/packet-too-large.html)になる可能性があります。
whereの条件にはサブクエリ(ActiveRecord_Relation)を指定する事ができるので、こういう場合はpluckではなくて、selectでidを指定してサブクエリにします。

```Ruby
hoge_id_query = Hoge.select(:id)
fugas = Fuga.where(hoge_id: hoge_id_query)
```
そうすると発行されるSQLは以下の通りです。
```SQL
SELECT `fugas`.* FROM `fugas` WHERE `fugas`.`hoge_id` IN (SELECT `hoges`.`id` FROM `hoges`)
```
サブクエリになったことによりクエリ長が短くなり、max_allowed_packetの制限に引っかかる可能性がかなり低くなります。
しかし、重いサブクエリに関しては、あえて先にpluckしてidを取得している実装等もあります。(Hubbleにもあえてそうしている実装があります)
もちろん、JOINで条件を絞れるならそれが良いですが、ここらへんは実行計画を見ながらという形になります。

## 2. クラスのプロパティに非必須なプロパティを設定する
こちらもコードを見ると理解しやすいと思います。

```Ruby
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インスタンスを使い回すことができず、バリデーションが必要なたびにそのバリデーション専用のインスタンスを生成することになってしまいます。
そのため、原則としてクラスのプロパティはそのクラスが成り立つために必要最低限に抑える必要があります。

```Ruby
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通信をしています。そのため、バックエンド側でエラーが発生した場合、そのメッセージをフロントに返す必要がありますが、それが例外で起きた場合、そのままエラーメッセージを返してはいけません。
具体例としては下記です。
```Ruby
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はユーザーフレンドリーであることと、セキュリティリスクが少ないことが重要です。

```Ruby
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. 変数名に型情報をつける(いわゆる[ハンガリアン記法](https://ja.wikipedia.org/wiki/%E3%83%8F%E3%83%B3%E3%82%AC%E3%83%AA%E3%82%A2%E3%83%B3%E8%A8%98%E6%B3%95))
これはチームのコード規約によるので一概に違うとは言えないですが、少なくともHubbleでは変数名に型を付けるハンガリアン記法は取り入れていないため、レビューの対象となります。
```Ruby
# bad
item_array

# good
items

# bad
item_json
item_hash

# good
item
```
これが見られる傾向としては、すでに変数として使用している単語が衝突するため、回避的に使われることがあります。(Rubyではprefixに`_`を使用できないため`_item`などに出来ないのも影響していると思います。)
そういう場合は実際にした処理を変数名につけると良いです。
```Ruby
# 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で考えると以下になります。
```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だと以下になります。

```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回目[^2]の @KOHETs さんです！
[^2]:1回目の投稿は[こちら](https://qiita.com/KOHETs/items/bfcbd2b8a5a69dab37fe)