Bad examples in README

[Mange]

So this was extracted from Rails since most people used observers wrong. Don't you think that examples that uses them in a bad way might invite people to make those mistakes again and again?

Observers should work with persistence, with concerns that shouldn't really be directly in the model. Here's some examples:

class PostObserver < ActiveRecord::Observer
  def after_destroy(post)
    destroy_unused_tags(post.tags)
  end

  private
  def destroy_unused_tags(changed_tags)
    changed_tags.each do |tag|
      tag.destroy if tag.taggings.empty?
    end
  end
end

class PageViewingObserver < ActiveModel::Observer
  def after_create(page_viewing)
    # Assuming the following models work by caching counters and then incrementing them (speed by denormalization, MongoDB, etc.)
    VisitorStatistics.increase_monthly_stats(page_viewing.visitor)
    PageStatistics.increase_monthly_stats(page_viewing.page)
  end
end

Sure, these might still not be optimal, but they are better than an example that sends an email from the persistence layer, every time a model is persisted. Thoughts?

[rafaelfranca]

I'm fine with the examples.

It was extracted from Rails because it is something that we don't use in our applications and because we don't want to encourage it as best practice, not because people use it wrong.

Either in your example I would not encourage the usage. It hides logic adding an another level of indirection.

[senny]

I would agree that sending an email from the observer is not the best use-case but as @rafaelfranca stated the example with the tags is not really a good abstraction either.

Let's look for a better example to put into the README.

[rafaelfranca]

I don't have any better idea, but we are open to suggestions.

[Mange]

It was extracted from Rails because it is something that we don't use in our applications and because we don't want to encourage it as best practice, not because people use it wrong.

My mistake, then. Most blog posts talking about this say things along the lines of "they could be useful, but most people just put stuff in there that really shouldn't belong in them". I assumes that was one of the reasons. :-)

Either in your example I would not encourage the usage. It hides logic adding an another level of indirection.

Well, you either choose indirection or coupling. That's a tradeoff one must pick. It's like Law of Demeter vs. duplication. I think most persistence models should not know about other models and just be focused on the object they are about the persist. Observers are a nice layer in-between that can know about the "hacks" between the persisted models and keep them in sync on different changes.

I would agree that sending an email from the observer is not the best use-case but as @rafaelfranca stated the example with the tags is not really a good abstraction either.

I agree with that too. Tags may not be the best example, but it's miles better than the email sending. That's just wrong on so many levels. The main problem with it is that in real-life, the code inside the observer would just be a delegation to something else, like Tag.prune_unused_tags or something, but that requires the delegate to be documented in the example as well. It's also bad since it's business logic (in most cases; tags could be something automatically created by the system, not something that is even visible to the users) and business logic should not be in there.

I personally like the second example I presented the most since it's the most common, and most usable case for observers - denormalization. When using ORMs for databases that does not work with relational data, that is something that is present in pretty much all models and the coupling is the reverse (e.g. saving a Tag modifies all Posts with that Tag, so it should not be in an after_save in Tag since Tag then needs to know that Post embeds them. Post can know about this, and so could some other object with the sole purpose of keeping this in sync.)

Here's another one:

class ThreadObserver < ActiveRecord::Observer
  def after_save(thread)
    User.update_watched_threads(thread)
  end

  def after_destroy(thread)
    User.remove_watched_threads(thread)
  end
end

class User
  def self.update_watched_threads(thread)
    denormalized_attributes = User::DenormalizedThreadSerializer.new(thread).attributes
    watching_thread(thread).each do |user|
      user.watched_threads.find(thread.id).update_attributes(denormalized_attributes)
    end
  end

  def self.remove_watched_threads(thread)
    watching_thread(thread).each do |user|
      user.watched_threads.pull(thread.id)
    end
  end
end

(Breaking LoD to keep example short. Not the best code; I would structure it to use atomic operations rather than iterating users.)

This example makes sure that if a thread has its name (or whatever, this code doesn't know) changed, the denormalized documents in User updates to reflect this change. Thread is an ActiveRecord::Base in this example, while User uses some other ORM.

It's not proper for Thread to know that User denormalizes parts of it in watched_threads, so no callback in there can know about it. It's also an artifact of how the objects are persisted, so the answer is not to wrap everything in services. User knows about the denormalization, but doesn't know when Thread is saved. Associations cannot be used either (and are mostly bad ideas in complex cases like this).

The solution is to have a layer between the models that solve this problem. They lie in the persistence part of the codebase, and their only responsibility is to make sure that the persistence models keep their contracts intact.

[Xuhao]

For me, after a bad experience on this gem, I give up to try to use it.

Thanks for the bad example and release without test.

If you think it not good for use, just delete it. If you put it here and release on rubygems.org, you should make sure it can be work.

Otherwise it will make people waste lots of time on it.

[rafaelfranca]

Sorry, but I'm using this gem in a project and it work fine.

Before the release we also tested the gem either with the unit tests included in the gem and in a real application.

If it is not working for you, you could be more constructive and tell us what is wrong instead of taking your time to attack people that is trying to do the best release some software, totally free, to the community. On Sep 21, 2013 11:53 PM, "Xuhao" [email protected] wrote:

For me, after a bad experience on this gem, I give up to try to use it.

Thanks for the bad example and release without test.

If you think it not good for use, just delete it. If you put it here and release on rubygems.org, you should make sure it can be work.

Otherwise it will make people waste lots of time on it.

— Reply to this email directly or view it on GitHubhttps://github.com/rails/rails-observers/issues/2#issuecomment-24874992 .

[Xuhao]

Sorry I did not test it on a new rails4 app.

I just use it in my project which upgraded to rails4 form rails3.2. maybe it works in a new rails4 project. Sorry about that.

Can you have a look at #4 , I don't think it was been fixed.

[rafaelfranca]

Thank you for point the issue. I'll work to fix it.

[Xuhao]

That's cool, it's the best problem for me. Thanks a lot!