はじめに
早いもので、現職について1ヶ月が過ぎました。
初めてrailsを業務で扱っている自分ですが、バリバリの先輩エンジニアにメタメタのギッタギタにレビューをしていただいています。
とてもありがたい。。
備忘録の意味も込めて整理しておこうと思います。
指摘事項の雰囲気ごとにまとめました。
- 開発の常識だよ系
- 知っておこう系
- かっこよく書こう系
設計だったり実装方針だったりのレビュー事項は一般化しづらいので含めていません。
そんなこと言われなくてもわかるでしょという恥ずかしい内容も多々ありますが、駆け出しなんだし恥ずかしがらずに晒していきます。
いってみよう。
開発常識系
rubyやrailsに限らず、開発者としてやっておこう/意識しようという内容のもの
ファイル末尾に空行を入れよう
POSIXという偉い規格が定めているテキストファイルの定義に反するから
テキストファイルとは「1 つ以上の行」行は「0 個以上の改行以外の文字と末尾の改行」
不要なファイルはコミットしない
例えば、rails gで自動生成されたものでも不要なファイルは消そう
modelsのspecとかは絶対作るわけではないので
rails gはいろいろ作ってくれがちなのでmigrationファイルを作成するときくらいしか使わないかもとのこと。
不要なトランザクションは貼らない
当たり前なのだけれど
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が発行されるのかを確認した上で使うこと
関連付けの使用
不要なSQLを発行させていないか気をつける
post.user.id
#postに紐づくuserを取得するselect文が発行される
post.user_id
#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をつかわなくても書ける場合が大半
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でどのようなフィルタリングをするかがわかりやすくなります。
関連があるときは明示的に使おう
#もともとの自分の実装
Item.where(shop: current_user.shop).find(params[:id])
#レビュー後
current_user.shop.items.find(params[:id])
このように書くことで、 current_user に紐づく何かを処理しないといけないということを明確にすることが多い
partial collection
なんらかの配列があってそれぞれになにかを表示したいというケース
-# もともとの自分の実装
- hoge_array.each do |hoge|
= hoge.name
= link_to ..
...
-# レビュー後
= render partial: 'hoge', collection: hoge_array, as: 'hoge'
-# この上で下のような_hoge.html.hamlを準備する
= hoge.name
= link_to ..
...
最後に
本当は「要件と照らし合わせたときの実装方針」とか「責務の分担に関する考え方」みたいな自分が感動した部分に関して紹介したかったのですが言語化が難しかったです。。
この部分に熟練のエンジニア方の凄さがつまっているはずなので、またの機会にアウトプットできればと思います。
駆け出しエンジニアのみなさま
おれはこんなレビューされて痺れたぜという話があれば教えてください。