Welcome back to the Deep Dive. Today, we're opening up the hood
on Code Review. It's a practice that's just
fundamental to modern software development, isn't it?
But you know, it often feels like this really intense, super
specific kind of conversation you have at work.
Oh absolutely. It's probably the single most
effective safety mechanism any software team has.
And we're not just talking like catching typos.
We're talking about the real, medium and long term health of a
systems code base. Maintainability.
Yeah. And it's not niche anymore, is
it? This is pretty much everywhere.
It really is. It's standard practice.
I mean, the data backs that up quite strongly.
Right, you mentioned some data, the Stack Overflow survey.
Yeah exactly. Back in 2019 they surveyed
something like over 70,000 developers and the results were
pretty start only about 23.6%, so less than 1/4 said they
didn't use code review in their day-to-day work.
Wow, so more than 3/4 of developers rely on this?
It's huge. It is.
It shows how critical this feedback loop is considered
across the industry. OK, that sets the stage
perfectly. So our mission today then is to
do a deep dive into the source material.
We looked at specifically a 14 code review by Marco, Tulio
Valente and Aline Torres. We want to pull out the the core
mechanics, the reasons behind it, the motivations, and maybe
most importantly, the practical best practices for both sides,
right reviewers and authors. That sounds like a plan.
Where should we start the basic process?
Yeah, let's start there. Yeah, fundamentally, the concept
seems simple enough. A developer writes some code and
before it gets merged into the main system, at least one other
developer, the reviewer, has to inspect it.
Right. And that inspection kicks off
this well, this established dialogue.
It's got a structure. It usually starts when the
reviewer adds comments to the code changes.
Those comments can be about anything.
Really. Pretty much.
They might be asking for clarification, like why did you
do it this way, or suggesting improvements may be a better way
to structure something. Or, you know, the big one,
identifying an actual bug. OK, so the review happens,
comments are made, then the interaction really starts.
The author gets that feedback. Exactly, and they have to
respond professionally. There are basically 2 paths.
Either they take the feedback on board, modify their code based
on the suggestions, or they reply and justify their original
approach. Maybe there's a good reason it
was done that way. It's a conversation.
A negotiation, Almost. Kind of.
And that back and forth continues until the reviewer is
satisfied they give their approval, and only then does the
code actually get integrated. And we see this happening all
the time in tools like GitHub, right?
With pull requests or PRS. Yeah, PRS are the common
mechanism, and our sources had a great real world example from
APR on the Microsoft Disc Code Project Big.
Project. Oh yeah, huge.
What does that example show? It really highlighted the, let's
say, the delicate balance you need.
In this conversation, the reviewer did something
interesting. They started with praise,
congratulated the author, said the solution was elegant.
That's important, isn't it? Starting positive.
Crucial builds trust, makes the critique easier to swallow.
Because then they switched gears.
OK, they pointed out several issues.
Very specifically, things like an internal design problem
exposing a method that shouldn't be public.
Technical stuff. Right, but also smaller things
like layout issues, colors, tab alignment, code hygiene.
OK, so big picture in small details.
Exactly, and then this was key when they found an actual bug.
They didn't just say this is broken, they attached a video
showing how to reproduce it. Oh nice.
That's helpful. Super helpful.
So you see that mix praise, specific critique covering
different levels and clear evidence for bugs.
It's it's serious technical feedback, but delivered
respectfully. OK, that nails down the how
pretty well, so let's move to the why.
Why do teams spend so much time on this?
I mean, finding bugs is the obvious one, right?
That's usually the first thing people think of.
Yeah. And it came out on top in that
big 2013 study by the Kelly and Bird.
Right, the Microsoft one. Nearly 900 developers and
testers surveyed. They asked them straight up,
what are your main motivations for doing code review?
And number one was finding bugs. No surprise.
There no surprise at all. It's the immediate, tangible
benefit, catching problems before they hit production.
But the study found other reasons too, didn't it?
Deeper ones. It did, and these are arguably
where the like the long term strategic value really comes in.
They identified three other big ones.
First, just improving the code overall, making it cleaner,
easier to maintain later. OK, makes sense.
Second, proposing alternative solutions.
Maybe there's a more efficient algorithm or design pattern that
fits better? Right.
Different perspectives. Exactly.
And 3rd and this one I think is huge is knowledge sharing.
OK, let's unpack that knowledge sharing.
How does that work in review? Well, it flows both ways.
You see, the author learns from the reviewer.
Maybe the reviewer has more experience or knows that part of
the system better. But the reviewer also learns
they have to understand the new code.
Maybe it touches a part of the system they weren't familiar
with, or uses a technique they haven't seen.
So it stops knowledge getting siloed like preventing those
knowledge islands people talk about.
Precisely. Those islands are so dangerous
in software projects. Yeah, where only one person
understands how something critical works.
And if that person leaves or is unavailable, you're stuck.
Major risk often called the bus factor, right?
How many people need to get hit by a bus before the project
stalls? Grim.
But yeah, code review directly fights that.
It forces knowledge to be shared.
Multiple people see the code, understand the logic.
It makes the whole team and the project more resilient.
OK, that makes the strategic value really clear.
Finding bugs is just the start. It's about quality, alternatives
and, crucially, spreading knowledge.
Exactly. All right, so now let's imagine
I'm the reviewer. I've got APR in front of me.
What specifically should I be looking for?
What's on the the checklist beyond just?
Does it work? It's a pretty comprehensive
checklist actually. You start with the basics, of
course. Bugs and functionality does it
do what it claims to do. But you need to go deeper.
Think about concurrency problems, race conditions,
deadlocks, things that might only show up under load.
Tricky stuff. Yeah, and error handling.
Are exceptions caught properly? Are error messages useful?
That sort of thing. OK, so functional correctness
and robustness. What about the codes structure,
The design? Hugely important for the long
term. You're looking for code that's,
well, overly complex. Code that's hard to read, hard
to understand. That's a major red flag for
future maintenance. Complexity kills.
It really does. Also, does it violate any
established design principles the team follows?
SOLID principles maybe? Or does it clash with the
overall system architecture? You need to protect that
architecture. OK, design principles,
architecture. What about efficiency?
Performance. Yeah, that's on the list too.
Is the author using fundamentally inefficient
algorithms or data structures Like scanning a huge list
repeatedly when a hash map would be instant, right?
Obvious performance bottlenecks. Obvious ones, yes, but you also
need to watch out for the opposite premature optimization.
Optimizing code that doesn't need it.
Exactly. Spending ages making some rarely
used function a tiny bit faster when that time could be spent on
features or fixing real bottlenecks.
It's a balance. Got it.
And then there's the the hygiene factors, the small stuff.
Yeah, code hygiene. This is where things like code
smells come in. Code smells for listeners.
That's not a literal bug, right? It's more like a warning sign.
Precisely. It's a symptom in the code that
suggests A deeper problem might exist in the design or
structure. Things like methods that are way
too long, or lots of duplicated code, or classes doing too many
different things. Okay, this category also covers
the basics like are they following the teams naming
conventions? Is the indentation and layout
consistent? Readability matters.
Absolutely. And finally, what about the sort
of safety net stuff? Tests, documentation, security.
Essential checks? Is there adequate test coverage,
or maybe no tests at all? Is complex logic properly
documented with comments and crucially, security?
Are there potential vulnerabilities, improper
handling of sensitive data, privacy issues, and even things
like are they using third party libraries or APIs correctly, or
are they pulling in some unauthorized library that could
cause licensing headaches or security risks later?
It's a lot. The reviewer is really acting as
a gatekeeper for overall system health.
That's a good way to put it, A holistic gatekeeper.
Which brings us to the human element.
Yeah, because reviewing based on that huge checklist, well, it
requires tact, doesn't it? If the feedback isn't delivered
well, the whole process can break down.
Completely. This is where soft skills become
just as important as technical skills.
Michaela Greylers work analyzing hundreds of GitHub comments gave
some great pointers here. Okay.
What were the key takeaways for reviewers?
First rule is probably restraint, especially with
alternatives. Only suggest a different way if
it's clearly objectively better, not just because you would have
written it differently. That's a tough one, resisting
the urge to impose personal style.
It is. You have to avoid purely
subjective comments, like arguing about variable names,
unless they're genuinely confusing or violate
conventions. Focus on substance.
And professionalism in tone is paramount, I assume.
Absolutely no offensive language, no sarcasm, no irony.
Keep it professional and keep the comments strictly about the
code. No personal stuff, obviously.
OK, so maintain professionalism. What about specific tactics to
make the feedback constructive? The sources mentioned framing
things as questions. Yes, that's a great technique.
It softens the critique immediately.
Instead of saying this variable is useless, you ask, hey, is
this variable actually used anywhere?
Maybe we can remove it? Right, opens a dialogue instead
of issuing a command. Exactly.
And another powerful one is using inclusive language we
instead of you. How does that help?
It reinforces that you're a team working together on the same
code. So instead of you should make
this private track. Could we make this attribute
private? Might encapsulate things better.
Small change, big difference in feel.
Yeah, I can see that more collaborative.
Definitely also back up your comments if you suggest a
change, especially a non obvious one.
Provide a reason linked to documentation, internal style
guides, external articles, justify.
It and that justification is even more important for junior
developers, right? Yeah, It becomes a teaching
moment. Absolutely don't just tell a
junior dev. Use a hash map here, not an
array list. Explain why for this kind of
data access, a hash map gives us much faster lookups when the
data set gets large. Teach the reasoning.
OK, justify suggestions. What else?
Positive feedback. Yes, don't forget praise.
If the code is well written or the tests are really thorough,
say so. A simple nice work on the clear
test setup made review much easier.
Goes a long way. It balances the critique.
Guilds Goodwill. Totally.
And finally, humility. As a reviewer, you will be
wrong. Sometimes you'll misunderstand
something. When the author corrects you
just acknowledge it gracefully. OK, I see what you mean now.
Thanks for explaining. No need to get defensive.
And what about disagreements when you just can't agree
through comments? That's when you might need a
synchronous meeting like a quick call, but that should be rare,
really reserved for significant disagreements, or when you're
just not converging on approval via comments.
Make it the exception. OK, that's a great set of
guidelines for the reviewer. Now let's flip the coin.
What about the author? The person receiving all the
scrutiny? What's the advice for them?
The first thing for the author is mindset.
Be professional, be polite, but crucially understand that code
review isn't like a test of your overall programming skill.
Right, it's about the specific code submitted.
Exactly. Don't take the feedback
personally. It's about making this piece of
code Better Together. OK, mindset is key.
What about practical tactics for the author to make the process
smoother? There's 1 golden rule, maybe the
most important piece of advice for authors who want good fast
reviews, which is submit small pool requests, small PRS, ah.
Keep the changes focused and limited in size.
Why is that so critical? Because reviewers are human.
The authors of Software Engineering at Google are really
clear on this. They recommend APR should
ideally be no more than say 200 lines of code. 200 lines that
sounds manageable to review properly.
It is. It lets the reviewer focus,
understand the context and actually find subtle issues.
When PRS get huge, quality just plummets.
Yeah, there's that great quote. Wasn't there something about
reviewing 20 lines versus 500? Oh yeah, the tweet cited in the
sources. It sums it up perfectly.
Ask a programmer to review 20 lines of code.
They'll find 7 issues. Ask them to review 500 lines and
they'll find 0 issues. Yeah, it feels true, though
reviewer fatigue is real. It absolutely is.
If you dump 1000 lines on someone, they're just going to
skim it. They might catch obvious stuff,
but the tricky bugs, the design flaws, they'll likely slip right
through. Small PRS force focus.
So small PRS mean better quality reviews, faster integration, and
ultimately less risk going into the main code base.
Couldn't have said it better myself.
And beyond size, authors also need to write clear PR
descriptions, explain what the change does and why it's needed,
give the reviewer context. And be prepared to justify
decisions if asked, just like they expect reviewers to justify
suggestions. Exactly.
It's a two way St. of professional justification and
explanation. OK.
So if we zoom back out, the big picture here is that code review
isn't just a gatekeeping step, it's this really vital
collaborative dialogue. It ensures the code itself is
robust, yes, but it also drives that crucial knowledge sharing,
keeps the team aligned, and ultimately keeps the whole
system healthy. But it relies on everyone
approaching it with professionalism intact.
That's it in a nutshell. It's really the engine room of
high quality sustainable software development.
Get it right and so many other things fall into place.
Get it wrong and you're storing up problems for the future well.
Put a critical process that needs care and attention from
everyone involved. Thank you for joining us for
this deep dive.