Ruby
コードレビュー

たかがFizzBuzz、されどFizzBuzz。初めてのコードレビュー体験

More than 1 year has passed since last update.

どんな内容?

現在、未経験からエンジニアへの転職を控え、Rubyの勉強をしているものです。
今回以下記事で紹介されているリポジトリを利用して、初めてのコードレビューを体験しました。
この記事はそのレビューの備忘録です。
実際のやり取りは、以下githubのリポジトリ上で行われました。
https://github.com/chooyan-eng/code-your-ruby/pull/1

code_your_rubyとは?

chooyan_engさんが作成しこちらの記事で紹介されていたリポジトリです。
リポジトリをforkしてclone、例題に対してのコーディング、それをプルリクすることで、有志の方からレビューしてもらえるというものです。
詳細は記事をご覧ください。

fizz_buzz問題とは?

今回行ったのは、練習用の例題として挙げられていたfizz_buzz問題です。
fizz_buzz問題とは以下の要件を満たす関数を作るというものです。
かなりメジャーな問題で、転職面接の際にもホワイトボードにロジック書いてみてと言われたことがあります。

  • 整数が3で割り切れる場合は「Fizz」を出力
  • 整数が5で割り切れる場合は「Buzz」を出力
  • 整数が3と5で割り切れる場合は「FizzBuzz」を出力
  • それ以外はそのままの数値を出力

レビュー前のコード

ただのfizz_buzz出力だけでは単純なので、練習として3と5以外の数値も設定できるような関数を作成しました。
以下がレビュー前の関数とそのテストです。
こちらのコードを元にレビュー頂きました。

関数

# fizz_buzz問題を出力
def fizz_buzz(number, set_fizz=3, set_buzz=5)
  fizz = (number % set_fizz).zero?
  buzz = (number % set_buzz).zero?
  if fizz && buzz
    'FizzBuzz'
  elsif fizz
    'Fizz'
  elsif buzz
    'Buzz'
  else
    number.to_s
  end
end

テスト

require_relative '../lib/fizz_buzz'
require 'minitest/autorun'

# fizz_buzz_問題のテスト
class TestFizzBuzz < Minitest::Test
  def test_fizz_buzz
    assert_equal '1', fizz_buzz(1)
    assert_equal '2', fizz_buzz(2)
    assert_equal 'Fizz', fizz_buzz(3)
    assert_equal '4', fizz_buzz(4)
    assert_equal 'Buzz', fizz_buzz(5)
    assert_equal 'FizzBuzz', fizz_buzz(15)
  end
end

指摘された内容

デフォルト引数ではなくキーワード引数を使用すべき。

設定用の引数が複数ある場合、デフォルト引数をしようすると、片方だけ変えたい時不便(今回だとset_buzzだけどか)。キーワード引数を使用するとよい。

+ def fizz_buzz(number, set_fizz: 3, set_fizz: 5)
- def fizz_buzz(number, set_fizz=3, set_buzz=5)

成功パターン以外のテストも実装すべき

テストコードが成功パターンのみなので、'十五'などエラーになる値についてのテストを追加すべき。

def test_output_error_message
    e = assert_raises ArgumentError do
      fizz_buzz('十五')
    end
    assert_equal '数値に変換できない値が入力されています。値:十五', e.message
    e = assert_raises ArgumentError do
      fizz_buzz(10, fizz_number: '十五')
    end
end

想定外の入力にも対応する処理を追加すべき(文字列の数値化)

入力値がintegerの1だけでなく文字列の'1'だった場合も考慮すると良い。
数値に変換できる文字列は数値に変換し、それ以外はエラーを出すとか。

# 引数の型チェックと変換を追加
if number.is_a?(Integer)
  val
elsif !number.to_i.zero? || number == '0'
  val.to_i
else
  raise ArgumentError.new("数値に変換できない値が入力されています。値:#{number}")
end

型判定はis_a?ではなくkind_of?を使うべき

Matzによるとis_a?は非推奨なのでkind_of?を使うと良い。

- if number.is_a?(Integer)
+ if number.kind_of?(Integer)

変数名は動詞にしない(set_fizz → fizz_number)

引数の名前がset_fizz, set_buzzと動詞で始まっている。
特にRubyはカッコなしでメソッドを呼べるため、この名前だとコード中でメソッド名と勘違いされる可能性がある。名前をつけるときには品詞を意識すべき。

- if fizz_buzz(number, set_fizz: 3, set_buzz: 5)
+ if fizz_buzz(number, fizz_number: 3, buzz_number: 5)

想定外の入力に対応したテストも追加すべき(現状を追認するテスト)

想定外の入力に対するテストを追加すると、テストケースという表現で挙動が明らかになるため、レビューアーや関数利用者が仕様を把握しやすくなる。またまずい仕様にも築きやすい。
手順は、、
1. 試しにfizz_buzz(0)実行
2. 何が起きるか確認
3. 確認できた内容を期待値としてテスト追加
など

  # 特殊な値の入力に対応するか
  def test_unexpected
    assert_equal 'FizzBuzz', fizz_buzz(0)
    assert_equal 'FizzBuzz', fizz_buzz('0')
    assert_equal 'FizzBuzz', fizz_buzz(-15)
    assert_equal 'Fizz', fizz_buzz(6, fizz_number: -3)
    assert_equal 'Buzz', fizz_buzz(10, buzz_number: -5)
    assert_equal 'FizzBuzz', fizz_buzz(15, fizz_number: -3, buzz_number: -5)
    assert_equal 'Buzz', fizz_buzz(10, fizz_number:3, buzz_number:5)
    assert_equal 'FizzBuzz', fizz_buzz(10, fizz_number:5, buzz_number:5)
  end

レビュー後のコード

上記レビューをもとに以下のような実装になりました。

関数

# fizz_buzz問題を出力
def fizz_buzz(number, fizz_number: 3, buzz_number: 5)
  # 引数の型チェック
  number, fizz_number, buzz_number = [number, fizz_number, buzz_number].map { |val|
    if val.kind_of?(Integer)
      val
    elsif !val.to_i.zero? || val == '0'
      val.to_i
    else
      raise ArgumentError.new("数値に変換できない値が入力されています。値:#{val}")
    end
  }

  # fizz_buzz判定(割り切れるかどうか)
  fizz = (number % fizz_number).zero?
  buzz = (number % buzz_number).zero?

  if fizz && buzz
    'FizzBuzz'
  elsif fizz
    'Fizz'
  elsif buzz
    'Buzz'
  else
    number.to_s
  end
end

テスト

require '../lib/fizz_buzz'
require 'minitest/autorun'


# fizz_buzz_問題のテスト
class TestFizzBuzz < Minitest::Test

  # ノーマルパターン
  def test_normal_number
    assert_equal '1', fizz_buzz(1)
    assert_equal '2', fizz_buzz(2)
    assert_equal 'Fizz', fizz_buzz(3)
    assert_equal '4', fizz_buzz(4)
    assert_equal 'Buzz', fizz_buzz(5)
    assert_equal 'FizzBuzz', fizz_buzz(15)
  end

  # アレンジパターン
  def test_arrange_number
    assert_equal '6', fizz_buzz(6, fizz_number: 7, buzz_number: 11)
    assert_equal 'Fizz', fizz_buzz(7, fizz_number: 7, buzz_number: 11)
    assert_equal 'Buzz', fizz_buzz(11, fizz_number: 7, buzz_number: 11)
    assert_equal 'FizzBuzz', fizz_buzz(77, fizz_number: 7, buzz_number: 11)
  end

  # 数値に変換できる文字列が入力された時、数値に変換し出力できるか
  def test_change_string_to_integer
    assert_equal '2', fizz_buzz('2')
    assert_equal 'Fizz', fizz_buzz('3')
    assert_equal 'Buzz', fizz_buzz('5')
    assert_equal 'FizzBuzz', fizz_buzz('15')
  end

  # 特殊な値の入力に対応するか
  def test_unexpected
    assert_equal 'FizzBuzz', fizz_buzz(0)
    assert_equal 'FizzBuzz', fizz_buzz('0')
    assert_equal 'FizzBuzz', fizz_buzz(-15)
    assert_equal 'Fizz', fizz_buzz(6, fizz_number: -3)
    assert_equal 'Buzz', fizz_buzz(10, buzz_number: -5)
    assert_equal 'FizzBuzz', fizz_buzz(15, fizz_number: -3, buzz_number: -5)
    assert_equal 'Buzz', fizz_buzz(10, fizz_number:3, buzz_number:5)
    assert_equal 'FizzBuzz', fizz_buzz(10, fizz_number:5, buzz_number:5)
  end

  # エラーの際に適切なエラーメッセージを出力できるか
  def test_output_error_message
    e = assert_raises ArgumentError do
      fizz_buzz('十五')
    end
    assert_equal '数値に変換できない値が入力されています。値:十五', e.message
    e = assert_raises ArgumentError do
      fizz_buzz(10, fizz_number: '十五')
    end
    assert_equal '数値に変換できない値が入力されています。値:十五', e.message
    e = assert_raises ArgumentError do
      fizz_buzz(10, buzz_number: '十五')
    end
    assert_equal '数値に変換できない値が入力されています。値:十五', e.message
  end

end

所感

はじめてのコードレビュー。といいいますかプルリク自体も初めてだったのですが、本当に勉強になりました。
「簡単な問題だしレビューもそう無いでしょ」と思っていたのですが、これほど詳細にレビュー頂けるとは驚きです。
まだまだツッコミどころあるコードで実務レベルではないとですが、最初よりはまともなコードが書けるようになったかと思います。
私と同じように未経験で転職を控えている方には、スキルを学ぶ良い機会だと思いました。