If we are speaking in terms of Clean Code, the entire method should be revised.
Here are the main points that I can criticize:
if ... else if ...
branches are used to match Strings: this approach is too verbose and not very flexible.
URL
is instantiated in two different places, with same semantics.
The Exception
thrown in else
block is immediately caught and thus is useless.
The catch blocks hide exception details and trace, which is annoying for debugging.
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
.