How I Review Code
Ok, I’ve had a few beers, and I’ve got my favorite record playing (2112 by Rush, of course) so now I can write about code reviews.
Maybe you’re like me in your general outlook on the world and you want people to like your work. Maybe you’re not…maybe you are some sort of pathological person that wants people to dislike your work…in which case, I can’t relate to you. But, if I had to guess, there are probably more with my pathology than yours.
So here we are, wanting to make our code better for everyone (especially the people that use it and that have to read it later) so how do we do it?
I don’t know.
Well…shit. Sorry about the blog post getting your hopes up, I don’t actually know…but I am keeping a running tally of what sucks less, so maybe it’s good reading nonetheless.
Here’s my checklist to keep in mind while doing a code review:
- Be Positive
- Ask Questions
- Think of Limits
and here’s my purpose in doing code reviews in the first place:
- Ship it!
- No New Bugs!
Ok, so let’s go backwards with this thing: why do code reviews in the first place?
Ship the Feature Stupid!
That was unnecessarily hostile, I agree, but the primary point of writing software is to make something (and sometimes that making is unmaking bad somethings we call bugs). So when you ship that code, you want it to actually be the real feature your Project Manager/TPM/Pointy-Hair wanted to get out there.
Code reviewing then is making sure that you actually did it…sure there is QA that will fiddle with bits on some interim database…but who better than your peers in the codebase to confirm that your plausible explanation for the bug is actually corrected by the patch. I think we all have an intrinsic awareness for a logical flow, so if we’re presented with a pull request that makes sense, we can affirm it’s working.
Speaking of logical flow, mine was just interrupted by a six month old micro human requiring assistance with sleeping.
So where was I? Oh yeah…changes to code need to deliver on their promise, or what’s the point? No brainer…moving on.
No New Bugs!
What’s the point of adding a cool new feature if you break the older, yet still awesome, feature? A good bit of bad email…that’s what!
We can do tons of things with our fancy automated test suites and all that, but there’s still the last line of defense that is the SUPER SMART HUMAN that can look at your code and see what you couldn’t! I mean, you’re around smart people, so why not hive-mind the problem and see around personal limitations and stuff? I think the strategy of reviewing limits helps with this one.
There are many things more annoying, but it’s still quite a bit of cognitive burden and aesthetic horror to find code written by another human that abides by a different sense of style than your own. Aside from whether it’s ok to wear white before Labor Day, it’s quite useful to have code that appears to be written by one mind, so agree on some sort of styleguide and then have that be a non-issue. Something not conforming? First check that everyone really is using the same styleguide, and then mention, “hey, looks like a few style things to clean up.” Remember, this delivers no value to the end user and marginal value to future you and current comrades, so let automation take care of most of it. Bikeshedding has no place in a code review, that’s for deciding on the styleguide (OMG, NO SEMICOLONS?! [email protected][email protected])
Checklist all The Things!
Ok, so…make sure it does what it says on the tin, that you didn’t break something, and that it feels like it belongs in the existing codebase…great! What else?
Like I said earlier, unless you have a different pathology than I do, you probably are pretty vulnerable when it comes to your “work of art” that you just slung between ping-pong breaks…so…I think it’s important to have an overall positive attitude when you approach reviewing someone else’s code.
Maybe they are having a hard day? Maybe they anguished over this thing and feel like it’s the best they could do that day? Bottom line, compliment the parts that truly are good. I’m not saying you should make up some bullshit, “the lighting was really good” compliment (that’s a “30 Rock” reference btw) but I am saying that if they are a member of the elite team you’re on, then they must have done something right in this code. Say nice things about that.
Ok, odds are they also totally did something evil in this code review. Well, you can’t be positive about that (it is pure evil after all), but the way you approach the evil can be quite positive.
“Instead of invoking
pure_evilin this method, could we look at something from the Standard Library instead? I’ve had good luck with
pure_good, want to try that?”
Nothing generates more sadness and stifles the creativity of our inner Mozart (or Salieri, can’t remember which) than saying, “this sucks, you shouldn’t have done this.” It ends the conversation which is the code! Instead, embark on a conversation to understand what goal was being attempted and be a wizened guide to help them reach that goal. Being a Debby Downer helps no one.
Think of Limits
Back when I was in Physics, there was a really easy way to cheat on stupid tests like the GRE. Ok, it wasn’t cheating, as all you were doing was using your brain, but it worked so well it might as well have been cheating: CHECK THE LIMITS!
What the hell am I talking about? In that context, if there was an equation given, what happened when it went to ZERO and what happened when it went to INFINITY…you can learn a lot about a function via those interesting points.
In code, what happens when a user gives
PATHOLOGICAL_INPUT_X, or when this thing
gets deleted (or its buddy model gets deleted)? Will this still work if that
other dependency happens first instead of last? You get the idea…test the
assumptions of the code base, go down the unhappy paths, and then be positive in
bringing them to light:
“Do we need to worry about when Y gets deleted?”
I 100% guarantee that for any significant feature, every single one of us would implement it differently…so I think code reviewing is a case of picking your battles. Absolutely every line could be written differently, if I, in my infinite wisdom, had been the enlightened hand behind it (that’s sarcasm folks) but we really need to ship software. We don’t have time to have two (or more) developers reimplementing the same features over and over…so with the code that’s written:
What are the fewest tweaks I can offer to make it the best that I can?
To do that, you have to spend time on it. You have to prioritize (see what I did there) doing code reviews as a vital part of the whole teamwork thing. I think it can be rad, so it’s worth it. I remember back to college where we had these symposiums where scientists would present their work and the conversation afterwards was always stimulating…I would love that to be the case with code reviewing.
I’m also a part of a team, so if I constantly just :+1: stuff without reading it and bugs sneak through OR I comment…on…every…little…thing…then I’m developing a reputation that may not help me.
I mean, we all want to be smart, we all want to be useful, and we all want to be respected by our peers (if not, see my comments at the beginning), so I think we have to take some long term views.
Oh yeah…and bonus one I didn’t list at the beginning…
Maybe I’m Wrong
I might be wrong. Correct that, I’ve been wrong constantly as I have grown in this practice of software development…maybe that thing I “don’t like” is perfectly fine and some nice person is going to stand on a stage with a bunch of pretty slides and make IT fashionable.
It’s going to be ok. Embrace the beginners mind and enjoy the ride…few things are better than learning from friends.