はじめに

早いもので、現職について1ヶ月が過ぎました。
初めてrailsを業務で扱っている自分ですが、バリバリの先輩エンジニアにメタメタのギッタギタにレビューをしていただいています。
とてもありがたい。。

備忘録の意味も込めて整理しておこうと思います。
指摘事項の雰囲気ごとにまとめました。

開発の常識だよ系
知っておこう系
かっこよく書こう系

設計だったり実装方針だったりのレビュー事項は一般化しづらいので含めていません。
そんなこと言われなくてもわかるでしょという恥ずかしい内容も多々ありますが、駆け出しなんだし恥ずかしがらずに晒していきます。

いってみよう。

開発常識系

rubyやrailsに限らず、開発者としてやっておこう/意識しようという内容のもの

ファイル末尾に空行を入れよう

POSIXという偉い規格が定めているテキストファイルの定義に反するから

テキストファイルとは「1 つ以上の行」行は「0 個以上の改行以外の文字と末尾の改行」

引用元

不要なファイルはコミットしない

例えば、rails gで自動生成されたものでも不要なファイルは消そう
modelsのspecとかは絶対作るわけではないので

rails gはいろいろ作ってくれがちなのでmigrationファイルを作成するときくらいしか使わないかもとのこと。

不要なトランザクションは貼らない

当たり前なのだけれど

example_update.rb

def update
  @hoge = Hoge.find(params[:id])
  if Hoge.transaction_with {update_with_huga} 
    redirect_to ..
  else
    ...
  end
end

private

def update_with_huga
  #呼び出し元でトランザクション貼ってるからこっちでは貼る必要なし
end

※transaction_withはswitch_pointのメソッド

カラムの追加位置を意識しよう

add_column時はbeforeオプションを使って適切な位置に追加する

知っておこう系

「知らないとやばいよ」〜「知っておくと便利だよ」まで含めて

外部キー貼るとき

外部キーを表すときは、t.bigintではなく t.references で設定する
勝手にindexを貼ってくれる

.first / .last

.first だと、全てのデータをSELECT文で取得して最初のカラムを返す
レコードの数が増えていくであろうモデルに対しては使わないように気をつける

clients = Client.first
#SELECT * FROM clients ORDER BY clients.id ASC LIMIT 1

Active Record クエリインターフェイスはどんなSQLが発行されるのかを確認した上で使うこと

sanitize_sql_like使おう

そもそもsanitizeってなんだよと最初は思いました。
ものすごく簡単に言うとユーザーからの不正なSQL実行を防ぐための入力値のエスケープ処理というイメージであっているはず

#一般的なlike検索 -> 文字列にsql入れられたら実行されて困っちゃう
User.where('name LIKE ?', `%#{args[:name]}%`)

#sanitize_sql_like -> 文字列にエスケープかかるから安心
User.where('name LIKE ?', "%#{sanitize_sql_like(args[:name])}%")

validationはいろいろ準備されてるよ

例：入力値として整数だけを受け付けたいカラムが存在するとき

数字のみ受け付けたい

# もともとの自分の実装
VALID_NUMBER_INPUT_REGEX = /\A[0-9]+\z/.freeze
validates :hoge, presence: true, format: { with: VALID_NUMBER_INPUT_REGEX }

# レビュー後
validates :hoge, numericality: { only_integer: true }

numericality以外にもたくさんある
validationに限らずRailsGuideは一通り目を通すべきと痛感しました

かっこよく書こう系

こうするとrubyぽいよ、railsぽいよ、逆にそう書くとかっこわるいよという指摘

()いらない

java出身だと最初は違和感が抜けないです

@post = current_user.post.build()
#()は不要だよ
@post = current_user.post.build

不要な返り値いらない

場合によりけりですが、例えばなにかのbefore_actionで認証して失敗したらリダイレクトさせるときなんかは返り値不要

return true if @account&.authenticated? #このtrueいらない
redirect_to hoge_path

冗長なifはださい

これはrubyだからというわけでもない気がしますが。
シンプルな分岐はifをつかわなくても書ける場合が大半

booleanを返すメソッド

if account.admin?
  true
else
  account.shop.id == record.id
end

#こう書ける
account.admin? || account.shop_id == record.id

scopeを使おう

ItemはShopに所属している前提

もともとの自分の実装

#コントローラ
@items = Item.search(current_user.shop_id, params[:search_word]).page(params[:page]).per(USER_PER_PAGE)

#Userモデル searchメソッド
def self.search(shop_id, name)
  Item.where(shop: shop_id).where('name LIKE ?', "%#{name}%")
end

レビュー後

#コントローラ
@items = Item.where(shop: current_user.shop).keyword_by(params[:search_word]).page(...
# keyword_byは、model側にscopeを作る

#Userモデル scope
scope :keyword_by, ->(search_word) do
  if search_word.present?
    where('name LIKE ?', "%#{name}%")
  end
end

scopeを用いたほうがsearch の中に処理を内包するより、Controllerでどのようなフィルタリングをするかがわかりやすくなります。

partial collection

なんらかの配列があってそれぞれになにかを表示したいというケース

haml

-# もともとの自分の実装
- hoge_array.each do |hoge|
  = hoge.name
  =  link_to ..
  ...

-# レビュー後
= render partial: 'hoge', collection: hoge_array, as: 'hoge'

-# この上で下のような_hoge.html.hamlを準備する
= hoge.name
=  link_to ..
...

最後に

本当は「要件と照らし合わせたときの実装方針」とか「責務の分担に関する考え方」みたいな自分が感動した部分に関して紹介したかったのですが言語化が難しかったです。。

この部分に熟練のエンジニア方の凄さがつまっているはずなので、またの機会にアウトプットできればと思います。

駆け出しエンジニアのみなさま
おれはこんなレビューされて痺れたぜという話があれば教えてください。