More than 5 years have passed since last update.

ベーシックAdvent Calendar 2016

@ykyk1218

俺のコードレビューまとめ from 2016 to 2017

Last updated at 2016-12-01Posted at 2016-11-30

今年もAdventCalendarの季節がやってきました。
株式会社ベーシックのトップバッターは @ykyk1218 がつとめます！！

初回からボリューム満点ですが、どうぞお付き合いください。

今回はコードレビューに関する話になります。

2016年のまとめとして、自分がコードレビューをする時（もしくは自分がコードを書く時）にどういう点に気をつけているのかというのを振り返ってみました。

大きく3つの構成になってます。

一応同じ処理をかかないとか、ある程度プログラムを書いている人にとっては当たり前すぎるのは書いてないのでこれで全てではないです。（今回あげているのも当たり前の部類に入ってしまうかもしれませんが、、、）

対象
「あんまりいいコード書けないけど、ある程度言われたものは作れるようになってきた」
ぐらいのレベル感の人が見るといいことがあるかもしれません。

（仕事ではrubyをメインの言語としているので、多少ruby固有の話が出てきます。）

とりあえず押さえておくべきポイント

細かいプログラムの書き方よりも、とりあえずこの辺はちゃんとやっておきましょう、というところになります。

通信エラー対応をちゃんとやる

例）外部APIを実行する時の例

def index
  uri = URI.parse('http://qiita.com/api/v2/items?page=1&per_page=20') 
  json= Net::HTTP.get(uri)
end

rubyでQiitaAPIを実行してます。正常に動いた場合は問題ないのですが、 これはタイムアウトなどで通信がエラーになった場合 の処理が考慮されていません。

時と場合によりますが詳細なエラー通知をする必要がなければ問題なさそうではありますが、そういうケースが必要な場合は例外処理をいれるとよいです。

require 'net/http'
require 'logger'

begin
  uri = URI.parse('http://qiita.com/api/v2/items?page=1&per_page=20')
  html = Net::HTTP.get(uri)
  p html
rescue => e
  p e.message
  log = Logger.new('error.log')
  log.error("通信エラーが発生しました。: " + e.backtrace.join("\n"))
end

とはいえ普通にRailsを使っていれば、エラーが発生すればエラー画面を表示して、ログに出力してくれるのでそこまで問題はないです。

ただ、ここでもう一つ考慮をしたいところが...！！

API自体は正常に実行されたけど、APIに渡すパラメータ不正やAPIの認証処理などでエラーになった場合です。
この場合はgemなどを使ったAPIのラッパーを使わない限りは例外は投げられないので、上記の処理だけでは若干不足です。

API実行時のエラーはAPIの仕様によって違うと思うですが、QiitaAPIの場合はhttpステータスコードで判断できるので、200番以外はエラーとします。

require 'net/http'
require 'logger'
require 'json'

begin
  uri = URI.parse('http://qiita.com/api/v2/items?page=1&per_page=20a') # per_pageパラメータがおかしいのでエラーになる
  http = Net::HTTP.new(uri.host, uri.port)
  response = nil
  http.start do |h|
    response = h.get(uri)
  end
  
  log = Logger.new('error.log')
  case response
    when Net::HTTPSuccess
      json = response.body
      result = JSON.parse(json)
    when Net::HTTPClientError # 4xx系 apiを叩く側のパラメータ不足とか認証エラーとか
      json = response.body
      result = JSON.parse(json)
      log.error "http status: " + response.code + " , " + "message: " + result["message"]
    when Net::HTTPServerError # 5xx系 サーバー側でなにかあった場合
      log.error "api server error: " + response.code
  end
rescue => e
  p e.message
  log.error("通信エラーが発生しました。: " + e.message)

  # slackとかに通知できるとなおよし
end

こういうAPI実行時の4xx系のエラーの場合はAPI側でエラーメッセージを返すことが多いと思うので、ちゃんとAPI側から受け取ったエラーメッセージをログに吐き出すようにしておくと、のちの調査に役立ちます。

ちなみに↑の例ではQiitaAPIのパラメータ不正になるためエラーになります。その場合のQiitaAPIのエラーメッセージはこんな感じ。

{"message":"Bad request","type":"bad_request"}

これだけだと調査にはちょっと厳しいかも...
とはいえBad requestと書いてあれば、送信しているパラメータがおかしいのかな？と予測は立てやすくなります。

APIの実行結果のエラー判断がhttpのステータスコードで判断できず、返却された値で判断するような場合は、適宜ちゃんと処理をするようにして何が起きたのかを後で調査できるようにしておきましょう。

ajax通信の場合

ajax通信する場合もerrorの場合をちゃんと考慮しておきましょう。
通信エラーが起きたらなんらかの方法でユーザーへ通知させるのが親切ですね。

  $.ajax({
    type: 'GET',
    url: '適当なURL',
    dataType: 'json',
  }).done(function (response, textStatus, jqXHR) {
    console.dir(response)
    alert("成功")
    alert(response)
  }).fail(function (jqXHR, textStatus, errorThrown) {
    /* エラーの場合の処理を忘れずに！！ */
    alert(errorThrown.message)
    console.error(errorThrown.message)
  });

参考URL
例外処理に関しては、こちらの考え方が大変参考になります。
Railsアプリケーションにおけるエラー処理（例外設計）の考え方

トランザクション処理を忘れない

2つ以上のテーブルを更新する可能性がある場合は、ちゃんとトランザクションいれておきましょう。
例えば下記のような場合。(ActiveRecordを使ってます）

def update
  ActiveRecord::Base.transaction do
    # 何かしらの注文データを更新
    @order.save!

    # 注文データの更新履歴を作成
    OrderHistory.create!(order_id: @order.id, order_email: @order.email, order_status: @order.status)
  end
end

railsの場合ActiveRecordで関連テーブルとかまとめて1回のsaveで済むけど実際発行するSQLは更新するテーブル数分ある、なんていう場合はちゃんとrails側でトランザクション処理をしてくれてるようです。

あと、トランザクション処理はデータベースだけとは限りません！

外部の決済サービスを使っているような場合、DBへの登録と決済サービスへの登録、この両方がちゃんとできていないとデータに矛盾が生じてしまいます。

データの整合性を保つために、注文データ登録後に決済サービスとの通信でエラーになった場合はロールバックするなどして対応する必要があります。

将来的に分岐処理が増えてしまわないか

条件分岐をするときif文を使いますが、いろんな条件分岐をいれたらif〜elsifで埋め尽くされてしまわないか、というのを考えます。

例
とあるECサイトで、とある商品の場合に、特別な処理（価格を変えたりとか）を入れる場合
（ActiveRecordを使ってます。）

if @product.name == "hoghoge"
  @product.price = @product.price - 200
end

これだと、「とある商品」が増えた場合にあらたな分岐処理を追加しなければ、ならない可能性がありますね。
特に上記の場合商品名をみているので、最悪@product.name == "hogehoge" || @product.name == "fugafuga"なんていう状態になりかねない。

ならばとこんな感じで...
campaignモデルを用意してproductと関連付けるようにしてみます。

if @product.campaign.code == "special"
  @product.price = @product.price - 200
end

これで対象商品にcampaign_idを付与すれば、campaignだけの特別処理がいれられます。

ただ、この場合も商品Aの場合はOKだけど、商品Bの場合はspecialキャンペーン内でやっていることと微妙に違ってくる、なんていう場合があるかもしれません。
（いわゆる仕様追加とか、イレギュラーと呼ばれるプログラマが好きじゃないやつ）

if @product.campaign.code == "special"
  @product.price = @product.price - 200

elsif @product.campaign.code = "special2"
  @product.price = @product.price - 400
end

するとこうなってしまう。

そんなに増えないのであれば問題はないですが、この調子でどんどん増えるとちょっと見通しが悪くなってしまいますね。

単純に価格を変更したいだけであれば、campaignモデル側で、それぞれの割引額みたいのをもたせればOKなのですが...
（※discountカラムに割引額があるという前提の例）

price = @product.price - @product.campaign.discount

なんか価格変更だけでなく、special2の場合はspecial2購入フラグを立てたり、購入したら運営側にメール通知をしたいとか、処理がいっぱいある場合困ってしまいます。

そこでキャンペーンクラスに

class Campaign
  def special
    <何かの処理>
  end

  def special2
   <何かの処理>
  end
end

としておいて、sendメソッドを使い

Campaign.send(@product.campaign.code)

こうすると、ある程度複雑なキャンペーンの処理を実行することができます。

なんか処理がほとんど同じなんだけど、微妙に違う

railsで同じ処理がある場合なんかは、親クラスつくったり、モジュール化してincludeさせたりとかすると思うのですが、9割一緒だけどちょっと違う、みたいな場合。

例
いい具体的なサンプル思いつかなかったので、教科書的なサンプルコードになります。

こっちが普通にやった場合の処理。
呼び出すメソッドの引数で処理を分岐するようにします。

module AnimalBehaviro
  def make_noise(animal)
    p "#{animal}がないた"
    if animal == "イヌ"
      p "bow bow bow"
    elsif
      p "mew"
    end
    p "鳴き終わった"
  end
end

class Dog
  include AnimalBehaviro
  def bow
    make_noise("イヌ") 
  end
end

class Cat
  include AnimalBehaviro
  def mew
    make_noise("ネコ") 
  end
end


dog = Dog.new
dog.bow

cat = Cat.new
cat.mew

こっちがブロック構文を渡して処理する場合。
if文書かなくて済むのでいくらかスッキリ。

module Animal
  def make_noise(animal)
    p "#{animal}がないた"
    yield 
    p "鳴き終わった"
  end
end

class Dog
  include Animal
  def bow
    make_noise("イヌ") do
      p "bow bow bow"
    end
  end
end

class Cat
  include Animal
  def mew
    make_noise("ネコ") do
      p "mew"
    end
  end
end


dog = Dog.new
dog.bow

cat = Cat.new
cat.mew

このぐらいであればif文でわけてもいいと思いますが、共通処理のメソッドの中で、if文が3つも4つも増えてしまう場合や、if文で分岐したあとの処理がもう少し複雑な場合には、こちらの方法を検討してもよいかもです。

ブロック構文とかProcの説明に関しては、こちらが大変わかりやすいです。
[Ruby] ブロックやProcの使いどころを理解する

メソッド名と戻り値を気にする

プログラム書き始めの頃に意識するポイントではあるので、基本的なところだと思いますが、特に気にするところとしては

get多用しない
checkXXXというメソッドはisXXXで代用できないか

というところです。

あとはメソッド名とあってない戻り値も困るので
isXXXで0とか、エラーメッセージとかを戻さないようにします。
この場合はあくまで戻すのは true/false

メソッド名のつけ方に関しては参考になるサイトはいっぱいあるのですが、
うまくメソッド名を付けるための参考情報

はよくまとまっていて助かります。

後はあまり使わないほうがよい言葉などは下記にあります。
プログラミングでよく使う英単語のまとめ【随時更新】

セキュリティを確保できているか

railsを普通に使っている分には正直あまりセキュリティ的なところを意識せずとも、セキュリティが担保できているケースが多いです。（どういうことをやるとセキュリティ的に問題になるのか、ということを知らないとダメですが）

SQLインジェクション

問題があるケースとしては文字列でクエリを作成するようなパターン。

# 問題ない
User.where(email: params[:user][:email])

# ダメ
User.where("email = '#{params[:user][:email]}'")

アクセスしてはいけないページに直URLでアクセスできないか

例えばユーザーが好きなようにプロフィールページを作成できるようなサービスの場合
ユーザーごとにプロフィールページの管理画面が存在することになります。

その管理画面のプロフィール情報を入力するURLが
https://<適当なドメイン>/users/<ユーザーID>

みたいになっていてユーザーIDを適当に変えたときに、別のユーザーのプロフィールが見えるとやばいです。

細かいところ（だいたいruby限定の話）

ここからは少々細かいプログラムの話。

後置if、三項演算子を使ってif文の行数を短くする

if status == "cancel"
  message = "キャンセル"  
else
  message = "通常"
end

↓

message = (status == "cancel") ? "キャンセル" : "通常"

そのメソッド内でしか使わないようなものはprivateメソッドに押し込める

そのまんま
制限がかけられるところは、制限をかけます。

参考
[クソコードにならない為に、これだけは守って欲しい７つのこと]
(http://qiita.com/koitaro/items/c3720d9c3ac1e7b0590f#狭く強く優しく)

（おまけ）privateメソッドあれこれ

その１）親クラスのprivateメソッドは普通に呼び出せる

class Parent
  private
  def parent_private
    p "parent_private"
  end
end


class Child < Parent
  def sample
    parent_private
  end
end


ch = Child.new
ch.sample

普通に実行できます。

その２）sendメソッドを使うと外から呼べる

class Human
  private
  def secret
    p "secret"
  end
end

hu = Human.new
hu.send(:secret) # エラーにならない
hu.secret # エラーになる

詳しくはこの辺読むとわかるかもよ！
http://qiita.com/kidach1/items/055021ce42fe2a49fd66

引数が3つ以上になるなら、キーワード引数にする

引数が3つ以上になってしまう状態自体が、あまり良くないかもしれませんが、、、どうしようもない状況というのはあるかと思います。

そんなときはメソッド引数の順番を意識して混乱しないようキーワード引数を使いましょう。

例）引数で指定した日付のデータをアーカイブするメソッド

archive(start_date: start_date, end_date: end_date, archive_path: path)

eachをmapとかselectなどの配列操作メソッドに置き換えられないか考えてみる

例
each と ifを使った処理をselectに置き換える

def amount_order_details(order)
  amount_orders = []
  order.order_details.each do |order_detail|
    if order_detail.quantity > 100
      amount_orders.push(order_detail)
    end
  end
  amount_orders
end

↓

def amount_order_details(order)
  order.order_details.select{|v| v.quantity > 100}
end

思いとか思想的な話

プログラムを書くとき、コードレビューをするときの、技術とはあまり関係ない心の持ち方的な話です。

似たような感じで、技術的負債のコードに対する考え方は下記ブログにしたためました。
お時間ありましたら、是非読んでみてください！
技術的負債に立ち向かうための心構え

かもしれないプログラミング

自動車免許を取るときの講習で「だろう運転」ではなく、「かもしれない運転」をしてください、みたいなこと言われませんでした？？

それと同じ考えです。

仮に下記のようなファイルパスを渡して、ファイルを読み込み結果を返すメソッドがあったとします。

name = read_member_file(file_path)

正常なケースだけを考えるのであれば、問題ないですが、

このファイルパスにちゃんとファイルが存在しないかもしれない
本当にこのメソッドは、値がちゃんと返ってこないかもしれない。

というのを考えないといけません。

この場合ファイル存在チェックは、メソッド内で実装するのが正しいと思うので、呼び出し側ではあまり意識させてはダメなのですが、もしコードレビューをするときにこのメソッドの実装箇所をみる場合には、その辺考えてみる必要があります。

値がちゃんと返ってきたかどうかは呼び出し元でチェックする必要があるので、メソッドが何を返すかを把握して適切にハンドリングする必要があります。

常に エラーが起きるかもしれない という考えでプログラミングをしましょう。

この辺りについてはPHPカンファレンスでいい資料がありましたので、是非読んでみてください。
PHP7で堅牢なコードを書く - 例外処理、表明プログラミング、契約による設計 / PHP Conference 2016

圧倒的感謝

みなさんはコードレビューで指摘を受けたときどんな気持ちになるでしょうか？

細かいな
この場合は、それ気にしなくてもよくない・・・？
そこ後で直すから、とりあえずマージさせてくれ！

こんな感じでしょうか？
さらにここから議論が発展して、お互いの人格攻撃に・・・ということにはならないかもしれませんが、あまりいい気分じゃない人が多いかもしれませんね。

ここでコードレビューを円滑にするためのポイント。
それは見出しにも書いたように

感謝

この気持ちを忘れないようにしましょう。

コードレビューをしてくれてありがとう
新しい考え方教えてくれてありがとう
気づかなかったところを教えてくれてありがとう

常に

感謝 ...！！

これ、もとはうちのエンジニアがTGIF（社内勉強会）で発表した内容にあって、「あ、これはめっちゃいい」と思って勝手に拝借いたしました。

ありがとうございます。

You get articles that match your needs
You can efficiently read back useful information
You can use dark theme

What you can do with signing up