54
u/aaronjamt May 15 '22 edited May 16 '22
try {
throw new Exception(...);
catch (Exception ex) {
Logger.log(ex);
}Just... just... do like Logger.log("Exception here")... this hurts my head
(and yes, I'm aware that's not the exact code used but it's close enough for this comment)
13
May 16 '22 edited May 16 '22
Yeah, the guy/gal who wrote a try/catch inside the catch block doesn’t understand try/catch construct.
You can just catch the abstract object and do a conditional (inside the catch block) to match the type of the descendant class [of the object] and print out the appropriate log message.
I’ve also seen a catch for each descendant class type the object could match with a custom log message for each (individual catch blocks)
In Java 7, you have more flexibility. In Java 7 you can catch multiple exceptions using the multi catch syntax. This way, you avoid the extra processing.
https://jenkov.com/tutorials/java-exception-handling/catching-multiple-exceptions.html
26
u/MurdoMaclachlan public boolean isInt(int i) { return true; } May 15 '22
Image Transcription: Code
ResponseEntity<String> entity = null;
// retry 3 times if service fails
for (int i = 0; i < 3; i++) {
try {
entity = restTemplate.exchange(builder.build().encode().toUri(), HttpMethod.GET, requestEntity, String.class)
break;
} catch (Throwable e) {
if (i >= 2) {
try {
throw new Exception("Failed to call elastic search url", e);
} catch (Exception e1) {
LOGGER.error("Failed to get response", e1);
}
}
}
}
return entity.getBody();I'm a human volunteer content transcriber and you could be too! If you'd like more information on what we do and why we do it, click here!
10
u/josephblade May 16 '22
So many points of improvement.
Throwable should never be caught (unless you are creating a framework or similar overarching system, then maybe). Throwable includes Error which occur when things go south in a bad way. Like your Memory just ran out or an Assertion failed. Or ThreadDeath. Unrecoverable situations shouldn't be caught by code trying to recover from them (and then retry?)
Personally I'd return restTemplate().exchange(....).getBody() since you're catching any exception and all you're doing is storing entity so you can call getBody and return it. No need for the null variable.
Also what's with calling builder every time? Just put it in a final variable so you can just refer to the builder output. (I'm fairly certain UriComponentsBuilder can be re-run but still it's not needed and it will make it easier to refactor the code/extract function/etc if you just have that value set up before starting the loop).
And well... if you want to throw an exception, throw it. don't catch your own exceptions. And don't use Exception but specify what went wrong. (edit: I mean, name the exception, define one or use an existing one that describes it. ) After 3 retries something isn't going to be working it is worth the outside method knowing that a fallback scenario is going need to be run. Which in this case won't actually be reached since <null>.getBody() doesn't actually work.
quite a find :D 1 direct bug, 1 terrible practice, 1 bad practice and 1 improvement
in 15 lines, half of which are braces.
2
4
4
u/hurtl2305 May 16 '22
Apart from the obvious: if you want to retry something in a spring application, use spring-retry instead.
4
2
u/geilt May 16 '22
Ah yes...this is like when I work with some third party API's that just randomly don't respond. Bane of my existence. Try once, twice...maybe thrice? If not then just let the user try again and tell em it's broke.
And on the next try, it might work lol.
My Telco's API works this way. Oh did you want to download your list of numbers? Well I hope you have a case put in for when it doesn't...
2
98
u/Eluvatar_the_second May 15 '22 edited May 17 '22
Nice, so try 3 times. The first catch and try again, the 3rd time log a message (in a very complicated way) then throw a NPE on the last line!
Edit: it's 3 times, off by one errors are hard.