リファクタリング - Rubyエディション読書会(1)

cuzic です。

リファクタリング - Ruby エディション」読書会を主催してきました。

リファクタリング - Ruby エディション」は、かの名著 Martin Fowler のリファクタリングをサンプルコードを Ruby で書き換え、説明しなおすという野心的な試みを行っている書籍です。

その読書会を行った結果について、簡単に書くと、

  • 結構人が集まった、(10人以上)
  • みんないいペースで一章を読み終えることができた。
  • わりとなごやかで発言しやすい雰囲気で進めることができた。
  • 懇親会を非常に安価で実現できた。(近くにショッピングモールがあると便利)
  • リファクタリング - Ruby エディションはいろいろと不具合が多い書籍

というようなかんじでした。

リファクタリング - Ruby エディション」には、下記の修正すべき点がありました。

  • コードのインデントが一部ずれている
  • P27 の下にある図の days_rented の矢印が誤り(正しくは、a rental まで)
  • P28 の Movie.NEW_RELEASE は Movie::NEW_RELEASE の誤り(ほかにも同じ誤り多数)
  • 文章中には、テストの重要性を繰り返し述べているにもかかわらず、テストコードが記載されていない。
  • P33 P40 の each.movie.title は element.movie.title の誤り
  • P71 の include Price は include DefaultPrice の誤り
  • P72 の Movie クラスの定義は def initialize が欠けている。

あと、個人的な感想として本にツッコミたい点として、

  • P35 のコードは書き換えた後に、変数 result があるが、これはそもそも不要な変数と感じました。

具体的に引用すると、

def amount_for(rental)
  result = 0
  case rental.movie.price_code
  when Movie::REGULAR
    result += 2
    result += (rental.days_rented - 2)*1.5 if rental.days_rented > 2
  when Movie::NEW_RELEASE
    result += rental.days_rented * 3
  when Movie::CHILDRENS
    result += 1.5
    result += (rental.days_rented - 3) * 1.5 if rental.days_rented > 3
  end
  result
end

のところについては私であれば

def amount_for(rental)
  case rental.movie.price_code
  when Movie::REGULAR
    if rental.days_rented > 2
      return 2 + (rental.days_rented - 2)*1.5
    else
      return 2
    end
  when Movie::NEW_RELEASE
    result rental.days_rented * 3
  when Movie::CHILDRENS
    if rental.days_rented > 3
      return 1.5 + (rental.days_rented - 3) * 1.5
    else
      return 1.5
    end
  end
  return 0
end

のように書いて、不要なローカル変数を削除するかな、と思いました。

P65 から P72 で導入されているポリモーフィックにするための変更だが、ポリモーフィズムな振る舞いをさせる必要があるメソッドが1つである間は、私なら case when 文のままにしておきます。この本の中では今後の料金計算の方法の変更など仕様変更が待っているとのことであるので、そのような場合であればポリモーフィズムを導入させるかもしれませんが。

最後にこれはたんなる感想になるが、最終形のコードが

 # 最新作の映画の作成
 movie = Movie.new("The Watchman", NewReleasePrice.new)
 # 最新作から通常作品に価格体系が変更
 movie.price = RegularPrice.new

というように、 Dependency Injection というか Constructor Injection のパターンにしているのは、自分としては少し違和感を感じた。私であれば、

 class Movie
   def self.createNewRelease
     return self.new("The Watchman", NewReleasePrice.new)
   end
 end
 movie = Movie.createNewRelease("The Watchman")

というようにあわせて Factory Method も作成して、使う側からはファクトリメソッドを使わせるようなインタフェース設計にするように思った。

しかしながら、今書きながら考えていると、結局、

 movie.price = RegularPrice.new

とするケースがあることなど、内部の実装を使う側も意識する必要があり、完全な隠蔽はできないし、この場合する必要もないことから、この本の書いている方法でよいですね。