hiyoko-programingの日記

プログラミングを勉強したてのひよっ子。　　　エンジニア目指して勉強中。

コードレビュー時のポイント

プログラミング・テキスト

コードレビュー時のポイントは以下の通り。

  • 文法
  • テクニック
  • 可読性
  • 設計
  • その他

文法

・古い記法を用いていないか？

検索して見つけたコードは、意図せずして古くなっている場合がある。
なるべく最新の記法を用いるようにする。

1
2
3
4
5
 
# ☓：ハッシュロケットを使用している
:name => :true

# ◯：最新の文法を使用している
name: true

・if文の条件の書き方が冗長になっていないか？

複雑な条件を設定して処理を分けたい場合、if文のネストが深くなり、行数も多くなってしまい、読みにくくなることがある。条件をまとめたり、三項演算子を用いることで、少ない記述で条件分岐できるようにする。

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
 
# ☓：if文のネストが深くなり、読みにくくなっている
if hogehoge == 2
 if fugafuga == 3
  if foo == 4
    puts  "Hello, World"
  end
 end
end

# ◯：条件をまとめてネストを浅くしている
if hogehoge == 2 && fugafuga == 3 && foo == 4
 puts "Hello, World"
end

# ◎：1行で書けるものは1行にまとめている
puts  "Hello, World"  if hogehoge == 2 && fugafuga == 3 && foo == 4

# ◎：三項演算子を使用している
hogehoge == 2 && fugafuga == 3 && foo == 4 ? puts "Hello, World" : return false

テクニック

モデル

・見た目に関するロジックをモデルに置いていないか？

人の年齢を表すカラムに「歳」を加えたり、重さを表すカラムに「キログラム」を加えるメソッドは、ビューでのみ使用される見た目を整えるロジックである。このような見た目に関わるロジックは、ヘルパーかデコレーターに記述する。

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
 
# ☓：ビューでしか使用しないメソッドをモデルに定義している

# 例) Userモデルにcreated_atを年, 月, 日の形に変換するメソッドを定義している
class User < ApplicationRecord
  def adjust_created_at
    created_at.to_date.strftime("%Y年%m月%d日")
  end
end

 # ◯：ヘルパーファイルにビューで使用するロジックを定義している
module ApplicationHelper
  def set_created_at(created_at)
    created_at.to_date.strftime("%Y年%m月%d日")
  end
end

# ◎：ビューで使用するロジックのうち、表示フォーマットを整えるものを、DraperもしくはActiveDecoratorを導入して実装している

# 例) ActiveDecoratorを使用してcreated_atを年, 月, 日の形に変換するメソッドを定義している
module UserDecorator
  def set_created_at
    created_at.to_date.strftime("%Y年%m月%d日")
  end
end

・１つのメソッドで多くのことをやりすぎていないか？

下記の例では、Userモデルに定義されたクラスメソッドchange_attributesが、tweet, commentの変更まで行なってしまっている。
１つのメソッドで行う処理はなるべく簡潔にまとめるようにする。

1
2
3
4
5
6
7
8
 
# X:Userモデルに定義された、change_attributesメソッドが、Tweet, Commentまで変更してしまっている
class User < ApplicationRecord
  def self.change_attributes(user, tweet, comment)
    user.attributes = { family_name: "Hoge", first_name: "Fuga" }
    tweet.attributes = { body: Foo }
    comment.attributes = { body: Bar}
  end
end

・スコープを活用しているか？

1
2
3
4
5
6
7
8
9
 
#  ◯:コントローラでよく使用するロジックをスコープとしてモデルに定義している

class User < ApplicationRecord
# 〜省略〜
 scope :team_red, -> { where(team: 0) }
 scope :captain, ->{ where(captain: true) }
end

# 上記のスコープにより、User.team_red.captainのように直感的にロジックを記述できる

コントローラ

・不必要な自作アクションはないか？

基本的にはrailsの7つのアクションで実装するようにする。
アクションの内部の記述が複雑になってしまう場合は、モデルやモジュールを活用して、別のファイルに処理を移す。

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
 
# X:makeアクションはcreateとやっていることが変わらない

def create
  @user.create(create_params)
end

def make
  @user.create(create_params)
end

# ◯:なるべく7つのアクションの範囲でルーティングを定義する

def create
  @user.create(create_params)
end

・N+1問題は起こっていないか？

ローカル環境では目立ちにくいN + 1問題だが、本番環境で多くのレコードを扱うようになると、アプリケーションの動作に致命的な負荷を与える可能性がある。has_many, belongs_toなどアソシエーションを設定しているモデルを扱う際には、includeなどの対策を忘れずに行うようにする。

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
 
# X:1対Nの関係で、includeをしていない
# 1つのCompanyが複数のEmployeeをもっている例
# includeをしないとEmployeeの数だけCompanyをSELECTするSQLが発行されるため、レコードが増えるほど重くなる

def index
  @employees = Employee.all 
end

# ◯:変数を定義する際にincludeしている
# includes(:employee)と記述することで、SQLの発行回数を抑えて動作を軽くできる

def index
  @employees = Employee.all .includes(:employee)
end

ビュー

・部分テンプレートを活用しているか？

特定のビューを繰り返したい時は、eachより動作の軽い部分テンプレートを用いる。

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
 
# X:レコードの数だけ同じビューを繰り返す部分で、eachを使用している
h2
- @users.each do |user|
  .user-container
    = user.name
    = user.profile
    = user.image
end

# ◯:繰り返しを部分テンプレートで記述している
# 部分テンプレートを使用する理由は動作が軽いから
h2
  = render partial: 'user-contaier', collection @users

ルーティング

・不要なルーティングを定義していないか？

不要なルーティングを定義してしまうと、万が一該当するパスにユーザーが遷移した時に500エラーが表示されてしまう。また、意図せずupdate, destroyなどのメソッドが動いてしまう危険性もあるため、必要なルーティングのみを定義するようにする。

1
2
3
4
5
 
# X : indexしか必要ないのに、resourcesで丸ごと定義している
resources :items

# ◯:only, exceptを使用して必要なルーティングのみ定義している
resources: items, only: :index

可読性

・スペースの数、ネストの深さは適切か？

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
 
# X:スペースの数がバラバラになっている
# fugafugaもhogehogeも同じクラスのインスタンスメソッドなのに、スペースの数が多い

def hogehoge
  puts "こんにちは"
end

  def fugafuga
    puts "Hello"
  end

# ◯:スペースの数が揃っている

def hogehoge
  puts "こんにちは"
end

def fugafuga
  puts "Hello"
end

・ファイルの最後の行が空行になっているか？

命名規則

・変数の名前は適切か？

変数の名前は、一目で変数の中身が分かるような命名を心がける。

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
 
// X:アルファベット1文字
// ※Javascriptのローカル変数ではたまに1文字だけの変数名を定義するが、rubyではあまり使わない

p = Person.first
s = Ship.all

// X:変数の中身が分からない名前
// 「一体どのモデルのeverythingなのか？」となってしまい、変数の目的が伝わらない

@everything = Tweet.all

// X:略語
// 略語は元の単語を想像しづらく、略し方も人それぞれ異なってしまうので使うべきでない
// 変数の中身が単数なのか複数なのかも分かりづらい

@au = User.admin

// ◯：
first_person = Person.first
ships = Ship.all

@tweets = Tweet.all

@admin_users  = User.admin

・キャメルケースとスネークケースを使い分けているか？

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
 
# キャメルケースは先頭が小文字で、単語の区切りを大文字で表す
# アッパーキャメルケースは先頭から大文字を使用する

キャメルケース: adminUserCommentCreator
アッパーキャメルケース: AdminUserCommentCreator


# スネークケースは単語の区切りをアンダースコアで表す
スネークケース: admin_user_comment_creator


Railsの慣習的な命名規則として、下記のように使い分ける。

クラス名：アッパーキャメルケース
メソッド名：スネークケース
変数名：スネークケース

また、Javascriptでは一般的にキャメルケースを用いて記述する。

・ハイフンとアンダースコアを使い分けているか？

・BEMでクラス名を定義できているか？

ハイフンとアンダースコアを明確に使い分けて命名を行うと、可読性が大きく向上する。CSSも書きやすくなる、BEM記法を用いてクラス名を定義していく。

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
 
// X:ハイフンとアンダーバーを使い分けていない

.review_box
  %ul.reviews-list
   %li.review_1
   %li.review_2
   %li.review_3_red
   %li.review_4


// ◯:ハイフンとアンダーバーを使い分け、BEMでクラス名を定義している
// 親要素のクラス名からアンダースコアを２つ並べて、クラス名を定義している
// 同じ要素のうち、差分を定義するクラス名をハイフンを使って定義している(-red)

.reviews
  %ul.reviews__list
    %li.reviews__list__row
    %li.reviews__list__row    
    %li.reviews__list__row.reviews__list__row-red
    %li.reviews__list__row

設計

・単一責任原則

単一責任原則とは、あるクラスを変更するべき理由は2つ以上あるべきでないというオブジェクト志向の原則の１つ。当該原則を満たしていないコードは、バグの原因となりやすく、将来の変更にも弱くなってしまう。
単一責任について詳しくは下記のQiitaの記事も参考になる。

単一責任の参考記事(リンク)

https://qiita.com/UWControl/items/98671f53120ae47ff93a#%E5%8D%98%E4%B8%80%E8%B2%AC%E4%BB%BB%E3%81%AE%E5%8E%9F%E5%89%87

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
 
# ☓:Gearクラスが車輪に関係するリム、タイヤの情報を持っている

# 自転車のギアを表すGearクラスが、「車輪のリムの直径、タイヤの厚み」まで責任を持ってしまっている
# Wheelクラスを作り、rim, tireの定義を移すべき

class Gear
  attr_reader :chainring, :cog, :rim, :tire
  def initialize(chainring, cog, rim, tire)
    @chainring = chainring
    @cog       = cog
    @rim       = rim
    @tire      = tire
  end

  def ratio
    chainring / cog.to_f
  end

  def gear_inches
    ratio * (rim + (tire * 2))
  end
end

# 下記のコードはギアに関する情報しか渡していないにも関わらずエラーになる
puts Gear.new(52, 11).ratio
# ArgumentError: wrong number of arguments (2 for 4)


# ◯:Wheelクラスを作り、車輪に関する定義を移す

class Gear
  attr_reader :chainring, :cog, :wheel
  def initialize(chainring, cog, wheel=nil)
    @chainring = chainring
    @cog       = cog
    @wheel     = wheel
  end

  def ratio
    chainring / cog.to_f
  end

  def gear_inches
    ratio * wheel.diameter
  end
end

class Wheel
  attr_reader :rim, :tire

  def initialize(rim, tire)
    @rim       = rim
    @tire      = tire
  end

  def diameter
    rim + (tire * 2)
  end

end


@wheel = Wheel.new(26, 1.5)
puts @wheel.circumference
# -> 91.106186954104

puts Gear.new(52, 11, @wheel).gear_inches
# -> 137.090909090909

・単一責任原則に違反していないかチェックする方法

(1)クラスの持つメソッドを質問に置き換える

1
2
3
4
5
6
 
「Gearさん、あなたのgear_inchesを教えてください」
# Gearにギアに関する質問をしているのでセーフ

「Gearさん、タイヤの厚みとリムの直径を教えてください」
# Gearと無関係なタイヤの質問をしているのでアウト
# 該当する定義、メソッドを別クラスに切り出すべき

(2)1つの文章でクラスを説明してみる

1
2
3
4
5
6
7
 
「Gearクラスはチェーンリングとコグ、その比率を持っています」
# ギアの役割を短い文章で表現できているのでセーフ

「Gearクラスはチェーンリングとコグ、その比率を持っており、更に関連した車輪のタイヤの厚みとリムの直径を持っていて、ギアインチを計算することがで計算することができます」

# 文章が長くなりすぎている
# 上手く文章にできない場合は、1つのクラスで多くのことをやりすぎている可能性が高い

その他

・コピペでコードを書いてしまっていないか？

ある実装を行う際に、ネットで検索したコードをコピーしてくることは、実際の開発現場でもよくある。しかし、コードの意味が分からないまま丸写しすると、予想外のバグを生みかねない。明らかにコピペだと分かる場合には、レビュー時にコードの意図を確認する。

・コミットが大きくなりすぎていないか？

１つのコミットで5~10ファイルを変更するのは、コミットが大きく、レビューアーの負担になる。
１つの変更につき1コミットが理想的。

・コミット名は適切か？

「fix」「特になし」「あ」などのコミットメッセージは、コミットの意図が名前から分からず不適切である。長くなってしまってもいいので、そのコミットで実現したいことが分かるコミット名をつける。
なお、コミットメッセージは日本語でも英語でも構わない。

X:修正
◯:Userのidが40未満だとusers#indexに表示されない不具合を修正

・マジックナンバーを使用していないか？

変数を定義する際、メソッドで数値をやり取りする際などに、生のまま扱われている数字をマジックナンバーという。
マジックナンバーを含むコードは、数字を見ただけでは「その数字が何を意味するのか」が分からないため、デバッグし辛いコードになってしまう。コメントアウトで数字の説明をする、定数を使う、といった工夫をして、なるべく数字に意味を持たせるようにする。

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
 
# X:インスタンスを作成する際にマジックナンバーを用いて値を代入している
# コードを見ただけは「type: 1」がどういった商品タイプを示しているのか分からない

Product.create {name: 'hogehoge', type: 1}

# ◯:コメントアウトで数字の説明をしている

# type: 1は日本産の商品
Product.create {name: 'hogehoge', type: 1}


# ◯:定数を定義して値を代入している
# コードを読むだけで「type: 1」が日本に関連した商品であることがわかる

JAPANESE_PRODUCT_TYPE = 1.freeze
Product.create {name: 'hogehoge', type: JAPANESE_PRODUCT_TYPE}

・変数にnilや0が入っても大丈夫な構造か？

バリデーションをかけていない、null制約を定義していないなどの理由で、変数にnil、0が入ってしまうことがある。このままnilの変数に対してメソッドの呼び出しを行うと、Undefined method <メソッド名> for for nil:NilClassというエラーが出てしまう。nilの場合は呼び出しが行われないようにするなどの対策を取る。

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
 
# X:非ログイン時に表示されるビューで、current_user.idを指定している
# current_userはログイン時のみ使用できるdeviseのヘルパーなので、非ログイン時に使おうとするとエラーになる

= link_to "プロフィール", "users/profile/#{current_user.id}"


# ◯:該当箇所がログイン時のみ読み込まれるように条件を設ける

- if user_signed_in?
  = link_to "プロフィール", "users/profile/#{current_user.id}"

・コンフリクトを解決しないままレビュー依頼を出していないか？

コンフリクトは解消しなければマージを行うことができない。コンフリクトが起きているブランチをレビューするということは、「レビューの後にコンフリクト解消のための修正が行われる」ことを意味する。レビュー後に行われた修正で、新たなバグが生まれたり、好ましくないコードが追加されてしまってはレビューの意味が薄れてしまう。レビューを行う際はコンフリクトが起きていないか確認する。

コメントの書き方

命令形にならないようにする

「こんな書き方ありえない、修正してください」と言われたら、誰でも修正する気がなくなってしまう。
「こっちの書き方の方がXXの理由でよさそうです」など、なるべく提案する形でコメントする。

良いコードを見つけたら良いと言う

コードレビューは何も良くないコードを指摘するだけの場所ではない。
綺麗なコードを見つけたり、自分の知らないgemを使っていたり、こんな書き方があったのかという気づきがあったら、コメントして伝えてあげる！

最後に

「バグを完全になくす」ことを目的にレビューを行うのは、レビュアーの負担が大きすぎる。どんなに丁寧なレビューを行なっても、バグは発生してしまう可能性があるし、レビュアーがバグの有無まで責任を持ってしまうと、毎回のレビューが億劫になってしまう。
「レビューする前よりちょっとだけコードを良くする」ことを意識してレビューを行う！