How to do a code review
google.github.io
google.github.io
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.
In my experience, projects using Java are the worst offenders here.
> 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.
I agree so much. Zero-cost abstraction is a misnomer.
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?
For example, in Python, there's no need to go for a class when a simple function does the job. And once you do need a class, it's fairly trivial to swap one for the other, especially in a good IDE.
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.
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.
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.
So, adding some extra architecture solves it?
"Lean" tells you that small batches reduce the waste.
As an analogy, think about looking in front at the road that unwinds passing through cities and attractions until finally it becomes too small to see ahead. You feel that if you stay on the road, it will likely bring you to your destination.
Nevertheless you are not sure. As you move ahead it gets clearer were you are and where you will have to go.
So how far should you go before stopping?
It depends. It is all about how far you can see and how confident you are that it is the right road.
If you do not see too far ahead, it is probably a good idea to just reach the next city at hand and spend some time checking if you are on the right road. Otherwise you may waste time going too far on the wrong path. The further you go, the further you will have to backtrack.
But if you can see further ahead, you want to move faster and skip the pitstop.
This analogy only goes so far. But depending what you are building and were you are in process, and more importantly, how sure you are of what you actually need to build, you may less time in planning and just building what is merely necessary vs building a more complex architecture.
So you may say: "let's try to find as much we have to go and where before we start. But even the pathfinding activity takes time...
That said...
> Note how Google does NOT say "make sure the code is properly architected".
is not accurate. The very first paragraph on the same page is:
> Design
> The most important thing to cover in a review is the overall design of the CL. Do the interactions of various pieces of code in the CL make sense? Does this change belong in your codebase, or in a library? Does it integrate well with the rest of your system? Is now a good time to add this functionality?
Emphasis mine. But that sure sounds like they care about proper architecture. They just also care about avoiding over-engineering. They're not mutually exclusive.
Of course they care about architecture—I don’t think anyone implied otherwise. But at good companies like Google good architecture is a given. Top developers often fall into the pit of over-engineering, and almost never under-architect. So at top companies code reviewers have to be vigilant about over-engineering and rarely have to worry about under-architecting.
- Is `good architecture` a given in this team?
- What are they doing to avoid `over engineering`?
What are the specific questions one can ask to find out the answers to the above two?
Absolutely not. Google’s interview process and inflow of fresh graduates does not bode well for good architecture. Having spent time at G and FB, I can certainly tell you that employees at both are no better at architecting code in a sane way than SWEs at other companies.
Code architecture requires experience. Google does not.
I've seen more horrible code at a FAANG company than in many other places. Complete absence of overflow analysis in C++, race conditions, architecture astronauts, exploding code bases that no one understands any more.
Features are being added on a daily basis, basically what matters is a high line count.
It’s like requiring candidates to deadlift 500 lbs and then assuming it means they can all run marathons.
"Architecture" is very vague and used a lot of different ways.
I think that is part of the scrum philosophy: management can question whatever it wants and whenever it wants to; if the timeframe explodes that that is the blame of the developer.
Asking because we might have a different process for PRs or a different definition of what it means to submit a PR - on many teams I’ve been on it’s been encouraged to publish a WIP PR in order to facilitate these conversations. GitHub recently added a feature to formalize that but before this we used labels such as “WIP” and “Ready for review”
There shouldn't really be a PR review where the author is seeking input on naming/testing completeness and major design choices. Those two types of reviews are mutually exclusive.
Alternatives to WIPs include whiteboard discussions, pair programming, and written proposals.
Not to say you're wrong, but some hard human problems to solve are:
- getting people to not be defensive during code reviews
- getting people willing to be critical (constructively) of their peers work
Emphasizing that code reviews with productive design comments indicates a failing seems more likely to stop the comments, not to improve the design. Most people wont want to do the work multiple times and will quickly learn to do as you desire in the face of repeated code reviews that have comments pointing out design problems.
The programmers I work with can be defensive of their architecture, but they also love to talk about architecture generally. I think there's a "yes and" [0] way to bring this up that engages the whole team.
Sometimes a prototype can help shake these things out, though. It's often okay to go an extra stage beyond what's been reviewed/approved (e.g., send out a prototype implementation before design review is complete) if you're willing to throw it away. If you'd be upset by someone saying "try this better design", you need to wait for a design review before writing any code.
Another thing to be careful about with a prototype is keeping it as far away from the normal production serving path as possible to avoid compromising the essentials of reliability, privacy, and security. (Ideas along those lines: separate production role, separate server, flag-enable it only in a test environment, dark-launch it, experiment-gate it, etc.)
https://ronjeffries.com/xprog/articles/practices/pracnotneed...
Do as I say, not as I do I guess.
Google absolutely want developers that are capable of over-engineering.
Anyone can over-engineer. It’s not a compliment nor a desirable quality.
Why, so, when they get on the job, they need to be told not to over-engineer?
Just recently I was looking at Angular, Google's web frontend framework. Services, modules, directives, angular-specific markup. Coming from React, I find this grossly over-engineered.
In general, yes we do practice it. They are guidelines, not rules tho.
Is Angular grossly over-engineered? Depends on how you look at it.
Any code's complexity reflects the complexity of its use case and Angular, for example, needs to fit A LOT of use cases at Google. So yea, sometimes, I think it can be a big hammer for a small nail. In other cases, all that abstraction helps. I guess that's the nature of how code eventually matures over time.
Not really. It’s a sign of too many responsibilities being given to one project. Feature bloat is a real thing.
As code matures it shouldn’t be getting more and more abstraction and APIs, it should be stabilizing with fewer changes. If the former is happening, it’s a sign it needs to be broken into multiple projects.
Breaking something in smaller projects also has its own associated costs.
Suppose you have a 2^n x 2^n sized courtyard. You have one 1x1 statue, and unlimited L pieces (2x2 with a corner missing).
You would like to have a layout which places the statue in one of the centermost tiles, and fill the rest with L pieces.
----------------
Solution
----------------
Define a layout function with allows for the empty tile to be in any corner. This is trivial for 2x2, as it's just an L peice.
By tiling these squares, you can solve for the next size up. Put three with the hole in the center and fill in with an L piece to get the bigger square.
At the end, take 4 2^(n-1) squares and put the holes in the middle. Add one L and you are done.
What was the point of that?
By generalizing your solve function to have more outputs, it lets you build up a structure from it, in which it's easy to solve your actual goal.
I don't think this is a good way to do things, but it's mostly what happens IME.
I do believe though that's really hard to discuss effectively as there seems to be no good, and common definition on what over engineering actually is, except in retrospect.
I've seen teams where they were so "good" at avoiding over engineering and "architecture astronauts" that thousand line functions with a Byzantine labyrinth of conditional was preferred to even the most basic of design.
With that said, what would you consider over engineering of the kind that never works, and in what kind of systems?
People tend to do it to get promo/bonus.
These days, it’s easier to do a search, grab a snippet, and paste. There’s little need for reusability. Extrapolating a bit, I imagine in a few years, we’ll have one big ‘library’ of functions built into our IDE (built, and copyrighted by Google or Microsoft, of course).
Programming used to be skilled labor, but now it’s kind of dumbed down for higher productivity. A natural evolution.
Personally, I’m trying to re-train myself for the more modern rapid-fire programming. I’m not completely convinced that it results in better products, but it does feel good to be constantly committing.
I would not call them 'very good developers' in that case.
Leave the code in a state where it's understandable.
ie don't over specify - not just simple things like interfaces, but also things like premature decomposition, that's exposed at the higher level for configurability.
While on the face of it, having everything as small interacting functions means to change any function is quite easy, changing overall behaviour might be very hard - hard to understand all the moving parts, and even worse, if the code split is wrong for future needs, very hard to change.
I used to be one of them. "Growing up" I realized that striving for simplicity and adding just complexity when necessary is the ultimate sophistication.
Good engineering values and a shared vision of what "doing a good job" means helps out a lot. If we prioritize "development speed" and "quality from a user prospective", extrinsic complexity clearly becomes a burden.
Instead, if these values are not shared, the main motivation for smart people may easily become "ego" and showing off code that may look clever and supporting a lot more use cases but most likely it did not need to be written, wasting time and adding unnecessary complexity with the risk of introducing cognitive overload.
If they can't write idiomatically, are they "very smart?" Code is a communication medium as much as a functional one, and if someone doesn't understand you when you're talking to them, such that you have to say, "well I use a cutting edge grammar, aren't you familiar with Lakoffian generative semantics? Imre Lakatos has a sick presentation on Vimeo," you're not maintaining communication skills. I imagine this could be what "unshared values" means.
The way I see it, it's like driving: people who don't stop at stop signs, who don't use turn signals, who don't know right-of-way rules are not good drivers. These things are a part of driving just as much as knowing where you're going and trying to run the Nurburgring in under 8 minutes. They also demonstrate a concern and respect for the health and wellbeing of your fellow drivers (not to mention pedestrians, cyclists, etc.).
This is not a matter of education, age, or maturity, it's manners. It's saying "please," and "thank you." It's parenting yourself if your actual parents didn't teach you. I'm not sure any of that can be chalked up to surmounting ego. "Ego" is just an excuse and probably not based on actual psychological concepts anyway.
the main motivation for smart people may easily become "ego" and showing off code that may look clever and supporting a lot more use cases but most likely it did not need to be written, wasting time and adding unnecessary complexity with the risk of introducing cognitive overload.
"Very smart?" ;)
For example, had a user who wanted a document produced in one specific format. They promised this was what they wanted and it wouldn't change. It changed 5 times as we were nearing release. So I over-engineered it to allow a template to produce the document that can easily be changed (compared to the release process for a code change). It ended up being used numerous times, despite the problem were given clearly states an unchanging template.
I agree with this when it's internal interfaces. When you have public interfaces that you expect to have to support in a backwards-compatible manner for (ideally) years in the future, it's worth taking some time to think about how people might want to extend it in the future.
This assumes that the interface is a function or method call within a binary. If it's an RPC, then making changes is much trickier, since you can't assume that both sides of the call were built at the same commit. This requires a lot of thought to make sure any changes to your RPC messages are both backwards- and forwards-compatible.
> 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.
This, combined with a large portion of developers lacking social empathy, poor communication skills, and (unfortunately) a desire to appear to the the smartest person in the room, can lead to severe demotivation and stress for both experienced and new team members.
I've seen junior developers welcome feedback or run from it, and I think that has as much to do with the reviewer as the author of the code.
On the flip side, I've learned almost as much from reviewing more senior engineer's code and just asking dumb questions or looking up functions in documentation that I didn't know about. I think there's huge potential to learn as the code reviewer.
IMO the first few code reviews should be done face to face so some rapport can be built. Receiving feedback from a 'human' is far easier to process than a faceless Github profile picture.
This is one reason I liked Phabricator’s review system, which allowed drafting comments on an entire PR before submitting the comments. This allows you to be as nit-picky as you want when reading the PR, and then delete or modify any of them at the end. Instant submission of line-level comments, on the other hand...
I think reviewers should have the awareness to understand not to over-burden the person whose code they are reviewing.
For example, if I’m reading some very junior code and finding lots of issues, maybe I should delete all my comments about minor things like style and only leave the comments requesting major fixes like code design or potential bugs? When the developer re-submits the code after fixing the main issues, maybe it will be less frustrating to get a review back with “this looks great, can you just fix these small issues that don’t conform to our style guide and it’s good to go.”?
1. The commit/change is too large.
2. The reviewer is nit-picky.
3. The person that wrote the code made a lot of mistakes.
I really feel like if a reviewer is making 100 comments on a change, they're doing something wrong even if there are a lot of mistakes. That reviewer should really reach out to the person that wrote the code and talk to them human to human.
If I just ended up talking to the guy, I'd have no record and he would have no record (unless he took notes) of all the places he has to fix.
The reason for 100+ comments was a junior developers code and far too large a changeset.
Telling someone that the changes they made are too much for a commit would probably go a lot farther.
Identifying common architectural problems would also reduce what you need to connunicate. Maybe they made a lot of mistakes but I would find it difficult to believe there were 100 unique mistakes.
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.
On balance, formatting is an important part of code health. Hopefully, using auto-formating tools (gofmt and friends) should hopefully make code formatting issues not take up time in code review.
"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.
Yes, one should avoid misspelling words when you are about to submit a manuscript for content, but wordsmithing all the the sentences is a mistake.
Likewise, one should run an auto formatter before sending a proof of concept off for review, but a reviewer who is all tied in knots about low-level nits when looking at a general proof of concept is editing at the wrong level.
Otherwise you end up with a fully-baked, carefully written solution that satisfies all the nits, but was a giant waste of time because it takes the wrong overall approach.
[1] https://www.amazon.com/Thanks-Feedback-Science-Receiving-Wel...
It's much harder to power through (2) than (1).
When you've been working for a while you can filter out the noise regarding formatting and other smaller issues (or, ideally, get autoformatting set up), and it gets easier to separate your ego from your code. When you're new though, and showing someone else your code for the first time it's hard to see a huge volume of what seems like substantial negative feedback on your code (and by extension yourself).
I realize the "right" solution here is that people shouldn't equate criticism of their code with criticism of themselves, but I think that's a huge thing to expect at first and contributes to making the field unwelcoming.
When the reviewer communicates this to the coder, this often comes across as criticism of code that is in reality perfectly fine.
Many people often think their skills and actions are them, instead of things they've acquired.
The question you're asking is oddly similar to asking why people get stressed and mentally suffer at all. It's a deep and complex subject. Taking is personally only scratches the surface.
Eg, I know someone who switched her career over it. I asked her why she left and she told me they kept criticizing her, and she took it as if they were attacking her and trying to get her to quit.
I was the primary architect and reviewer for a complex real-time mathematical application. When reviewing code, I was pretty much a tyrant: the code had to be correct, well tested, conform to the theory, interface with the rest of the system correctly, etc., in order to be allowed in. I remember leaving some pretty brutal reviews when the proposed design was different than what I thought it needed to be. I thought I was doing the right thing, the project lead thought I was doing the right thing, but maybe I was just making my teammate's lives hell.
In a subsequent job, I was on the receiving end: I was again the primary architect and maintainer, but still needed to seek review from a larger team (my component was part of a larger project they owned). The experience was not enjoyable: reviews sometimes took months (I sure wish I was kidding), sometimes were passthrough "LGTM! I don't understand it at all!", sometimes asking questions like "why is this mutex here" and then I have to spend 3 hours writing up an explanation for how threads and locks work in this case. I found that my mental model shifted: instead of committing small improvements here and there like cleaning up comments or renaming something I just... didn't. I didn't want to deal with a multi-day process of bugging someone to review (they were always busy), dealing with the roulette wheel of comments that might come up, the possibility that I might have to justify some minor thing that I don't even remember the reasoning for. It felt like making a PR opened you up to an uncomfortably invasive inspection, one where the reviewers look down their nose at you and ask you to elucidate why you chose to wear the red shirt today instead of the blue one, as if you're supposed to have some grand unified theory of shirt colors when the actual reasoning is "I thought red would work and it did". How are you supposed to justify why you didn't do all the things you didn't do?
I think an issue is that there's always a different approach that could be used and in a perfect world perhaps we'd iterate endlessly until we found the best one. I've seen plenty of systems that have a design very different than what I think I would do, but as it turns out those systems work too.
I've honestly become less convinced that code reviews are the answer. Is there a possibility for learning reviewer <-> submitter? Of course. Do some teams find code reviews to be hugely beneficial? I would assume so. But I don't know if an organization-wide mandatory absolutely-zero-exceptions is the way to go.
Reviewing can thus be experienced as opening up yourself to critique by essentially anonymous "kingmakers", which at any time can make you look like a clown, if they want to. This experience might not be rational, but I believe it's still extremely common, as an experience.
It's made worse by the fact, that if the review culture is anything else but great, it's not at all unlikely to try and take advantage of that - in one of many ways - in order to feel a bit better about themselves.
At our core most of us are social creatures, and thus most of us easily pick up on that danger/risk, including most that are otherwise not socially adept, as they have usually suffered bullying of some kind and have learned to avoid those exposed situations.
The basics of how they accomplished this was:
- The obvious- no personal / destructive attacks or insults, no cussing, no comments on any person's abilities.
- Having your code picked apart is a badge of honor, and you were expected / required to do the same to the most senior of your teammates when they submit code.
- Collective ownership- it's never "your" code, it's the team's code.
- Constant acknowledgement that our system is hard and complex, and it is really important that it works as expected / promised.
That last part was an ingredient I never found anywhere else. The opposite seems to be more common- every other team I've worked on tended to underestimate or even trivialize the difficulty and complexity of the systems they work on. By acknowledging that this work is hard for everyone involved, and that despite this, it must be well-done and function properly, the code review and the ensuing discussions became a very welcome and encouraged part of this process. It also helped defeat newcomers' imposter syndrome because this mentality was effective at making everyone feel like they had an important role to play, and that even the most senior folks often felt like a noob when they screwed up.
I'm usually okay with preemptively accepting code with only nit comments too just to signal that those comments are not too big of an issue (if at all).
Personally I try to make them often, but I also try to be thoughtful about making them as to not come off patronizing like you mentioned.
Though I’ve never felt like a positive comment ever made me feel patronized in any situation, I think a good way to not come off patronizing is to to make the comment more personal than general, ie:
“This is a cool way to use the rest operator (or whatever thing), Ima steal this”.
Obviously it needs to be an authentic comment, but I think it’s a significant difference vs
“Great use of the rest operator!”
When the interpretation on the other end might be “oh so this person is surprised I know these basic language constructs”.
I learn things from doing code reviews all the time, and I like letting my coworkers know that I feel like I’ve gotten better at my job as a result of reviewing their contributions.
I think it’d be a shame if I didn’t let them know.
Sometimes, it is worth the risk of sounding patronising.
Communicating positive feedback is crucial for teamwork, teaching, and passing ideas on.
Nurses don’t compliment each other for using a new pair of gloves on each patient.
Feeling valued is linked to job performance. And if I notice any of a) a generally interesting piece of code b) the engineer being a boy scout and fixing something in that particular module to make his changelist better c) a novel/comprehensive way of testing the code automatically d) elbow grease to just go the extra mile in terms of doing a great job (without adding unnecessary complexity) I will call it out in the code review along with the regular 'fix this' feedback. As a principal engineer my word carries some amount of weight and I truly want the person to feel good about their work when they deserve it.
Another thing I often do is add a 'thank you for taking the time to do this' whenever someone slightly decreases the amount of tech debt (by refactoring, or removing code/complexity) when it was obvious it wasn't absolutely necessary to get their work item completed. Basically whenever someone shows they're thinking strategically rather than tactical about their work, I want to make sure that person knows that I noticed and that I value that.
People aren't robots, we're all professionals and we all like to feel good about our work.
Not really. Positive comments help teach engineers which things they've done that conform to local best practices (and why) without them having to meticulously dig those up (assuming they're even documented). A lack of positive comments leaves engineers to learn them only by running afoul of them. Effectively it provides direction only when some threshold of badness is crossed, while leaving positive comments on good code (especially for new team members or junior engineers) provides a beacon pointing away from the badness threshold entirely.
Put differently: commenting on good code makes for swifter and less eventful code reviews by steering engineers away from the bad practices that make code challenging to review in the first place.
Also, it's just nicer to spend forty hours a week with people who demonstrably appreciate their peers' good work.
The only positive thing to report should thus be that all is fine if the reviewer did not find any issues.
Now, if you are conducting the review in a meeting then obviously you can make oral comments in passing. If you're using a software tool for reviews then the all the comments should be on point. Nothing prevent you from talking to your coworker afterwards to spread some love if you want to.
Why not?
(Convoluted comments should be made more concise)
As said, if you want to praise then you are free to do it offline.
This is a professional procedure in a professional setting, not warm words from an encouraging teacher at school... I don't want to have to go through comments that do not add any value to the exercise of finding issues, and I have never seen people leave such comments in 20 years.
Citing a good practice in someone's code as "yes, please do more of this" alongside "don't do this please" is not, in my opinion, fluff.
> I don't want to have to go through comments that do not add any value to the exercise of finding issues, and I have never seen people leave such comments in 20 years.
So only pay attention to unresolved comments?
That simply isn't the purpose of a code review.
Good practices should be documented externally, so you can check them consistently during code review ;) It's also quite useful to have a checklist when doing a code review.
Automated tools here have also drastically reduced the number of comments I get on CLs at Google, as people don't bicker about the little things as much anymore.
Unfortunately many people stink at communication. The best thing a reviewer can do during a code review is ask questions. A reviewer who comes in and just says 'this is wrong' misses an opportunity to either learn themselves or actually teach the submitter. A much better approach is to ask the submitter why they did something certain way. What were their thoughts, and what did they see.
IME, asking questions leads to much better outcomes. Sometimes the submitter will just admit they were not thinking about anything and see their own errors. Upside, they found the issue themselves. Other times they'll explain some complexity or use case only they could see while deep in the code. Upside, I just learned something.
In general the best teachers ask questions. It doesn't put people's egos on the defensive, and tends to help people learn more effectively (the goal if any review IMO). I've seen this work on me in other scenarios like Jiu-Jitsu. My teacher will ask mid-roll or afterwards why I did something, no matter how stupid it was. Explaining my thought process helps me to better recognize the error, and I'm not immediately on the defensive. I'm then much more receptive when the teacher goes "that wasn't a terrible idea, but if you did this...".
I got 21 comments to remove blank lines and none about the code. Thanks. Really useful. This was the team of "experts" on the code base.
Then a principle engineer reviewed some other code and oh look, actual useful comments.
Nitpicking can be worse than useless. Obfuscates or ignore real issues in the code. Seems to usually be a sypmtom of the reviewing not being competent enough to actually review the code properly.
Trivial comments are a sign of that.
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.
1) Decision to use some kind of 3rd party library that purportedly speeds up handling primitive types (Trove specifically. Almost never worth it, and it's extra painful because it doesn't plug in well into the rest of the standard library).
2) Using mutable objects to avoid the inefficiencies of allocation and copying with immutable patterns. Can lead to a brittle stateful design with hard to track down bugs.
3) Reusing serialization artifacts across the call stack to save copying/allocation, again like 2). Now you end up coupling the API interface to layers deep in the call stack making things harder to modify.
Also best review advice I got and follow: be the first reviewer yourself! Looking at a nicely formatted diff really lets you see your code in a new light, and usually I end up doing several iterations before I run out of things to tweak. Things like forgetting debug log statements or commented out code, or dev settings, as well as other easy but effective improvements.
I've been enjoying lots of comments in this thread, but unequivocally THIS. My company (for no particular reason) has two code review tools. Each team picks which one they like. Early in development I'll shelve a CL into the tool my team doesn't main, and just review and annotate diffs. It's a good place to keep TODOs and notes to myself.
Additionally, if I share it with my team, its clear that its "pre-review" quality and not to nitpick. "WIP" in the subject line helps, but being in an entirely different application really solidifies the point.
Then, when ready to go into real review, I've got a really good sense of how the CL looks as a diff, and where any remaining annotations need to be placed (that aren't truly a comment).
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.
I came across a comment in google base libraries that said "This is faster because cache line is 32 bytes". It had been written by a very famous engineer, and it was even true in the days of the Pentium III processor. But at the time I found it it was not only false but the code as written was slower on modern CPUs than the shorter and totally obvious equivalent.
You would want to ban that sort of comment?
If you see a piece of code written oddly, then someone telling you why they wrote it that way is very helpful. If later you have to refactor it then it's doubly nice to know the reason it was written that way doesn't apply any more.
I suspect your problem is with pre-mature optimisation rather than commenting, and if so I imagine the majority of programmers would share your views. But if that is the case banning comments that make it plain something may have been pre-maturely optimised doesn't seem like a good way of solving the problem.
But if you're attending cppcon, two engineers in Google's production toolchain organization are giving a talk on "Releasing C++ Toolchains Weekly in a 'Live at Head' World" where they'll discuss (among other things) how we use various microbenchmarks to catch performance regressions.
If it isn't worth your time to write a benchmark then it isn't worth my time to change the code.
Everyone thinks their own performance tweaks are simple and obvious, but I've rarely seen measurable performance boosts from comments in a typical PR.
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.
In this particular case I'd expect the change to be motivated by profiling of the real production instances. And that makes it pretty obvious how the change should be evaluated. "We're spending more time than is reasonable in linear scans, so the worst case inputs must be worse than expected. Switch to a data structure more suited to large inputs, and see if CPU use improves in the next rollout."
No one here is agreeing with that.
BUT I think a more important consideration is readability and clarity. Make it work, make it pretty, make it fast - in that order. If there was anything I learned from a Go course some time ago (Ultimate Go iirc) is that speed is a natural result of readable code. Plus if your code is good and well structured it becomes trivial to benchmark, identify and resolve any performance issues.
Generally true, but not when prototyping. As a reviewer, I would like to see the general idea before spending time on cleaning up. If the grand design is wrong, time spent on cleaning up would be a waste.
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.”
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.
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...
Code reviews are mostly in the social domain and not the technical domain. Other than bugs and performance[1], everything in a typical code review is an opinion. Hence, you need guidelines on how to handle differences in opinions. Different work environments structure code reviews differently, but often the worst is where a reviewer finds problems, and the code review cannot be marked complete until the reviewer is satisfied. This often leads to "I have to change my code to the reviewer's preferred style" instead of "The reviewer found real issues". Other models, exist and are better.
[1] Actually, performance conversations are very often based on opinions. Things like what level of performance is acceptable, etc.
Which of course is not reality. I mean I get it, I too would, I think, love to be a hero developer and just immerse myself into code for weeks on end, but that's simply not the reality. I don't write code, I contribute to a customer-facing website / shop, and at the moment my time and energy would be better spent adding a date picker than to fantasize about rebuilding the back-end in Go.
Yes. Dorm life is supposed to provide the social aspect. And to a lesser extent, real group assignment/projects.
This is a great rule of thumb
Mind you that was before Prettier, nowadays a lot of these niggly things are automatically identified and fixed in editors and a pre-commit hook.
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?
> 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?
After they started enforcing that they got a lot more productive and started shipping on time much more often. Turns out the mad rush to meet an arbitrary deadline absolutely kills productivity.
1. Higher throughput, higer latency and correctness on one side.
2. Lower throughput, lower latency and some risk of faults on the other side.
It's like the difference between tcp and udp.
The question is: Where do you need to be? What are your requirements?
The logic was we have fewer users over the weekend, so if something goes wrong, fewer people will notice.
I justified as saying if something breaks we have everyone there to fix it. Plus I'm not going to spend my weekend working if I don't have to.
My app has global traffic. Sometimes stuff breaks Sunday night and Asia is the first to find out.
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.
The test of a good cd is that your ok with releasing on Friday at end of the day.
Strangely they also list as "not an emergency" rollbacks of clearly broken code, but that's an exception to review rules inside Google. Anyone can do a pure rollback of a change without getting the approval of the owners of the code, and there are automated tools that will roll back changes that appear to have broken at least 1000 tests, without human review.
Edit: There's even a button right in the code review web UI to roll back a committed change.
The threshold for this seems at least a little too high, doesn't it?
As long as everyone admits it, sounds like those are actual emergencies :)
And they really want to, because they don't want to have to do a bunch of merges when they come back...
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?
It's mostly a "I know it when I see it" kind of thing. We know most of the time what bad naming looks like - it creates confusion, tells you nothing about the variable ("i", "n", "val", etc). I guess this is not a science (yet?) and we're currently at the stage of defining anti-patterns, out of which maybe we'll figure out some actual desired patterns.
A Philosophy of Software Design by John Osterhout is a good book on these questions.
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?
Now, if you've been fortunate to work in places that more closely adhere to the original ideas and concepts that's great; count yourself fortunate.
----------------------------------
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").
- 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.
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.
Hiring someone just to review your code is not a sane business decision, so you might want to think about how you overvalued that aspect. I would be very surprised if someone could make a business out of 3rd party code reviews, but stranger things have happened.
Depending on how you define "make a business", this already happened. There are paid for code review and vulnerability scans. Sadly, I can't remember the companies that did them. I think one of them was by IBM... I saw them applied to new software (when it was nearly done) at two big, European companies. They were mostly worthless: The insights were barely above what Sonar gives you and many findings were "never gonna happen" edge cases.
Code is a bit like music, there is very little right or wrong, but a lot of flavours and opinions.
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.
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.
For example, their recommended order of reviewing code tests first doesn't exactly work when you have no tests, as is pretty common at startups.
At Google, every single line of code has to be signed off by at least one other engineer _before_ it can be committed to the official codebase, regardless of team size. Additionally, unit tests are required, typically at the same code gets reviewed. This all makes code submission significantly slower, but product development significantly faster. Why? Here's a few reasons I've personally observed:
1: Bus factor. People leave the team all the time, ideally not for the reason the concept is named after. Having someone thoroughly review your code means there are at least 2 people on the team who completely understand every single line of code. That guarantee isn't absolute just because of timing and churn, but it's correct often enough to be the rule. This simple fact has saved me, personally, _days_ worth of stumbling around. It saves you from precisely the failure condition you described.
2: Fewer changes. Especially paired with the "small CLs" advice, this helps you really quickly correct course when people are headed down the wrong solution. As a result, you end up shipping a much higher percentage of the code that you actually write.
3: Fewer bugs. This is the biggest item of them all. Bugs are the quicksand of coding... the metric you hear quoted is that companies are expected to waste 75% of their development time on debugging. Reviewers easily catch enough mistakes to more than pay for their time investment right there. This is especially true for misfeatures where the code is "correct", it just correctly does the wrong thing. Those mistakes are hard to catch with tests (because the tests will be equally wrong), but easy to catch with humans.
Also, this is where a zero-tolerance policy on requiring unit tests really shines. I've been at this for many years, and I have had zero (zero!) bugs in the unit-tested code that I've committed. My tests have found some that would have been real awful brain-melters though. I'm on slow end of things; it takes me about 5x as long to write the tests for a given module than to write the module itself, and I'm still WAY ahead on time when I account my own debugging savings (I think the 75% number is low if you want to hit the zero bug mark). On my (30-person) team, we spend about 5% to 10% of our time dealing with bugs in our own code, and regularly hit the "feature requests only" mark in our queue. And the bugs we do get we can generally eliminate as a class by teaching better review and test practices.
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.
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.
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
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"
> 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/
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.
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.
> 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.
Now if teams pay careful attention to this statement, the Code Review Guidelines can be shortened by an order of a magnitude! ;-)
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.
So why not do this?
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?
I am curious to know if code review is encouraged by a second pair in following XP practices ?
It's easy to see why. Code review you can focus on some code for awhile, pair programming you have to move on with the pace.
The tradeoff is that you've now effectively made the reviewer a full-time participant in the development process, which is much more of a resource drain than a code review role would have been. This may end up being a very wise investment, but smarter people than me have strong opinions on both sides of that one.
What do other people think?
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.
Google requires you sign CLA in order to make contributions :)
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.
There is no automated way to check intention. I can write perfectly correct code which also drops the database.
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)
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).
Here are the statistics just in case anyone else is interested:
1,519 Sources
38 Forks
18 Archived
6 Mirrors
--------------
1,581 Total
(Disclosure: I'm a Googler.)
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.
Google does enough different things that this is often unclear. The IARC program lets you get a clear opinion from the company on whether they consider your work to be fully separate.
Of course not a lot of people participate in 20% projects since just doing your main job is simpler than trying to balance two jobs. I only used it in short bursts myself, for example testing working on a project with a new team before transferring. However I feel it lives on in spirit, in that your coworkers and manager at Google typically doesn't know what you do from day to day and nobody will scold you or even notice if you work on a different project you feel is important for a week. If they make a fuzz just tell them it was your accumulated 20% time from the past 5 weeks.
In short, 20% time means that 80% of the time you belong to your manager and 20% of the time you belong to yourself. You are still judged by 100% of your contributions, so if your the new framework you decided to write didn't turn out to be useful then it wont be of much help for perf. But if it became a huge hit then that 20% project might be more important than your main project.
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.
Not necessarily. See this comment - https://news.ycombinator.com/item?id=15592993 - which is a child comment of the one you linked to.
Since I wrote it on my own time, I had my pick of going the IARC route vs going the Google open-source route, and I picked open-source with a Google copyright because it was simpler (though I fully intend to go with IARC on another project just to try it out).
It's on my own github instead of Google's because (again) it was simpler. Since it contains zero Google-specific anything and wasn't made on company time, they didn't care which I picked at the time. In retrospect it probably would have been cooler to publish it under their name instead of mine, but aw well.
Source: https://cacm.acm.org/magazines/2016/7/204032-why-google-stor...
• You can make a "branch" of portions of a codebase and have it appear side by side with the original. You don't have to keep doing `git checkout` to switch between your version and the original.
• Renames and copies are actually remembered and recorded, instead of haphazardly reconstructed after the fact using heuristics.
• Have multiple pending CLs. Sort of like having multiple independent WIP commits at the same time. It's quite powerful when you need to do two different things and more convenient than `git add -p` to select hunks.
> 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.
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.
"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.
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-...
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.
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.
WRT "brittle", sometimes a test fails when you're doing maintenance; you can look at the test and sometimes it makes sense to delete it, sometimes it shows you that your maintenance has unintended consequences. You can't make that judgment call if you didn't write/keep the test.
It's not a perfect heuristic but it's an easy one to get a team aligned on. IMO the biggest challenge in scaling a team and maintaining code health is setting standards like this that are easy to explain and easy to maintain. There's an unfortunate human tendency to cut corners in the absence of clear rules here (even the best engineers will do this in many circumstances).
You can always exercise judgement on the cost/benefit of adding the test though, it's just a good default.
https://www.youtube.com/watch?v=qrYt4bbEUrU starting at 14:20
I briefly considered writing a debugger script to pause at the right spots so QA could replicate the issue but would be a waste of everyone's time and I wouldn't want that in an automated suite.
I make a lot of comments along those lines when I review code and I think overall it's helped significantly with technical debt and it would be very limiting to not be able to use my judgement on these matters when reviewing code. I definitely always provide some explanation, e.g. some examples of why the test coverage is inadequate, or why the code seems more complex than necessary. But this still relies to a significant degree on the reviewer's intuition and judgement.
I think it's reasonable to look at code and ask "will someone be able to understand this in a year's time?" or "how likely is someone to introduce a bug when changing this logic?". You won't get totally objective answers but I think you get some worthwhile insight.
There are various measures you can look at as heuristics - cyclomatic complexity, that sort of thing. I think also requiring good test coverage steers things in the right direction since it's often easier to simplify the code than to achieve full coverage.
The "finding bugs" criteria also doesn't really work if the code is so complex that a reviewer can't convince themselves of its correctness.
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).
The problem is how often devs forget about “temporary” code.
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.
Perhaps some people use code review that way but what is "bad code"? Code that doesn't match your style guide? Style checks are better applied by automated tools. If you mean code that isn't as polished as possible, then the solution is to agree upon expectations with the rest of your team appropriate to the project and/or task.
Better goals for code review are knowledge transfer (both ways), design review (ideally on an early WIP branch), and catching mistakes and oversights.
I am the only person looking at my manuscripts before I finish a second draft. There is no risk that my half-finished work is going to introduce cruft into a shared "codebase" and/or "bugs" into "prod". Either they make it into that second draft state and receive feedback as whole, finished things, or they die inside my computer.
This is very different from typical software development as part of a team, and your teammates should recognize incremental green-field work and adjust their review criteria accordingly. It's still important to review this sort of code if it goes anywhere near production systems, though, for obvious reasons.
Ironically, since I write in Vim and auto-wrap with par, it's harder for me to generate nice diffs than it would be if I just wrote in Google Docs like a good citizen of the future. When it comes to interacting with editors and other collaborators, though, it's super important to collect and review "diffs" of some form. Horror stories abound of unexpected changes making it to print.
As the guidelines say, they goal is not to check in perfect code, but to move the project forward. Sometimes that means adding code with a TODO comment (and an accompanying bug number) to remove a hack or implement something that's missing, and that's fine.
The main benefit of code review is that you're sharing and documenting your development process. Your teammates, by reviewing your code, certify that they understand what you're doing and why. Every commit you make should be explicable and justifiable in the context of code review; if not, then why are you making the commit?
That is to say, a change doesn't have to be perfect on its own, but the direction should be clear, and it should be locally well constructed.
In my last job we didn't have a review requirement for commits; we mostly relied on this form of catching problems after the fact (and, of course, an extensive test suite). For experienced developers I thought it worked fine; it was only some of the junior people of whom I thought we should probably be reviewing their code more regularly.
In my current job we do have such a requirement, and I have mixed feelings about it. There are still plenty of times I don't manage to catch mistakes before they get merged in, and the delays getting someone to review my PRs can be frustrating. The system clearly has some value, but it also has costs. I think I would allow most developers, after an initial probation period, to commit without approval, keeping in mind that someone who shows a pattern of committing bad code may lose this privilege.
That's a pretty presumptuous comment.
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.