Exceptions and model logic

rubyonrails-talk
1

Hi, When is it correct to raise an exception? I have an order model and in my order controller I have an action to mark the order as shipped. I want to ensure that if the order has been cancelled i.e. its status is "cancelled", that it cannot be shipped.

I was tempted to do this with validations, but I think they are more for data validation rather than this sort of logic.

I thought about doing something like this:

controller:

  def mark_as_shipped     order = Order.find(params[:id])     begin       order.mark_as_shipped       flash[:notice] = "Order has been updated"     rescue OrderCancelledException       flash[:notice] = "Order has been cancelled"     end     redirect_to :action => 'list'   end

model:

  class Order << AR:B     def mark_as_shipped       if self.status == 'cancelled'         raise OrderCancelledException       end       # do shipping stuff       save     end   end

What do you think?

Cheers, Jordan

2

Ouch, this has always been a touch subject for me as well. I've never quite felt comfortable with a philosophy on when to use Exceptions and when to not use them. Perhaps its just one of the many brain blocks I have been blessed with.

Perhaps these 5 points may be of some help:

* Exceptions are expensive to use. They can, in some cases materially, affect the performance of your applicaiton. Unwinding the stack hurts. * Exceptions should be used for just that, exceptions. An exceptional condition is perhaps trying to allocate memory for a new object but it just doesn't happen. That's definitely exceptional. :slight_smile: * Use checks, of a return value or 'global' (ack) indicator, when possible or prudent. I prefer to see a check for success or failure than to rely on too many exceptions. Part of that can be due to my old COM programming days on Windows (don't tell anyone I admitted to that). * Catch the exception/error as soon as possible. React/log the gory details and then push up if necessary with more generalized information. * Relying on exceptions too much can get ugly to read. Personally I find it hard to read code that uses exception catches all over the place. But then again, that's just me.

I could see going both ways in your example. The <mark_as_shipped> method sets an expected change of state to the object. Simple enough, but if it doesn't work the caller may wish to know that's the case - throw an exception. However, if this isn't THAT exceptional perhaps you could consider a return value of the state. The caller can check to see if the state was indeed changed to what was expected. Maybe even just use true/false return value for success or failure. Its possible some callers (perhaps a batch process) may not care. if so, why force a call to wrap it in a rescue block?

So, while I didn't exactly answer the question I hope that gave you some thoughts from someone who doesn't claim to know much about the subject at all. :slight_smile:

3

Ouch, this has always been a touch subject for me as well. I've never quite felt comfortable with a philosophy on when to use Exceptions and when to not use them. Perhaps its just one of the many brain blocks I have been blessed with.

You're not alone :slight_smile:

Perhaps these 5 points may be of some help:

* Exceptions are expensive to use. They can, in some cases materially, affect the performance of your applicaiton. Unwinding the stack hurts.

I don't think this is really a problem for this particular app, as this function is limited to the backend and there won't be much going on at first.

* Exceptions should be used for just that, exceptions. An exceptional condition is perhaps trying to allocate memory for a new object but it just doesn't happen. That's definitely exceptional. :slight_smile:

Yeah, I see where your coming from. I suppose that's what I was really asking. The thing is that the button or whatever that activates this action won't be visible if the order has been cancelled. So, it someone tries to ship it, is that exceptional? I'm on the fence really.

* Use checks, of a return value or 'global' (ack) indicator, when possible or prudent. I prefer to see a check for success or failure than to rely on too many exceptions. Part of that can be due to my old COM programming days on Windows (don't tell anyone I admitted to that). * Catch the exception/error as soon as possible. React/log the gory details and then push up if necessary with more generalized information.

If I simply return true or false, how can I see what caused the record update to fail. For instance, I may want to extend my example to the below. Tell me if this is crazy :wink:

class Order << AR:B    def mark_as_shipped      if self.status == 'cancelled'        raise OrderCancelledException      end      if self.status == 'unpaid'        raise OrderUnpaidException      end      # do shipping stuff      save    end end

When I call the controller method and the update fails, if I just use a return value I won't know from the controller, why it failed.

* Relying on exceptions too much can get ugly to read. Personally I find it hard to read code that uses exception catches all over the place. But then again, that's just me.

I don't think I've written or read enough code to call that yet :slight_smile:

I could see going both ways in your example. The <mark_as_shipped> method sets an expected change of state to the object. Simple enough, but if it doesn't work the caller may wish to know that's the case - throw an exception. However, if this isn't THAT exceptional perhaps you could consider a return value of the state. The caller can check to see if the state was indeed changed to what was expected. Maybe even just use true/false return value for success or failure. Its possible some callers (perhaps a batch process) may not care. if so, why force a call to wrap it in a rescue block?

If someone calls the mark_as_shipped without the order being ready to ship, that should be exceptional as they shouldn't have the ability to do that through the interface. I think.

So, while I didn't exactly answer the question I hope that gave you some thoughts from someone who doesn't claim to know much about the subject at all. :slight_smile:

Oh yes, thanks for your input :slight_smile:

4

Just found this thread which is making me lean towards doing this with an exception.

http://thread.gmane.org/gmane.comp.lang.ruby.rails/80403/

5

That thread suggests using exceptions as some sort of defensive programming paradigm, by basically claiming that if you ignore an error returned the resulting program may be harder to debug than if you ignore an error that is raised. It also suggests that you may be able to carry around more error context by raising an error than by just returning a value.

While on this subject, we should note that rails documentation suggests that one *must* raise an error in order to rollback a transaction.

In C, we can't of course raise exceptions very easily. We can treat SIGSEGV and other types of signals along those lines as exceptions. That philosophy says: sure, go ahead and raise an exception if you really must, but it really is a very rare exceptional condition where your program probably contains a bug and is having a very hard time limping further along.

We note that if routines are willing to sometimes return a flag and sometimes raise an exception, then we will pretty much need to wrap every routine in logic to catch both a raised exception and to handle a return code.

We also note that if we are going to not raise an exception, then we need to build our own error handling paradigm. Typically we would pass either a structure or a buffer down through our call stack so that when an error occurs, it can record the __FILE__ and __LINE__ where the error occured and save any parameters that may be of interest. Higher layers on the stack may save additional data. And this would get logged at an error or warning or info level.

Also, with errors, we kind of want a nice succinct well defined error code to be generated so that when we are close to the HTML we can decide what language we want to render the error in.

So, rails is supposed to be handing us all the important paradigms on a platter. But it isn't giving us a canned packaged error handling paradigm. And it isn't making the language translation issues any easier.