5t111111
5t111111
2015/12/15 19:39:14 投稿
2

「【Refactor Me】複雑な仕様に対抗するため、早くリファクタリングしないといけない」をリファクタリング

  • 仕様にある各項目をそれぞれハッシュで辞書として定義しました
  • 項目はクラスの属性として持つようにしました
  • if ... else ...ではなくインスタンスの状態にしたがってメニューの文字列を構築し返すようにしました

Before

require 'minitest/autorun'

module StarBacchus
  class Menu
    def create(base, options = {})
      if base == :latte && options[:milk] == :soy
        'ソイ スターバッカス ラテ'
      elsif base == :latte && options[:hot] && options[:espresso] == :extrashot
        'エクストラショット スターバッカス ラテ'
      elsif base == :latte && options[:iced] && options[:espresso] == :extrashot
        'アイス エクストラショット スターバッカス ラテ'
      elsif base == :latte && options[:hot]
        'スターバッカス ラテ'
      elsif base == :latte && options[:iced]
        'アイス スターバッカス ラテ'
      end
    end
  end
end

class TestMenu < MiniTest::Unit::TestCase
  def setup
    @menu = StarBacchus::Menu.new
  end

  def test_latte
    assert_equal 'スターバッカス ラテ', @menu.create(:latte, hot: true)
  end

  def test_iced_latte
    assert_equal 'アイス スターバッカス ラテ', @menu.create(:latte, iced: true)
  end

  def test_extrashot
    assert_equal 'エクストラショット スターバッカス ラテ', @menu.create(:latte, hot: true, espresso: :extrashot)
  end

  def test_iced_extrashot
    assert_equal 'アイス エクストラショット スターバッカス ラテ', @menu.create(:latte, iced: true, espresso: :extrashot)
  end

  def test_soy_latte
    assert_equal 'ソイ スターバッカス ラテ', @menu.create(:latte, hot: true, milk: :soy)
  end
end

After

require 'minitest/autorun'

module StarBacchus
  ESPRESSO_TYPES = { extrashot: 'エクストラショット', ristretto: 'リストレット' }
  MILK_TYPES = { lowfat: '低脂肪タイプミルク', nonfat: 'ノンファット', soy: 'ソイ' }
  EXTRA_TYPES = { vanilla: 'バニラ', caramel: 'キャラメル' }

  class Menu
    def create(base, options = {})
      coffee = StarBacchus.const_get(base.capitalize).new(options)
      return coffee.errors.join(' ') unless coffee.validate
      coffee.menu
    rescue NameError
      "#{base.capitalize}という種類のメニューは存在しません。"
    end
  end

  class Coffee
    attr_reader :name, :iced, :espresso, :milk, :extra, :errors

    def initialize(options = {})
      @iced = options[:iced] ? 'アイス' : nil
      @espresso = ESPRESSO_TYPES[options[:espresso]]
      @milk = MILK_TYPES[options[:milk]]
      @extra = EXTRA_TYPES[options[:extra]]
    end

    def validate
      @errors = []
      if @extra == EXTRA_TYPES[:vanilla] && @iced
        @errors << "#{@iced}の場合には#{EXTRA_TYPES[:vanilla]}は選択できません。"
      end
      @errors.empty? ? true : false
    end

    def menu
      [@iced, @espresso, @milk, @extra, @name].compact.join(' ')
    end
  end

  class Latte < Coffee
    def initialize(options = {})
      @name = 'スターバッカス ラテ'
      super
    end
  end
end

class TestMenu < MiniTest::Unit::TestCase
  def setup
    @menu = StarBacchus::Menu.new
  end

  def test_latte
    assert_equal 'スターバッカス ラテ', @menu.create(:latte, hot: true)
  end

  def test_latte_without_hot_option
    assert_equal 'スターバッカス ラテ', @menu.create(:latte)
  end

  def test_iced_latte
    assert_equal 'アイス スターバッカス ラテ', @menu.create(:latte, iced: true)
  end

  def test_extrashot
    assert_equal 'エクストラショット スターバッカス ラテ',
                 @menu.create(:latte, hot: true, espresso: :extrashot)
  end

  def test_iced_extrashot
    assert_equal 'アイス エクストラショット スターバッカス ラテ',
                 @menu.create(:latte, iced: true, espresso: :extrashot)
  end

  def test_soy_latte
    assert_equal 'ソイ スターバッカス ラテ',
                 @menu.create(:latte, hot: true, milk: :soy)
  end

  def test_iced_extrashot_lowfat
    assert_equal 'アイス エクストラショット 低脂肪タイプミルク スターバッカス ラテ',
                 @menu.create(:latte, iced: true, espresso: :extrashot, milk: :lowfat)
  end

  def test_iced_extrashot_lowfat_caramel
    assert_equal 'アイス エクストラショット 低脂肪タイプミルク キャラメル スターバッカス ラテ',
                 @menu.create(:latte, iced: true, espresso: :extrashot, milk: :lowfat, extra: :caramel)
  end

  def test_create_should_return_combined_error_string_when_validation_fails
    assert_equal 'アイスの場合にはバニラは選択できません。',
                 @menu.create(:latte, iced: true, extra: :vanilla)
  end

  def test_create_should_return_error_when_provided_base_class_does_not_exist
    assert_equal 'Sushiという種類のメニューは存在しません。', @menu.create(:sushi)
  end
end

class TestCoffee < MiniTest::Unit::TestCase
  def test_coffee_should_have_iced_word_for_iced_when_iced_is_provided
    assert_equal "アイス", StarBacchus::Coffee.new(iced: true).iced
  end

  def test_coffee_should_have_nil_for_iced_when_hot_is_provided
    assert_nil StarBacchus::Coffee.new(hot: true).iced
  end

  def test_coffee_should_have_nil_for_iced_when_neither_hot_nor_iced_is_provided
    assert_nil StarBacchus::Coffee.new.iced
  end

  def test_coffee_should_have_espresso_type_word_for_espresso_when_provided
    assert_equal "エクストラショット", StarBacchus::Coffee.new(espresso: :extrashot).espresso
  end

  def test_coffee_should_have_milk_type_word_for_milk_when_provided
    assert_equal "ソイ", StarBacchus::Coffee.new(milk: :soy).milk
  end

  def test_coffee_should_have_extra_type_word_for_extra_when_provided
    assert_equal "キャラメル", StarBacchus::Coffee.new(extra: :caramel).extra
  end

  def test_validate_should_fail_when_both_iced_and_vanilla_provided
    coffee = StarBacchus::Coffee.new(iced: true, extra: :vanilla)
    assert_equal false, coffee.validate
    assert_equal ['アイスの場合にはバニラは選択できません。'], coffee.errors
  end

  def test_menu_should_return_expected_combined_string
    assert_equal 'アイス リストレット ノンファット キャラメル',
                 StarBacchus::Coffee.new(iced: true, espresso: :ristretto, milk: :nonfat, extra: :caramel).menu
  end

  def test_menu_should_return_empty_string_when_no_options_provided
    assert_equal '', StarBacchus::Coffee.new.menu
  end
end

class TestLatte < MiniTest::Unit::TestCase
  def test_latte_has_correct_name
    assert_equal 'スターバッカス ラテ', StarBacchus::Latte.new.name
  end
end

みんなのコメント

youchan0813
youchan0813
2015/12/16 12:44:07 投稿

StarBacchus.const_get(base.capitalize)がやばそう。base:coffeeが選べるのは仕様ですか?

5t111111
5t111111
2015/12/16 14:58:33 投稿

コメントありがとうございます。突っ込まれるだろうなあ…とは思ってました。

NameError のハンドリングだけは落ちるので入れちゃいましたが、あくまでリファクタリングのお題と捉えて、元々の仕様/テストケースで想定されていない呼び出しについてはあえて何も考えないようにした、というのが base:coffee っていう、明らかに意図としては抽象クラスだろ、ってやつを選べてしまう仕様をそのままにしている背景というか言い訳です (他だと例えば、hot: trueiced: true が両方渡された場合どうするの?とかを無視) 。設計としてはやばいですよね。

クラスで表現するやり方自体はそのままで行きたいので、ファクトリっぽくインスタンスを返すような機構に書き直そうかなと思います。