This is a companion discussion topic for the original entry at https://thoughtbot.com/upcase/videos/law-of-demeter
I’m still not sure what solution to fix your last example on charging on credit card?
class CreditCardsControlller < ApplicationController def charge_for_plan if current_user.account.credit_card.valid? price = current_user.account.plan.price current_user.account.credit_card.charge price end end end
I think you can to create a new object like this :
class AccountCharger def initialize(account) @credit_card = account.credit_card @plan_price = account.plan_price end def charge @credit_card.charge(@plan_price) if @credit_card.valid? end end
And use it like this :
class CreditCardsControlller < ApplicationController def charge_for_plan AccountCharger.new(current_user.account).charge end end
Am I right @jferris and @benorenstein?
Totally agree to refactor into this service object, it is good for encapsulate a business logic. I still think we haven’t removed the problem yet, so the structure is still the same. In your code, there are multiple assignments that’s what @jferris explains in the episode.
If we check the definition :
A method of an object should invoke only the methods of the following kinds of objects:
- its parameters
- any objects it creates/instantiates
- its direct component objects
We can see then we can talk to components. In my solution,
account is a component of
AcountCharger so I think it’s ok to call methods on it.
I think what @jferris said is about variable assignement in the same method and the same object.
I’ll print out and pin this guidelines list on the wall in front of my desk!
@Guirec_Corbel I think it’s helpful to know the exact rules, but when applying it to code, it’s more helpful to think about what Law of Demeter violations are telling you, rather than trying to fix them with delegators or assignments.
The Law of Demeter is about dependencies. In the
AccountCharger example, there are a few dependencies:
- In order to construct an
AccountCharger, you need an object that responds to
- In order to
charge, you need an object that responds to
valid?, as well as a
This means that the
AccountCharger class depends on
account.credit_card.charge, as well as
account.credit_card.valid?, because you can’t construct an
AccountCharger without passing it something which satisfies that dependency graph.
More importantly, thinking about cohesion, you can see that this class is actually largely related to credit cards. It doesn’t follow Tell, Don’t Ask: if first looks to see if the credit card is valid, and then makes a decision on the credit card’s behalf based on the outcome.
Tell, Don’t Ask is frequently at odds which separation of concerns, but in this case that
credit_card object is already burdened with knowledge of charging and validation. The answer that jumps out at me is to move this logic into
credit_card and flatten the dependencies:
class CreditCard def charge_valid if valid? charge end end end
However, I think it’s also taking a few steps back to think about why you’re getting close to charging a credit card before knowing whether or not it’s valid.
@jferris, I think your right about dependencies but I’m not sure if the
charge method is at the right place.
It depends of what it do but I assume than CreditCard contains information about a credit card. If it’s a model in the sense of ActiveModel (but without persitence), I think its responsability is to keep data, check validity and manage persitence (probably already too much) .
charge will probably use other things than only what ActiveModel do and will talk to external services (like a Payment API) and it will may be use other models (like ordre or account).
I think it will take too much responsability and it’s probably better to use a service object. AccountCharger is probably a good place to do all things related to payment.
Am I right? I may be too far from the original problem and I think about solution of problem that does not exist.
I think it would be reasonable to move
charge away from
CreditCard entirely, and have
CreditCard only concern itself with persistence of tokenized credit card information.
The final code could be something like :
class AccountCharger def initialize(account) @credit_card = account.credit_card @plan_price = account.plan_price end def charge Order.create(title: 'something', price: @plan_price) Payment.new(credit_card: @credit_card, price: @plan_price).pay AdministratorNotifier.notify_for_new_payment(@plan_price).deliver #.... end end
Are you agree? There is a lot of dependencies but all dependencies are related to the responsability of
AccountCharger so I think it’s ok. Am I right?
@jferris Don’t we have exercises for this episode? So that we could expand our understanding in a real context.