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


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

# code_your_rubyとは？
[chooyan_eng](https://qiita.com/chooyan_eng)さんが作成し[こちら](https://qiita.com/chooyan_eng/items/46f5724202f90ab4004d)の記事で紹介されていたリポジトリです。
リポジトリをforkしてclone、例題に対してのコーディング、それをプルリクすることで、有志の方からレビューしてもらえるというものです。
詳細は[記事](https://qiita.com/chooyan_eng/items/46f5724202f90ab4004d)をご覧ください。

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

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

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

### 関数
```ruby
# 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
```

### テスト
```ruby
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だけどか）。キーワード引数を使用するとよい。

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

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

```ruby
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'だった場合も考慮すると良い。
数値に変換できる文字列は数値に変換し、それ以外はエラーを出すとか。

```ruby
# 引数の型チェックと変換を追加
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?を使うと良い。

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

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

```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. 確認できた内容を期待値としてテスト追加
など


```ruby
  # 特殊な値の入力に対応するか
  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
```


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

### 関数
```ruby
# 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
```

###テスト
```ruby
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
```

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