Hey, I'm Dave, Welcome to my pickup truck. Now the fact that I'm coming to
you live from the cab of my GMC pickup truck tells you two important things. First off,
it means that the information I'm about to relay to you is critically important, absolutely true,
and so secret that I had to sit in the cab of my pickup truck to even - talk to you about it!
And second, I am probably in my pickup truck to make a potentially quick getaway
if I am confronted by the authorities not wanting me to give out the information that
I am about to divulge to you. Because they do not want you to know the truth,
and that truth is that the world is divided into two camps. The people who correctly assert that
the code review is an absolutely essential part of any quality software development process,
versus the lamestream sheeple who foolishly claim that the code review has become an
outmoded waste of time. I will give you my completely objective opinion on which one of
those is dangerous and foolhardy talk. There are those who would denigrate our great
traditions like the code review all for the sake of staying home and streaming Ric and Morty during
their long compiles. Instead of the careful scrutiny of a skilled engineering coworker,
they have their code reviews performed by their 12 years daughter. Granted she did
find three bugs and so you took her Coldstone to buy her silence, but nonetheless, I am here
today to give you all the secret dirt on the virtues of the professional code
review. [Intro]
Hey, I'm Dave, Welcome to my shop! I'm Dave Plummer, retired software engineer from Microsoft
going back to the MS-DOS and Windows 95 days, and today, I'm going to make the case for the
code review and fill you in on precisely how we did them at Microsoft back in the golden days.
First, I'll give you the Top 5 reasons you should still be doing them today no matter how agile and
flexible you think you may be. Next I'll give you the anti-patterns, the Top 5 things you should
absolutely NOT be doing in your code reviews. And then stay tuned for a bit of late-breaking
info from the pickup truck at the end. Before I get underway, I'd like to ask you
to check your subscription status. Of course, if you're not already a subscriber I'd be honored if
you'd consider subscribing to my channel! But my concern is actually with people who thought
they were subscribed. There's something going on with YouTube where they're scrubbing subs, and
a number of people have reported to me that they got unsubscribed somehow and weren't sure how.
So please take a quick look at the button and make sure it says "SUBSCRIBED". If it's still
red and say subscribe, give it a click! Otherwise who knows what kind of gold you're missing!
Back to code reviews, a lot of things we did at Microsoft in the 80s and 90s would
be considered outdated at best by today's standards. For example, long before 'git'
or 'subversion' or 'source depot', we used a system called SLM, for Source Library Manager.
It was even released as a product called Microsoft Delta at one point. How slimy was it? Well, it was
a source code manager that didn't support branches or forks, so pretty slimy. I mean it was great for
its day, but it was really more of a wrapper over diff, and times have changed. Some are
even saying that about the code review itself. And yet, even with many of the processes and
procedures we followed back then likely being outmoded today, I believe the code review
itself was the single most important thing we did. And not simply because we did it,
but because we did it the right way. And today I'm going to tell you not only why the code
review is so important but also how to do it old school and yet updated for a post-Covid world.
By the way, the idea for doing a code review episode was suggested originally long ago in
a video comment by Ben Bradbury, so thanks to Ben for the idea! And thanks to David Sobeski,
a former exec at Microsoft who prodded me into finally getting around to making it!
It was in talking to David that I learned that many companies, and even groups within Microsoft
today, have eschewed the code review for what they perceive as faster, more agile development.
Bugs will be found by the beta users, I guess. He argues, and I agree, that this is misguided - that
the purpose and point of the code review has evolved and changed over the years. Today his
focus is less on scrutinizing individual lines of code and more on assessing the performance,
efficiency, and identifying issues like cross-team and cross-group dependencies.
Let's take a step back and look at the bigger picture: what exactly is a code
review and why should you be doing them? Put most simply, a code review is a peer
review of any changes that you plan to make to the source code tree. You get someone of
comparable skill and knowledge, and you sit down with them - at one time in person, but
more likely virtually today - and your peer reviews your code literally line by line.
Not skimming page by page, but scrutinizing line by line. In the olden days a reviewer might go
so far as to track register contents as they went, but today the purpose is to track key state as you
mentally walk that state through the code changes. The reviewer is acting as a super-compiler and a
processor at the same time - interpreting and working through the code at a level of detail
sufficient to allow them to track the application state. Branches must be both taken and not taken,
and error paths and edge cases inspected carefully. The level of detail should NOT
depend on the volume of code to be gone through. It's been established that the proper speed for a
code review is about 200-400 lines of code per hour. That's about 6 pages per hour,
or 10 minutes per page of code. That feels about right to me for actual programming
logic. If you're reviewing safety-critical embedded software you might be much slower,
and if you are skimming user interface metadata you may be much faster, but it's been shown
that that going much faster than required may simply be too fast to find many serious errors.
You want your code reviewer to be a devil's advocate of sort, but not an actual adversary.
I liked to have my reviews done by someone with deep knowledge in the area I was touching,
and ideally it would be someone smarter than me or senior to me so
that I could learn something at the same time. Now a lot of the time it was a guy named Bob who
was what I like to call "the smartest guy down the hall", and that's who I liked to have my
code reviews done by. Bob Day, Raymond Chen, Brian Aust. Guys like that would never allow themselves
to look past anything, and in return you'd better have your code in good order before you get
there. But if you do, then you're in for a wildly educational ride. A good code review could be like
a college tutoring session from your technical heroes, and next to watching them debug code live,
a code review was the best way to learn. Under the leadership of new CEO Satya Nadella,
Microsoft has become a much kinder and gentler place, even for those who've stayed with it
over the long haul. And since it's also wildly profitable, no one is complaining,
because if you can achieve the same results without the brutality, all the better!
In the old days there was an overarching fear that you might do something stupid like break
the build. It's so easy to do... a header file gets picked up from a different include path on
your machine and hence it compiles for you but not the build lab, for example. Instant build break.
The chief architect of Windows NT was Dave Cutler. Cutler is one of those guys who, let's just say,
doesn't suffer fools gladly. And it was said that if you broke the build at an inopportune time,
DaveC would walk through your wall. Dave was our looked like a 1960s astronaut and had the compact,
muscular frame of one too, so maybe he did take a lot of drywall shortcuts, I don't
rightly know. But one time we were preparing to do a code review for a new hire named Shaun
that worked for me, and so Shaun started a code sync right before we left for lunch.
A sync just pulls the newest source down from the server to merge it with your local changes.
What Shaun didn't know - nor did I, yet - was that a sync could actually obtain a lock on
the server in some cases. When his sync then hit a prompt and needed a yes/no decision,
it just sat there holding a lock over the entire NT source tree. No one else could do a thing with
it. For the entire hour. And DaveC didn't take a lunch break that day. When we got back from ours,
we could hear a phone ringing the entire way down the hall. It was Shaun's and it could have
been ringing for several minutes for all we knew. When we finally got to his office, we all saw the
jammed sync operation on the console and DaveC's names on the phone's callerID. Shaun looked at me,
a little terrified. DaveC was infamous even to the new guys. We told him he had to answer it. When he
did, Dave's voice came roaring over the handset: RELEASE YOUR F'ING LOCK!
OK, but back to the topic itself - Code Reviews. What's the purpose of a code review,
and why should your team be doing them? I'll give you the Top 5 reasons.
Number 1 Find Defects
The additional scrutiny of a fellow developer is ultimately intended to find any remaining
bugs in your code before you check them into the main source tree. It's just way easier and more
efficient to find bugs in a quick inspection pass than it is to rely on an after-the-fact
test apparatus to find, debug, assign, correct and retest bugs after they are introduced.
Defects come in multiple forms, however. Not all are as simple as an off by one error walking an
array. You want to find not only problems with correctness, but also with performance, security,
backwards compatibility, and so on. Having someone with a different mental toolbox review your code
can go a long way because they're bringing a certain diversity of experience to the problem.
Security is an example where code can look entirely fine and be technically correct but
seriously broken at the same time. Any code that runs on the machine perimeter or with
elevation must be carefully inspected by someone who's familiar with the issues surrounding it.
This is all the more important if you're weak in a particular area, like security,
or graphics, or parsing, or whatever. Find the local expert and let them do
your review. I know the instinct is not to expose yourself to that kind of scrutiny,
but its' good for the soul. And the codebase. Beyond Simple Defects is Number 2, Code Quality
What's the difference? Bugs and defects are cases where the code doesn't do what it's supposed to
do. But even when performing correctly, there are many other dimensions to code quality,
such as maintainability, readability, uniformity, understandability, and so on.
Your team should have a style guide, and everyone should follow it. For things that
you do not have a style guide for, you should follow the precedent of any existing code.
If someone wants to change that precedent, they should get buyoff from the team, fix all existing
instances, and then proceed. You certainly don't want to mix and match styles within a project.
All that said, the last thing you want a code review to devolve into is a religious war over
tabs and spaces, bracing style, argument casing, and so on. All those things should
have a right and a wrong answer and everyone should agree and follow them. The code to be
reviewed should have all of those issues addressed long before the code review.
Next we have Number 3, Knowledge Transfer I learned more in code reviews at Microsoft than
practically anywhere else. Remember, at the time there was no web to speak of, so no stack overflow
or github or any of that. It's actually hard to believe that I got a degree in computer science
before you could search the web. You either read it in a book, figured it out yourself, or you saw
it done in someone else's code. Those were really your three vectors for new information. Well,
there was gopher and archie, but... But beyond just learning in general,
there are several specific types of knowledge transfer that are afforded by the code review.
First and foremost is that on a larger project it enables new developers to learn the codebase.
Assuming everyone writes the same amount of code, you'll be familiar with twice as much
if you review someone else's at the same rate. One of the things that surprised me the most
when I first started on the MS-DOS team was that there were no shared libraries of utility code.
If you couldn't do it through an existing Int 21 into the MS-DOS dispatch table,
then it didn't exist. If you wanted to do something as fundamental as just to output a
decimal number, you had to write your own code to convert it. I could only imagine how many
times stuff like that had been duplicated over time, and how often subtle bugs were introduced.
If you do have code libraries that your team uses, however, seeing other people use them
effectively is a great way to learn them, and sometimes, a code review around consuming a
library is like getting a personal tutorial on the subject. If a picture is worth a thousand words,
then an example can be worth a million. Knowledge transfer also extends to common
solutions that your team uses to learn the standards that are hopefully set out in your
style guide. But there are a lot of undocumented style points that might simply convention
like error handling and how it's done in your particular project. Seeing it in action is likely
a lot faster than learning from a document. At number 4, we have Mutual Responsibility
There is a lot to be said for collective code ownership and in treating your codebase as a
precious ecology that must be preserved and protected. Just as you wouldn't pour a bucket
of motor oil into a local stream, nor would you introduce a similar digital detritus into your
codebase. People tend to look after things that are theirs. When people feel like they
have ownership and authorship of the code, I believe they tend to look after it better.
And the code review is the perfect opportunity for everyone to play forest
ranger and limit what is allowed to come into your codebase and what is allowed to stay.
I've never worked at a place where I didn't have a sense of ownership, but if it were the case,
I can imagine there could be a sense of washing your hands of responsibility once your code
was checked into the main tree, as if it were now someone else's problem. To help combat this, being
the code reviewer for a piece of code means you were the gatekeeper, and it was you who let it in.
As the responsible party, you are the one who owns its ultimate success or failure in the
product as much as the code's author. At Microsoft, once a check-in was made,
we'd send sync and build instructions to the NTDev alias and it would be signed off
by both the author of the code and the code reviewer. Often times I took greater solace
in a change based more on who reviewed the code than who wrote it in the first place.
I always felt like the Windows code I wrote was my own. I mean it was clearly 100% the
property of Microsoft Corporation, I get that. But I'm fortunate to have worked on a number
of components where my code has lived on for 25 years and will likely exist for as long as Windows
compatibility remains a thing. I'm proud of that and I only feel that way because of the sense
of ownership that I had over the code, especially of the components that I was directly responsible
for. In fact, we never said "Who's responsible for X", we'd always asked "Who owns it?".
It's a minor thing, but I think code reviews can also help build a sense of solidarity in
a project. By conforming to shared standards and practices, and by giving everyone a role
in maintaining and enforcing them you give everyone a direct and essential role in the
product's success. It's as close to wearing a uniform as most developers will ever get.
And finally, Item #5: MISCELLANEOUS
Here in this final bucket I'll simply place all the other benefits of code reviews,
of which there are many, that don't belong in the previous categories. Things like generating
new ideas and patents, or for generating ideas for new products or solutions that transcend
the specific code being reviewed. In a way, it gives you the best of the "co-programming"
paradigm where two programmers are assigned to work together over each other's shoulders.
I guess in the formal version, one holds up the printout from Stack Overflow and the other one
types it in! Ha! Thanks, I'm here all week. In reality, I think the code review allows
you to reap many of the collaborative and quality benefits of co-programming when it is most useful,
and then people can go back to working independently and each can write code
productively on their own until it's time to collaborate on its finalization again,
where more eyes are better. You might also have goals specific to
your company, such as conforming to ISO or other standards and industrial management systems.
The code review might be the best, if not only, place to perform some of those functions.
OK, with the benefits and best practices nicely summarized we should look at some
of the anti-patterns, the Top 5 things you absolutely should not be doing in a code review.
Number 1, no nit-picking This requires cooperation on both sides.
The person writing code must accept that there is a style guide, that personal expression is
best left to performance artists, and that the issues surrounding tabs, spaces, braces,
and capitalization have long since been worked out collectively as a team. They need to own
bringing their code up to snuff with respect to that style guide. That needs to happen before the
code review, and if there is a ton of stuff that needs to be fixed and cannot be overlooked far
enough to continue with the actual code review, it should be tabled until the style issues are
resolved. The code review isn't the appropriate venue or opportunity to debate the style guide.
Number 2, Ping Pong The last thing you want is an
iterative code review that you need to keep coming back for. A code review should be a single pass,
or two at most if there are actual issues that you need to take back to the drawing board for
revision... but if you find yourself in a situation where the reviewer sends the
same code back several times, something needs to be done soon, as it's incredibly
demotivating process and indicates a lack of something, be it communication or acceptance.
But if it takes multiple passes to get it right, then someone isn't paying attention!
For an excellent story of ping pong in action at Microsoft, check out my book in the video
description, which contains a notoriously frustrating example from my own career.
Number 3, Inconsistency While no two reviewers are ever
going to be the same, they should be as similar as possible. If one code reviewer focuses on
performance to the exclusion of all else while another is concerned only with robustness,
then you're not getting good code review coverage from either of them. This can happen when your
team does not have established best practices for everyone to shoot for. The more crisply defined
those standards and practices are, the more consistency you can achieve between reviews.
You would never want to do it literally, but every code review comment or change request
should stem from a material breach of those policies. They can't be too general, like "don't
check bugs in", but they can't be so specific that no one reads the thing. It's a delicate
balance and perhaps best pursued as if you were writing a tutorial and not a legal document.
Number 4, Design changes at Review Time We all know this one. Some smartass waits until
you've written the actual code to tell you how you should have designed it the first time but didn't.
It doesn't matter if they're wrong or right about the premise because a code
review is neither the right time nor the right place to be designing the product.
Likely that should have taken place days, weeks, or even longer ago.
Waiting until code review to voice design concerns is counter-productive for everyone involved.
What if, however, the reviewer is right about it? Well, all you can do is look hard at the
way you're doing things that allows bad design to make it that far into production. Something
needs to change upstream in the process to make sure that bad design isn't still around to be
caught that late in the process! And finally, Number 5, Ghosting
Have you ever needed a code review, asked or assigned it to someone, only to be continually
pushed back on when you need the results? The reviewer is too busy, needs a bigger block of
open time, as they don't have the time to "look at it properly" or "give it the time
it deserves". That's BS. If code reviews are to be an integral part of your development process,
then they must be highly prioritized. Higher often than doing your own work,
because successfully completely a review unblocks two developers rather than just yourself.
To help avoid ghosting, keep code reviews small, a couple of pages at most ideally.
Throwing your fat code manifesto over the wall and expecting a quick answer isn't fair, but
both teams and individuals must allocate enough time for code reviews; they must be seen as time
well spent, and not an intrusion or interruption. There you have it, the Top 5 reasons to do a code
review and the Top 5 things to avoid while doing them.
If you have any interest in matters related to ASD or Asperger's, or just want more stories from my
Microsoft days, please stop by Amazon to review the free sample chapter of my book, Secrets of
the Autistic Millionaire. The eBook is on sale for under ten dollars so please check it out!
If you enjoyed this episode, please remember that I'm mostly in this just for the subs and likes,
so please be sure to leave me one of each before you go.
In the meantime and in-between time, I hope to see you next time, right here in Dave's Garage.