Return response headers for successful calls against the API #15
Conversation
akrantz
left a comment
There was a problem hiding this comment.
Since this was my first code review, I felt compelled to add some feedback. The overall changes are fine. Let me know whether you want to make the suggested changes, but if you don't I would be fine with it.
| } | ||
|
|
||
| @Override | ||
| public <T> T deserializeObject(final String inputString, final Class<T> clazz, Map<String, java.util.List<String>> responseHeaders) { |
There was a problem hiding this comment.
I believe that you could provide only one overload for the deserializeObject function by making the responseHeaders parameter accept the default value of null. I would favor using default parameters to minimize the overloads, and having separate overloads only when necessary. You would also need to update the interface definition to match, but I believe that would be an OK change to make.
There was a problem hiding this comment.
Since Java doesn't support default parameters, the two simplest ideas I had were to use overloading or check for a null responseHeaders list. Since the latter requires changing the method signature across the library, I figured the former was a bit cleaner.
There was a problem hiding this comment.
OK, I mistakenly thought Java had added default parameters.
|
|
||
| JsonObject res = new GsonBuilder().create().fromJson(jsonString.toString(), JsonObject.class); | ||
| return res.get("access_token").toString(); | ||
| return res.get("access_token").toString().replaceAll("\"", ""); |
There was a problem hiding this comment.
It would be helpful to add a comment that quotes are being removed.
| try { | ||
| list.add(String.format("%d", connection.getResponseCode())); | ||
| } catch (IOException e) { | ||
| throw new IllegalArgumentException("Invalid connection response code: ", e); |
There was a problem hiding this comment.
Need to concatenate the error response code.
There was a problem hiding this comment.
By definition of this error, it can't be done (connection.getResponseCode() itself needs to be wrapped in a catch). However, the JavaDoc explains that an IllegalArgumentException is thrown when a connection cannot be made, so I added this to the error.
This will be added under the "graphResponseHeaders" node in the additionalDataManager: