- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
Probably it started off as just the if, then someone noticed that certain bad zip files would cause Unzipper.CheckArchive to raise an exception instead of returning false, so they added the try as a workaround, and didn't notice or didn't bother to clean up the weirdness of raising an exception in the if and then immediately catching and reraising it.
Admin
That's a very strong WTF in and of itself.
Admin
To clarify, it would certainly be a bug for that to happen, not an intended part of the interface. But it's pretty plausible that it might, for example, try to check the data at a certain offset within the file without first checking that the file is large enough for that offset to exist, or something along those lines. Validation is hard, let's go shopping.
Admin
I don't know(1) if Delphi has an equivalent of C++'s
throw;, but creating a new exception erases the backtrace of the initial exception, so good luck to anyone trying to debug getting this exception somewhere up the stack.(1) Translation: I k'bah to look it up.
Addendum 2023-06-13 07:52: OK, I 'bah to look it up, and Delphi does have a rethrow-current-exception statement:
raise;.Admin
` end; {if}
These kind of comments set my teeth on edge.
Admin
It could have been an afterthought directive to add exception handling to everything without modifying existing code. Unfortunately I've seen that happen where every layer simply catches and rethrows exceptions.
Admin
We were kidding this whole time, but the canonical (true, false, file_not_found) enum is indeed needed! :)
Admin
As pointed out Unzipper may be the real WTF (sometimes throwing exception; sometimes returning false)... so the posted code is a reasonable solution...
Admin
I can see the CheckArchive throwing--it's purpose is to check the archive, what if it's not pointed at an archive or can't access the archive it's pointed at? It shouldn't say it's not valid when it didn't even read it.
Admin
But then the exception shouldn't say "Corrupt Zip file.", it shouldn't say it's corrupt when it didn't even read it!
Admin
Not only does this uselessly catch and re-raise the exception for "Corrupt Zip File.", but it also transforms any other exception that CheckArchive might raise into "Corrupt Zip File.".
Admin
Well that's the worst way how to handle return codes by turining them into an exception that is caught and the thrown again. That's 2 stack unwinds and two exception scopes for literally nothing :-)
Admin
Speak for yourself!
Admin
Obviously 'Unzipper.CheckArchive' can return 'true', return 'false', or fail and raise an exception. The author didn't care what kind of exception was raised, and in fact decided to lump in a 'false' return with the exceptions. They all mean the same thing: Have another shufti at that .zip file.
The type and message of the first 'raise' statement is irrelevant, because the 'except' block throws it away. If the author had used two different messages he might have been called upon to explain his reasons.
Admin
Efficiency-wise it doesn't matter - most of the time you'd expect the archive check to be fine. If you're in a system design where you're expected to raise exceptions then exceptions is what you do. Like most languages with exceptions, library docs are not great at listing all possible exceptions so rather than playing whack-a-mole with catches I don't think it's that unusual to rename the exception so there's just the one to catch. Having said that, I do think the 2nd exception should be adding the details of the underlying exception to the message string. I'd also add a check for the archive (file?) existing before doing anything else, because that's cheap and I suspect that'd be 99% of the likely problems. And the archive name/path should be included in the messages, to give the user something useful to check.
Admin
Except that as written, the exception from the unzipper is caught and then a new exception of the same type is thrown.
Admin
Here, it isn't being renamed. The new exception is the same type as the unexpected one. (It's an API that returns an error state, but can also throw in weird cases, so it's unexpected...)
Admin
The only reason for the try ~ except block would be if the call to Unzipper.CheckArchive could result in an exception.
If the actual call fails because the Unzipper object is no longer valid (use after free etc.) that's bad enough on its own. But if CheckArchive raises an exception instead of returning false, that's worse, because why make it a function in the first place?
But if CheckArchive spews out an exception because of the code inside? That's 50 shades of awful and reeking of a complete lack of proper exception handling.