We were unable to load Disqus. If you are a moderator please see our troubleshooting guide.

brendan6 • 10 years ago

Interesting article, you address refactoring techniques that I like to do myself. However, I do find myself often struggling with one key aspect of this type of refactoring: Is it really best to create a tightly coupled service object instead of simply keeping this logic inside the model?

In your example you create ShoppingCartCalculator which is tightly coupled to both Cart and CartItem. If you were to change #cart_items to simply #items, your test would not break however it would also not function in the real world. Likewise if you were to change #discount to #discount_percent on CartItem.

Now, your integration tests should pick up an integration issue like this (after all, that's what they are there for) but the tight coupling still exists which I was always taught as a no-no. Anyways, just an internal debate I often have with myself trying to do the same thing and was wondering if you had any other thoughts in the matter?

soulcutter • 10 years ago

RSpec 3 will have verifying doubles, which mitigates that problem somewhat. See http://rhnh.net/2013/12/10/...

Marko Anastasov • 10 years ago

I personally don't mind the coupling when the classes are so close relatives, so to say. In more complex cases intermediate data objects may be helpful. Having integration tests is a must though.

briannorton • 10 years ago

I find that breaking out logic into new modules (decorators) proves helpful and gives myself and others on the team a really good chance at understanding what is going on. By using the patterns of breaking out things into semi-related functional groups, I find that my team looks first for functionality outside the model. The model is a series of validations, associations and defaults -- specifically not a location for logic.

Christopher Hendrix • 10 years ago

Can we stop overriding the term "model" to apparently mean "ActiveRecord::Base" derived class? Model means domain object, that's it. It just so happens that Rails has had so much influence in the conversation, that their poor defaults with respect to the coupling of what they call "models" and their ORM has become default.

Your ShoppingCartCalculator is just as much a model as your ShoppingCart.

With that, can we also stop relegating our PORO models to the lib/ garbage bin? Put them where they belong, in app/models

Chuntao Lu • 10 years ago

Great article. I completely forgot the laziness of 'let' and had to use instance variable instead. Now I know where the problem was. Thanks

Guest • 10 years ago

When testing counts, it's a good idea to assert deltas, rather than absolute numbers. By saying "x should go up by 1" instead of "x should equal 1", you make your tests more independent of each other.

Dennis • 9 years ago

> Non-lazy `let!` is an alternative, but it’s not worth the trouble. Simply use before blocks to initialize data.

As far as I know, `before` blocks should be used actions (e.g. setting up a stub or mock), while `let` should be used for dependencies (e.g. instance variables).