プログラミング上達には「コードレビュー」が大事という話
はじめに
こんにちは。
最近、「コードレビュー」で自分の未熟さを思い知ったと同時に、プログラミング上達には「コードレビュー」を受けることが大事だということを実感しました。
なので今回は「コードレビュー」について書こうと思います。
「コードレビュー」のススメ
レビューって何？
IT用語辞典より引用。
システム開発において、作成された仕様書やコードなどの成果物を開発者とは別の人が詳細に調べ、仕様や要求を満たす内容になっているか、誤りや不具合が無いか、資源の浪費や不必要な冗長さ、極端な低性能などの問題が無いかなどについて開発者にフィードバックする工程をレビューという。
また、一般の外来語としては、書籍や映像・音楽作品、ビデオゲーム、公演など鑑賞したり、店舗や施設を利用したり、製品を購入・利用したりした人が、対象を評価することや、感想や論評を文章や評点などの形に表したものをレビューということが多い。
レビューする人のことを「レビュワー」(reviewer、レビューアとも)、レビューされる人のことを「レビューイ」(reviewee、レビュイーとも)と呼ぶ。
「コードレビュー」をすると何がいいの？
自分がレビューの中でも大事だと考えるのは「コードレビュー」です。
中でもプログラミング上達過程における「コードレビュー」は、実際に手を動かすことと同じくらい勉強になります。
考えられる効果は下記の通りです。
- プログラミングに関する良いお作法、書き方を知ることができる。
- 自分の書き方の悪いクセを知ることができる。
- いわゆる「クソコード」がどんなものか理解できる。
- 開発言語に関する良い知見を得ることができる。
どんな場所でコードレビューして貰えばいいの？
社内でしてもらえる場合はそれでもいいですが、オススメは勉強中の言語のコミュニティの勉強会に参加してレビューをお願いすることです。
社内だとその会社の技術力が低かった場合、逆に悪い習慣を身につけてしまうこともあります。
その点、コミュニティの勉強会には少なくとも1人はその言語に精通した人がいるので、良い知見を得られやすいです。
関西、かつrubyコミュニティだと西脇.rb、Kobe.rb、Shinosaka.rb、Kyoto.rb、Rails Follow-up Kobe、Rails Follow-up Osakaなどがあります。
効果の高い「コードレビュー」をする上で必要なものは？
「コードレビュー」の効果を最大限に引き出すために必要なものがあります。
それは「テストコード」と「GitHub」です。
まず「テストコード」。
これはレビュアーからの指摘を受けてリファクタリングする際に役立ちます。
これがないとリファクタリング後に新たなバグを埋め込んでしまっていないかどうかの担保が取れません。
次に「GitHub」。
プログラミングスキルが低いうちはコードレビュー中に次々に指摘を受けていっぱいいっぱいになりがちです。
そのため、事前にレビューしてもらうコードのプルリクをGitHubに作成しておきレビュアーに公開、コメントをつけてもらうことでレビューの時間を効果的に使うことができます（プルリクに関する説明はここでは省きます）。
また、レビュー後の備忘録、メモとして使うこともできます。
参考リンク
下記リンク先のQiita記事にテストやコードレビューの必要性が詳しく書かれています。
ぜひご一読ください。
qiita.com
コードレビューの事例
ここからは実際のコードを見て、コードレビューの効果を実感してみましょう。
おまけ付きの駄菓子屋さんでもらえる飲み物の本数を求める
まずはbonus_drink.rbというプログラムです。
コードレビューとリファクタリングはRails Follow-up Kobeでしてもらいました。
仕様に関する問題は下記の通りです。
ある駄菓子屋で飲み物を買うと、空き瓶3本で新しい飲み物を1本プレゼントしてくれる。
最初に購入した飲み物の本数から、トータルで飲める本数を算出するプログラムを作成せよ。
また、最初に100本購入した場合、トータルで何本飲めるか。
次にソースコードです。
まずはリファクタリング前を見てみましょう。
class BonusDrink def self.total_count_for(purchase) purchase + calculate_bonus(purchase) end private def self.calculate_bonus(purchase) bonus = 0 if purchase >= 3 purchase -= 3 bonus += 1 bonus = bonus + (purchase / 2) if purchase > 0 end bonus end end
コードの説明をすると、まずtotal_count_forではpurchase（購入本数）とcalculate_bonusで算出したおまけの本数を足しあわせてトータルで飲める本数を算出しています。
calculate_bonusではまずbonus（おまけの本数）を0で初期化し、purchase（購入本数）が3本未満である場合は0を返すようにしています。
purchaseが3本以上であった場合、purchaseから最初のおまけと交換した本数を省き、bonusを1本分カウントアップしています。
そして2本目のおまけ以降は交換したおまけを含めて3本になったらおまけと交換できます。
つまり2本購入すればおまけと交換できるわけです。
そこで最初のおまけに購入本数を2で割った商を足しあわせ、最終的なおまけの本数として返しています。
説明するとこんな感じなのですが、上記のソースコードを初めて読んだ人にとってはロジックの根拠がわかりづらい書き方になっています。
それではリファクタリング後のソースコードを見てみましょう。
class BonusDrink BONUS_PER = 3 BONUS_GET = 1 class << self def total_count_for(purchase) purchase + calculate_bonus(purchase) end private def calculate_bonus(empty_bottle) # 空き瓶3本目まではおまけなし return 0 if empty_bottle < BONUS_PER # 最初のおまけ + (最初のおまけと交換した空き瓶を引いた残り本数) / (2本目のおまけ以降はおまけ一本を含めて3本) BONUS_GET + (empty_bottle.to_i - BONUS_PER) / (BONUS_PER - BONUS_GET) end end end
リファクタリング後はリファクタリング前と比較して大分すっきりしましたね。
特にcalculate_bonusに関しては7行（空白行を除く）あったソースコードがたった2行（空白行とコメント行を除く）になっています。
ではどう変わったのか見ていきましょう。
まずはクラスメソッドのprivate化に関してです。
下記のように書くことでcalclate_bonusはprivateなメソッドになっていそうですが、実は外部から呼び出すことができてしまいます。
private def self.method_name # 実装 end
irb(main):001:0> require './bonus_drink' => true irb(main):002:0> BonusDrink.calculate_bonus(10) => 4
下記のように書くことで、privateなメソッドになり外部から呼び出せなくなります。
class << self private def method_name # 実装 end end
irb(main):001:0> require './bonus_drink' => true irb(main):002:0> BonusDrink.calculate_bonus(10) NoMethodError: private method `calculate_bonus' called for BonusDrink:Class from (irb):2 from /Users/moritsukamasatoshi/.rbenv/versions/2.1.1/bin/irb:11:in `<main>'
次に変数名です。
リファクタリング前はcalculate_bonusの仮引数名がpurchase（購入した本数）になっていましたが、おまけの交換対象は購入した瓶の本数ではなく空き瓶の本数です。
そのため、リファクタリング後は仮引数名をempty_bottle（空き瓶の本数）に変更しています。
最後にcalculate_bonusの実装と定数化に関してです。
リファクタリング前は数値がベタ書きされていてどういう値なのかよくわかりませんでした（いわゆるマジックナンバー）。
リファクタリング後はおまけと交換する本数とおまけでもらえる本数の2つの数値が定数化されており、どういう数値かはっきりわかり、ロジックの意味もわかりやすくなっています。
定数化したおかげで本数の条件が変わった場合でも改修しやすくもなっています。
更にコメントで補強することでよりリーダブルなコードになっています。
このようにロジックの根拠がわかりづらいケースでは、コメントを有効利用すると読みやすくなります。
また、リファクタリング後はcalculate_bonusの最初で条件に合致した場合0を返すようにしたこと及び式をまとめたことにより、より簡潔な記述になっています。
ちなみにrubyでは最後に評価された値を戻り値として返すため、今回のcalculate_bonusの先頭行のように明示的に値を返す場合以外はreturnを省略するのが主流な書き方です。
三角形の形を求める
次はtriangle.rbというプログラムです。
コードレビューとリファクタリングはKobe.rbでしてもらいました。
引数で指定した辺の長さで形成される三角形の形を表示するプログラムです。
まずはリファクタリング前のソースコードです。
require 'nkf' class Triangle class << self def display_shape(line_a, line_b, line_c) lines = [line_a, line_b, line_c].map { |line| NKF.nkf('-m0Z1 -W -w', line).to_r } # 引数に整数、小数、分数以外が指定された場合String#to_rで0/1に変換されるため、ここでメッセージを表示する return '辺の長さは0より大きい整数、小数、分数を半角または全角で入力してください！' if or_less_zero?(lines) return '三角形じゃないです＞＜' unless triangle?(lines) if equilateral?(lines) '正三角形ですね！' elsif isosceles?(lines) '二等辺三角形ですね！' else '不等辺三角形ですね！' end end private def or_less_zero?(lines) (lines[0] <=> 0) <= 0 || (lines[1] <=> 0) <= 0 || (lines[2] <=> 0) <= 0 end def triangle?(lines) (lines[0] + lines[1] <=> lines[2]) == 1 && (lines[1] + lines[2] <=> lines[0]) == 1 && (lines[2] + lines[0] <=> lines[1]) == 1 end def equilateral?(lines) lines[0] == lines[1] && lines[1] == lines[2] end def isosceles?(lines) lines[0] == lines[1] || lines[1] == lines[2] || lines[2] == lines[0] end end end puts(Triangle.display_shape(ARGV[0], ARGV[1], ARGV[2]))
簡単にソースコード、主にdisplay_shapeの説明をします。
まず渡されてきたコマンドライン引数を全角から半角に正規化し、Rationalクラスの（有理数、つまり分数を扱うクラス）オブジェクトに変換します。
次にコマンドライン引数のバリデーションを行い、有効な値だった場合は三角形の形を判定し、その結果を表示します。
ソースコードを読むと、三角形の形を判定しているメソッドの中身の条件式が非常に冗長な書き方になっていますね。
Rationalクラスのリファレンスを読んだ際に<=や<等が載っておらず、<=>（UFO演算子）のみがあったため、Rationalクラスでは<=や<等の演算子が使えないと勘違いしたことが原因です。
実際はRationalクラスはComparableモジュールを継承しているため、<=や<等の演算子も使えます（リファレンスに載っていないのは<=や<がオーバーライドされていないからです）。
それではリファクタリング後のソースコードを見ていきましょう。
require 'nkf' class Triangle class << self def display_shape(line_a, line_b, line_c) lines = [line_a, line_b, line_c].map { |line| NKF.nkf('-m0Z1 -W -w', line).to_r } return '辺の長さは0より大きい整数、小数、分数を半角または全角で入力してください！' if zero_or_negative?(lines) return '三角形じゃないです＞＜' unless triangle?(lines) if equal_all_lengths?(lines) '正三角形ですね！' elsif equal_two_lengths?(lines) '二等辺三角形ですね！' else '不等辺三角形ですね！' end end private def zero_or_negative?(lines) lines.any? { |line| line <= 0 } end def triangle?(lines) (lines[0] + lines[1] > lines[2]) && (lines[1] + lines[2] > lines[0]) && (lines[2] + lines[0] > lines[1]) end def equal_all_lengths?(lines) lines.uniq.size == 1 end def equal_two_lengths?(lines) lines.uniq.size <= 2 end end end puts(Triangle.display_shape(ARGV[0], ARGV[1], ARGV[2]))
まず不要なコメントに関してです。
BonusDrinkの時と違い、リファクタリング前ではコードの悪いところ（to_rが変換できない文字列の場合は0/1を返すことを利用し、コマンドライン引数が0以下かどうかを判定する箇所で一緒に判定してしまっている）を補強するためにコメントを使っています。
このような場合はコメントを使うのではなく、コードの方を直しましょう（今回は時間の関係で直していません）。
リーダブルコードにもありますが、「優れたコード > ひどいコード + 優れたコメント」です。
次にコマンドライン引数が0以下かどうかを判定するメソッドに関してです。
名前が文法的におかしかったため、変更されています。
negativeはrubyのメソッドnegative?から来ています（negative?は0未満、つまり負の値かどうかを判定するメソッドです）。
実装の方を見ると、リファクタリング後は下記のようになっています。
lines.any? { |line| line <= 0 }
any?の説明に関しては下記を参照してください。
any? (Enumerable) - Rubyリファレンス
rubyにはこのような便利なメソッドが多数用意されているため、これらを駆使することで簡潔なコードを書くことができます。
また、ここは下記のような書き方もできます。
!lines.all?(&:positive?)
all?の説明に関しては下記を参照してください。
all? (Enumerable) - Rubyリファレンス
上記の引数のような書き方の説明は下記を参照してください。
Rubyのmap(&:name)というのはどういう意味？ - QA@IT
今回はより意味を解しやすいという理由で前者の書き方を採用しました。
次に三角形かどうかを判定するtriangle?メソッドに関してです。
リファクタリング後はかっこで囲うことで1つ1つの条件式がわかりやすいようになっています。
次に三角形の形を判定するメソッドに関してです。
リファクタリング前はequilateralやisoscelesといった意味のわかりにくい名前でしたが、リファクタリング後は平易な単語を使用して意味がわかりやすいようになっています。
また実装部分では、リファクタリング後はレシーバの配列のユニークな値の配列を返すuniqメソッドと配列のサイズを返すsizeメソッドを使ってより簡潔なコードになっています。
最後にリファクタリング後のソースコードに残っている課題に関してです。
今回は時間の関係で着手しませんでしたが、リファクタリング後のソースコードにはまだ改善すべき点が残っています。
まずdisplay_shapeメソッドにいくつもの処理を含んでしまっている点です。
メソッドの再利用性を高めるためには、適切な単位に処理を分割しておくことが大切です。
display_shapeでは下記の処理を行っており、それぞれのメソッドを作成することが必要です。
- 入力値の正規化（半角文字への変換）
- バリデーション（入力値のチェック）
- 三角形の形の判定
- 文字列の表示
またTriangleクラスはインスタンス変数やインスタンスメソッドを持たずクラスメソッドのみのため、クラスではなくモジュールにすべきという指摘も受けました。
最後に
ここまでお読みいただきありがとうございました。
何かご意見・ご感想あればコメントもらえるとありがたいです。
