リファクタリング - 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
とするケースがあることなど、内部の実装を使う側も意識する必要があり、完全な隠蔽はできないし、この場合する必要もないことから、この本の書いている方法でよいですね。