3

I have two methods

private String handleResponse(HttpResponse httpResponse){
    if (response.getStatusCode() / 100 == 2) {
        return response.getEntity().toString()
    } else {
        throw handleException(response.getStatusCode());
    }
}
private RuntimeException handleException(int errorStatusCode){
    switch(errorStatusCode) {
        case 400:
            return new RuntimeException("Invalid request");
        case 401:
            return new RuntimeException("User not authorized");
        default:
            return new RuntimeException("Unkown exception");
    }
}

Everything works as expected, but is this approach correct? I mean to return new RuntimeException from a switch in a method and then throw the whole method ? Something tells me that it is not and I would like to know why and how can i improve it..

John Kugelman
  • 349,597
  • 67
  • 533
  • 578
MarkyMark
  • 199
  • 1
  • 15
  • 1
    Why don't you make your `handleException` throw the exception immediately instead of returning it? – Anis R. Oct 16 '19 at 21:01
  • 2
    Plus, as your code is currently working and you are just seeking advice, I believe this question is more for the [Code Review](https://codereview.stackexchange.com/) StackExchange forum. – Anis R. Oct 16 '19 at 21:03
  • Then return the message from the switch and throw the exception at the callsight. – Boris the Spider Oct 16 '19 at 21:14
  • @AnisR. I would like to but then Idea is complaining that also else branch should return some statement – MarkyMark Oct 16 '19 at 21:14
  • 3
    `response.getStatusCode() / 100 == 2` Dude, really? Is `response.getStatusCode() == 200` for the weak? – Andrew Tobilko Oct 16 '19 at 21:17
  • @AndrewTobilko Reason of that is API which i'm calling can return as a successful response 200, 201, 204. But i'm open to any suggestions – MarkyMark Oct 16 '19 at 21:27
  • @MarkyMark I mean `201`, `204` are way more descriptive than `1`, `4`. Don't bring unnecessary complexity. – Andrew Tobilko Oct 16 '19 at 21:28
  • You know what's even more readable? Writing a proper, typeseafe, [OO API](https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/http/HttpStatus.html#is2xxSuccessful--). This is Java, not C. – Boris the Spider Oct 17 '19 at 06:48

3 Answers3

1

You can make it a bit shorter and more intuitive in the following way:

private String handleResponse(HttpResponse httpResponse){
    if (response.getStatusCode() == 200) {
        return response.getEntity().toString()
    } else {
        throw new RuntimeException(getErrorMessage(response.getStatusCode());
    }
}

private String getErrorMessage(int errorStatucCode){
    switch(errorStatucCode) {
        case 400:
            return "Invalid request";
        case 401:
            return "User not authorized";
        default:
            return "Unkown exception";
}
Arvind Kumar Avinash
  • 71,965
  • 6
  • 74
  • 110
  • I was also thinking of that option, but what if i once need to create RuntimeException with another constructor using Throwable? For example `throw new RuntimeException(getErrorMessage(response.getStatusCode(), cause);` – MarkyMark Oct 16 '19 at 21:33
1
  1. Get rid of response.getStatusCode() / 100 == 2. Write response.getStatusCode() == 200 or response.getStatusCode() == HttpStatus.SC_OK instead.

  2. Remove the else branch and throw after the if statement.

  3. Rename the method to getExceptionByStatusCode or generateExceptionForStatusCode. You don't handleException, you decide which one to throw.

  4. Choose the right return type. Don't use RuntimeException. It can be ResponseStatusException or any other type appropriate to the HTTP/your domain abstraction.

  5. For each case, decide what type you want to return. Again, not RuntimeException.

A bit improved version would be

private String handleResponse(HttpResponse response) {
    final int statusCode = response.getStatusCode();

    if (statusCode == HttpStatus.SC_OK) {
        return response.getEntity().toString();
    }

    throw getExceptionByStatusCode(statusCode);
}

private MyDomainHTTPException getExceptionByStatusCode(int statusCode) {
    switch (statusCode) {
        case HttpStatus.SC_NOT_FOUND:
            return new MyDomainHTTPException("...");
        case HttpStatus.SC_UNAUTHORIZED:
            return new MyDomainHTTPException("...");
        default:
            return new MyDomainHTTPException("...");
    }
}

That said, returning an exception in Java doesn't feel right.

It works fine but it doesn't seem absolutely correct because of the "special" status of exceptions. It can be justifiable when there is lazy evaluation, and you will be throwing the exception when you meet a certain condition in the future, or in cases with Optional.orElseThrow.

It might be confusing to some people who got used to the throw new template and never thought that returning an exception is a compilable option.

In large frameworks (Spring, PrimeFaces - if my memory serves me correctly), I saw exception factories used to compose exceptions based on the given context and rules. They definitely employ exceptions more extensively than we do. So you may ignore my feeling ;)

Andrew Tobilko
  • 48,120
  • 14
  • 91
  • 142
  • I would `return String` and call the method `getErrorMessageByStatusCode()`. It removes the duplication, allows you to throw on creation and generally makes the code more sane. – Boris the Spider Oct 17 '19 at 06:50
  • @BoristheSpider you may want to return different (more descriptive) types with a different number of params – Andrew Tobilko Oct 17 '19 at 08:14
0

There's nothing wrong with what you have, but you may find it more natural to immediately throw the exception when it's created:

private String handleResponse(HttpResponse httpResponse){
    if (response.getStatusCode() / 100 == 2) {
        return response.getEntity().toString()
    } else {
        throwException(response.getStatusCode());
    }
}

private void throwException(int errorStatusCode){
    switch(errorStatusCode) {
        case 400:
            throw new RuntimeException("Invalid request");
        case 401:
            throw new RuntimeException("User not authorized");
        default:
            throw new RuntimeException("Unkown exception");
    }
}

The API I'm calling can return as a successful response 200, 201, 204. But I'm open to any suggestions.

If you'll accept any 2xx status code, how about:

if (response.getStatusCode() >= 200 && response.getStatusCode() <= 299) {
John Kugelman
  • 349,597
  • 67
  • 533
  • 578