Exceptions and model logic

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

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:

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:

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/

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.