Single class which holds response and error message

Question

I am working on a library which will make HTTP call to my rest service basis on inputs passed to this library. And whatever response comes back from service whether it is successful response or failure coming from the service or something happened in this library like timeout or any other errors, I am returning a response object back to the customer which customer can look into and iterate the object accordingly.

My response object is like this as of now and this is what I am returning back to the customer and customer will use this object and iterate it to figure out whether it is successful response or some failure and use it accordingly.

public class DataResponse {
 // response data
 private final String response;
 private final String date;
 private final String confidence;
 private final String mask;
 // what are the errors?
 private final ErrorCode error;
 // whether it is successful response or not
 private final StatusCode status;
 public DataResponse(String response, String date, String confidence, String mask, ErrorCode error, StatusCode status) {
 this.response = response;
 this.date = date;
 this.confidence = confidence;
 this.mask = mask;
 this.error = error;
 this.status = status;
 }
 // getters here
}

And below is my ErrorCode class which contains all the errors happened at client level or service level so that the customer can know what happened.

public enum ErrorCode {
 OK(200, "NONE", "Response is success."),
 NO_CONTENT(204, "No Content", "Response is success."),
 CLIENT_TIMEOUT(1007, "Timeout", "Timeout has occured on the Client.");
 // some more error messdates, keeping it short to show the idea
 private final int code;
 private final String status;
 private final String description;
 private ErrorCode(int code, String status, String description) {
 this.code = code;
 this.status = status;
 this.description = description;
 }
 // getters here
 @Override
 public String toString() {
 return code + " : " + status;
 }
}

And below is StatusCode class basis on which customer will decide whether it is successful response or not.

public enum StatusCode {
 SUCCESS, ERROR;
}

Customers of this library - they will use this DataResponse object in two ways:

• Firstly, they can serialize DataResponse object to JSON as per their needs if they want to do it but it's up to them.
• Secondly they will use this DataResponse object as it is and just iterate it normally.

So wanted to make sure from both the perspectives if the current design is good? If not what can be the improvements.

Please review my code. If you have any questions, please ask. If you have any comments, please give them.

Answer 1

Is that error, or status, or both, or neither?

This API is a bit confusing. You have ErrorCode and StatusCode enums. But ErrorCode contains things like:

OK(200, "NONE", "Response is success."),
NO_CONTENT(204, "No Content", "Response is success."),
CLIENT_TIMEOUT(1007, "Timeout", "Timeout has occured on the Client.");

So two of the "error codes" indicate success.

public enum StatusCode {
 SUCCESS, ERROR;
}

... And StatusCode can indicate success or error.

... The confusing nature of these elements make it difficult to understand the API, and the distinction of its building blocks.

Your error codes resemble HTTP status codes. Why not go a bit further in following HTTP practices, and have just one Status enum, that contains status codes like in the HTTP protocol, some of which are success, and others are error.

DataResponse

I like that the class is immutable. But I'm concerned about the long parameter list of the constructor, especially considering that the first 4 values are all of String type, which can lead to errors such as mistaken ordering.

I would suggest to consider adding a builder, that will have well-named setters to set the parameter values, possibly allowing some sensible defaults.

Lastly, the type of the date field is a String. It would be good to add a JavaDoc about the required format. (For that matter, a JavaDoc for the other String parameters would be good too.)

• david: So my ErrorCode has a number and small message and long message by which we can figure out what is the actual error when we are logging. These are just some useful information for us to debug the issue. And StatusCode is just to tell whether it is successfull or not. We are following HTTP practices but there are some errors which we want to differentiate so that's why we gave our own code number for those.
• janos: I think it will make more sense if you separate the HTTP status code you will return, and the application errors that are specific to your domain. Don't mix the two

Answer 2

This crazy long line:

public DataResponse(String response, String date, String confidence, String mask, ErrorCode error, StatusCode status) {

Should be split into two, as 80 characters is the standard line limit:

public DataResponse(String response, String date, String confidence,
 String mask, ErrorCode error, StatusCode status) {

Aesthetics-related note:

Would you rather see this:

200 : NONE

(looks like part of a ternary operator to me)

Or this:

200: NONE

I would go for the second one, which is the one your toString() method is not doing:

@Override
public String toString() {
 return code + ": " + status;
}

Here:

public enum StatusCode {
 SUCCESS, ERROR;
}

I kind of expected SUCCESS and FAILURE, because they are antonyms... but I don't know exactly what you want to do with the StatusCode, so I can't say much.