A Real Code Review

This an anonymous blog post. I write code for a living. My peers write code for a living. We are cats. Don’t fucking try to herd us.

At the same time… yeah… that’s why that dot-com-era ad was funny. We must be herded to get anything done.

Well, maybe? Some people seem to think I’m a big fucking fluffy cat at this point, but I don’t want to be. I want to mind-meld all the cats in the same general direction. And then let them go, so they can lick their private parts and write great code in private. That’s why we’re cats.

I need to talk to a co-worker tomorrow. What will I say?

To start this off bluntly, if I were to describe your code in one word, I would say “baroque”. Somehow you write it more complexly than it need be. Before you commit, do a git diff. Become more modest. Really, you are showing your what to me? Tuck it in. Don’t make me do the code review that you should do yourself. Look at every line. Ask: Why the fuck did I write this? Is this fundamental to accomplishing the task at hand? BTW… what the fuck is the task at hand, anyway?

If you don’t immediately know the task at hand, on a scratch-pad, write a brief summary of what you are doing. Do that. Yes, just THAT. Nothing more. Yes you are awesome, but right now you are doing X. Put Y and Z to the side. git stash. git diff, and if it’s not part of X, then stash it into Y.

Every line you write may be valuable. But it may not be relevant. It is astounding how time-dependent the “value” variable is. Separate. Separate. Separate. Create an “OTHER” bucket. It will last for years, with personal things coming and going.

Do you know yarchive? Norman might be a bit obsessive (compulsive? – I don’t know), but that’s what powers the tech world. These are the quiet-spoken OCD anchors of the tech world. He has collected some notes regarding submitting patches to the Linux kernel. These mailing-list notes are old, but true. Read them. Imagine Linus over your shoulder. He’s a bitch. But a good bitch. Let him ride you.

Ask yourself: WWLD? (What would Linus do?) Or even better: WWPLO? (What Would Piss Linus Off?)

When I have too many improvements in flight at once, Stacked Git is invaluable. Other improvements should be stashed to the side. I love pull requests with the phrase “no functional changes” in the comment. PLEASE pile on a fuck-ton of improvements where the only things required of me as a reviewer are to verify that:

  • It doesn’t obviously fucking suck
  • It doesn’t obviously change behavior

…and yes, my answer will be, YES! YES! YES! FUCKING MERGE IT! MAKE MY LIFE BETTER!

Are you aware of “bikeshedding”? If you can focus your important pull requests, you can more easily get them through. Put the cleanups to the side. People will pile-on debating about the bullshit changes. Don’t give them the bullshitting opportunity.

Keep “code smells” in mind. Too many parameters? You might be starting to smell like WIN32 APIs; there are books written there, so don’t repeat it. Simplify your APIs, and require the caller to do appropriate setup in-between calls. Parameters by non-const reference? You’re clouding ownership semantics. Good C people will frown at you, but C++ people will crap their pants. And always ask yourself, what is the purpose of this class? If you cannot express it in a sentence, reconsider. Smaller, simpler, more focused.

Ask yourself: What was I trying to do? Why is my pull request larger than that? How can I decrease the lines of change? Lines-of-change is friction.

Yes, strive to make your new code perfect. Make it perfect and minimal. Yes, old code sucks. It always sucks. Fixing the world is not your job with this PR. That’s a separate PR. Separating the improvements out is not a failure: it’s maturity. Guide me, as a reviewer, to understand what you are doing now.

Let me return to my thesis:

Don’t mix other cleanups in with fundamental core changes. If you do that, my response is: Fuck you. You might be right. It might be great. But still, FUCK YOU, I don’t know. You are wasting my time. Weeks ago, months ago (?) I approved the design, but now I’m on the hook for WHAT? I don’t fucking know, it’s buried in tons of other bullshit. NO. And now I’m the asshole preventing the whole god-damn thing from being merged. I’m fucked either way.

Please simplify.


Lesser minds hate C and POSIX because it is rigorous. Terse. Greater minds understand that power. Use it to piss off the C++ purists, and make them smile while they take it up the ass about strict correctness. Small, correct, up-the-ass, in that order.

And then thy PR will be approved.

And thy subsequent cleanup PRs will be approved will little fanfare.

Focus. Separate. Pick thy battles.

January 2, 2018
845 words





ccoffing on GitHub

ccoffing on LinkedIn