Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Software Engineering at Google

books.google.com

Post reply on HN
Print view View original

61–70 of 73 posts

Re: Software Engineering at Google

#61

Post by thedance »

shaftway wrote: post #60
thedance wrote: post #57

Earlier quoted context omitted.

The readability process at Google is a way to gain the ability to submit code without a language style reviewer. To submit code into google3 (their main repo for server-side stuff), a changelist requires acceptance by a person or people who collectively own all of the code being changed, and a person or people who collectively "have readability" in all of the languages in the change. The simplest example is there is…

The way I've described it to people is that you need to get at least one review from someone who... - is an Owner of the code - has Readability in the language of the code - is not you If you are an owner and have readability, you just need someone who isn't you to sign off on it. If you don't have ownership or readability you need to get someone who has those things to sign off. If you can find someone who has both,…

Definitely. Being an SRE with C++ readability, for example, is like a permanent employment guarantee.

I'd like to acknowledge the brilliance of people who name their code files "foo.javascript" to avoid having to get JS readability.

Re: Software Engineering at Google

#62

Post by shaftway »

thedance wrote: post #61
shaftway wrote: post #60

Earlier quoted context omitted.

The way I've described it to people is that you need to get at least one review from someone who... - is an Owner of the code - has Readability in the language of the code - is not you If you are an owner and have readability, you just need someone who isn't you to sign off on it. If you don't have ownership or readability you need to get someone who has those things to sign off. If you can find someone who has both,…

Definitely. Being an SRE with C++ readability, for example, is like a permanent employment guarantee. I'd like to acknowledge the brilliance of people who name their code files "foo.javascript" to avoid having to get JS readability.

I'd lump them into the category of people who would make production code depend on things in experimental to get around code reviews.

Re: Software Engineering at Google

#63

Post by thedance »

shaftway wrote: post #62
thedance wrote: post #61

Earlier quoted context omitted.

Definitely. Being an SRE with C++ readability, for example, is like a permanent employment guarantee. I'd like to acknowledge the brilliance of people who name their code files "foo.javascript" to avoid having to get JS readability.

I'd lump them into the category of people who would make production code depend on things in experimental to get around code reviews.

I’m pretty sure that will trigger reviewed-and-submitted code audits for any role that handles PII. Don’t want the HN-reading public to get the wrong impression.

Re: Software Engineering at Google

#64

Post by jbn »

shaftway wrote: post #59
jbn wrote: post #55

Earlier quoted context omitted.

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).

Every team I've been on at Google has presubmits that enforce this, along with command line tools to "fix" it. There's no arguments about spacing. Nobody really cares about extra lines, but if you use tabs instead of spaces you're just wrong and your CL can't be submitted. It's actually nice. I disagree with some of the rules and would like them to change. I think that if your if-block has a single statement and ther…

w.r.t "if you use tabs instead of spaces you're just wrong", can you expand on that?

Do you mean 'wrong' in the sense of 'wrong according to the rules enforced by the linter' or 'wrong philosophically' ? If it's the latter, how is that wrong and is not even a subject for debate?

This is an honest question, I guess I just don't see how something like this can "just" be wrong (to me it's irrelevant and not worth the ink used to write such statements in code reviews... though I'm playing devil's advocate here obviously :-) ).

PS: of course if pre-submit checks enforce the rules, then as you said it becomes a non-issue. I have been in environments where there are no such tool-enforced checks, yet people comment on it endlessly, routinely blocking code reviews for whitespace issues...

Re: Software Engineering at Google

#65

Post by jbn »

thedance wrote: post #58
jbn wrote: post #55

Earlier quoted context omitted.

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).

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.

I agree, automating this so as to make it a non-issue is really the only way to deal with this at scale (i.e. in presence of a lot of change and many reviewers).

Re: Software Engineering at Google

#66

Post by kortilla »

joshuamorton wrote: post #43
kortilla wrote: post #41

Earlier quoted context omitted.

The comment I replied to is literally highlighting people nit-picking punctuation as if it’s a good thing.

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…

I’m not assuming, this is based on my experience at Google. People that focused on punctuation rarely provided anything more than the superficial review.

Re: Software Engineering at Google

#67

Post by joshuamorton »

kortilla wrote: post #66
joshuamorton wrote: post #43

Earlier quoted context omitted.

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…

I’m not assuming, this is based on my experience at Google. People that focused on punctuation rarely provided anything more than the superficial review.

Again you're begging the question.

People who focus on punctuation do so at the expense of everything else, sure. But people who give thorough reviews give thorough reviews, including nitpicky comments.

I've given and received thousands of reviews at Google (in multiple languages, across multiple teams), and can say that my experience has been fairly consistent: code review is high quality and detail oriented, including, but not at all limited to, small things like grammar and comment formatting.

Re: Software Engineering at Google

#68

Post by kortilla »

joshuamorton wrote: post #67
kortilla wrote: post #66

Earlier quoted context omitted.

I’m not assuming, this is based on my experience at Google. People that focused on punctuation rarely provided anything more than the superficial review.

Again you're begging the question. People who focus on punctuation do so at the expense of everything else, sure. But people who give thorough reviews give thorough reviews, including nitpicky comments. I've given and received thousands of reviews at Google (in multiple languages, across multiple teams), and can say that my experience has been fairly consistent: code review is high quality and detail oriented, includ…

I’m not begging the question, I’m just sharing my experience from working at Google and other companies. By a wide margin, the most useless reviews were ones nit-picking formatting, punctuation, etc.

> But people who give thorough reviews give thorough reviews

Ok.

> including nitpicky comments.

Disagree. Pointing out that someone forgot to capitalize an initialism in a comment is not thorough, it’s a time suck. A thorough and competent reviewer will realize it doesn’t matter and will know not to distract the CL owner will such irrelevant trash.

Sure, there is an a small section of the Venn diagram that leaves good review comments and sprinkles in “nit:” turds, but that group is much smaller than both the fully incompetent nit-pick group and the competent reviewer group.

Seriously, unless a code comment is illegible or it’s being compiled into user-facing docs, it’s absolutely useless to leave something like “nit: it’s TCP, not tcp”.

Re: Software Engineering at Google

#69

Post by joshuamorton »

kortilla wrote: post #68
joshuamorton wrote: post #67

Earlier quoted context omitted.

Again you're begging the question. People who focus on punctuation do so at the expense of everything else, sure. But people who give thorough reviews give thorough reviews, including nitpicky comments. I've given and received thousands of reviews at Google (in multiple languages, across multiple teams), and can say that my experience has been fairly consistent: code review is high quality and detail oriented, includ…

I’m not begging the question, I’m just sharing my experience from working at Google and other companies . By a wide margin, the most useless reviews were ones nit-picking formatting, punctuation, etc. > But people who give thorough reviews give thorough reviews Ok. > including nitpicky comments. Disagree. Pointing out that someone forgot to capitalize an initialism in a comment is not thorough, it’s a time suck. A th…

> Seriously, unless a code comment is illegible or it’s being compiled into user-facing docs, it’s absolutely useless to leave something like “nit: it’s TCP, not tcp”.

I think you're describing style errors, not grammar errors. For example, the Google style guide says that (certain) comments should be complete sentences. Asking people to use good grammar (and capitalize/punctuate) those sentences as though they were documentation (because they are!) is valuable for reviewers.

I say this as someone who, when reading unfamiliar code, is able to understand it more readily if the inline documentation is easy to read. "tcp" vs "TCP" doesn't impact that, but other grammatical nits absolutely due.

Re: Software Engineering at Google

#70

Post by kortilla »

joshuamorton wrote: post #69
kortilla wrote: post #68

Earlier quoted context omitted.

I’m not begging the question, I’m just sharing my experience from working at Google and other companies . By a wide margin, the most useless reviews were ones nit-picking formatting, punctuation, etc. > But people who give thorough reviews give thorough reviews Ok. > including nitpicky comments. Disagree. Pointing out that someone forgot to capitalize an initialism in a comment is not thorough, it’s a time suck. A th…

> Seriously, unless a code comment is illegible or it’s being compiled into user-facing docs, it’s absolutely useless to leave something like “nit: it’s TCP, not tcp”. I think you're describing style errors, not grammar errors. For example, the Google style guide says that (certain) comments should be complete sentences. Asking people to use good grammar (and capitalize/punctuate) those sentences as though they were…

> I think you're describing style errors, not grammar errors.

The entire thread (not started by me) is about nit-picking punctuation and now you’re trying to explain to me that I’m describing style errors. So what? It’s useless feedback regardless of which chapter in the Holy Google Docs covers it.

> Asking people to use good grammar (and capitalize/punctuate) those sentences as though they were documentation (because they are!) is valuable for reviewers.

Right, it’s valuable for the reviewers because it makes it seem like they provided feedback when they have nothing of substance to add. It’s not valuable for the author though nor is it realistically valuable to future people reading the code in question.

>I say this as someone who, when reading unfamiliar code, is able to understand it more readily if the inline documentation is easy to read.

I think maybe you don’t know what “nitpick” means. https://www.dictionary.com/browse/nitpick

If it has a meaningful impact on the ability to read it, it’s not a nitpick.

Post reply on HN