All threads / Law of Demeter - Question

Ask A Question

Notifications

You’re not receiving notifications from this thread.

Law of Demeter - Question

David McDonald asked in Rails

Had a question about how to refactor some of my code. I've been reading some books AntiPatterns & Refactoring. Given the code block here, should the method here be in Evaluation or Skill? If I'm following the "rule" in AntiPatterns for using only one dot, and reports is knowing too much about skills and evaluations... how would this look like if I wanted to retrieve the accuracy evaluation from my report object?

class Report < ApplicationRecord
  has_many :evaluations
  def accuracy
    evaluations.joins(:skill).where('skills.code = (?)', 'ACC').try(:first)
  end
end
class Evaluation < ApplicationRecord
  belongs_to :report
  belongs_to :skill
end
class Skill < ApplicationRecord
  has_many :evaluations
end

I've never really agreed with the single method call (dot) advice, personally.

One thing, you don't need the .try because where will always return an ActiveRecord::Relation object which will always respond to first. It may or may not return a record, so anything you call after that would need to check if accuracy was nil.

A little cleaner version is to use find_by

# Returns an Evaluation
def accuracy
  evaluations.joins(:skill).find_by(skills: { code: 'ACC' })
end

Thanks! I appreciate that note - it is cleaner. As I'm tryng to apply it to the single method call I've come up with this, but it's currently not giving me the same result as the older code.

class Report < ApplicationRecord
  has_many :evaluations

  def accuracy
    evaluations.accuracy
  end
end
class Evaluation < ApplicationRecord
  belongs_to :report
  belongs_to :skill

 delegate :accuracy, to: :skill

end
class Skill < ApplicationRecord
  has_many :evaluations

 def self.accuracy
   find_by(code: "ACC")
 end

end

Yeah, that is not equivalent there because you're not filtering the evalulations, only the skill. You need to have it in a single query.

DHH doesn't like the Law of Demeter, so I wouldn't worry too much about trying to follow it. He's written about it a few times: https://signalvnoise.com/posts/3341-pattern-vision

Thanks that's very helpful I'll read it. Perhaps I'll go with the follow that seems to be inbetween the two?

I'm going to think on your version and this one a little more. Try and work out the pros/cons to placing them in both of their places. Thanks again!

class Report < ApplicationRecord
  has_many :evaluations

  def accuracy
    evaluations.accuracy
  end
end
class Evaluation < ApplicationRecord
  belongs_to :report
  belongs_to :skill

  def self.accuracy
    joins(:skill).find_by(skills: { code: "ACC" })
  end
end
class Skill < ApplicationRecord
  has_many :evaluations
end
Join the discussion

Want to stay up-to-date with Ruby on Rails?

Join 53,406+ developers who get early access to new tutorials, screencasts, articles, and more.

    We care about the protection of your data. Read our Privacy Policy.

    logo Created with Sketch.

    Ruby on Rails tutorials, guides, and screencasts for web developers learning Ruby, Rails, Javascript, Turbolinks, Stimulus.js, Vue.js, and more. Icons by Icons8

    © 2021 GoRails, LLC. All rights reserved.