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

261–270 of 376 posts

Re: How to do a code review

#261

Post by siempreb »

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?

Re: How to do a code review

#262

Post by rajangdavis »

siquick wrote: post #22
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'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…

100's of comments should raise a red flag. I think there are 1 of 3 scenarios at play:

1. The commit/change is too large.

2. The reviewer is nit-picky.

3. The person that wrote the code made a lot of mistakes.

I really feel like if a reviewer is making 100 comments on a change, they're doing something wrong even if there are a lot of mistakes. That reviewer should really reach out to the person that wrote the code and talk to them human to human.

Re: How to do a code review

#263

Post by dgellow »

Arnavion wrote: post #92
compiler-guy wrote: post #32

Earlier quoted context omitted.

People have their own internal stories about how good they are, and all that feedback can be very painful. It doesn't help that many comments are nitpicks and pedantry, made by people without much social empathy. This is especially true when sending a patch for a high-level review of your proof of concept, and the next thing you know people are complaining about your formatting.

It can be hard to review badly formatted code. The reviewer has to put in effort to understand your change, but they have to put in more effort to parse the change if the formatting is bad. "Why is this function's return value not being checked? Oh, turns out this person used a yoda conditional even though the rest of the code doesn't do that." "Why is this block of code running even when the condition is false? Oh,…

That’s where gofmt shines :-)

Re: How to do a code review

#264

Post by stebann »

euske wrote: post #7

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…

Never heard about effective patience and respect for other people's work? After all, people who wrote this have some experience...

Re: How to do a code review

#265

Post by antpls »

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.

Re: How to do a code review

#267

Post by afpx »

petilon wrote: post #99

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…

It’s an old person thing. We learned to program way before the internet, when one only had a language reference, some vague requirements, and a lot of time. Things took much longer, and communication tools didn’t exist. Maintainability was once a big deal.

These days, it’s easier to do a search, grab a snippet, and paste. There’s little need for reusability. Extrapolating a bit, I imagine in a few years, we’ll have one big ‘library’ of functions built into our IDE (built, and copyrighted by Google or Microsoft, of course).

Programming used to be skilled labor, but now it’s kind of dumbed down for higher productivity. A natural evolution.

Personally, I’m trying to re-train myself for the more modern rapid-fire programming. I’m not completely convinced that it results in better products, but it does feel good to be constantly committing.

Re: How to do a code review

#268

Post by mytailorisrich »

jsolson wrote: post #223
amxpd wrote: post #96

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

> Unless there's something extremely interesting, overly cautious phrasing and "positive" comments are mostly noise. Not really. Positive comments help teach engineers which things they've done that conform to local best practices (and why) without them having to meticulously dig those up (assuming they're even documented). A lack of positive comments leaves engineers to learn them only by running afoul of them. Effe…

A code review is not the time and place to report positive or convoluted comments. A code review is to find issues and to report them clearly.

The only positive thing to report should thus be that all is fine if the reviewer did not find any issues.

Now, if you are conducting the review in a meeting then obviously you can make oral comments in passing. If you're using a software tool for reviews then the all the comments should be on point. Nothing prevent you from talking to your coworker afterwards to spread some love if you want to.

Re: How to do a code review

#269

Post by ithkuil »

syspec wrote: post #266

what is a CL

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

Re: How to do a code review

#270

Post by shusson »

antpls wrote: post #265

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 c…

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

Post reply on HN