During a code review, another dev and I had a small debate about the proper or correct way to handle multiple returns, and if assigning a pointer to null, and then setting it yields any difference compared to simply returning the value, an later returning null.
private ServiceParam getRequestServiceParam(SlingHttpServletRequest request) {
ServiceParam sp = null;
try {
sp = new ServiceParam(IOUtils.toString(request.getReader()));
} catch (IOException e) {
LOGGER.error("IOException", e);
}
return sp;
}
vs
private ServiceParam getRequestServiceParam(SlingHttpServletRequest request) {
try {
return new ServiceParam(IOUtils.toString(request.getReader()));
} catch (IOException e) {
LOGGER.error("IOException", e);
}
return null;
}
Functionally they seem identical, but we don't know if one my be more correct than the other. Though I fear that this may just be a tabs vs spaces argument.
Whether more than one return from a method is ok or not is a matter of opinion. But there are trade-offs here to talk about where some of the solutions are better than others. Sonarqube complaints are a good place to start, but there are larger issues.
Some people think it's bad to have more than one return and they want to follow the structured programming rules no matter what. Structured programming is better than using goto, and it does produce consistent-looking code.
Other people point out that enforcing one return makes control statement nesting deeper, resulting in code that is harder to read. In the case of the first example I find myself scanning up and down in the method looking for where the local variable gets a value assigned to it, and I think that's annoying.
(Sonarqube doesn't count cyclomatic complexity correctly when you have multiple returns, btw. So if it nags you to avoid that practice maybe there is an element of self-interest there; it's telling you, don't go writing code that we have trouble figuring out.)
Also there is no performance implication to any of this. It is just a matter of awkward coding and less-than-ideal exception-handling. When you return null you create an opportunity for the calling code to fail to check the reference for null, it would be better to let the exception get thrown.
I would not use Optional for this unless it looked like something I'd use in a monadic context where I'm chaining stuff together and using flatMap to handle the Optional empty cases for me. Otherwise it is annoying to have to wrap and unwrap this, it is a bit better than returning null, only because it forces the calling code to consider the empty case.
In this case the actual awkwardness is caused by reading from the httprequest, which is what throws the IOException. You could add throws IOException to the method and let the exception be thrown. But it would be better to move that code to whatever servlet or web controller is getting the httprequest, and have any IOException thrown by calling it get handled in the same way as whatever else the servlet or controller does gets handled.
Once you move that request-reading code there is no compelling reason for this method to exist at all. Deleting unnecessary code is a good thing.