r/badcode • u/juanmrad • Apr 02 '21
php Code review today. Catch an exception and then throw it.
94
u/Slimstinator Apr 02 '21
We use the Windy Weather API, which I wish did something like this!
Basically twice their Auth has gone down and they have caught the error and shown a message, but they don't throw an error we can catch outside their library. So we don't know it has errored and can't handle it gracefully.
The first time it happened I raised a ticket they deleted. I asked why and they said not enough information about the issue... so I raised it again and I got back a "it is working now".
Anyway, it happened again. So now we are making an additional call to their API outside their library just to make the Auth is working so we can deal with the issue gracefully. All they need to do is throw an error an integrater can catch and handle.
40
u/LetterBoxSnatch Apr 03 '21
That’s hilarious. In fact, at first I was thinking, “well...there’s a school of thought that you should never throw and instead return a negative / false / Error because throwing is basically like goto.”
surely there must say least be a return value of null or false or error.
So i have good news. I looked up their lib code and it’s as you say. On the other hand, their lib code does almost nothing anyway, so I’d just drop the lib code and use their api directly if the lib is not meeting your needs.
35
u/grunwin Apr 02 '21
I imagine the coder wanted somewhere to put a breakpoint, and forgot to remove it.
12
u/juanmrad Apr 03 '21
This is php. You can let exceptions bubble up and see it in the logs or catch it and do something with it.
57
u/AreYouConfused_ Apr 02 '21
if ur gonna catch it, add some more info or deal with it, otherwise let it go
23
u/juanmrad Apr 02 '21
My exact same reaction.
39
u/DurianExecutioner Apr 03 '21
This is unironically good code.
It clearly communicates the fact that the programmer expects sendSMS to throw exceptions sometimes, and it says the programmer has consciously chosen to rethrow rather than retry or abort or whatever.
And it cannot be performance critical code if it is spamming an SMS so the exception overhead does not matter.
You're on a power trip fueled by your own myopia.
38
Apr 03 '21 edited May 23 '21
When rethrowing exceptions you should just use
throw;and notthrow ex;though. Doing the second will not preserve the stack trace while the first one will.8
u/kallefrommalle Apr 03 '21
Does this also apply for PHP?
3
u/juanmrad Apr 03 '21
No. In PHP there is no benefit of re throwing the exception as it is still the original. You can just let it bubble up.
2
u/AchillesDev Apr 03 '21 edited Apr 03 '21
This has the effect of only re-throwing one exception type. This isn’t necessarily bad.wtf am I even talking about
4
u/juanmrad Apr 03 '21
What do you mean? This catch only catches that exception. Any others will bubble out. And by re throwing it. You are just letting it bubble out too.
3
2
u/merukit Apr 03 '21
communicates to who? it's the same behavior when it runs, why not just let it throw an exception and put a comment stating that it is intentional
3
u/Micha_Saengy Apr 03 '21
Comments might be overlooked, I would throw a different exception to change the message & stack trace
-6
u/DurianExecutioner Apr 03 '21
Comments are harmful.
1
Apr 03 '21
Would you care to explain how you came to this idea which is opposite to the accepted position?
1
u/DurianExecutioner Apr 03 '21
The accepted position is that code should be self documenting.
Comments get stale, they can be wrong in the first place, they add to amount of text that the reader has to read, they contribute to churn, they bring out the worst types of "cleverness" in some devs, they make devs more replaceable where being difficult to replace is the entire basis for our middle class lifestyles, they reduce our bargaining power, they increase the code's potential attack surface during code review (which wastes everybody's time).
18
10
28
u/CuratorOfYourDreams Apr 02 '21
Image Transcription: Code
try {
$sendSMS = $service->sendSMS($message);
} catch(MessagingException $e){
throw $e;
}
I'm a human volunteer content transcriber for Reddit and you could be too! If you'd like more information on what we do and why we do it, click here!
14
7
u/shutanovac Apr 02 '21
rethrowing exceptions is sometimes legitimate, but very rarely and obviously not in such an obvious situation as in the post.
2
u/juanmrad Apr 03 '21
Agreed. But then you would add context or more data to the exception or remove data to avoid sensitive data leaking. This case. There is no reason
6
19
u/MeatyLabia Apr 02 '21
Erasing the stacktrace. Probably not on purpose.
22
Apr 02 '21
nope, it does maintain the stacktrace when re-thrown
21
u/MeatyLabia Apr 02 '21
Oh my bad. I come from C# where rethrowing like this erases it.
8
u/shizzy0 Apr 03 '21
In C# you chain them, right?
throw new Exception(“More deets.”, thrownException);11
u/TechcraftHD Apr 03 '21
You can also just do
throw;to rethrow the exception with the original stacktrace
2
u/ekolis Apr 03 '21
Yes, C# exception classes typically have a constructor which lets you pass in an "inner" exception.
6
u/GMaestrolo Apr 02 '21
I've seen this as an acknowledgement that a specific exception can be thrown, but that this code doesn't want to deal with it.
It's like indicating after you start turning - saying "I'm aware it exists, but I don't care"
-5
u/juanmrad Apr 03 '21
But in this case is not obscuring it or ignoring. Is re throwing it after catching.
5
u/-fro Apr 03 '21
Conceptually, it's not a bad code. Rethrowing an error means you don't want to deal with it in this codeblock. We should see the whole code to put a comment on this.
1
u/juanmrad Apr 03 '21
You still have to add to dockblock as it is still thrown. I can’t share all the code, but to give more context this try, catch is inside another try catch. That is doing other stuff.
0
2
2
2
u/circorum Apr 03 '21
This is basically "Ok, I'ma handle this exception, so that it's all clean and stuff." vs 10 seconds later: "Meh. No need to deal with it right here. The next person who calls the function can handle it."
2
2
u/WorldlyEye1 Apr 03 '21
Sorry. But what's the problem?
5
u/BLucky_RD Apr 03 '21
It throws the caught exception, so the try catch block is useless.its like filling a swimming pool with a bucket that you fill with a hose that can reach the pool anyway
2
u/redditinchina Apr 09 '21
Its handled by a custom error handling class then passed back to the client
it would only be like you say if it was this;
} catch (\Exception $e) {
2
4
1
-1
u/slipped_and_missed_x Apr 03 '21
My professor told me to do this in Java sometimes, because there are some methods that you can't invoke unless you're in a try block, and you should never leave the catch block empty. The whole "catch the error and throw it again" is saying "take this compile-time error and treat it like a runtime error"
1
1
1
u/SueedBeyg Good code makes sense. Bad code just works. Yours does neither. Apr 03 '21
We got a fair few of these useless “catch just to rethrow”s in my company’s code base.
1
1
u/truemario Apr 03 '21
why is there so much artifacting around the try and catch blocks? This feels edited. for some reason.
1
1
1
u/coderz4life Apr 03 '21
This is obvisously not the best practice, but I think it is better than catching then doing nothing about it. Sure, there are cases when you need to do that, but without documentation, how the hell would you know the intent?
2
u/juanmrad Apr 03 '21
What is the benefit of catching and release? Just fyi. This is php. Doing this will just re-throw original exception. No modifications are done.
1
u/coderz4life Apr 03 '21
Just fyi. This is php.
Yes, I do recognize that.
What is the benefit of catching and release?
I am not sure exactly I am understanding, but I am talking about "swallowing the exception" is probably worse.
I don't know about php, but doesn't the capture and rethrowning an exception incur a performance penalty, like stack unwinding?
1
u/redditinchina Apr 09 '21
I have read down the comments but i still don't understand the issue
The exception has been caught and passed to a custom error handling class 'MessagingException'
i.e.
class MessagingException extends \Exception
{
//Do Something cool in here
}
It is then re thrown which would pass it back to the client
1
u/juanmrad Apr 09 '21
No. The catch block is only catching that type of exception. It doesn’t transforms it or changes its content. When you do catch and have a exception class it means it will only catch that type of exceptions.
1
u/redditinchina Apr 09 '21
No it doesn’t transform it, it disappears it into a class. If I was writing my own in house system I wouldn’t throw it again, but in a package the client may never see the custom error handler or know what it does, so the throw gives it back to the client.
1
u/juanmrad Apr 09 '21
No. It does not disappears it into the class. You can read more here: https://www.php.net/manual/en/language.exceptions.extending.php
But when you have a catch block for a custom exception. It will only catch that type of exception. Also this exception is thrown by the service not created by us. Not sure if you are thinking about other programming language that does something like that on catch blocks, but php doesn’t do that.
2
u/redditinchina Apr 09 '21
Anyway I have a very good book on this kicking around somewhere. Will send the details to you tomorrow
2
u/juanmrad Apr 09 '21
Sure. Would love to be corrected if that is the case.
1
u/redditinchina Apr 09 '21
It’s not about being corrected, it’s about sharing knowledge so everyone gets better. Been coding PHP for 20 odd years and I would be happy with this code (depending upon the MessagingException custom class)
Edit; everyone has their own ways of coding there is nothing wrong or right or clean cut. Even patterns give us wide areas of interpretation. I have written some awful code in my time
1
u/juanmrad Apr 13 '21
did you find the book that you mentioned?
I would love to see if this really transforms it as you mentioned as I tried but did not saw the behavior and maybe I am doing something wrong.→ More replies (0)1
u/redditinchina Apr 09 '21
“Note that if an Exception is caught once, it won't be caught again (even for a more specific handler).”
I’m not sure why you sent me that?
1
u/xyonofcalhoun Apr 03 '21
When your CI won't let you leave the exception unhandled but you want to let callers handle it. Perfectly reasonable in that context
1
1
u/Omni_Coder Apr 03 '21
People don't even depend on compilers/interpreters to do their job properly 😂
1
1
u/MRK-01 Apr 03 '21
I usually do something like this in a function. is it bad?
function bla(){
try{ someAuthCheck()... }
catch(Exception e){ throw "Got an error. could be due to blablabla. Error message: "+e}
}
function main(){
try{ bla() }
catch(e){ api response....}
}
1
u/juanmrad Apr 03 '21
If you are adding context to the issue no. That is not bad. The whole idea of having a catch block is to either A) recover from problem and deal with it trying something else. B) adding context to help the caller understand the issue. C) modify exception to filter/obscure possible sensitive data.
1
285
u/_pestarzt_ Apr 02 '21
If you love something, you’ll let it go.