
How to do a code review - iamaelephant
https://google.github.io/eng-practices/review/reviewer/
======
petilon
Here's the part that resonated with me most:

    
    
      A particular type of complexity is over-engineering, where developers have made the code more generic than 
      it needs to be, or added functionality that isn’t presently needed by the system. Reviewers should be 
      especially vigilant about over-engineering. Encourage developers to solve the problem they know needs to be 
      solved now, not the problem that the developer speculates might need to be solved in the future. The future 
      problem should be solved once it arrives and you can see its actual shape and requirements in the physical 
      universe.
    

Note how Google does NOT say "make sure the code is properly architected".
Instead they say "make sure the code is not over-engineered"! At top companies
like Google, projects rarely fail because there isn't enough architecture.
Instead projects end up costing 10x to 30x because of unnecessary complexity.
Over-engineering is a trap that very good developers fall into all too often.
I am glad to see Google has cautioned against this, because I can now point my
fellow developers to this when they are about to fall into the same trap!

~~~
derekp7
It sounds like walking a tight line between over engineering, and falling into
technical debt. If you design code specifically solves the immediate need,
that may need to be thrown away or extensively worked on / around when future
needs come up. On the other hand, you can write code that solves future needs
that never appear, and still fail to solve the actual needs that end up
appearing.

For me, I would rather put a bit of additional effort up front gathering
enough information about the direction of the project or usage of it to better
formulate an attack plan so that it does actually become more future proof.
But even that is subject to failure, so what do you do?

~~~
pjm331
I think of it this way: How can I build this so that it only solves today’s
problems but doesn’t make it overly difficult to solve tomorrow’s problems?

Loose coupling, dependency injection, composition over inheritance, and
similar techniques tend to be good answers to this question in my experience.

In contrast, over engineering attempts to solve tomorrow’s problems before
they arrive and, if they arrive differently than predicted, makes them harder
to solve, because when you have to change something it’s less clear which
parts of the design were necessary to solve the original problem and which
were only necessary for the future problem that was incorrectly predicted.
Often times you might end up having to rethink the entire architecture rather
than having the relatively simple problem of adjusting things to meet new
requirements.

~~~
ScottBurson
Very well said!

My own experience is that effort invested in removing restrictions and
handling corner cases is generally well spent. It may not seem too onerous to
have to remember that a particular function works only for nonempty inputs or
when called after some other function, but in a large system where many
operations have such restrictions, keeping track of them quickly overwhelms
the capacity of human memory. Sooner or later someone is bound to forget, and
it may even be the author of the code in question. I try to ask "what are
people going to expect this code to do?" and then, if within reason, to make
it do that (and failing that, to protect it with assertions). Alternatively,
someone may be forced to make some other part of the system more complicated
to work around the restriction, leading to excessive coupling.

I suppose this practice sometimes risks over-engineering, but I have found the
risk to be worth taking. As you say, it makes the system easier to extend
further.

~~~
pjm331
Yeah absolutely. I think a big part of it is considering possible edge cases
or failure scenarios and not necessarily solving them immediately but
considering how your design might need to be changed in order to solve them.
Many times I have found there’s a solution that requires little or no more
work than a naive implementation but is far more robust to future changes.

To use inheritance as an example - if there’s a conceivable possibility that
some future requirement might lead you to add the same functionality to other
classes, you probably don’t want to have to deal with a long and convoluted
inheritance chain when you could compose some set of functionality onto one
class as easily as many classes.

Or in the case of dependencies, wrapping some third party mail sending library
in your own generic API is barely more work than using the third party library
directly and will pay dividends if you change mail providers in the future.

One very specific example that I worked on recently: In most cases there are
one of X, but in a handful of cases there are many of X. My initial
inclination was to branch off and handle the many case as an exception, but
then I realized that you could eliminate that corner case entirely if you
handled everything as a collection of X, even if in most cases that collection
only has one item in it.

~~~
papito
That's a pattern I learned a long time ago. It's much more future-proof to
treat items as an array, even if it seems over-engineering at first, because
scope-creep dictates it's more likely you are going to handle more items in
the future, not fewer. And dealing with one item vs. many can change your
architecture pretty drastically.

------
aviraldg
This is great advice and isn't followed often enough, especially when
reviewing code written by people new to an organization/team:

> If you see something nice in the CL, tell the developer, especially when
> they addressed one of your comments in a great way. Code reviews often just
> focus on mistakes, but they should offer encouragement and appreciation for
> good practices, as well. It’s sometimes even more valuable, in terms of
> mentoring, to tell a developer what they did right than to tell them what
> they did wrong.

I've seen cases where people got hundreds of comments (many of them minor,
nitpicky) from more experienced developers and were discouraged by the sheer
number of them. That most new developers naturally suffer from imposter
syndrome is not helped at all by 100% critical code reviews.

~~~
mieseratte
Why is it that people feel so discouraged by loads of review, especially early
on? I always had a good bit of imposter syndrome early on, but never
considered quitting. I always assumed that you have a lot to learn, that it’s
expected you’re going to suck at some level.

~~~
compiler-guy
People have their own internal stories about how good they are, and all that
feedback can be very painful.

It doesn't help that many comments are nitpicks and pedantry, made by people
without much social empathy.

This is especially true when sending a patch for a high-level review of your
proof of concept, and the next thing you know people are complaining about
your formatting.

~~~
Arnavion
It can be hard to review badly formatted code. The reviewer has to put in
effort to understand your change, but they have to put in more effort to parse
the change if the formatting is bad.

"Why is this function's return value not being checked? Oh, turns out this
person used a yoda conditional even though the rest of the code doesn't do
that."

"Why is this block of code running even when the condition is false? Oh, turns
out the else branch already ended and this part of the code is just indented
wrong."

And so on. Even something as minor as `foo ()` vs `foo()` can stand out and
act as a constant stream of mental speedbumps.

Claiming your change is high-level / only looking for feedback to the overall
design doesn't change that. You're asking for a code review because you want
the reviewer to read your code, but reading it is exactly the part that
they're finding hard to do.

And it's not excusable, but the reviewer might be insulted that the reviewee
is wasting their time and their feedback may be ruder / snappier as a result.
After all, the reviewee could easily have put in the effort to run the auto-
formatter, follow the existing code's style, etc.

~~~
the_arun
The formatting & style could be automated via IDE configurations, so that when
the code is submitted for review it is already clean. This will make reviewer
to focus on the design/logic instead of formatting.

~~~
compiler-guy
Autoformatters fix many things, but not all of these issues.

------
bt848
I'd add two things, from a decade of experience at Google:

Review code in this order: protocol buffers, unit tests, headers,
implementation. It's common for a new employee to be an expert on C++ or Java
or whatever languages but it's very uncommon to meet anyone who knows how to
define a decent protocol message. The tests should give the reviewer a very
clear idea of what's happening in the code (and this should be congruent with
the description of the change), if not send back to author at this point. The
headers should contain clear interfaces, types, and comments and should not
contain anything that's not part of the API (when this is technically
possible). Finally look in the CC file; at this point the reviewer should see
things they were already expecting and no funny business.

Any claims about speed, efficiency, or performance whether in comments or
during the review must have accompanying microbenchmarks or they should be
deleted. Wrong ideas about software performance abound, and even correct
beliefs become incorrect with time, in which case the microbenchmarks are
critical to evaluating the continued value of the code.

When sending a changelist for review, always clear all automated warnings or
errors before wasting the reviewers' time. Nobody wants to see code that
doesn't build, breaks a bunch of tests, doesn't lint, etc.

~~~
polishTar
> Any claims about speed, efficiency, or performance whether in comments or
> during the review _must_ have accompanying microbenchmarks or they should be
> deleted.

That's way too strict. If I want to suggest moving a statement that's inside a
loop to the outside, I don't want to waste my time writing microbenchmarks
showing that it improves performance.

If the suggestion is complicated or non-obvious, and it's in a really critical
part of the code, then sure, write a benchmark. Outside of that though, it's
rarely a good use of developer time.

~~~
molodec
I agree that "must" is too strict. If I replace linear search with binary
search, or hashmap/hashset lookup I don't need to write a benchmark to prove
it improves performance. There is math, logic, Big O analysis that allows to
reason about performance and speed without microbenchmarks.

~~~
BeetleB
bt848 already said it, but you picked a really poor example. In many real
world scenarios, a linear search beats a binary search due to a lack of
overhead.

Along those lines, in C++, a vector very often performs better than a
theoretically better data structure. I learned C++ fairly well from a very
experienced guy in the company, and he said that you should always benchmark
against a vector. I suspect if I was in his team and he were reviewing my
code, he wouldn't let the code pass unless I had benchmarks that show whatever
data set I picked is faster than a vector.

He wasn't against other data structures - he just wanted proof they would
perform better. In quite a few cases, they didn't.

~~~
molodec
Yes, the size of the data matters, and linear search may be the right choice
for many use cases and may outperform more advanced data structures. You learn
it on the job as it is not a topic usually studied in universities. But I
don't agree that we need to completely throw away theory, O complexity
analysis where N - input size is the main variable. Microbenchmarks are hard
to write correctly. Recreating the scenarios occurring in production is not
always possible in the microbenchmark settings. You can write a microbenchmark
that shows your code works great, but fails to perform in production because
the input data is totally different. bt848 mentioned load test, which is not
the same as microbenchmark. Load test that simulates production settings is a
better tool to validate the optimizations.

~~~
BeetleB
>But I don't agree that we need to completely throw away theory, O complexity
analysis where N - input size is the main variable.

No one here is agreeing with that.

------
mattsfrey
Would help if they mention what a "CL" is somewhere before using it
ubiquitously

~~~
jdm2212
It refers to "changelist". Google uses a Perforce-like VCS internally, and
they kept the Perforce terminology:
[https://www.perforce.com/perforce/doc.051/manuals/p4guide/07...](https://www.perforce.com/perforce/doc.051/manuals/p4guide/07_changelists.html)

~~~
jonny_eh
aka "Pull Request" in Github parlance.

~~~
hota_mazi
aka "Merge Request" in Gitlab vernacular (which makes more sense than "pull
request" in my opinion).

~~~
aledalgrande
aka "patch" for the Linux kernel development community

~~~
xixixao
aka "diff" or "revision" at FB .)

~~~
o10449366
aka "CR" or "Change Request" at Amazon

------
ridaj
It's very interesting that so much of it is about the social aspects of
engineering - a lot of it kind of reads like " _don 't be a jerk_". In CS
engineering classes, where presumably we would learn to become great
engineers, I don't recall learning about any of this, and instead I remember
the emphasis being on technical knowledge and accomplishment. I'd probably
have been a better engineer in my early career if I'd understood how much the
ability to exchange clear, constructive, and nonthreatening critique with
peers actually mattered. It's reminiscent of how training in technical fields
such as architecture and medicine often put the emphasis on technique over the
service aspects of the job such as client interactions and bedside manner.

~~~
pjm331
Code review is a social activity and when you frame it as such it’s not so
surprising that a lot of the discussion of it involves these social aspects.

I definitely agree that my experience was the same w/ regards to an almost
exclusive focus on the technical aspects of programming. I only had one CS
project in my senior year that involved building something with another person
and even in that case it was only one other person.

I don’t think this problem is simple to solve or specific to programming. On
the one hand, educators have to asses individuals so it makes sense that
they’d want to isolate work to the individual. On the other hand, work after
education is never (I hesitate to generalize but this seems like a safe
generalization) an individual activity, and the rare dreaded group project in
school is the norm afterwards.

I am not sure what the solution to this problem is. Even in a situation where
you have version control and can look at each individuals contribution to the
code itself, unless 100% of the discussion around that code happens in
comments on a platform like github, it’s still difficult to assess things like
how much the input of one individual contributes to the output of another.

In addition to this, teaching effective approaches to technical problems is
easy compared to teaching effective approaches to human interactions.

~~~
ridaj
> I only had one CS project in my senior year that involved building something
> with another person and even in that case it was only one other person.

I had many group projects in school, but they didn't set it up so that we'd
review each other's code. We'd just break it up into modules, split the work
accordingly, and each worked on a different part.

A more fundamental limitation of school projects is that they have by
definition a limited lifespan, so that the notion of code debt, the importance
of ensuring that code be readable by current and future colleagues, etc. are
irrelevant - in all the projects I've done in school, even group ones, I was
the only one reading my own code and it would never be run after class
finished. In such a situation, "improve overall code health" makes no sense.
You write only what you need to pass the class, which is usually some kind of
demo feature.

Maybe making contributions to well-established open source projects should be
encouraged as part of the curriculum, at least to experience the receiving end
of it...

------
stackzero
>In general, reviewers should favor approving a CL once it is in a state where
it definitely improves the overall code health of the system being worked on,
even if the CL isn’t perfect.

This is a great rule of thumb

~~~
roryokane
It’s a nice rule of thumb for CLs (changelists) that provide a bug fix or
refactoring. But I think it’s a terribly useless rule of thumb when evaluating
a CL that just implements a new feature.

Imagine some new features such as “support exporting to CSV” or “auto-generate
avatars for new users”. Implementing them will necessarily add more code, and
more complexity, to the codebase. And adding more complexity makes the code
harder to work with – in other words, harms “the overall code health of the
system”. Businesses develop new features because of the _improvements to the
product_ they bring and the reputational or monetary gains that come from
that, _in spite_ of the damage done to the codebase. The only way you could
get a new-feature CL to pass by the standard you quoted (the standard of
“improving the overall code health of the system being worked on”) is to
refactor existing messy code written by someone else, and that would not be
sustainable.

Does anyone see a way to interpret the rule, or have a preferred variation of
the rule, that makes it useful for evaluating new-feature CLs?

------
aledalgrande
In the "What Is Not An Emergency?" section:

> It is the end of the day on a Friday and it would just be great to get this
> CL in before the developer leaves for the weekend.

I laughed out loud because it reminded me of so many times I have seen it
happen and then someone had to fix in the weekend.

Who shares the same experience?

~~~
Glyptodon
Some places I've worked explicitly won't push/deploy/similar on Friday.

~~~
joefkelley
I worked at one (horrible) place where we explicitly _only_ deployed on Friday
afternoon.

The logic was we have fewer users over the weekend, so if something goes
wrong, fewer people will notice.

~~~
devnulloverflow
It depends on what company you are.

Some who administers some SAP thing for a huge supermarket chain tells me they
do all of their upgrades on weekends because their users aren't at work.

Makes sense for them, but it would be a crazy thing to do the consumer-focused
interweb company that I work for.

------
siempreb
What is good design, what is good naming, over-engineering, etc, etc.. How on
earth can a group of people agree on all those points? Look around here on HN,
all those discussions.. Do we have consensus? No, and that is OK, but it's not
in a team by code review.

There is only one way a group of people can have healty code reviews, that is
when they like each other so much, or enough to accept what they not agree on.
But man, if one or more dev's are not you're best friend, you have a problem
there. Because it is not who has the best code or ideas, it's who fits best in
the group, not?

But I totally agree code that comes into a code-base needs to be checked. And
IMAO only by 1 developer, the LEAD who is responsible for the code-base. No
democracy, no endless discussions, no 6 people spending 1 hour a day to do
this.

Personally I prefer to do only gigs where there is a lead dev. I try to stay
away from the popular self managing teams. I've seen enough horror, where a
good developer had to leave the team because they were constantly frustrated,
endless discussions about futilities, and in the end it all became personal..
Is it only me who sees this?

~~~
0x445442
I agree with you. This "Agile" cancer that has infected the industry needs to
die now. Someone here called it collectivized micromanagement and that's the
best term I have heard for what's become of "Agile".

~~~
erikpukinskis
I don’t think you should accept bastardizations of terms into your lexicon.

Yes, acknowledge when someone means “micromanaging” when they say “agile”, and
act accordingly. But don’t redefine the word in your own head, otherwise how
can you even speak?

Like, what word do you use now for the basic principle of agile development
now that you’ve changed it to mean micromanagement in your head?

~~~
97b683f8
I'd say Agile development is about "agorisation" of the development process.
"Agorisation" is the act of making a place (virtual or not) amenable to
proposing, discussing, and setting up and in motion _categories_ that will
enhance this development process in both qualitative and quantitative aspects,
such that agents who collaborate into it can live through this process like a
citizen through the city.

\----------------------------------

category (n.)

1580s, in Aristotle's logic, "a highest notion," from Middle French catégorie,
from Late Latin categoria, from Greek kategoria "accusation, prediction,
category," verbal noun from kategorein "to speak against; to accuse, assert,
predicate," from kata "down to" (or perhaps "against;" see cata-) + agoreuein
"to harangue, to declaim (in the assembly)," from agora "public assembly"
(from PIE root *ger- "to gather").

------
NKCSS
Here's my workflow and it works very good to get everyone on the same level:

\- Each night, I go over all the commits of the day and do a code review \-
Each morning at 9:00, we go over all the comments on the commints, with
everyone, talk about it and make sure everyone understands.

This allows me to explain more advanced or new concepts that one developer
uses to make sure everyone understands. It allows me to introduce improvements
and explain why they are beneficial, and it allows me to signal when people
don't follow the standards we impose; giving not only the person who did it
wrong a refresher, but it makes sure everyone is reminded of the way we should
write code, etc.

The feedback has been amazing and everyone loves it because it helps everyone
grow and they feel like they are allowed to focus on quality; due to time
pressures, coders tend to let standards slip when under pressure. When you
focus on quality every day, it stays with them, because they know, if they
don't deliver quality, it will be in the daily review. So far, this has not
hurt production speed at all and quality has gone up a lot since we started
doing this.

~~~
infinitezest
This is an interesting approach. How many engineer hours do you think you
spend each day doing this? If you're reviewing CLs as a group, it seems like
it could get very expensive very quickly, no?

~~~
NKCSS
They last anywhere from 15 minutes, to 90 minutes, depending on the topics and
it replaces a lot of other meetings/sharing that normally takes place.

------
shakezula
> if a reviewer makes it very difficult for any change to go in, then
> developers are disincentivized to make improvements in the future.

This is a problem I see far too often but it’s rarely talked about. Too often,
engineers misinterpret “quality” code for “their” code. Code review turns from
“what should we be doing here?” Into “what would this reviewing engineer name
this one unimportant variable here?”

There needs to be a happy medium between velocity and quality, and increasing
velocity doesn’t necessarily mean decreasing quality.

------
Traubenfuchs
I have never not seen code reviews end up being a massive social/cultural pain
point for most people involved. Most people hate getting their code reviewed
in depth. In situations where there isn't just one lead dev who is responsible
for reviews, this leads to "Merge Request symbiosis", where two people
uncritically approve each others requests so they don't have to deal with the
third.

~~~
meitham
I agree, my role of thumb is to only review code if I was asked by the author
to do so, and never offer to review someone's else code.

Code is a bit like music, there is very little right or wrong, but a lot of
flavours and opinions.

~~~
Traubenfuchs
Very little wrong? You haven't seen enough code! I have seen everything in
merge requests: From liquibase scripts that would have wiped the production
database to production secrets in yml files.

------
esolyt
Helpful and interesting.

However, you can tell this was written by an engineer. The acronym CL is used
about a hundred times. There is not a single page where the first usage of the
acronym explains the full term.

~~~
Deradon
It is explained at the top-level document: [https://google.github.io/eng-
practices/#terminology](https://google.github.io/eng-practices/#terminology)

------
awinter-py
in real companies that don't have G's resources, quality and velocity are
tradeoffs, not things you can have both of

god bless them for trying though

in particular, the idea of having one or more reviewers doing multiple rounds
in a day is pure fiction in strapped startups. Rejecting a code review for
readability isn't always an option. Even getting a timely review from someone
who understands this part of the codebase can be unrealistic -- sometimes
there is no such person. Sometimes nobody is left in the company who
understands the affected module and even the PR author is taking confession
before merging it.

Love the phrase 'unless it's an emergency'. In resource-constrained cos
_everything_ is an emergency because to get any non-emergency work done
requires emergency level urgency.

G's way of life requires an even power balance between eng & other silos. And
also requires a baseline level of technical involvement by PMs which isn't
present outside big tech cos. Most companies just aren't hiring PMs with a
programming background. They could never justify the spend.

Major missing section in this is 'what _shouldn 't_ you review' \-- various
kinds of nits and out-of-scope changes that derail the process, waste
expensive CI cycles, and make people take the rest of the day off from
frustration.

~~~
Cthulhu_
And yet. Technical debt costs you time and money over time. Fixing code six
months down the line takes 5-10x as much effort than just fixing it while it's
still fresh. Worst case scenarios is where you end up throwing away an
application and rebuild it because the technical debt has made progress grind
to a halt.

If there's nobody left who understands the code, or if there's hero coders who
'own' one section of the application, it's a huge risk to the company - which
can, and has, bring a whole company down (there's a few stories on the daily
wtf iirc).

A recurring thought with me and my projects after about six months is always
"I wish we had done this properly the first time around"; in hindsight there
was more than enough time, and it's costing a lot more time to fix it now.

~~~
awinter-py
when a group of people decide together what 'properly' will look like in
hindsight in 6 mos, you can end up with some interesting outcomes

------
flerchin
When I receive a PR that has actual problems, I usually make a branch from the
code in question, and submit a PR to that branch with tests highlighting the
problem, or the beginnings of a refactor that I'm asking for. I often don't
have time to fully flesh out the problems, but helping to write the software
with the requestor makes for much faster and effective feedback than plain
text comments.

~~~
tome
You are a hero and deserve a medal!

------
tylerl
A bit of useful flavor of how this looks in real life:

On my current team a typical code review of about 300 lines will involve about
3 to 4 reviewers, 20 comments, and maybe 10 changes, usually over the course
of about 48 hours. Because this is a culture shock to people who join the
company, we explicitly train every new team member ahead of time that code
review suggestions are _not_ a reflection on your code quality or your coding
ability. Reviewers are expected to err on the side of raising a comment when
in doubt, even (and especially) when the issue is about the approach being
taken rather than the correctness of the code itself. Not every suggestion
needs to be accepted, but every concern should be addressed. While not an
explicit requirement, the expectation (and typical outcome) is that the
committed code represents the combined expertise of the author and all the
reviewers, and everyone is happy to go on the record as having been
"responsible" for the code that got submitted.

A zero-question LGTM isn't an indication of good code, it's a red flag for an
incomplete review.

------
heelix
I feel embarrassed to even ask... what does CL stand for?

~~~
jmgao
Changelist, it's perforce for "patch"

------
Noxmiles
Terminology:

There is some Google-internal terminology used in some of these documents,
which we clarify here for external readers:

CL: Stands for “changelist,” which means one self-contained change that has
been submitted to version control or which is undergoing code review. Other
organizations often call this a “change” or a “patch.”

LGTM: Means “Looks Good to Me.” It is what a code reviewer says when approving
a CL.

quote from: [https://google.github.io/eng-
practices/#terminology](https://google.github.io/eng-practices/#terminology)

------
jdhzzz
Nit: Under _Every Line_ The term "scan" is used where "skim" would be a better
term.

Scan can mean _either_ "to investigate thoroughly by checking point by point
and often repeatedly" _or_ "to glance from point to point of often hastily,
casually, or in search of a particular item". Skim leaves no room for doubt:
"to read, study, or examine superficially and rapidly"

------
sixstringtheory
I love how the first-mentioned "hardest thing in computer science," naming
things, is one of the shortest sections:

> Did the developer pick good names for everything? A good name is long enough
> to fully communicate what the item is or does, without being so long that it
> becomes hard to read.

They mention several times a "Nit:" or other prefix for nonblocking comments.
I wonder if that is purposeful and/or reflects the experience of the company.

I also like a lot of the points made in the CL Author's Guide [0]. I've
recently finished a similar document at work on how to form good Git commits.
I generally like to be able to review a GitHub PR by stepping through a
sequence of well-formed commits one by one, each of which sounds like what
Google is describing in this document. Regardless of how it's sliced up, by
commits or PRs or whatever, in general I just wish I could get more folks on
board with this part of writing code.

[0]: [https://google.github.io/eng-
practices/review/developer/](https://google.github.io/eng-
practices/review/developer/)

~~~
9nGQluzmnq3M
"Nit" is a Google convention for comments that don't block an overall
approval. In other words, you can fix the nits and submit without needing
another round of review.

------
ChrisCinelli
A great guide. Maybe it is out of scope but I would have liked to see what
they do BEFORE they start writing code.

The reason is because by the time to get to a code review may be too late. The
comment that you should give is "this is not how it should be implemented. You
need to rewrite it from scratch." But most of the time you do not.

~~~
ChrisCinelli
In my team I like to have a "technical kickoff meeting".

But before that, the person that will implement the story will goes through a
checklist that I started and we improved during retros. The engineer sits down
with another engineer and they go over again together. If they did their
homework it is usually pretty quick.

Sample for web dev:

-Does this feature heavily relay on or modify code that someone else wrote? Should we talk with her? Should we HAVE THEM in this MEETING? In particular for BE work who should we talk?

\- What names are we going to use for Components, Pages, Folders, etc?

\- Is the visual design/flow clear and complete? \- Do we have every assets
from design? Do we need to ask for more? \- Do we have dependencies on other
work being done now on a branch or in the near future ? \- Do we have overlap
with other pieces of code (possible conflicts)?

\- Will we reuse some other components or any other relevant utility
functions?

\- What code can be reused?

\- Do we want to make some of the code for this feature reusable?

\- (Redux) What reducers will we use (if any)? In particular do we need new
ones?

\- What are the most important tests to write ?

\- Are we going to phase the feature?

\- Do we have to compromise and create some tech debt? If yes, what, why and
when do we plan to pay it?

I found that this meeting makes most of the code reviews fast as well.

------
chairleader
I appreciate this:

> In general, reviewers should favor approving a CL once it is in a state
> where it definitely improves the overall code health of the system being
> worked on, even if the CL isn’t perfect.

Reviewers are human too, and can occasionally get lost in the weeds nitpicking
a PR/CL.

------
whydoyoucare
"Technical facts and data overrule opinions and personal preferences." So
true, so true!

Now if teams pay careful attention to this statement, the Code Review
Guidelines can be shortened by an order of a magnitude! ;-)

~~~
electrograv
As a general rule, absolutely! Unfortunately, if applied too strictly though,
it can be weaponized or stifling in ways you may not anticipate (which tends
to be true of almost any rule used too universally).

For example: I’ve seen cases where there’s disagreement as to which of two
technical paths to pursue, where one side (not liking the way consensus is
currently leaning) demands a data driven proof that one of the choices is
best. Ordinarily this would be fine, but in this case, it would take no less
work/time to gather said data than to entirely implement _both_ solutions and
directly compare them.

~~~
enneff
> in this case, it would take no less work/time to gather said data than to
> entirely implement both solutions and directly compare them.

So why not do this?

~~~
electrograv
In this case either of the solutions would satisfactorily solve the problem,
so implementing both would be redundant and wasteful. The problem appears in
this case when there’s disagreement on which of the two satisfactory solution
paths should be taken. Implementation work cannot proceed until it’s decided
_which_ solution path is to be taken (even though they both work), but there’s
no way to prove which is best aside from general intuition arguments, or
actually implementing both (which as mentioned, would be a waste of resources
for no benefit).

------
mendeza
I found the DS100 course textbook and the author describes the phenomenon
here:
[https://www.textbook.ds100.org/ch/02/design_srs_vs_big_data....](https://www.textbook.ds100.org/ch/02/design_srs_vs_big_data.html)

This is insightful, but how does one deal with determing if your dataset is
non-random? I can imagine manually inspecting the data and using cross-
validation are ways to help identify skewed datasets. Are there any other ways
to test if your data is non-random?

------
royalghost
When a development team is following XP Principles, mostly two developers are
doing Pair Programming with Test Driven Development (TDD) and rotating pairs
every week, how much value will it add with code review ? We recently had a
discussion where some of us think it is still good to skim through the change
request by a second pair while other's think it's just a waste of time.

I am curious to know if code review is encouraged by a second pair in
following XP practices ?

~~~
bruckie
At Google if person 1 and person 2 pair, then one can be the official author,
and one can be the reviewer. Depending on the people and the change, they may
also ask for review from others as well.

~~~
packetslave
Jeff Dean and Sanjay Ghemawat coded like this for years.

------
ekianjo
If you were wondering what CL stands for since its not defined anywhere in
this doc, it is apparently akin to a Pull Request.

------
alexnewman
One of the things not being discussed on this thread is all to the
requirements around the side of the commit and how that impacts everyone else.
I wonder when companies or projects get mature enough for these types of
practices to apply. I know google has a complex lifecycle for projects
internally as well.

------
hagy
What about the importance of handling high-level architecture and design
before writing code? This can be done in design docs, discussion on tickets,
slack discussions or in person with a whiteboard. I think it's more productive
to handle these discussions and reviews before writing any code.

What do other people think?

~~~
afarrell
I would agree. I think if you're talking about high-level design in a code
review then either:

A) You've revealed a need to get more alignment on design, and should probably
be explicit that the discussion has this purpose.

B) You're doing a code review of a spike so that you can get alignment about
the design lessons the spike teaches...and should be explicit that the
discussion has this purpose.

------
afs35mm
What's really amazing about this whole thing is that it's more a guide as to
not give bad (nit-pickey, overly critical, discouraging) code reviews, than
how to give good ones. I think that alone should give us an idea about the
state of code reviews in our current industry as a whole.

------
lunchables
I had to Google it because I couldn't find any definition anywhere. A "CL" is
a change list - maybe, that's what I found from my Google search. It is a term
used dozens of times on that site but never defined, as far as I could find.

------
ed_balls
> Also, if a reviewer makes it very difficult for any change to go in, then
> developers are disincentivized to make improvements in the future.

Google requires you sign CLA in order to make contributions :)

------
aercolino
I stopped reading before finishing the third page. It seems to me that what
many paragraphs advise is to be reasonable, which is meaningful, but a bit too
loose for guiding code reviews.

------
merb
OT: but does somebody know a good code review tool, that fits nicely into git?
(Bonus points if it works containerized) Should work for 5 people.

~~~
avel
GitLab

------
antpls
How at this point code review not entirely automated?

There are numerous fuzzy tools, memory leak detectors, linters and standards.
Code is data, and it can be analyzed as much as one wants with programs.
Sonarqube for example can find bad patterns, code smells, etc, there is no
limit to what can be added to such tools.

As long as you don't want to prove that the code terminates, you can probably
write an automated checker for the rule.

~~~
shusson
> How at this point code review not entirely automated?

There is no automated way to check intention. I can write perfectly correct
code which also drops the database.

~~~
antpls
In this case, wouldn't you use some kind of permission/security groups/roles
at the db level or firewalls that would block such intents?

Wouldn't it be possible to add an automatic linter pass that does "grep DROP"
on the source code? Even unusually things such as code obfuscation can be
automatically detected to some extent.

An automated way sounds more fair, robust and scalable. To me, there is
something in human code review that is not engineering, it's all about judging
people (unless the code review is fully anonymous for both side)

~~~
shusson
Dropping the db just an extreme example. There are infinite ways a developer
can use tools correctly but build wrong things. Don't get me wrong, automating
things is great, but you're never going to replace code reviews. Unless you
have some crazy advanced AI that can interpret requirements and detect
incorrect implementations. But at that point I would imagine the crazy
advanced AI might as well be creating the implementation in the first place...

------
cellular
Mandatory pre checkin automated testing. Did you do this? Ok, you're good! ;-)

------
ElonMuskrat
Excellent guide. It would be fantastic for Google to adopt these practices.

------
techsin101
At my first job at front Dev higher up on chain from me left every code review
with "we here don't do this.." .. it wasn't long before I caught onto his
brewing imposter syndrome

------
syspec
what is a CL

~~~
ithkuil
A CL is a "Changelist". The terminology comes from the Perforce version
control software. Technically analogous to a "commit" in git.

The CL terminology stuck within Google even when the underlying version
control system is not perforce. For example the Gerrit code review system also
calls them "change lists".

It's the unit of granularity for code reviews at Google. In that respect it's
analogous to a Pull Request or Merge Request in the outside world.

CL are meant to be updated (edited) in place during the code review process.
This is something some people would also do with git commits (amending commits
and force pushing) while others would add many individual commits to address
code review issues (That's why PR is a better analogy).

------
imglorp
As a side note, their github hosts 1600 repos. Just wanted to say thank you,
google.

~~~
ipsum2
I would wager that a vast majority of them are personal projects. If you work
at Google and want to do some open source project on the side, the code is
owned by Google and is under Github.com/google. See:
[https://news.ycombinator.com/item?id=15592968](https://news.ycombinator.com/item?id=15592968)

~~~
zamadatix
Here I am just wishing my company would even allow me to post the source of a
project I've worked on with any license and apparently Google isn't worth
praise for hosting all such projects with an Apache license by default and a
review process if you'd like to try to keep copyright.

[https://opensource.google.com/docs/iarc/](https://opensource.google.com/docs/iarc/)

~~~
dmitrygr
If you live in CA, you're good to do it:

[https://codes.findlaw.com/ca/labor-code/lab-
sect-96.html](https://codes.findlaw.com/ca/labor-code/lab-sect-96.html)

as long as you do it on your time, using your own resources, and do not
compete with your workplace.

The fact that google (or other employers) do not go out of their way to tell
you this is, of course, understandable.

~~~
tylerl
You'll be hard pressed at a well-established tech company like Google to find
even one programming topic wherein you're not legitimately competing with your
employer.

Google's IARC process (where they officially sign over their claim to a given
invention) is really, truly unusual. They absolute _are_ going out of their
way to be accommodating. Even their (super liberal) open-source process was
practically unheard of when they started it. The fact that Chris DiBona runs
that office is probably (definitely, absolutely) a factor.

Also, I would exercise extreme caution when using the section 2870 argument
with a random employer; there's nothing in there that says you can't be fired
if you try it, even (and especially) if you win.

~~~
dmitrygr
Being fired isn't a big deal. In silicon valley you could spit, and hit three
companies that'll hire you, at least one of them good

------
userbinator
I feel like the term "open source" is overused. For code, which is where the
term came from in the first place, it's relatively easy to define; but what is
"source" for something non-code? Is Google really wanting you to contribute to
changing its internal processes? I'd say ..."are now public" makes more sense
for releasing this document.

~~~
dang
The submitted title was "Google's internal code review guidelines are now open
source". We've since replaced that with the article title, in accordance with
the HN guideline, " _Please use the original title, unless it is misleading or
linkbait; don 't editorialize._"

[https://news.ycombinator.com/newsguidelines.html](https://news.ycombinator.com/newsguidelines.html)

------
yegor256a
Just be prepared to say NO, this is what a good reviewer is all about:
[https://www.yegor256.com/2015/02/09/serious-code-
reviewer.ht...](https://www.yegor256.com/2015/02/09/serious-code-
reviewer.html)

------
jorblumesea
CL? Is that perforce? Hopefully it works better than the real perforce. A
really tough and confusing versioning tool.

~~~
topher200
> CL: Stands for “changelist,” which means one self-contained change that has
> been submitted to version control or which is undergoing code review. Other
> organizations often call this a “change” or a “patch.”

source: [https://google.github.io/eng-
practices/](https://google.github.io/eng-practices/)

~~~
jorblumesea
Yes, I understand, I used it every day. Just very surprised google used
perforce, given my bad experiences with it as a whole.

------
andrew_
It disturbs me greatly that this is needed. I understand why it is needed, but
it's still disturbing.

------
Iv
That advice is what I try to do more. I think people who veer more into the
logic/engineering side of it forget some obvious emotional common sense:

> If you see something nice in the CL, tell the developer, especially when
> they addressed one of your comments in a great way. Code reviews often just
> focus on mistakes, but they should offer encouragement and appreciation for
> good practices, as well. It’s sometimes even more valuable, in terms of
> mentoring, to tell a developer what they did right than to tell them what
> they did wrong.

------
YZF
> Some hardward manufacturers only ship new hardware once a year.

A line snuck in through their code review.

One big issue that I'm not really seeing discussed is that in many cases code
reviews end up being very subjective. "hard to maintain", "not readable",
"brittle", "needs more tests" are all super subjective. There is no magic tool
that can tell you the future maintenance costs (vs. the current costs),
whether some area is more likely to see issues, etc. A lot of this is a matter
of judgement. One old recommendation I recall is to limit the scope of code
reviews to finding bugs. If there is a bug that's pretty objective, the code
does not work.

~~~
bt848
You can draw bright lines about some of these things. If a change claims to
fix some bug, a test must demonstrate that. If I patch just the test into HEAD
and run it, it should fail. If this is not the case then the change "needs
more tests".

"Not readable" is why Google also has the "readability" process. A person
without readability needs the pre-submit approval of someone with readability
in that language. After a while, new people become acculturated and gain
readability.

"Matter of judgement" is why hiring is absolutely critical to success in
software. You must hire people with good judgement, and to the extent that you
might intentional or unintentionally hire bozos, do so at a low enough rate
that you can indoctrinate them.

~~~
YZF
"every bug fix must be accompanied with a test that demonstrates the bug/fix"
is just dogma IMNSHO. (also see "closing the barn doors after the horses have
escaped" or "lightning doesn't strike twice at the same place")

The typical rationale is the tests will now catch a re-introduction of the bug
or regression. The reality is that often the tests bloat the code base, they
cost in future maintenance making the code harder to refactor, they may
introduce their own flakiness (the test itself might have bugs), and the bug
that's fixed is generally unlikely to recur. You want good tests- not just a
random collection of tests that reflect the history of bugs in the code. I
often end up writing a test as part of trying to understand or reproduce the
bug, and that test will often stay... But not always.

Sometimes it's the right thing to do. Sometimes it's not. I.e. subjective. The
lines aren't always so bright.

To complicate things further it's very hard to gauge the effectiveness of
various processes. Is following those specific guidelines a net positive or
negative? who knows. I think there's consensus that code reviews in general
are a good idea so probably just by having a second person read the code, have
a discussion with the author, with no guidelines, you're getting most of the
benefit (obvious issues surface, more than one person is familiar with the
code etc.). My gut feel is that the rest is in the noise.

I do agree hiring is critical ;) yet another subjective process there. Ideally
you'd only hire people with a track record of producing good software in your
area of business, most other indicators are probably pretty random. If you're
looking to build a first person shooter you should hire John Carmack. Hope he
likes your code review guidelines though.

Another random side comment is that IMO design issues should probably be
identified before code is written.

EDIT: random semi-related trivia: [https://kotaku.com/the-exceptional-beauty-
of-doom-3s-source-...](https://kotaku.com/the-exceptional-beauty-of-
doom-3s-source-code-5975610)

~~~
bt848
[https://mobile.twitter.com/dvyukov/status/116954416787113165...](https://mobile.twitter.com/dvyukov/status/1169544167871131653)

This tweet perfectly captures why all bug fixes must have tests. Linux has no
tests and no testing culture and it is 17 million lines of juicy hot garbage.
Google is mostly tests and it is one of the largest and most successful C++
projects in history of our industry. I feel justified in standing my ground
when reviewing under-tested code.

~~~
YZF
> Google is mostly tests and it is one of the largest and most successful C++
> projects in history of our industry.

I'm not quite sure what you mean here? Are you talking about Search? Ads?
Cloud? Self driving cars? Google has various products of varying business and
technical success levels. Some stuff I feel is a good example of quality
software (let's say Go for example or maybe Maps) and some stuff seems to just
be very poor software (let's say Google Hangouts Chat or Google Sheets). Some
stuff is really good business and some gets cancelled (with little apparent
correlation to quality).

Google is so rich that perhaps it is succeeding despite some practices and not
because of them. It's initial success probably predates all these practices
and there's plenty of quality software in the history of the industry that has
been produced using a variety of other practices.

Even if you are right and Google is _the_ most successful C++ code base ever
that's still not evidence that some particular practice is the cause of that.
There is no way to measure any of this and "feeling" doesn't really cut it.
Some practices apply more to certain kinds of software and possibly less to
others.

Doesn't Google use Linux? For juicy hot garbage it's done pretty well. With a
budget that's a fraction of what Google has. It's also not C++.

re-EDIT: Another thing to note is that Google has a large number of engineers
developing internal tools, build systems, test engineers etc. With all the
automation around dealing with flaky tests and testing in general at scale
perhaps this works better for Google. Other companies who do not have the
luxury of having 100's of engineers work on their build or test systems may
run into different problems when they try to do things the Google way. Because
Google is overall fairly opaque (the open bits are exceptions) it's hard to
really come to a conclusion about how well they do software. I've heard
different stories from different sources and I'll bet there's lots of internal
variation as well. The success as a company is undeniable but there's more to
that than software.

------
d_burfoot
Am I the only software engineer in the world who hates diff-based code review?

Imagine trying to evaluate the quality of a novel by examining diffs to the
manuscript. Reviewing every diff means that every single change needs to yield
a good novel. But that's an absurd constraint on the creative process - what
if you want to introduce a new important character? You check in a new version
of the first chapter where the character figures prominently, but the reviewer
complains that the new character's plot arc never goes anywhere.

The main goal of this diff-based code review seems to be to prevent people
from checking in bad code. But of course you should sometimes check in bad
code (especially in the early stages of a project)! You might consciously
check in some bad code temporarily, intending to upgrade the code at some
future point. More importantly, you generally don't know how or if you're
actually going to end up using any given piece of code. Spending a lot of time
polishing and reviewing code that doesn't eventually go into the project is a
classic example of premature optimization (the root of all evil).

~~~
ng12
Because grokking 20 diffs across 5 files is a lot easier than grokking the
entirety of those files and the context on which they live. If you assume the
rest of the codebase meets some reasonable standard of quality the diff should
be the only important thing.

That said, for particularly complicated changes I'll check out the code and
read through it locally but diffing is surely optimized for the most common
case.

------
cosmotic
> At Google, we hire great software engineers, and you are one of them.

That's a pretty presumptuous comment.

~~~
username90
I don't know a single company that doesn't hire great engineers.

------
euske
To me this doesn't look a very good guideline. It's needlessly verbose and
unintuitive. Instead of writing a long sprawling list of things, just using
bullet points and figures would be better.

It mentions the importance of getting a big picture of the code in question
first, but the guideline itself is pretty bad at presenting its big picture.

Here's the gist of it as I understand:

\- Long-term health of the code is important.

\- Development should be incremental and steady.

\- Beware of the contexts: style, use cases and technical complexity.

\- Suggest them changes with reasons.

\- Make it quick enough.

~~~
rammy1234
Guidelines this big is there for a reason , lot of minds have gone behind this
and every thing is a read to resonate with and come up as you understand it.
Bulleted points and gist are good , but we are here to revise what we studied
last night. This is a senior developer talking to a junior developer who is
starting out. Also this is something go back and discuss and enhance.

