Law of Demeter

Upcase
1
The Law of Demeter.
This is a companion discussion topic for the original entry at https://thoughtbot.com/upcase/videos/law-of-demeter
2

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
3

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?

1 Like
4

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.

5

If we check the definition :

A method of an object should invoke only the methods of the following kinds of objects:

  1. itself
  2. its parameters
  3. any objects it creates/instantiates
  4. 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.

6

Iā€™ll print out and pin this guidelines list on the wall in front of my desk! :smiley:

7

@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 credit_card and plan_price.
  • In order to charge, you need an object that responds to charge and valid?, as well as a @plan_price.

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.

8

@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.

9

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.

10

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?

11

@jferris Donā€™t we have exercises for this episode? So that we could expand our understanding in a real context.

12

@samnang we havenā€™t created an exercise for Law of Demeter yet, but weā€™re adding new exercises all the time. Law of Demeter may be an upcoming topic.

1 Like