One thing that's sort of missing from either version of your code is state checking -- is this is a valid state transition? (Am I actually able to become "ACCEPTED" -- what if I already am?)

Which is actually a slightly annoying thing to do using Rails if you're just using state= type methods and after_update -- you'll need to add on some code to check against the previous value. Or put the check in state=, but if you reject it there, then perhaps all the other assignments you've done are invalid. (You'd have to throw an exception and catch it.)

What this does is actually increase coupling between your model and your view (or at least your controller) -- what if I don't want to maintain state using an attribute, or I want to maintain state using two attributes (or something like that).

I agree on the emailing thing though, that needs to be done after a successful save.

Presumably in your second code block, you mean

def after_update :create_mutual_friendship

and not the function you've defined there?

Presumably in your second code block, you mean

def after_update :create_mutual_friendship

and not the function you've defined there?

Nick, good points. I like the idea of not having to worry about model state and only allowing CUDly modifications. I've found a few times where that is the only way I could safely update the model.

You could also save yourself one line in your model by replacing:

<code>friendship_request.attributes = params[:friendship_request]
friendship_request.save
</code>

with:

<code>friendship_request.update_attributes params[:friendship_request]
</code>

Lastly, I've never been a fan of sending emails from the model. I think they fit better in observers.

I agree with brandon on the the email thing. The only problem I see with this is you basically have to create a lot of virtual attributes. For instance with user activation. Right now I have a singleton method find and activate. It'd be nice to mask that with an update, but then I'd have to pass additional params to first find the user and then create a virtual attributes that would store the activation code and then check that against the current user. I like it in most cases, but it has its flaws.

I've got to say that I'm not convinced by the email example, either.

If you have a method that sends an email whenever an attribute is assigned to, and that's actually not what you want to do, then you've got a bug. Making methods that do the wrong thing, and then making them public is always going to lead to trouble.

However, if that's in fact the correct behavior (highly doubtful), then sending the email from the setter is the simplest thing that could possibly work. Making the same thing happen in #update is going to be more complex.

In fact, restricting our public writable interface to three methods means the probability that those methods will be more complicated than the several methods they replace. What if there's more than one way to update (or add or delete)? Do you pass another parameter to indicate this? (no different from having multiple methods) Or does the model figure it out?

The other thing, that others have mentioned, is that email is a presentation feature, and should be avoided in the model, if possible.

I found your entry stimulating and thought-provoking (else I wouldn't be commenting:).

///ark

I can't get behind the callbacks. If I'm creating Friendships in an after_update, I'll be creating two Friendships every time I call save, update_attributes, and even update_attribute (which a lot of people call to bypass validations, but it still calls callbacks). If I don't want that to happen, I'd need a host of other attributes to make sure that code in the callback is only executed when I want, which adds bloat and confusion.

Why have the callback if it will only be used once? You're accepting the invitation and establishing a Friendship. This is not generic code! Put the code where it needs to go. Don't spread the code over the whole file when it makes sense to keep it in the same method. Context, consistency, and clarity are more important than feeding a skinny method fetish.

I was playing around with this. I think the best way to implement it is to use virtual attributes as flags and then use callbacks or overrides to delegate behavior to methods depending on which flags are set. This keeps logic out of the callbacks and keeps everything neat, clean, and readable.

I'm really stoked to have provoked some controversy with these ideas. Thanks for the great feedback guys. To address a couple criticisms...

1. Jon Yurek & Simon: both complained that we need to ensure the side-effects occur only when the state transitions from PENDING to ACCEPTED--we don't want it happening twice. With the ad hoc method, #accept!, this can be done with a guard clause, but this means the caller needs to dispatch on return value and display an error message in an ad hoc way (i.e., not using ActiveRecord Errors and so forth). It is better to make it impossible to accept a FriendshipRequest twice by ensuring #accept! is not a public method! It's actually quite easy in ActiveRecord to model a state machine. There are two options.

a) Look up attribute_method_suffix. We can do something like:

<code>  attribute_method_suffix '_changed?'
  attribute_method_suffix '_old'
</code>

and therefore we have methods like state_changed? and state_old. We can write code like:

<code>if state_old == PENDING and state_new == ACCEPTED ...
</code>

A disadvantage of this approach is that it isn't guaranteed to be 100% correct (the old value really needs to be read from the database during the save transaction).

b) Implement your own acts_as_state_machine (I've been working on a CUDly version of this for the last week or so). Then we can say:

<code>class FriendshipRequest
  on_transition_from(REJECTED).to(ACCEPTED) :create_mutual_friendships
end
</code>

We also want to be able to express rules saying that it is valid to go from A to B but not B to A, etc. It's surprising how often we want to model a state-machine, and ActiveRecord makes it really easy to do something like that. This is precisely why it's more natural to have something like after_update rather than a custom method, we're really modeling a state-machine.

1. Mark said, What if there's more than one way to update (or add or delete)? Do you pass another parameter to indicate this? (no different from having multiple methods) Or does the model figure it out?

If you're modeling the triggering of these side-effects using a virtual attribute like:

<code>attr_accessor :should_send_email
</code>

Then this is ridiculous. But if, on the other hand, you are modeling something like: if a user registers with an invitation token, don't send an email address verification request; or if an order is created with a coupon, don't bill the credit card--this sort of modeling is best done with a virtual attribute, not a custom method.

It's surprising how often the best way to model these side-effects is as a state machine or as a virtual attribute. It's much more common than people realize; if you're looking for the pattern though, you'll see it.

In my experience if you're looking for opportunities to be CUDly, your code will be much more concise, consistent, and well-encapsulated. You will recognize the patterns (state machines, virtual attributes, etc.) that are the best solution to the problem--not an awkward one. CUDliness might seem radical and unnatural at first, but give it a try. It's a but like REST in that way; at first we think it's fitting a square peg into a round hole and then we find it provides beauty and order where there was chaos.

Modeling it with an actsas_statemachine thing is a good idea; I've been working on something similar.

You make a good point about the error messages, and you do say "this is how ActiveRecord is meant to be used". You may be right, but that does actually highlight an issue with Rails. You can only have one save method, and one set of validations (unless you want to get happy with :if clauses).

I still think the coupling between the model and controller/view is too high; there also isn't a clear interface -- what attributes do I need to set to change from one state to another? If there were a method, you could document exactly what's needed. (I suppose you could document what's needed either way, actually, but it still seems a little dirty).

So, it's fine if you're just changing one attribute (state), but if it's more complex than that (state changes have parameters), it gets a bit dirty.

Simon -- I disagree about the coupling. See my examples of Formulaic controllers above. The Controller NEEDS to be ignorant of anything going on in the model, all it can do is call attributes= and save; it shouldn't pass in attributes beyond what's in the http request.

Interestingly, the view is coupled to model... But there's no way around this, regardless of whether you're CUDly. The invocation of the method has to be triggered from a link_to or a form post; thus, the view has to trigger the different side effects either with query strings, form bodies, or hitting a different url altogether. The great thing about CUDly models is that the Controller layer withers away altogether, and you can expose your models almost as if they were Resources directly.

wrt/ to Validations, ActiveRecord is fairly powerful. Yes, there's :if, but there are also custom validations:

<code>validate_on_update :something_something
def something_something end

validate :something_else
def something_else
  errors.add(:virtual_attribute, "This state txn is invalid") if xxx
end

validate_on_create :blah
def blah end
</code>

The only thing lacking, IMO, is the ability to validate a destroy. The only way to halt a destroy is:

<code>before_destroy :halt
def halt
  false
end
</code>

You cannot safely provide error messages, since no validation phase happened.

Regarding this...

<code>before_destroy :halt
  def halt
    false
  end
</code>

How come the equivalent methods in an observer don't behave the same? For example...

<code>class UserObserver &lt; ActiveRecord::Observer

  def before_destroy(user)
    return false if user.director?
  end

end
</code>

Still lets the user be destroyed. Is there documentation on these discrepancies?

Nick,

Right on. Here's a simplified example extracted from an app I wrote:

<code>class Order &lt; ActiveRecord::Base
  before_update :update_status
  has_many :order_lines

  def canceled?
    status == "CANCELED"
  end

  def closed?
    status == "CLOSED"
  end

  def opened?
    status == "OPENED"
  end

  def original_status
    new_record? ? nil : Order.find(id).status
  end

  def status_changed?
    status != original_status
  end

  private

  def decrease_quantity_in_stock
    order_lines.each { |line| line.decrease_quantity_in_stock }
  end

  def increase_quantity_in_stock
    order_lines.each { |line| line.increase_quantity_in_stock }
  end

  def update_status
    if status_changed?
      decrease_quantity_in_stock if opened?
      increase_quantity_in_stock if canceled?
    end
  end

end
</code>

Nick

I find this approach really very interesting indeed, I currently use acts_as_state_machine and really didn't like the idea of adding extra actions to my controllers for every event that I defined, or on the other hand adding logic to the update action to manage these events.

I was just wondering if you got anywhere with your CUDly version of aasm, I would be very interested to see how you have tackled this if you would be happy to share it.

Thanks for an insightful article.

Joe

These ideas really sit well with me.

Regarding Brandon's example, you can now get the _changed? functions for free with Rails 2.1 (aka "dirty attributes", see Railscast #109: http://railscasts.com/episodes/109)