Don't Leave Comments on Pull Requests - Leave Pictures
Tags: github, codequality, programming, webdev
Published: 2021-03-06 21:57:23

A picture says a thousand words, right?  Whenever you're reviewing pull requests (or reviewing code in any other context), you may find yourself writing copious notes, carrying on ad hoc conversations, and even doing "social media" kinda stuff - like `+1`-ing something, or "like"-ing it, or slapping emojis all over it.  

But I had a little epiphany this week:  Why am I burning all this time trying to type out coding instructions, and explaining application flow, and suggesting alternative methods?  The fact is that my oh-so-handsome mug is, quite often, perfectly capable of communicating _everything_ that I'm thinking on a given PR.  So why not just slap the appropriate picture on it... and be done?

I know.  You're probably feeling pretty skeptical about that idea.  You may even think that I've finally reached the point where I need to find a new career, hang up the keyboard, and abandon my Dev.to ramblings.  But you're only thinking that because you haven't yet seen the pictures that I'm gonna start slapping on PRs.

Observe... and be awed!
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/qnubrk60305bvboy19gm.jpg)
## The Pencil-Whipped Approval
Ever had someone ask you to "review" their PR - but then it becomes immediately obvious that they don't want you to _review_ it - they just want you to slap an _approval_ on it?  In fact, they may even get kinda snippy if you start adding comments to it.  They don't want your input.  They just want _someone_ to stamp an approval on it so they can get it merged. In this scenario, Ima drop this pic on the PR.  

**NOTE:** I didn't say that I'm gonna _approve_ it.  I'm just gonna put this pic on it.  When they come back and ask me why I haven't _approved_ it yet, I'm just gonna ask them, "Dooood - did you _not_ see the righteous thumbs-up that I gave it???"
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/leg1l1c1qqzuwa9jx4rx.jpg)
## You Ain't As Stealthy As You Think
Ever seen a PR containing code that you _know_ the dev was hoping to just "sneak by"? Could be something that skirts company policy. Could be something that is questionable from a performance or a security perspective. Could even be something that's been consciously obfuscated. This pic tells the dev that I know _damn well_ what they're doing.

**NOTE:** This doesn't necessarily mean that I won't approve the PR. Heck... we all have those `<ahem>`moments`</ahem>` when we feel that there's just something that _needs_ to be merged - for any myriad of reasons. Some of those reasons can even be borderline "legit". But this pic makes it clear that your effery has not gone unnoticed by me. And as long as _you know_, that _I know_, then we're all good.
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/3z0v4p1lkjtlpvzro1pi.jpg)
## _Sooooo_ Much "Nope"
Look - declining a PR is something that I almost never do.  I will almost always try to put constructive comments on the still-open PR.  Or start a real-time convo with you to address some of my concerns.  Or even _ignore_ the PR.  So if I go so far as to _reject_ a PR, it probably means that you've done something (or are _continuing_ to do something) in the code that's really starting to **tick me off**.  In those oh-so-rare scenarios, a plain ol' rejection just ain't gonna do.  _This pic_ will certainly do a much better job of voicing my true feelings.
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/fjpw5rsrbjqlurvxcimt.jpg)
## The Supa-Nope
Have you ever pointed out a problem on a PR, then the coder withdraws the PR, then at some point in the future, they re-submit the PR _with the exact same code in it_???  I've even seen the PR re-submitted at a time when they assumed I was out of the office and not paying attention (for which case, I need to come up with a pic that has _three_ thumbs down).
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/2v2amsxsxze840qw0itx.jpg)
## A Conspicuous Lack of Back-Scratching
_My_ PR's been sitting there for a week-and-a-half.  You haven't approved it.  You haven't commented on it.  I've even reminded you that I need a review on it - but you still haven't touched it.

But _now_... you got an urgent PR and you're hyper-eager for me to jump in and approve it??  Yeah... sure.
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/3kj0st0rk8gmugwx6xct.jpg)
## You, Sir, Are One Clever MF'er
Maybe you think that all my PR responses are negative, but nothing could be further from the truth.  Occasionally, your colleague submits something that's legitimately brilliant.  At times like those, it seems a shame to do nothing more than slap an "Approve" on it.
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/x3modg2siplrfvtgj00l.jpg)
## Be Gone!
There are a few things that will make me end a code review immediately.  Like, if your new JavaScript code has a `var` in it - _any_ `var` in it - the review's done.  I ain't even bothering to read the rest of it.  Get this _ish_ off my screen.
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/797k8zmg4q0uwpq8fxo5.jpg)
## Just Here For The Comments
Have you ever pulled up a PR and it already has some epic thread on it that looks like a refugee from 8chan?  Arguments.  Dogmatic declarations.  Someone quoting "company policy" (usually the same douchebag who can't be bothered to follow "company policy" in _his own_ commits).  Someone else arguing about the performance benefits of a `for` loop versus `.forEach()`.

I usually won't touch those PRs with a 10-foot pole.  Once the battle lines are drawn, my only "role" is to sit back and enjoy the ride.
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/jym6kldn3aeod6ohkrhd.jpg)
## The Sanskrit Whisperer
There are 1,000 LoC in the PR - and half of them consist of complicated regular expressions.  The coder absolutely _adores_ Lambda expressions - only because it allows him to write an entire component _on a single line_.  _Every_ variable name is an abbreviation - e.g., `getUser()` becomes `gtUsr()`.
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/nbimakj56r9x68hmgapi.jpg)
## Let Us Never Speak Of This Again
You're gonna delete that PR.  I'm gonna pretend that I never saw it.  And there's no reason why either of us ever again needs to think about the _ish_ that you just tried to commit.  This is usually reserved for those times when someone's committed a chunk of code with _egregious_ security flaws.

**NOTE:** This is truly a speaks-for-itself image.  I have stared into the void (your code) and it has stared into me.  And we are both shaken by the experience.  All I should have to do is slap this image on the PR.  If this is my reaction, and you still need me to point out what you did in your code that sparked this reaction, then you either need to be gone, or I need to be looking for another gig.
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/rfjao8zvlljvchjtvezp.jpg)
## Oh, _Really_???
Have you ever known someone who tries to use future commits to "win" some kinda debate that you were previously having?  Like, he's convinced that _every_ string comparison should be done with a RegEx.  So in his next PR, he converts all of the _existing_ string comparisons in the codebase to regular expressions - even though that has nothing to do with the putative goal of his commit.
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/hlxuyyy65119wl7gq9zk.jpg)
## The Failed Experiment
I _totally_ see what you were trying to do here.  Heck, we may have even discussed this approach before you committed the code.  And while we were talking about it, _in theory_, it sounded like a potentially-viable approach.

But now... Now that I can actually _see_ the implementation?  Yeah, umm... no.  This gets pretty ugly _in practice_.  And we gotta think of some kinda better approach.
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/2i26w784u4znop9rafhh.jpg)
## The 50/50
Look... The code's not _wrong_.  But it's definitely not _right_.  I doubt I'm gonna be rejecting this PR.  But I don't know if I have the intestinal fortitude to _approve_ it either.  It's, well... it's _a start_.  There's some good stuff here.  But mannn... it still needs some TLC.
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/4k26dmxyntabl9jzpwbt.jpg)
## The Haircut
This really ain't a bad commit.  _Seriously._  There's some good stuff in it.  But I'm _prettttty_ certain that this 86-line function could do the exact same thing in, ohh... 10 lines or so.
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/q4p318zt4hk6tjwhvbrk.jpg)
## The Big Baller
Ima be _all over_ this PR.  I won't do anything with it that's _of use_ - to you, nor to anyone else.  But I'm definitely gonna be all over it.  

I'm gonna add comments, consisting of nothing but emojis, that bear no logical connection to the code where the comment is posted.  I'm gonna reply _to other commenters_ on the PR.  I'm gonna drop comments about a cool new package I've been looking into - that has nothing to do with your commit.

**NOTE:**  What I absolutely will _not_ do, at any time, is approve the PR.  Or reject it.  Or make any direct reference to _the actual code_ in your commit.
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/tbg46quxbdgojacyl511.jpg)
## The Inspector
Do you remember the last PR that _I_ submitted?  I'm sure you do, because you gave me 37 suggested changes - for a commit that only contained 20 LoC.  You quibbled with my naming conventions, suggesting that I convert `getUser()` to `getMember()`, only to ask me to change it back to `getUser()` when you realized that `user` is consistent with the rest of the codebase.  You had me change my arrow-syntax functions into old-style `function` declarations, because you just don't _like_ the arrow syntax.  You insisted that the state values be drawn out into their own reducers, in their own separate files.  You asked me to convert `style={{marginTop: 2}}` into its own standalone CSS class.

Consider this pic to be your fair warning.  If you truly _want_ me to continue with this review, I absolutely will.  In fact, I may not do _anything else_ but this review today.  Cuz Ima pour through every single line with the petty, spiteful, immature spirit of vengeance.
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/zqb313z4lv1bw171gu86.jpg)
## You Da Man!
Have you ever reviewed someone's code when you realize that they've rewritten something _that you originally wrote_ - and it's immediately obvious that they did a **much better job**.  When that happens (and it happens quite frequently), Ima slap this pic on the PR.
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/1g7g4hh4zrjxf51uhuqk.jpg)
## The Histrionic Avenger
This pic isn't for any particular observation on the code itself.  Rather, this is my response for anyone who can't help but launch into histrionics whenever they're reviewing _your_ code.  

Have you ever worked with a guy who can't just _comment_ on your code, but instead has to act as though every minor "mistake" is a massive knock against your character?  

> "OMG, everyone!  Check this out!!  _Adam forgot to add a semicolon at the end of this line!_  Who taught this noob how to get on the interwebs, _amiright_???"

For guys like that, Ima slap this pic on _every_ piece of the code that requires a comment.  I don't care how minor the observation may be.  I'm gonna treat _every_ iota of feedback as though it's **The Worst Code I've Ever Seen**.
<br/><br/>


![Alt Text](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/kd0hwjpab9n8e7as6eqj.jpg)
## Conclusion
Now that you've read this article, I'm pretty sure we can all agree that plain ol' _comments_ just have no place anymore in a code review.  _Typing_ your feedback is just soooooo... six-months-ago.  As long as you have enough unique expressions, you'll never need to _type_ anything again!