22 ms·

How to do a code review

• mattsfrey 7y ago
  Would help if they mention what a "CL" is somewhere before using it ubiquitously
  • enjoyyourlife 7y ago
    They do define it, see https://google.github.io/eng-practices/ https://google.github.io/eng-practices/. It means "change list."
    • eddieh 7y ago
      Where does that define CL?
      • x3n0ph3n3 7y ago
        Under "Terminology"
        • eddieh 7y ago
          My bad. For some reason I was looking at this https://google.github.io/eng-practices/review/developer/ https://google.github.io/eng-practices/review/developer/ after opening a new tab.
  • deleted 7y ago
    [deleted]
  • jdm2212 7y ago
    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_changelists.html https://www.perforce.com/perforce/doc.051/manuals/p4guide/07...
    • jonny_eh 7y ago
      aka "Pull Request" in Github parlance.
      • hota_mazi 7y ago
        aka "Merge Request" in Gitlab vernacular (which makes more sense than "pull request" in my opinion).
        • cpeterso 7y ago
          A Perforce changelist is more like a git commit than a GitHub pull request.
          • jefftk 7y ago
            The way Google uses it, a "CL" means both "a thing you sent out for pre-commit review" and "a submitted change". It's very similar to a GitHub PR, if you use a master branch and always squash your PRs when merging to master.
        • aledalgrande 7y ago
          aka "patch" for the Linux kernel development community
          • xixixao 7y ago
            aka "diff" or "revision" at FB .)
            • o10449366 7y ago
              aka "CR" or "Change Request" at Amazon
        • sanderjd 7y ago
          Yeah, merge request seems like the best terminology of the ones I've seen.
  • atsaloli 7y ago
    It's defined on the front page, <https://google.github.io/eng-practices/> https://google.github.io/eng-practices/>, under Terminology: 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.”
• userbinator 7y ago
  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.
  • dymk 7y ago
    They're accepting pull requests: https://github.com/google/eng-practices/pulls?q=is%3Apr+is%3Aclosed https://github.com/google/eng-practices/pulls?q=is%3Apr+is%3...
  • dang 7y ago
    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
• YZF 7y ago
  > 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 7y ago
    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 7y ago
      "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-code-5975610 https://kotaku.com/the-exceptional-beauty-of-doom-3s-source-...
• d_burfoot 7y ago
  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).
  • stiglitz 7y ago
    “You might consciously check in some bad code temporarily, intending to upgrade the code at some future point.“ The problem is how often devs forget about “temporary” code.
  • ng12 7y ago
    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.
  • bt848 7y ago
    There's no reason you can't incrementally develop your change over time and then send it when it looks right. You don't need review for every edit. At Google you only need review to commit your change to the mainline repository. For long-running projects it's also permitted at that company to check in whatever you'd like into //experimental/users/<your_username>/...
• imglorp 7y ago
  As a side note, their github hosts 1600 repos. Just wanted to say thank you, google.
  • arthurcolle 7y ago
    Wow, that is really a huge number of repos. I wonder how many are just forks though.
    • guessmyname 7y ago
      I will teach you how to check that: https://i.imgur.com/HjhAmQW.gif https://i.imgur.com/HjhAmQW.gif Here are the statistics just in case anyone else is interested: 1,519 Sources 38 Forks 18 Archived 6 Mirrors -------------- 1,581 Total Source: https://github.com/google https://github.com/google
    • timsneath 7y ago
      Also worth noting that a significant quantity of Google open source projects and contributions are in other GitHub orgs. For example http://github.com/flutter http://github.com/flutter, https://github.com/tensorflow https://github.com/tensorflow, https://github.com/angular https://github.com/angular, https://github.com/golang https://github.com/golang and https://github.com/dart-lang https://github.com/dart-lang. (Disclosure: I'm a Googler.)
  • ipsum2 7y ago
    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
    • 9nGQluzmnq3M 7y ago
      Not necessarily: here's Chris DiBona explaining how it works. https://news.ycombinator.com/item?id=15594045 https://news.ycombinator.com/item?id=15594045
• euske 7y ago
  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 7y ago
    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.
  • stebann 7y ago
    Never heard about effective patience and respect for other people's work? After all, people who wrote this have some experience...
• jorblumesea 7y ago
  CL? Is that perforce? Hopefully it works better than the real perforce. A really tough and confusing versioning tool.
  • krasin 7y ago
    Google uses Piper, that is vaguely inspired by Perforce workflow, but significantly less confusing. Source: https://cacm.acm.org/magazines/2016/7/204032-why-google-stores-billions-of-lines-of-code-in-a-single-repository/fulltext https://cacm.acm.org/magazines/2016/7/204032-why-google-stor...
  • nkskalyan 7y ago
    It is an internally built versioning tool with similar interface as perforce https://cacm.acm.org/magazines/2016/7/204032-why-google-stores-billions-of-lines-of-code-in-a-single-repository/fulltext https://cacm.acm.org/magazines/2016/7/204032-why-google-stor...
  • dekhn 7y ago
    http://info.perforce.com/rs/perforce/images/GoogleWhitePaper-StillAllonOneServer-PerforceatScale.pdf http://info.perforce.com/rs/perforce/images/GoogleWhitePaper...
    • jefftk 7y ago
      That's from when Google still ran on Perforce. Around 2012-2013 we moved over to an in-house system that provides a similar interface (see link in sibling comments).
      • dekhn 7y ago
        Yes, I know quite well. I was explaining to folks who were wondering how Google got perforce to scale when they used it. I thought using a RAMSAN was very.... creative.
  • topher200 7y ago
    > 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/
• aviraldg 7y ago
  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.
  • siquick 7y ago
    > 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.
    • hckr1292 7y ago
      Social empathy is a really important skill, especially when delivering feedback. However, the best way to improve as a junior developer is to welcome feedback, not panic. 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.
      • siquick 7y ago
        I'm sure we've all worked in places where a senior would not like it if a junior reviewed their code, regardless of the juniors intent. But yes, always a tonne to learn. 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.
• whydoyoucare 7y ago
  "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! ;-)
  • cameronbrown 7y ago
    Code Review Guidelines are an amalgamation of said technical facts and data, data gained through experience of the company's entire lifespan, but also mixed in with a ton of personal preferences along the way. The trick is separating the two ;)
  • electrograv 7y ago
    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.
    • heavenlyblue 7y ago
      Isn’t that simply a sign of deeper problems in the organisation?
      • electrograv 7y ago
        Yes most likely; that’s my point. It can become a serious organizational problem when useful codified heuristics like this are able to be abused in this way (by treating them as immutable rules, or used as a weapon to win arguments).
    • enneff 7y ago
      > 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?
• aledalgrande 7y ago
  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 7y ago
    Some places I've worked explicitly won't push/deploy/similar on Friday.
    • aledalgrande 7y ago
      That's a good policy. We currently don't deploy on Friday afternoons.
    • atdt 7y ago
      But it's such a trivial change...! What could go wrong?
    • Gibbon1 7y ago
      I've mostly worked for small industrial light manufacturers. First job I worked at they were always late with shipping stuff. And always taking stuff down to the UPS office after hours. After watching this for a few years I got the shop manager to adopt a rule. If it's not ready to be boxed up before lunch, it's not shipping today. 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.
      • winrid 7y ago
        Forgive me, I don't understand your last sentence. It sounds contradictory vs "they got a lot more productive".
        • LanceH 7y ago
• cosmotic 7y ago
  > At Google, we hire great software engineers, and you are one of them. That's a pretty presumptuous comment.
  • dang 7y ago
    Maybe so, but can you please not post unsubstantive comments to HN?
  • username90 7y ago
    I don't know a single company that doesn't hire great engineers.
• stackzero 7y ago
  >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
  • Cthulhu_ 7y ago
    Yeah, that was the first section I read and I raised an eyebrow, I hadn't thought of it like that yet. I can see how one would get paralyzed by a focus on perfection; I too have left idk, dozens of comments on a feature PR of a few hundred lines of code. 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.
  • roryokane 7y ago
    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?
• sixstringtheory 7y ago
  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/
  • justicezyx 7y ago
    Of course, the hardest problem is not meant to be mitigated by a common and daily behavior like reviewing code.
  • 9nGQluzmnq3M 7y ago
    "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.
• bt848 7y ago
  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.
  • foobarian 7y ago
    Re: performance claims, well put. I often see coding decisions and review claims made on untested or outdated facts. For Java specifically three most common things I run across are: 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.
• afs35mm 7y ago
  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.
• heelix 7y ago
  I feel embarrassed to even ask... what does CL stand for?
  • jmgao 7y ago
    Changelist, it's perforce for "patch"
  • deleted 7y ago
    [deleted]
  • deleted 7y ago
    [deleted]
• flerchin 7y ago
  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 7y ago
    You are a hero and deserve a medal!
• ekianjo 7y ago
  If you were wondering what CL stands for since its not defined anywhere in this doc, it is apparently akin to a Pull Request.
• petilon 7y ago
  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!
  • tayo42 7y ago
    If your making mistakes like over engineering are you actually a good developer then if that is part of what bad code is. Maybe you fall into the expert begginer at that point
    • abacadaba 7y ago
      I'll take a noob who doesn't understand basic syntax all day every day vs. the dev who finds a way to make everything complex. At least in the former the damage is limited.
      • colek42 7y ago
        Good dev's understand how to simplify complex problems by breaking it into smaller components. Bad devd add complexity to already complex problems.
        • badpun 7y ago
          Breaking down complex problem does not simplify it, you merely move the complexity to the graph of dependencies between smaller components.
• techsin101 7y ago
  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
• andrew_ 7y ago
  It disturbs me greatly that this is needed. I understand why it is needed, but it's still disturbing.
• royalghost 7y ago
  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 ?
  • hankstenberg 7y ago
    Good point. At our company, we try to always assign two devs to more complex tasks, the actual developer and a "feature buddy". They don't actually PP the entire thing, but they discuss the strategy in pairs before starting to code and clarify question along the way in pairs. When the feature is done, the buddy will already have a very good idea about the feature and already agree with the fundamental decisions that were made along the way.
  • UK-Al05 7y ago
    There's been various studies saying that pair programming is not as effective as code review. 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.
  • bruckie 7y ago
    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 7y ago
      Jeff Dean and Sanjay Ghemawat coded like this for years.
  • CobrastanJorji 7y ago
    The point of reviewing code is to get a second set of eyes on it, partly for perspective and partly to keep you honest. Pair programming is almost strictly better. 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.
• chairleader 7y ago
  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.
• shakezula 7y ago
  > 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.
• ridaj 7y ago
  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 7y ago
    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 7y ago
      > 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...
• alexnewman 7y ago
  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.
• deleted 7y ago
  [deleted]
• Iv 7y ago
  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.
• awinter-py 7y ago
  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.
  • BigJono 7y ago
    It's not supposed to be a one size fits all I guess. 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.
  • iamchrisle 7y ago
    I think the saying is "Quality, speed, cost. Pick two?"
  • tylerl 7y ago
    Having experienced in real life the benefits of this policy myself, I absolutely disagree. I would institute Google-style code reviews at any company I went to that was large enough to have teams of 3 people or more. This as much for efficiency as for quality. The development pace isn't set by the speed at which new lines of code get added, is the speed at which features can reach maturity, and in general more extensive code reviews can speed things up in the aggregate in addition to making things work better. 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.
• esolyt 7y ago
  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.
  • throwaway_2047 7y ago
    Agree. I only realise it when reading through comments in HN
  • Deradon 7y ago
    It is explained at the top-level document: https://google.github.io/eng-practices/#terminology https://google.github.io/eng-practices/#terminology
• tylerl 7y ago
  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.
• aercolino 7y ago
  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.
• NKCSS 7y ago
  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 7y ago
    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 7y ago
      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.
• Traubenfuchs 7y ago
  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.
  • mav3rick 7y ago
    Code reviews by senior people are massively helpful for junior devs. It's one of the perks of being in a good big company that very few startups can replicate.
    • afarrell 7y ago
      Agreed. I once left a job in large part because there wasn't anyone else to review my code.
      • reallydude 7y ago
        It seems incredibly self-defeating, to quit over a lack of CODE REVIEW at any point in someone's career. 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.
        • Thiez 7y ago
          How is that self defeating from the programmers perspective? Code reviews can contribute to growth as a developer, ultimately helping ones career. When a developer desires such reviews, and a company is either unwilling or unable to provide them, why should they stay? There are plenty of other companies around to work at.
        • afarrell 7y ago
          So suppose you met someone who graduated uni a year ago, was working as the sole developer at a non-profit, and wanted to learn to be better at recognising and writing well-structured code. Suppose they then asked you if they should stay at their job or go work for a company where they were working with other software engineers and you said they shouldn't. How would you advise them to develop their sense of good code style?
• deleted 7y ago
  [deleted]
• merb 7y ago
  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 7y ago
    GitLab
• siempreb 7y ago
  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?
  • d33 7y ago
    > what is good naming 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.
  • afarrell 7y ago
    > What is good design, what is good naming, over-engineering A Philosophy of Software Design by John Osterhout is a good book on these questions.
  • 0x445442 7y ago
    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 7y ago
• antpls 7y ago
  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 7y ago
    > 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 7y ago
      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 7y ago
        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...
• syspec 7y ago
  what is a CL
  • ithkuil 7y ago
    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).
• hagy 7y ago
  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 7y ago
    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.
• ChrisCinelli 7y ago
  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 7y ago
    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.
• ElonMuskrat 7y ago
  Excellent guide. It would be fantastic for Google to adopt these practices.
• yegor256a 7y ago
  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.html https://www.yegor256.com/2015/02/09/serious-code-reviewer.ht...
• Noxmiles 7y ago
  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
• ed_balls 7y ago
  > 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 :)
• mendeza 7y ago
  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.html https://www.textbook.ds100.org/ch/02/design_srs_vs_big_data.... 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?
• jdhzzz 7y ago
  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"
• cellular 7y ago
  Mandatory pre checkin automated testing. Did you do this? Ok, you're good! ;-)
• lunchables 7y ago
  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.
• patthebunny 7y ago
  It's a shame my irrational hatred and distrust of all things google prevents me from reading this document. Also is it really that insightful? Code reviews have been standard at competent companies for a long time.