Conversation
toddmazierski
left a comment
toddmazierski
left a comment
There was a problem hiding this comment.
Cool! One last little tip is to run the file through RuboCop, which even with the default rules may have a few good suggestions.
| end | ||
|
|
||
| class ItemUpdater | ||
| include Updater |
There was a problem hiding this comment.
Overall, I really dig this approach! My main point of feedback is although we're not technically using inheritance (ex. class ItemUpdater < Updater), including the same module in each of these classes is effectively the same thing. If this feels like a good design, I'd just commit to using actual inheritance.
There was a problem hiding this comment.
Yeah, I noticed that too! It really feels like this exercise "wants" classical inheritance.
|
|
||
| def decrease_quality | ||
| if quality > 0 | ||
| item.quality = item.quality - 1 |
There was a problem hiding this comment.
A nit-pick, but this could be shortened to item.quality -= 1, and further shortened to:
item.quality -= 1 if quality > 0| end | ||
|
|
||
| module Updater | ||
| attr_accessor :item |
There was a problem hiding this comment.
Is this a public interface? If not, it could be moved to private.
| include Updater | ||
|
|
||
| def update_sell_in | ||
| end |
There was a problem hiding this comment.
A nit-pick, but you can further express the "no-op"-ness of this method by saying: def update_sell_in; end on one line. It's sort of a way of saying "this page intentionally left blank" .
| end | ||
|
|
||
| class ItemUpdaterFactory | ||
| def self.create_updater_for(item) |
There was a problem hiding this comment.
A nit-pick: I think the verb "build" is more often used in the context of factories, instead of "create".
| item.quality | ||
| end | ||
|
|
||
| def increase_quality |
There was a problem hiding this comment.
A Rubyism is adding a bang to methods that have side-effects like mutation. So, increase_quality! might be a better name.
A warning, though: Ruby devs go overboard with them, so use sparingly. 😆
There was a problem hiding this comment.
The side-effect-iness of this solution in general feels kinda odd to me. I mostly kept the original code and functionality and just put it in different places, so I didn't try to make it non-side-effecting. As it is, I think just about every method needs a !!
| include Updater | ||
|
|
||
| def update_quality | ||
| 2.times { decrease_quality } |
| end | ||
|
|
||
| class ItemUpdaterFactory | ||
| def self.create_updater_for(item) |
There was a problem hiding this comment.
This could also be a static method on Updater if you want to reduce the number of classes flying around.
| "Backstage passes to a TAFKAL80ETC concert" => | ||
| BackstagePassesItemUpdater.new(item), | ||
| "Conjured" => ConjuredItemUpdater.new(item) | ||
| }.fetch(lookup_name(item), ItemUpdater.new(item)) |
There was a problem hiding this comment.
Consider making the Conjured thing a separate routine to try when the name match fails. It could be a big confusing to read when certain strings in the map are name matches and another is actually a match on a substring.
There was a problem hiding this comment.
@dhartunian I was definitely thinking of you as I mapped this map
3 participants
I wanted to learn about modules, so I tried using a module. Not sure if it was the right way to go...
Would also appreciate any feedback about Rubyisms, code style, etc. Nitpicking welcome!
There aren't many tests mostly because I was using the Gold Master (provided by the exercise) as I refactored.
Description problem: http://iamnotmyself.com/2011/02/13/refactor-this-the-gilded-rose-kata/