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.
PR Reviewavoid too many parameters
During code review I run into these two sections, which raised a flag. Can you tell why?
The problem with this type of code is two fold. First, we add optional parameters, to reduce the number of breaking changes we have. The problem with that is that we already have parameters on the call, and eventually you’ll get to something like this:
Which is the queen of optional parameters method, and you can probably guess how it looks internally.
In the first case, we can add the new optional parameter to the… options variable that we are already sending this method. This way, we don’t have to worry about breaking changes, and we already have a way to setup options, determine defaults, etc.
In the second case, we are passing two bools to the method, and there isn’t a preexisting parameters object. Instead of creating one, we can use a Flags enum, whose bits we can set to determine what exactly the behavior of this method should be. That is generally much easier to maintain in the long run.
PR Reviewthe errors should be nurtured
I run into the following in a PR I recently reviewed. Take a look and try to figure out why I’m pointing this bits out:
Those are bad errors. In the sense that they are hiding information. Sure, in 90% of the cases the user just put it the backup location or the restore location, so they know what the application is referring to. But the other 10% is looking at the exception from the logs, having “I got an error” support call or all sort of weird stuff.
In particular, one of the most annoying things is when the user and the application disagree on a relative path. If the user believes that the relative path is based on one location and the application another, hilarity ensues. But not in a fun way.
A better way to prevent all of those would be to just include the actual locations (fully resolved, mind) in the error. It doesn’t prevent support calls, but it does make them a lot easier to solve.