5 ms·

Chiming in here to agree with the others. In particular, finding simple, understandable solutions to complex problems is highly valued at Google. Complex solu

by alarge 7y ago

Chiming in here to agree with the others. In particular, finding simple, understandable solutions to complex problems is highly valued at Google. Complex solutions are an anti-pattern. And the comment about readability and testability is so far different from my experience that I have to wonder where the poster is getting their information. Google code reviews have to be among the most nit-picky I've ever seen in a 30+ year career, with serious attention to detail all the way down to punctuation in comments.

kortilla 7y ago
Nit-picky reviews are a smell, not a good thing. It can often mean that the reviewer is not really reviewing the big picture (is the intent of the change being fulfilled, is this the right place in the architecture, etc) and it making up for it by focusing on irrelevant details. Seriously, code review on punctuation in a comment is not good in any scenario and it speaks nothing to the legibility of the code itself. If anything, it might mean the code is unreadable and the reviewer is too embarrassed to point out that they can’t follow it.
- joshuamorton 7y ago
  This conclusion doesn't, at all, follow from the premise. While yes, nit-picky review could mean the reviewer doesn't understand the big picture, an LGTM with no comments at all is more likely to be indicative of missing something. What you really mean is that nit-picky reviews that don't pick up on actual logic or functionality issues are a smell. Which is true, but isn't usually the case at Google[0]. The chapter on teamwork covers some relevant concepts (like psychological safety). [0]: https://testing.googleblog.com/2018/05/code-health-understanding-code-in-review.html https://testing.googleblog.com/2018/05/code-health-understan...
  - kortilla 7y ago
    The comment I replied to is literally highlighting people nit-picking punctuation as if it’s a good thing.
    - joshuamorton 7y ago
      Which it is! You seem to be assuming, without prompt, that nit-picking punctuation comes at the expense of a thorough overall review. Code review on punctuation is absolutely an important part of readability. Consistency makes understanding and scanning (for human readability) and potentially parsing and modifying (for machine readability) much easier. As someone who both reviews a lot of code and reads a lot of code, consistent documentation with good grammar and punctuation is enormously helpful.
      - jbn 7y ago
        what about nitpicks related to whitespace? These are an endless source of time-wasting, IMHO.
        joshuamorton 7y ago
        At Google or in general? The closest I've seen to this is comments of the form "can you reformat this" or "this doesn't match the rest of the file". Google's style guides have fairly strong guidance on formatting, so there's usually a "right" way (enforced by a linter), and if not, then usually you defer to the existing style in the module.
        jbn 7y ago
        I meant in general. Except python-like languages that make it a non-issue, in C/C++/Java I think people attach way too much importance to spacing, trailing whitespace, tab vs spaces, and column width. I would agree with linting or just deferring to existing style, but in truth I care little about this, it's just not significant to me (yet others seem to overvalue it, to the point of talking exclusively about this).
        thedance 7y ago
        At Google (in google3) there are automatic linters that run on every change and if they fail then your change simply will not be approved for style. There's no debate about it. This is one of the great things about gofmt: either your Go program passed through it, or it did not and won;'t be accepted. Arguments about the formatting need to be in the form of a changelist against gofmt itself.

by alarge 7y ago

kortilla 7y ago

Nit-picky reviews are a smell, not a good thing. It can often mean that the reviewer is not really reviewing the big picture (is the intent of the change being fulfilled, is this the right place in the architecture, etc) and it making up for it by focusing on irrelevant details. Seriously, code review on punctuation in a comment is not good in any scenario and it speaks nothing to the legibility of the code itself. If anything, it might mean the code is unreadable and the reviewer is too embarrassed to point out that they can’t follow it.

joshuamorton 7y ago
This conclusion doesn't, at all, follow from the premise. While yes, nit-picky review could mean the reviewer doesn't understand the big picture, an LGTM with no comments at all is more likely to be indicative of missing something. What you really mean is that nit-picky reviews that don't pick up on actual logic or functionality issues are a smell. Which is true, but isn't usually the case at Google[0]. The chapter on teamwork covers some relevant concepts (like psychological safety). [0]: https://testing.googleblog.com/2018/05/code-health-understanding-code-in-review.html https://testing.googleblog.com/2018/05/code-health-understan...
- kortilla 7y ago
  The comment I replied to is literally highlighting people nit-picking punctuation as if it’s a good thing.
  - joshuamorton 7y ago
    Which it is! You seem to be assuming, without prompt, that nit-picking punctuation comes at the expense of a thorough overall review. Code review on punctuation is absolutely an important part of readability. Consistency makes understanding and scanning (for human readability) and potentially parsing and modifying (for machine readability) much easier. As someone who both reviews a lot of code and reads a lot of code, consistent documentation with good grammar and punctuation is enormously helpful.
    - jbn 7y ago
      what about nitpicks related to whitespace? These are an endless source of time-wasting, IMHO.
      - joshuamorton 7y ago
        At Google or in general? The closest I've seen to this is comments of the form "can you reformat this" or "this doesn't match the rest of the file". Google's style guides have fairly strong guidance on formatting, so there's usually a "right" way (enforced by a linter), and if not, then usually you defer to the existing style in the module.
        jbn 7y ago
        I meant in general. Except python-like languages that make it a non-issue, in C/C++/Java I think people attach way too much importance to spacing, trailing whitespace, tab vs spaces, and column width. I would agree with linting or just deferring to existing style, but in truth I care little about this, it's just not significant to me (yet others seem to overvalue it, to the point of talking exclusively about this).
        thedance 7y ago
        At Google (in google3) there are automatic linters that run on every change and if they fail then your change simply will not be approved for style. There's no debate about it. This is one of the great things about gofmt: either your Go program passed through it, or it did not and won;'t be accepted. Arguments about the formatting need to be in the form of a changelist against gofmt itself.