Machida.rb #04 に参加（FizzBuzz コードレビュー）

8月7日にオンライン開催された Machida.rb #04 に参加した。Machida.rb は先月に続き2回目の参加。参加者は約10人、ツールは ZOOM と esa。今回はコードレビュー会。

自分はコードレビューしてもらう枠で申し込んでいなかったのだが、運よくコードレビュー枠に空きがあり、先日書いた FizzBuzz を見てもらった！

masuyama13.hatenablog.com

コードレビュー前の FizzBuzz

require "test/unit"
require "./fizzbuzz"

class FizzbuzzTest < Test::Unit::TestCase
  def setup
    @fb = Fizzbuzz.new
  end

  def test_1を渡すと文字列1を返す
    assert_equal "1", @fb.fizzbuzz(1)
  end

  def test_3を渡すと文字列Fizzを返す
    assert_equal "Fizz", @fb.fizzbuzz(3)
  end

  def test_5を渡すと文字列Buzzを返す
    assert_equal "Buzz", @fb.fizzbuzz(5)
  end

  # refute_equal を使う練習で入れた特に意味のないテスト
  def test_15を渡すと文字列Buzzを返さない
    refute_equal "Buzz", @fb.fizzbuzz(15)
  end

  def test_15を渡すと文字列FizzBuzzを返す
    assert_equal "FizzBuzz", @fb.fizzbuzz(15)
  end
end

fizzbuzz.rb

class Fizzbuzz
  def fizzbuzz(num)
    if num % 15 == 0
      "FizzBuzz"
    elsif num % 3 == 0
      "Fizz"
    elsif num % 5 == 0
      "Buzz"
    else
      num.to_s
    end
  end
end

気になっていたこと

FizzBuzz クラスのオブジェクトはインスタンス変数などを持たないので、fizzbuzz メソッドはクラスメソッドにすべきか
もっといいテストの書き方がないか
putsなどで出力したものをテストすることができるか

いただいたアドバイスなど

fizzbuzz メソッドはクラスメソッドがよさそう
そうなると Fizzbuzz クラスはクラスじゃなくてモジュールでよさそう
assert_output を使うと標準出力をテストできる
条件判定の部分メソッドに切り出せそう
テスト名を日本語にしてるのがいい

コードレビュー後

皆さんにアドバイスいただきながらその場で修正した。

レビュー前後のコードはこちら=> https://github.com/masuyama13/fizzbuzz

fizzbuzz_test.rb

require "test/unit"
require "./fizzbuzz"

class FizzbuzzTest < Test::Unit::TestCase
  def wrapper(num)
    Fizzbuzz.run(num)
  end

  def test_1を渡すと文字列1を返す
    assert_equal "1", wrapper(1)
  end

  def test_3を渡すと文字列Fizzを返す
    assert_equal "Fizz", wrapper(3)
  end

  def test_5を渡すと文字列Buzzを返す
    assert_equal "Buzz", wrapper(5)
  end

  def test_15を渡すと文字列Buzzを返さない
    refute_equal "Buzz", wrapper(15)
  end

  def test_15を渡すと文字列FizzBuzzを返す
    assert_equal "FizzBuzz", wrapper(15)
  end
end

fizzbuzz.rb

module Fizzbuzz
  def self.run(num)
    if num % 15 == 0
      "FizzBuzz"
    elsif num % 3 == 0
      "Fizz"
    elsif num % 5 == 0
      "Buzz"
    else
      num.to_s
    end
  end
end

学んだこと

  def wrapper(num)
    Fizzbuzz.run(num)
  end

テストで同じ処理が何度もある場合、上のようにメソッドを定義する方法がある。FizzBuzz ぐらいの規模だと必要性は高くないが、大きいプログラムだと便利そう。覚えておきたい。

putsのテストにはまたチャレンジする予定。

ちなみに、Rails でよく使う以下の書き方は、test-unit でも使えた。Rails の拡張機能かと思っていた。

（8月10日追記：test-unit では使えるが、Minitest では使えない！）

  test "1を渡すと文字列1を返す" do
    assert_equal "1", wrapper(1)
  end

他のテスティングフレームワーク版

書き方は test-unit とほとんど変わらない。wapperメソッドは使っていない。

Minitest

require "minitest/autorun"
require "./fizzbuzz"

class FizzbuzzTest < Minitest::Test
  def test_1を渡すと文字列1を返す
    assert_equal "1", Fizzbuzz.run(1)
  end

  def test_3を渡すと文字列Fizzを返す
    assert_equal "Fizz", Fizzbuzz.run(3)
  end

  def test_5を渡すと文字列Buzzを返す
    assert_equal "Buzz", Fizzbuzz.run(5)
  end

  def test_15を渡すと文字列Buzzを返さない
    refute_equal "Buzz", Fizzbuzz.run(15)
  end

  def test_15を渡すと文字列FizzBuzzを返す
    assert_equal "FizzBuzz", Fizzbuzz.run(15)
  end
end

Rails 風

refute_equalの代わりにassert_not_equalを使いたかっただけ。

require "minitest/autorun"
require "active_support/all"
require "./fizzbuzz"

class FizzbuzzTest < ActiveSupport::TestCase
  def test_1を渡すと文字列1を返す
    assert_equal "1", Fizzbuzz.run(1)
  end

  def test_3を渡すと文字列Fizzを返す
    assert_equal "Fizz", Fizzbuzz.run(3)
  end

  def test_5を渡すと文字列Buzzを返す
    assert_equal "Buzz", Fizzbuzz.run(5)
  end

  def test_15を渡すと文字列Buzzを返さない
    assert_not_equal "Buzz", Fizzbuzz.run(15)
  end

  def test_15を渡すと文字列FizzBuzzを返す
    assert_equal "FizzBuzz", Fizzbuzz.run(15)
  end
end

masuyama13.hatenablog.com

Machida.rb の感想

今回は、私以外に2人が自作プログラムや実務レベルのプログラムのコードレビューを受けていた。正直難しくて少ししか理解できなかった。理解できないので質問や意見も出せない。

そんな中で FizzBuzz という簡単なコードを出すのは申し訳ないなと思ったが、皆さん歓迎してくれてとてもありがたかった。勉強になっただけでなく、楽しかった。

今の自分にわからないレベルの話も、早くわかるようになりたい！という学習のモチベーションになるので、わからないから参加しないというんじゃなくて、これからも積極的に参加していきたいと思った。知らない用語やメソッドを聞くたびに調べたりするので勉強になるし、わからないことがたくさんあるということも一人では知れないので、いい機会だと思っている。

こんな機会が探せばいくらでもある Ruby コミュニティは素晴らしい。