Robert's Homepage

Did You Forget to Review Your PR? And Other Habits Worth Unlearning.

#software development

In continuing to reflect on random lessons learned while in the field, a group of nasty habits I used to have popped into mind which I feel might be worth reflecting on. If you’re the type of developer who writes PRs and often wonders why no one reviews them, well here’s some of the reasons that might quell your resentment towards your team mates.

Did You Forget to Review Your PR?

Code reviews aim to catch mistakes, introduction of potential bugs and all manner and form of domain insight for maintenance or addition. Early on in my career, I often submitted pull requests to mid and senior level folks politely pinging in Slack threads or direct messages, all the while never taking them for a spin myself with a simple once over read through. Over time, this habit became rather embarrassing when team members would point out typo mistakes, dangling artifacts (e.g., TODO, FIXME), or other forgotten code paths. It’s worth reading your diff over once (or twice) to catch some of this stuff and can save a lot of repeat reviews on your PRs from folks.

CI is Failing (Red)

Let’s politely call it eagerness, but more practically call it inconsiderate: I frequently submitted PRs for review without waiting for CI to turn from pending (i.e., running) to green (i.e., passing) early on - they often ended up red.

There are times when this is reasonable. For example, suppose you work on a large dynamically typed product codebase and did not realize that setting a variable to nil in a follow up comment, suddenly causes spec/requests/foo_spec.rb:53 to fail for some reason. Or you forgot to run the linter and formatter on a recent file change. Or certain situations where it’s unavoidable, such as emergencies (e.g., incidents), where action and immediacy precedes politeness. But outside of these situations, it can start to become an annoyance that can deter reviewers. After all, consider that your team mates - who are busy too - might see it as (arguably valid) reason to procrastinate on review. If your code doesn’t work on CI yet it can’t be merged even if they review, so it might not be worth reviewing yet anyway… back to the bottom of the pile.

The Diff Is Huge

This a tricky one starting out. It takes experience and command on version control tools like git to become quicker at (i.e, multiple part branches, cherry picking, et cetera), and even then, sometimes, it’s completely unavoidable (e.g., large component or section refactor). Large diffs make reviewers anxious, especially the larger and more critical a codebase. Incremental change sets in a complex system have implicit risks associated with them; in addition to the reality that many such change sets are submitted by non-domain experts. Large diffs can happen for reasons like not seeing the opportunity for divide and conquer, not being strategic in change set construction, relocating different blocks of code to new homes, et cetera. A big problem I had that caused my diffs to blow up was following the Scout’s Rule too zealously (I owe Arnaud for that insight). As a new grad admonished in the pristine Apollonian character of academic programming - where writing out all the getters and setters, which let’s be honest is bloody satisfying, is rewarded with high praise and cheer - it can quickly become overwhelming how unbeautiful “real world” code can be. Sometimes, and this isn’t an excuse for smelly code, but sometimes, the 13 parameter function hides a lot of battle tested complexity that isn’t immediately obvious to a zealous refactorer; only after the first 1-2 hours grokking the damn thing and breaking it into pieces, do you realize how large a hidden world laid in that messy 40 line function sprawled across your IDE… diff churn.

Though I still struggle with this one to date, partly because I find readable clean code appealing - despite leading me into battle with multi-headed hydra’s when refactoring - it’s worth learning a few version control tricks like git cherry-pick and multi-branching approaches and submitting incremental diffs to break the habit.

There are a variety of other bad habits I have had (and still have) but for now these are just some of the ones that come to mind.