0
\$\begingroup\$

I have a method which is part of a request-handling class. It creates a URL, depending on the resource send request.

This is a part of the REST service:

private static URL addQueryToUrl(String qry) {
    try {
        if ("playerProfile".equals(requestingService)) {
            return new URL(PLAYER_URL + qry);
        } else if ("search".equals(requestingService)) {
            return new URL(SEARCH_URL   + qry);
        } else {
            throw new Exception();
        }
    } catch (MalformedURLException e) {
        System.out.println("wrong url format");
    } catch (Exception e) {
        System.out.println("no such a requestingService");
    }
    return null;
}

I have 2 questions:

  1. What exception should I throw if requestingService is not recognized? For now, my method is throwing Exception. Or should I handle such an exception on a higher level, when requestingService has an assigned value?

  2. If no URL is built, what value should I return? NULL? I'm now reading Clean Code by Robert C. Martin and he is stating that returning null is bad practise.

\$\endgroup\$

2 Answers 2

Reset to default
4
\$\begingroup\$

If we are speaking in terms of Clean Code, the entire method should be revised.

Here are the main points that I can criticize:

  1. if ... else if ... branches are used to match Strings: this approach is too verbose and not very flexible.

  2. URL is instantiated in two different places, with same semantics.

  3. The Exception thrown in else block is immediately caught and thus is useless.

  4. The catch blocks hide exception details and trace, which is annoying for debugging.

  5. Returning null is ugly. A better choice would be either to return an Optional or throw the exception outside the method.

This method does two distinct things:

1) Determine which URL to use depending on the requestingService value.

2) Instantiate a URL object with query suffix.

Each of these things should be wrapped in a dedicated method.

So let's first create the getServiceUrl method:

private static String getServiceUrl() {
  switch (requestingService) {
    case "playerProfile": return PLAYER_URL;
    case "search": return SEARCH_URL;
    default: throw new IllegalStateException("Unexpected requestingService value=" + requestingService);
  }
}

Here I suppose that requestingService member of this class can take only a predefined set of values (and even should be transformed into an enum!) and any value that is not in this set denotes an internal problem that puts the application into an invalid state.

BTW, since the original method is static, I suppose that requestingService is also a static member and therefore should be renamed according to style conventions. And I also suspect that there is some misuse of statics, but can't say more about it without having the entire code.

Second, we rewrite the original method:

private static URL addQueryToUrl(String qry) {
  try {
    return new URL(getServiceUrl() + qry);
  } catch (MalformedURLException e) {
    throw new IllegalStateException(e);
  }
}

Since PLAYER_URL and SEARCH_URL are pre-validated constants, I suppose that they cannot represent a cause for a MalformedURLException. But from the original code, it is not clear whether qry is validated as part of URL pattern or not. If so, an IllegalStateException would be appropriate here. Otherwise the catch block should log the exception message and return an Optional.empty() instead (also change the return type to Optional<URL> and the return statement respectively).

I'd also suggest to rename this method to buildUrlWithQuery.

\$\endgroup\$
0
\$\begingroup\$

What exception should I throw if requestingService is not recognized? For now, my method is throwing Exception. Or should I handle such an exception on a higher level, when requestingService has an assigned value?

I would suggest throwing an exception when requestingService is not set. I'd probably make a new Exception class to handle it, or just have the set method return false if it fails to set the value. Given you haven't shown where/how it is set, I can't offer more assistance than that.

If no URL is built, what value should I return? NULL? I'm now reading Clean Code by Robert C. Martin and he is stating that returning null is bad practise.

Personally, I think returning null is fine. If you want to avoid returning null, you could throw an exception, make the method declare that it throws the exception, and let the calling code handle it. I'd do the same with the exception handling you currently have... as a rule, services don't (shouldn't?) know enough context about what they are doing to properly handle exceptions.

\$\endgroup\$
2
  • \$\begingroup\$ I assume you meant "when requestingService is not set"? Since that is a String I think you may be overcautios assuming that setting it could fail in any non-catastrophic way. I unfortunately have to disagree about returning null. It's IMO a codesmell to even have the possibility of returning null in such an integral part of an application \$\endgroup\$ Commented Jul 27, 2016 at 7:41
  • \$\begingroup\$ Yes, I meant not set. Thanks. I have edited to fix. It may well be a String, but I think the exception should be thrown when it is set rather than when it is to be used... makes it easier to track the problem down. \$\endgroup\$ Commented Jul 28, 2016 at 0:58

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.