Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OOD improvement #17

Open
ghost opened this issue Apr 21, 2016 · 1 comment
Open

OOD improvement #17

ghost opened this issue Apr 21, 2016 · 1 comment

Comments

@ghost
Copy link

ghost commented Apr 21, 2016

I like this book. Thank you for writing it. I am reading through it and came across this line:

    def reset_authorships!
      model.authorships.each { |authorship| authorship.update_attribute(:confirmed, 0) }
    end

and I thought "isn't this doing too much?" It seems to be messing around with an association's AR internals, reaching 2 levels deep. Would it not be better to have a method on the authorship form object which we write, something like

def reset_and_sync!
  confirmed = 0
  sync
end

then the code in Thing could read

    def reset_authorships!
      authorships.each(&:reset_and_sync!)
    end

Just a thought. That way the thing doesn't need to know the AR internals of an associated form object.

@apotonick
Copy link
Owner

That's actually a cool idea! I didn't like this code myself, but saw it as a chance to show some more internals to users. However, you're right, it might mislead them to messing around with AR internals...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant