Earlier quoted context omitted.
what does the _CL_ notation indicate?
Changelist, perhaps?
Thanks. This would be a nice thing to explain in the document that's open sourced.
How to do a code review
171–180 of 376 posts
Re: How to do a code review
#172Earlier quoted context omitted.
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
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.
Good dev's understand how to simplify complex problems by breaking it into smaller components. Bad devd add complexity to already complex problems.
Re: How to do a code review
#173Earlier quoted context omitted.
At Amazon, all code is reviewed. Comments are constructive and straight to the point. Standard courtesy applies, but anything more than that is left out. Unless there's something extremely interesting, overly cautious phrasing and "positive" comments are mostly noise. Also, people tend to specify when comments are nits.
Why the scare quotes around "positive"? I have lots of positive thoughts when doing code reviews. I don't write them into comments all that often, but sometimes I do. "Oh I didn't know about this API, nice find!" "Ah, nice approach, this is a big improvement." "Thanks for improving the test coverage!" Maybe this seems like unactionable "noise", but it isn't, it encourages future actions of the kind being positively r…
Agree, I love to point out a nice approach or a TIL, and I like that Google put it down in writing. If Amazon really considers it noise, it is very descriptive of their culture.
Re: How to do a code review
#174Earlier quoted context omitted.
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.
I never considered quitting, but it still feels bad. Putting your code out there for the first time feels vulnerable (sort of similar to publishing your writing or public speaking) and it feels bad to get shit on. 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 co…
Your lead should really be setting you up for success on your first PR. I always made sure I paired with my new employees before they hit submit on their first few PRs. It is much easier to be empathetic when you are next to a person. It would also give me time to deep time into certain aspects of our codebase that I felt they could handle in future sprints.
Re: How to do a code review
#175Here'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…
I've been pushing the idea that if you're getting meaningful feedback on design (and over design) in your PR reviews than you've failed. That stuff should be shaken out long before you have working, complete code.
> I've been pushing the idea that if you're getting meaningful feedback on design (and over design) in your PR reviews than you've failed. That stuff should be shaken out long before you have working, complete code.
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.)
Re: How to do a code review
#176Earlier quoted context omitted.
Changelist, perhaps?
Thanks. This would be a nice thing to explain in the document that's open sourced.
The HN submission links directly to https://google.github.io/eng-practices/review/reviewer/ but if you start at the top namely https://google.github.io/eng-practices/ then there's a Terminology section right there on the first page. Maybe the GitHub Pages theme used by this site should be changed to one in which where every page links to its parent page (right now there doesn't seem to be any way to navigate upwards except by editing the URL).
Re: How to do a code review
#177It'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 u…
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 o…
> 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...
Re: How to do a code review
#178I'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 c…
> 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.
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.
Re: How to do a code review
#179This 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…
At Amazon, all code is reviewed. Comments are constructive and straight to the point. Standard courtesy applies, but anything more than that is left out. Unless there's something extremely interesting, overly cautious phrasing and "positive" comments are mostly noise. Also, people tend to specify when comments are nits.
If this is representative of the Amazon engineering culture, I'm gonna add this to the (already overflowing) pile of reasons I would never work there.
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.
Re: How to do a code review
#180Earlier quoted context omitted.
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…
Forgive me, I don't understand your last sentence. It sounds contradictory vs "they got a lot more productive".
They got fewer returns/defects when they stopped rushing.