なぜ書くのか

TECH::CAMPを卒業し、プロのエンジニアの方々のもとで、働き始め、3ヶ月半...
先輩方のリソースを、多く割いてしまったと痛感している事、No,1は、そう、コードレビュー。

多くのリソースを割いていただいているからこそ、必ず学びに＋　誰かの役に立てたらと思い、この記事を書きます。

言語はRuby on Railsです。

その１　命名はきっちりつける

まず基本中の基本ですが、意外に最初はできない変数やメソッド名などの命名についてです。
これは、変更を加える際や、他の人がコードを読んだときの可読性に大いに影響します。（僕は英語読めないので恩恵は受けにくいですが）

以下のコードを見てください。

a = "strawberry"
b = 100

極端な例ですが、この変数をどこがで使用する際にこんな変数名だと、何が入ってるのか全く推測できず、コードを読む際のストレスが大きくなります。
当たり前ですが、明示的に変数の内容がわかる、命名を心がけるべきです。

product_name = "strawberry"
price = 100

こうすることで別の場所で変数を使用する際に、変数の中身に何が入っているのか憶測することができ、わざわざ代入のところまで遡る必要がなくなります。

その２　インデックス番号を極力使用しない

入社間もない頃まで、私自身、必要なデータを、全て配列にぶち込み、それを引き回すという事をやっていました。
全く可読性に配慮もなかったために、下記のような要素呼び出しも、お構いなしに使ってました。

data = [["soccer", "baseboll"], ["taro", "ichiro"]]

# basebollをやっている子の名前を取りたい
puts data[0][1]    #=>baseboll
puts data[1][1]    #=>ichiro

上記のような記述がコードの中に乱立すると、書き手以外は、０番目の、2番の要素が何かを理解するために、かなりの労力をかけなければわかりません。
いや、少し経てば、書き手ですら、何のことやらわからなくなるはずです。

メンテナンス性を上げるためにも、読み手にも、そして自分にも理解しやすいコードにするためには、こういったデータを使用する際には、必ずハッシュを用いるようにしましょう

data = {soccer: "taro", baseboll: "ichiro"}

puts data.key("ichiro") #=>baseboll
puts data[:baseboll]    #=>ichiro

上記のように記述することで、呼び出している要素が何なのかが、読み手に明確に伝わるようになります。
Keyを元にValueを取得するのも、簡単ですし逆も然りです。

その３　Railsのコントローラにてindexメソッドを使用する際に、最初からワンレコードのみを取得することを想定したようなコードは書かない

こちらは現在行っているiOSアプリとRailsのバックエンドとのつなぎ込みを行う際にレビュー頂いた事です。
iOSから一つのテーブルの主キーを送り、複数のテーブルにまたがる検索をかけた後、結果として得られたワンレコードを、iOS側に返すという実装でした。
ワンレコードを返すのであればshowだろうと最初は思ったのですが、そのテーブルの情報は何一つ、Swift側からは知ることができないので断念...

ということでindexアクションを使用し、書いたコードがこちら（改変してあります + モデルに切り分けてるところも切り分けず集約しています）

def index
  render json: Item.includes(item_stores: :item_store_customer)
        .find_by(item_stores:{consignment_item_store_customers: {customer_id: (params[:customer_id]}}))
end

一見問題ないようにも見えるのですが、よくよく考えてみると、indexアクションとは、リストを返すものなので、最初から、ワンレコード目的の探査を書くのは、おかしいということですね。スコープとして切り分けたとしても、再利用性も低くなりますし、何より、他の人が見たときに、indexアクションの振る舞いとしておかしいので混乱します。

結果としては以下のコードに変更しました。

def index
  render json: Item.includes(item_stores: :item_store_customer)
        .where(item_stores:{consignment_item_store_customers: {customer_id: (params[:customer_id]}}))
end

find_byをwhereに変更しただけですが、こちらの方がindexアクションの振る舞いとしては正しく見えます。
アプリケーション全体としてみたときに、それぞれの振る舞いがお作法に則っているか、確認する癖を付けたいなと。
Rails側での振る舞いが正しくなり、Swift側では、配列の0番目を抽出するだけで取得できます。

その４　updateアクションを使用する際、必要であれば、対象がnilである可能性も考慮した方がいい

Railsのコントローラにてよく目にするこの記述

def update
  item = Item.find(params[:id])
  render json: item.update(item_params)
end

上記の記述でもいいこともあるのですが（理由は後述します）、findメソッドは、万が一、レコードが見つからなかった際に、ActiveRecord::RecordNotFoundの例外が発生します。

「そもそもこのupdateのタイミングでデータを喪失しているのはおかしい」
「ここからupdateのリクエストを飛ばして、中身が存在していないことは許されない」

と言う場合には、例外を発生させて、きちんと、エラーを把握する必要があるかもしれませんが、データが存在していないことが、考えうる場合には、下記のような記述が好ましいと思います。

def update
  item = Item.find_by(params[:id])
  render json: item&.update(item_params)
end

まず、findをfind_byに変更します。find_byであれば、対象のレコードがもし存在していなかったとしても、例外が発生するのではなく、nilが返ります。そして、item&.updateのように書くことにより、itemの中身がnilだった場合、&よりも後ろのコードは、読まれなくなります。

レコードが存在していないことが、アプリケーション的に、あり得る&許される場合には、存在していない場合の動きもきちんと掌握する必要があると言うことですね。

その５　一つのメソッドで、多くのことをしない

続いては、メソッドを肥大化、させすぎないということです。

例えば次のような、働いた時間と時給をもとに日給を計算するメソッド。
労働時間が8時間を超えていた場合には、その超過分の残業代は、1.25倍になるよう計算します。

def calculate_salary(working_time, hourly_wage)
  if working_time <= 8
    hourly_wage * working_time
  else
    hourly_wage * 8 + ((working_time - 8) * hourly_wage * 1.25)
  end
end

p calculate_salary(8, 100) #=>800
p calculate_salary(9, 100) #=>925.0
p calculate_salary(10, 100) #=>1050.0

上記のコードにはもちろんワンサカ問題点はありますが、まずはここで、一つ目を解決します。
上記は給与を計算するメソッドです。そうした視点で考えてみると、超過分の残業代を求める部分の処理は別のメソッドに投げた方が良さそうですし、他に投げて抽象化することで、「別のところで使いたくなった！」なんてときに簡単に再利用することができます。

なのでいっそのことメソッドを切り分けちゃいましょう✊

def calculate_salary(working_time, hourly_wage)
  if working_time <= 8
    hourly_wage * working_time
  else
    hourly_wage * 8 + excess_payroll(working_time - 8, hourly_wage)
  end
end

def excess_payroll(overtime, hourly_wage)
  overtime * hourly_wage * 1.25
end

p calculate_salary(8, 100) #=>800
p calculate_salary(9, 100) #=>925.0
p calculate_salary(10, 100) #=>1050.0

上記のように、給与のうち、超過分の給料のみ切り分けるようにすれば、より処理の流れが明確になり、演算を行うコードもグッと読みやすくなります。

その６　引数の情報をもとに条件分岐をしない

次に、引数についてです。メソッドで引数を受け取り処理を行う際に、受け取った引数の情報を元に、条件分岐をするのはやめよう、と言うものです。

じゃあどうするか。
分岐する処理それぞれをメソッドに分てしまい、呼び出す時点で、処理を確定してしまうのが好ましいです。

先ほどの、給与計算を行うコードでみてみましょう。

def calculate_salary(working_time, hourly_wage)
  if working_time <= 8
    hourly_wage * working_time
  else
    hourly_wage * 8 + excess_payroll(working_time - 8, hourly_wage)
  end
end

def excess_payroll(overtime, hourly_wage)
  overtime * hourly_wage * 1.25
end

p calculate_salary(8, 100) #=>800
p calculate_salary(9, 100) #=>925.0
p calculate_salary(10, 100) #=>1050.0

みてみると、calculate_salaryメソッド内で引数として渡された労働時間（working_time）の情報を元に条件分岐がされています。
これを細分化し、メソッドを呼ぶ段階で時間超過をしているのか、そうではないのかを切り分けましょう。

def calculate_salary(working_time, hourly_wage)
    hourly_wage * working_time
end

def calculation_of_overtime(working_time, hourly_wage)
  hourly_wage * 8 + excess_payroll(working_time - 8, hourly_wage)
end

def excess_payroll(overtime, hourly_wage)
  overtime * hourly_wage * 1.25
end

working_time = gets.to_i

if working_time <= 8
  p calculate_salary(working_time, 100) #=>working_time = 8 A.800
else
  p calculation_of_overtime(working_time, 100) #=>working_time = 9 A.925.0
end

そもそも時間を超過しているのかしていないのかをメソッド呼び出し前に分岐し、一つのメソッドの処理を呼び出し前から確定させることができました。

その７　マジックナンバーを使用しない

続いてはマジックナンバーについてです。
マジックナンバーとは、プログラム中に突如現れる意図のわからない数字のことです。
僕自身はインデックス番号に近いものだと思っていて、これがまた、コードの中に乱立すると、次に読む人が「理解できない」と言うことが起こります。

また先ほどから使用している給与計算をするプログラムを例に取りましょう。

def calculate_salary(working_time, hourly_wage)
    hourly_wage * working_time
end

def calculation_of_overtime(working_time, hourly_wage)
  hourly_wage * 8 + excess_payroll(working_time - 8, hourly_wage)
end

def excess_payroll(overtime, hourly_wage)
  overtime * hourly_wage * 1.25
end

working_time = gets.to_i

if working_time <= 8
  p calculate_salary(working_time, 100) #=>working_time = 8 A.800
else
  p calculation_of_overtime(working_time, 100) #=>working_time = 9 A.925.0
end

パッと目につくプログラム中の謎の数字は「８」と「1.25」、そして「100」ではないでしょうか？
こうしたものがプログラムの中に乱立することで、影響範囲の把握が困難になり、変更に手間と、リスクが伴うようになります。

ではどうするか？メソッドで包み込み、マジックナンバーに名前をつけましょう。

def regular_working_hours
  8
end

def hourly_wage_increase_for_overtime
  1.25
end

def hourly_wage_price
  100
end

def calculate_salary(working_time, hourly_wage)
  hourly_wage * working_time
end

def calculation_of_overtime(working_time, hourly_wage)
hourly_wage * regular_working_hours + excess_payroll(working_time - regular_working_hours, hourly_wage)
end

def excess_payroll(overtime, hourly_wage)
overtime * hourly_wage * hourly_wage_increase_for_overtime
end

working_time = gets.to_i

if working_time <= regular_working_hours
p calculate_salary(working_time, hourly_wage_price) #=>working_time = 8 A.800
else
p calculation_of_overtime(working_time, hourly_wage_price) #=>working_time = 9 A.925.0
end

このようにすることで、後から別の人が読んだ際に、なんの数字だ？なんて聞くこともなく、変数を見て、「なるほど、時給か！」となることができます。
また、時給を変えたくなった際に変更するのは、「hourly_wage_price」の中の数字を変更するだけなので、変更漏れのリスクも同時に減らすことができます。

その８　不必要な代入をしない

これが僕自身結構やってしまうことが多く、何度も指摘をいただいてしまいました。
たとえば以下のようなコードです。

def index
  item_list = Item.all
  render json: item_list
end

なんとなく良さげにも見えますが、DB返されたデータ、データをそのまま返しているだけです。
取得したデータや、代入したモノをその後の処理で使用することなく、出力するならば、不要な代入をせず、そのまま使用しましょう。

def index
  render json: Item.all
end

これでかなりスッキリしました。

その９　コードレビューを依頼する際に、レビュワーが必ず、実行環境にあるとは限らないため、テストと機能実装はセットでレビュー依頼を出す。

これも一度指摘いただきました。
レビューしてくださる方が、実装した機能を、実行できる環境にあるかどうかはわかりません。
実行できない環境の場合、コードを見て、処理がうまく流れるかどうかを、頭の中で考える必要があります。
レビュワーに負担をかけない、余計なことを考えさせないようにするために、「動く」という指標をたてるためのテストを、必ず、レビューの中に含めようということです。

これは、時と場合、状況によっても違うとは思いますが、ある程度成熟したプロジェクト（テストの基準、方針等が定まっている）であれば、「このテストが通るってことは基準は満たしているな」という判断材料になります。

とにかくレビュー依頼はコードの品質を見ていただく場にする、という根本的なモノです。

その１０　配列として結果を受け取る場合、同じ挙動になるならば、eachではなく、mapを使う

eachメソッドを使用している箇所を見てmapに置き換えられそうな場合、極力mapを使用しましょう、ということです。
パフォーマンスの面でも、可読性の面でも、eachよりもmapの方が優れています。

# each
int_list = (1..3).to_a

int_to_double = []
int_list.each do |i|
  int_to_double << i * 2
end

p int_to_double
# =>[2, 4, 6]

# map
int_list = (1..3)

int_to_double = int_list.map do |i| 
  i * 2
end

p int_to_double
# =>[2, 4, 6]

配列に格納するための空配列の準備がいらなくなるためかなりスッキリ描くことができます。

最後に

コードレビューの際に、いただいた指摘は必ず、自分に落とし込むよう心がけましょう。
レビューしてくださる方は、わざわざ自分の手を止めて、コードを読んでくれています。
どんな指摘も真摯に受け止め、誠実に対応しましょう。