PR ReviewIs your error handling required?
During reviewing a PR I run into what seemed like a strange thing. Take a look at this change:
This came with its own exception class, and left me pretty confused. Why would I want to have something like that?
Here we have some error handling code that doesn’t seem to add any additional value. Everything in the error here can be deducted from the details of the exception that will be thrown if we did nothing.
The fact we throw a specialized exception might be meaningful, but looking at the code, this isn’t actually used for anything.
Like all code, error handling needs to justify itself, and this one doesn’t pass the bar.
More posts in "PR Review" series:
- (19 Dec 2017) The simple stuff will trip you
- (08 Nov 2017) Encapsulation stops at the assembly boundary
- (25 Oct 2017) It’s the error handling, again
- (23 Oct 2017) Beware the things you can’t see
- (20 Oct 2017) Code has cost, justify it
- (10 Aug 2017) Errors, errors and more errors
- (21 Jul 2017) Is your error handling required?
- (23 Jun 2017) avoid too many parameters
- (21 Jun 2017) the errors should be nurtured
The only reason I would do this (and it's a lazy one) is if I know that it's an exception I would like to put a breakpoint on and catch all the places it would be thrown without searching for them.
I can imagine the error throwing method not showing up in the call stack due to inlining.