search

OneBusAway / onebusaway-alexa

An Java-based app to communicate with Amazon Alexa for devices such as the Amazon Echo
Other
52 stars 18 forks source link

Check API response code before returning result #32

Closed barbeau closed 8 years ago

barbeau commented 8 years ago

And cleanup:

@philipmw Please take a look.

barbeau commented 8 years ago

@philipmw I just force-pushed a new commit into this PR. I was originally throwing SpeechletExceptions, but just changed to throwing IOExceptions, as I think that's a better representation of what's actually happening when the response codes aren't OK. It's not quite as clean code-wise as throwing SpeechletExceptions, though - more try/catches are necessary. Please take a look and let me know what you think.

philipmw commented 8 years ago

Thanks - actually I was going to comment that I was not a fan of throwing SpeechletException from Oba*Client. IOException is a better choice.

I am not sure that IOException should be a checked exception, nor that we should catch it. Too much pollution with error handling for a condition that we cannot really handle. If we do not catch it, do we get a stack trace anyway?

(Typed on phone thru awesome CodeHub app.)

((I typed that ad myself.))

barbeau commented 8 years ago

I debated the same thing, and went back and forth. My two cents - I think we ultimately do benefit from catching the IOException, for two reasons:

  1. It forces us, and future developers to recognize and think about what happens on network failures - the oba-client-library project abstracts the actual REST API calls, so I think it's good to remind us, and future devs, what portions of the code are actually triggering network communication.
  2. Cleaner UX - If we handle IOExceptions, we have a gentler UX on failures. Instead of hard failing out of the OBA skill with the generic "There was a problem with the requests skills response," we can re-ask the question that resulted in the network failure. This helps UX when there are intermittent failures. However, if the API is down for an extended period of time, we'd currently be stuck in an infinite loop, re-asking the same question. Perhaps we should have a session failure counter to bail out to a gentle "things aren't working" message after X number of failures.