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

How to do a code review

google.github.io

Post reply on HN
Print view View original

41–50 of 376 posts

Re: How to do a code review

#41

Post by dmitrygr »

zamadatix wrote: post #24
ipsum2 wrote: post #16

Earlier quoted context omitted.

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

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

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

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.

Re: How to do a code review

#42

Post by fma »

joefkelley wrote: post #38
Glyptodon wrote: post #27

Earlier quoted context omitted.

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

I worked at one (horrible) place where we explicitly only deployed on Friday afternoon. The logic was we have fewer users over the weekend, so if something goes wrong, fewer people will notice.

I rewrote an app that used to only deploy on weekends because we do a lot of processing on weekday afternoons and nights. I changed the deployd to the mornings.

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.

Re: How to do a code review

#43

Post by vvanders »

mieseratte wrote: post #25
aviraldg wrote: post #13

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, i…

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

Thanks For The Feedback[1] is a pretty decent book on the subject. Highly recommended if you're interested both in the mechanisms and improving how you receive/give feedback.

[1] https://www.amazon.com/Thanks-Feedback-Science-Receiving-Wel...

Re: How to do a code review

#44

Post by jefftk »

cpeterso wrote: post #26
hota_mazi wrote: post #21

Earlier quoted context omitted.

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

A Perforce changelist is more like a git commit than a GitHub pull request.

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.

Re: How to do a code review

#45

Post by jefftk »

dekhn wrote: post #19
jorblumesea wrote: post #8

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

http://info.perforce.com/rs/perforce/images/GoogleWhitePaper...

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

Re: How to do a code review

#46

Post by jefftk »

dmitrygr wrote: post #41
zamadatix wrote: post #24

Earlier quoted context omitted.

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

If you live in CA, you're good to do it: 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.

> do not compete with your workplace

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.

Re: How to do a code review

#47

Post by ereyes01 »

aviraldg wrote: post #13

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, i…

I once worked on a team that specialized in very long littanies of code review comments... but they were able to bake this into their culture in fundamental ways such that it ended up being one of the most positive experiences in my software engineering career.

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.

Re: How to do a code review

#49

Post by sixstringtheory »

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

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

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

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

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

Re: How to do a code review

#50

Post by dahart »

aviraldg wrote: post #13

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, i…

Agreed. I find this extra important when working remotely or with other people who work remotely. If you’re not getting any face-time, critique has to be balanced with positivity. In my experience it’s true even with very experienced team members, and even with the very serious & stodgy people who avoid chit-chat and claim to not be bothered by feelings & opinions.
Post reply on HN