+972 52-548-6969
PR ReviewErrors, errors and more errors
time to read 1 min | 49 words
I’m harping on this, but this is really annoying. Here is the latest batch.
And this:
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
Comments
return E_FAIL;
I can understand the frustration of seeing this in PRs over and over again, but the individual comments don't seem like the most constructive way to deal with it. Seems like it would discourage those submitters from fixing, and/or from making future pull requests, especially without, say a link to your blog post about what is expected, giving them an opportunity to learn and submit better code in the future, without making them feel stupid.
These are obviously made after frustration with expressing the requirements in the past. Just saying "please add the exception" info after the first few times, and then continuing to see this, does need a sterner hand. At least the "why" is being given in the code comment.
From experience, if you've ever written code like that, and then needed to just once rely on those logs in production, you never write such code again.
Ian, obviously, but this is one of the things about coding in the open, you cant take out your frustration that the next dev didnt learn from the previous dev's mistakes, the next dev didn't see them, they aren't in the team.
From experience, people writing that style of code, often are never the ones troubleshooting the logs in production. Hence, teachable moment, if you accept code others, especially those contributing their own time/employers time to add to someone else's commercial product, then this is part of the cost of doing so. I do understand the frustration, I really do, but giving in to it, means others are less likely to contribute in the future and will scare away those with imposter syndrome, thinking "what if they leave a snarky comment in my PR, oh well, I'll just wait until someone else fixes it". Not good for anyone.
A sterner hand needed? Maybe. A snarky one? Nope.
True. Snark isn't helpful. I tend to sit for a few minutes on an email, or a code comment, written with annoyance at hand. Then skim and change if needed.
Adding code comments for good things, such as where the logging was done well, is also nice but can clutter the code review window depending on your system. Having said that, the positive reinforcement is more powerful, and more fondly remembered. It's also more likely to be remembered than a lot of coding standards up front.
Tom, These aren't comments made to random strangers. These are comments to people who work on the project long term, and were so focused on doing a specific feature they forgot to consider other aspect of the system.
There is also the fact that I usually get off my chair and sit with them to go over the code and fix that, so this isn't something in isolation. But if someone will read this post and think "maybe I need to work on the error message bit more", then my job is done.
Comment preview