Is checking whether a DataMapper (or other ORM) model is persisted a code smell?

Question

I've found myself starting to leverage checking persistence to get my models to 'work'. It seems convenient and correct to include a persistence check. On the other hand, it feels a little shady, as if I was being overly cautious or breaking the ORM abstraction in a small way.

An example might be:

class Shipment
  include DataMapper:Resource

  belongs_to :address, :required => false

  def shippable?
    valid? && persisted? && !address.nil? && address.valid? && address.persisted?
  end
end

In this case, I need to have a method telling me if a shipment is shippable. This is true when its valid, saved to the db, and has an address saved.

Another example might be using it in callbacks to determine whether certain things (price recalculation) need to happen.

Opinions? Is it unnecessary, paranoid or safe, correct?

Answer 1

Well, if it is really possible that you come to the point of performing the shipment without a confirmed persistance or a reload from the database, checking persistance seems absolutely necessary to me.

But is this really the case? How do you get to this shipping action? I would think of two workflows here:

1. There is an order which you want to ship. You click on "ship this shit!" and get to ShipmentsController#new, enter your data. By pressing "save" your shipment gets validated and persisted if valid. After the persisting is successful (I assume shipment.save returns false if not) you can directly move on with your shipping action (whatever needs to be done there). If shipment.save tells you it did not persist, the #new view gets rendered another time and shipping is not performed yet. So this is the idea of wizard-like workflow with a "gate" that only lets you perform the shipping if persistance has been successful.

2. The creation of shipments and shipment of shipments is completely decoupled. Lets say one guy plans shipments and another one performs them. The former creates a new, valid, persisted shipment. The latter begins with a list of readily planned shipments. This ShipmentsController#index loads the shipments directly from the database using Shipment.all. No need to check persisted? then in the ShipmentsController#perform action.

Ok. let's make it more complicated: Assume now we are in scenario 2 and the first guy could delete the shipment while the second one performs it. Without regarding this case, the naive application would have saved the shipment again after #perform has finished, setting the date of shipment. This would mean that the lost "delete" database anomaly has happened. However, won't you use a state machine anyway? So lets say there are the transistions planned -> deleted and planned -> shipped. If guy #1 sets the status to deleted while guy #2 is performing the shipment, the state machine would throw an exception: "No transition from deleted to shpped, my dear!" when trying to update the status.

So I can't really make up a scenario where checking persistence explicitly is not paranoid. But if you have one where it could really occur due to a 'unsafe' workflow, you should check for persistance.