Check Response Code before using conn.getInputStream

[mattinger]

If the response code is a non 200, use conn.getErrorStream instead.
This will allow things like token refresh errors to be handled correctly.
Currently, you get an AuthorizationException with a FileNotFoundException
as it's cause due to the call to conn.getInputStream.

---

This change is 

[codecov-io]

Current coverage is 79.23% (diff: 100%)

Merging #144 into master will decrease coverage by 2.98%

@@             master       #144   diff @@
==========================================
  Files            41         41          
  Lines          2121       2124     +3   
  Methods           0          0          
  Messages          0          0          
  Branches        258        260     +2   
==========================================
- Hits           1744       1683    -61   
+ Misses          377        362    -15   
- Partials          0         79    +79   

Powered by Codecov. Last update 2a10fb5...e8c6a32

[mfeltmann]

I'd prefer the solution to read the conn.getInputStream() and if it throws read the conn.getErrorStream()

What is the benefit of your status-code checking approach?

[mattinger]

Not a fan of using exceptions as a means of flow control

[mfeltmann]

Me neither, since I used to develop for iOS. But Java itself IS an exceptional programming language.

[mattinger]

Plus, performance wise, it's much faster to do a response code check than to wait for an exception to be thrown. Without getting into all the details, you essentially have to create and unwind new stack elements to do it. Avoiding the exception in the first place, IMHO, is usually the best approach.

[mfeltmann]

I guess we have a very different opinion in this topic.
According to my information based on a lot of Java code I've read and the official Java tutorial
https://docs.oracle.com/javase/tutorial/essential/exceptions/
an exception is THE solution for handling a disruption in a common instruction flow.
According to
https://docs.oracle.com/javase/tutorial/essential/exceptions/advantages.html
one is advised to rely on the appearance of exceptions instead of check everything by oneself.

I really neither like nor appreciate this exception stuff, but the whole language is based on this.
The redesign of HttpUrlConnection in API 19 to throw an exception when a status code >= 400 occurs serves Javas exception pattern as well.

If you prefer to completely rely on status checks then another pull request may fit your needs.

[mattinger]

To me, this is not disruption to the common flow. The fact is that the spec says if you have an error response (ie, a 400 Bad Request), then you should use getErrorStream instead. This is EXACTLY what my code implements. For example, which of these code blocks makes more sense to you:

  StringBuilder b = new StringBuilder();
  if (x != null) {
   b.append(x.toString());
  } else {
   b.append("null");

StringBuilder b = new StringBuilder();
try {
  b.append(x.toString());
} catch (NullPointerException e) {
  b.append("null");
}  

To me it's the first.

[mattinger]

In any case, whether it's my PR or your PR is really up to the project maintainers. But it's blocking my development :( hopefully @iainmcgin can get to this soon.

[mfeltmann]

Guess that's the main issue. To me it is the latter. (in Java)
Yes, @iainmcgin has to decide. Since both approaches seem to work it will be for sure a benefit for the enduser. :)

[iainmcgin]

Apologies, I've been out for a few weeks. I'll evaluate the PRs and include a fix for this for 0.5.0.

[iainmcgin]

---

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[iainmcgin]

Generally speaking, I agree with @mattinger - the API of HttpURLConnection provides a way to predict whether getInputStream() would throw an exception or not, so it is better to use this mechanism (check the response code).

If @mfeltmann's counter-argument were correct, then perhaps one would think the following code is acceptable:

Iterator<String> strIter = strings.iterator();
try {
  while(true) {
    System.out.println(strIter.next());
  }
} catch (NoSuchElementException ex) {
  System.out.println("END OF LIST");
}

Rather than using hasNext() as the loop condition.