【初心者向け】実務未経験がプルリクエスト時レビュアーに指摘されたこと-コードお作法編-

はじめに

実務未経験の自分がエンジニアスクールでレビューを頂いた時に、
おそらく現場でも指摘されるであろう項目をまとめていきます。
まとめきれていない部分もありますので、随時追記していきます。
特に、独学でRails等を学んでいる方の参考になれば幸いです。
【続】実務未経験がプルリクエスト時レビュアーに指摘されたこと-コミット・RSpec編-

※現場等でエンジニアをやられている方で、本記事に間違い等ございましたら、
大変恐縮でありますがご指摘いただければと存じます。
よろしくお願いいたします。

そもそもプルリクエスト（PR）って？

プルリクエストとは、自分がローカルで作成したコード（作成ブランチ）を本番環境（master）へ反映する前に、
担当者やレビュアーにコードの変更点を通知し、マージ許可依頼をする機能です。
本番環境への悪影響の危険性やコードがより良くなる等のコミュニケーションをとりながら、
結果的に品質の高いコードが実装できる。
参考URL： サルでもわかるGit入門#プルリクエスト

大前提な心構え

• レビューしてもらうことは当たり前のことではなく、自分で書いたコードには責任を持つこと
• 常に人に見せることを意識して見やすい・理解しやすいコードを書くこと
• 同じような指摘を繰り返さない
• gemなどを使用する際はREAD MEを含めたコードを確認すること

（※Webの記事に載っているからといってすぐ導入は危険！）

基礎編

読みやすいコードにする為に基本的なことから始める。

1. インデントを揃える & 余計な空白行は消す

インデントがバラバラで不規則に空白行があるコードはとにかく読みにくい原因の１つ。
でも意外と注意深くコードを見ないと見落としてしましがち。
その為にもリンターを導入するとこのような単純ミスは少なくできます！

###リンターとは？
リンターは、コーディングのルールを統一する為に、
余計なインデントや１行に長すぎるコードなどをチェックしてくれるもの。
私はその中でも、Rubyにおける構文解析ツールであるRubocupを導入していました。
rubocop-airbnbなどのgemを活用することで少ない設定で導入することができます。
コードを書き始める前に、準備しておくと良さそうです。
参考URL：
[Github] rubocop-airbnb
rubocop−airbnbを使うにあたって

2.余計なコメントを書かない

独学で勉強している頃は、
理解しやすいように、コメントをこまめに書いた方が良さそうと考えていましたが、
レビューする側にとっては「コメントを読む」→「コードを読む」という余計な時間を使わせてしまうことに。
コメントを書くことよりも、理解しやすい変数名を使うなど優れたコードを書くことを意識した方が良い！
リーダブルコードという著書に理解しやすい優れたコードについて書かれていました。
参考文献： リーダブルコード（著書）
参考URL： リーダブルコードを読んだので、殴り書き

3. 参考にするWeb記事や文献の鮮度に気をかける

わからないことをGoogleで調べれば、複数の情報をすぐ得ることができます。
少なくとも１年以上経っている情報は、
その情報が本当に今使われているか？、古い手法でないか？を常に気を配ることが大事。
責任を持って自分のコードに反映させること！

コード・文法編

controllerなどコード内でのSQL処理など個人的に指摘されたことをメモしていきます。

4. シンボル: 値 の記法でハッシュを書く

部分テンプレートを例にあげれば

# この書き方よりも
<%= render :partial => "friend_list", :locals => { user: @user } %>

# シンボル: 値
<%= render partial: "friend_list", locals: { user: @user } %>

の方が単純に見やすいです。
また、部分テンプレートは以下のように短縮できるのでより見やすくなる。

# 短縮できます
<%= render "friend_list", user: @user %>

###部分テンプレートの補足
partial内ではインスタンス変数（@変数）を直接使うのは避ける。
インスタンス変数を使用すると再利用しずらいpartialになる。

5. ループ内でのSQL発行は避けるべき

controllers/users_controller.rb

def show
  @users = User.all
end

views/users/show.html.erb

<% @users.each do |user| %>
  <% Micropost.where(user_id: user.id).each do |micropost| %>
    <%= micropost.content %>
  <% end %>
<% end %>

悪い例で申し訳ありませんが、例えばこのような状態。
where句を使うと必ずSQLが発行されてしまう為、別な方法で取得した方が良いです。
ここで本当にwhere句を使わなければいけないのか？をよく考えると、
ループ内で使うことは今のところ、1つもありませんでした。

###少し脱線してしまいますが
個人的にこれは良いのでは？と思うものを紹介。
※こちらは特にコレをやれ！と言われたわけではなく個人的に調べて良さそうだなと感じているものです。
他に良い方法があるかもしれません、、、

each(ループ)とパフォーマンス関連でpartialとcollectionを活用した例です。

controllers/users_controller.rb

def show
  @users = User.all
end

views/users/show.html.erb

<% @users.each do |user| %>
  <%= render 'friend_list', user: user %>
<% end %>

上記の場合、eachで回してpartialに変数を渡していますが、
collectionでローカル変数を渡した方がパフォーマンスが良いし、何より見やすくなる。

views/users/show.html.erb

# asを使うことで渡す変数名を指定することもできます。
<%= render partial: 'friend_list', collection: @users, as: user %>

参考URL:
Railsドキュメント＃render
partialをrenderするときのcollectionとは？こっちの方が速いみたい。

6. N+1問題を意識する

N＋１問題とは、
前項目で紹介したようなSQLが必要以上に発行されてしまうためにパフォーマンスを低下させてしまう問題。
N(データ量)＋１回分のSQLが発行されてしまう状態のことを言います。
具体的には以下のような例になります。

models/user.rb

class User < ApplicationRecord
  has_many :microposts
end

models/micropost.rb

class Micropost < ApplicationRecord
  belongs_to :user
end

controllers/microposts_controller.rb

def show
  @microposts = Micropost.all
end

views/microposts/show.html.erb

<% @microposts.each do |micropost| %>
  <%= micropost.user.name %>
<% end %>

上記のコードは正常に出力はされますが、コンソール上で見てみるとN+1問題が発生しています。

Micropost Load (0.3ms)  SELECT "microposts".* FROM "microposts"
User Load (0.1ms)  SELECT  "users".* FROM "users" WHERE "users"."id" = ? LIMIT ?  [["id", 1], ["LIMIT", 1]]
User Load (0.1ms)  SELECT  "users".* FROM "users" WHERE "users"."id" = ? LIMIT ?  [["id", 2], ["LIMIT", 1]]
User Load (0.1ms)  SELECT  "users".* FROM "users" WHERE "users"."id" = ? LIMIT ?  [["id", 3], ["LIMIT", 1]]
.
.
(Micropostのデータ分SQLクエリが発行されている)

このN+1問題を解決するために、includesを使って事前に使うデータをロードしておきます。

controllers/microposts_controller.rb

def show
  @microposts = Micropost.includes(:user) # Micropost.all.includes(:user)と同じ
end

controllerで実行された時に、以下のような処理が行われます。
includesする前と比べてみると

micropost_controller.rbが実行された時

# @microposts = Micropost.all
Micropost Load (0.3ms)  SELECT  "microposts".* FROM "microposts"

# @microposts = Micropost.includes(:user) 
Micropost Load (0.6ms)  SELECT  "microposts".* FROM "microposts" 
User Load (0.3ms)  SELECT "users".* FROM "users" WHERE "users"."id" IN (?, ?, ?)  [["id", 1], ["id", 2], ["id", 3]]

比べてみると、controller処理の時点でuserのロードが行われています。
もう一度viewで行われるSQL処理を確認すると

Micropost Load (0.5ms)  SELECT "microposts".* FROM "microposts"
User Load (2.2ms)  SELECT "users".* FROM "users" WHERE "users"."id" IN (?, ?, ?)  [["id", 1], ["id", 2], ["id", 3]]

2つの処理に収まり、N+1問題が解消されています！
個人的には、ただ見た目上が良ければ大丈夫というわけではなく、
パフォーマンスについても考えながらコードを書くということの重要性を学んだ項目でした。

N+1問題を検知し警告してくれるbulletというgemがあるので使ってみると、改善・学習になると思います。
参考URL：
RAILS GUIDS: Active Record クエリインターフェイス
[Github] bullet

まとめ

基本的には、レビュアーや数ヶ月後に見返す自分のために読みやすい・保守性の高いコードを書くことを心がけていきます。
また文量が多くなってしまうため、コミット・RSpecで指摘いただいた内容は他にまとめさせていただきます。
現場等でエンジニアをやられている方で、本記事に間違い等ございましたら、
大変恐縮でありますがご指摘いただければと存じます。
よろしくお願いいたします。