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

301–310 of 376 posts

Re: How to do a code review

#301

Post by afarrell »

mytailorisrich wrote: post #268
jsolson wrote: post #223

Earlier quoted context omitted.

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

> A code review is not the time and place to report positive ... comments.

Why not?

(Convoluted comments should be made more concise)

Re: How to do a code review

#302

Post by afarrell »

UK-Al05 wrote: post #202
sanderjd wrote: post #117

Earlier quoted context omitted.

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…

Depends how it's come across. If your not careful it can come across as patronising.

> it can come across as patronising

Sometimes, it is worth the risk of sounding patronising.

Re: How to do a code review

#303

Post by afarrell »

mav3rick wrote: post #254
Traubenfuchs wrote: post #245

I have never not seen code reviews end up being a massive social/cultural pain point for most people involved. Most people hate getting their code reviewed in depth. In situations where there isn't just one lead dev who is responsible for reviews, this leads to "Merge Request symbiosis", where two people uncritically approve each others requests so they don't have to deal with the third.

Code reviews by senior people are massively helpful for junior devs. It's one of the perks of being in a good big company that very few startups can replicate.

Agreed. I once left a job in large part because there wasn't anyone else to review my code.

Re: How to do a code review

#304

Post by matwood »

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.

Unfortunately many people stink at communication. The best thing a reviewer can do during a code review is ask questions. A reviewer who comes in and just says 'this is wrong' misses an opportunity to either learn themselves or actually teach the submitter. A much better approach is to ask the submitter why they did something certain way. What were their thoughts, and what did they see.

IME, asking questions leads to much better outcomes. Sometimes the submitter will just admit they were not thinking about anything and see their own errors. Upside, they found the issue themselves. Other times they'll explain some complexity or use case only they could see while deep in the code. Upside, I just learned something.

In general the best teachers ask questions. It doesn't put people's egos on the defensive, and tends to help people learn more effectively (the goal if any review IMO). I've seen this work on me in other scenarios like Jiu-Jitsu. My teacher will ask mid-roll or afterwards why I did something, no matter how stupid it was. Explaining my thought process helps me to better recognize the error, and I'm not immediately on the defensive. I'm then much more receptive when the teacher goes "that wasn't a terrible idea, but if you did this...".

Re: How to do a code review

#305

Post by afarrell »

siempreb wrote: post #261

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

> What is good design, what is good naming, over-engineering

A Philosophy of Software Design by John Osterhout is a good book on these questions.

Re: How to do a code review

#306

Post by afarrell »

hagy wrote: post #283

What about the importance of handling high-level architecture and design before writing code? This can be done in design docs, discussion on tickets, slack discussions or in person with a whiteboard. I think it's more productive to handle these discussions and reviews before writing any code. What do other people think?

I would agree. I think if you're talking about high-level design in a code review then either:

A) You've revealed a need to get more alignment on design, and should probably be explicit that the discussion has this purpose.

B) You're doing a code review of a spike so that you can get alignment about the design lessons the spike teaches...and should be explicit that the discussion has this purpose.

Re: How to do a code review

#307

Post by papito »

pjm331 wrote: post #213
ScottBurson wrote: post #191

Earlier quoted context omitted.

Very well said! My own experience is that effort invested in removing restrictions and handling corner cases is generally well spent. It may not seem too onerous to have to remember that a particular function works only for nonempty inputs or when called after some other function, but in a large system where many operations have such restrictions, keeping track of them quickly overwhelms the capacity of human memory.…

Yeah absolutely. I think a big part of it is considering possible edge cases or failure scenarios and not necessarily solving them immediately but considering how your design might need to be changed in order to solve them. Many times I have found there’s a solution that requires little or no more work than a naive implementation but is far more robust to future changes. To use inheritance as an example - if there’s…

That's a pattern I learned a long time ago. It's much more future-proof to treat items as an array, even if it seems over-engineering at first, because scope-creep dictates it's more likely you are going to handle more items in the future, not fewer. And dealing with one item vs. many can change your architecture pretty drastically.

Re: How to do a code review

#308

Post by mytailorisrich »

afarrell wrote: post #301
mytailorisrich wrote: post #268

Earlier quoted context omitted.

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

> A code review is not the time and place to report positive ... comments. Why not? (Convoluted comments should be made more concise)

Because a code review is not to pat each other on the back, it is to inspect and report issues. It is already costly enough without going off topic.

As said, if you want to praise then you are free to do it offline.

This is a professional procedure in a professional setting, not warm words from an encouraging teacher at school... I don't want to have to go through comments that do not add any value to the exercise of finding issues, and I have never seen people leave such comments in 20 years.

Re: How to do a code review

#309

Post by dekhn »

jefftk wrote: post #45
dekhn wrote: post #19

Earlier quoted context omitted.

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

Yes, I know quite well. I was explaining to folks who were wondering how Google got perforce to scale when they used it. I thought using a RAMSAN was very.... creative.

Re: How to do a code review

#310

Post by SkyBelow »

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…

The problem comes in when you can anticipate a problem and also anticipate not being given the resources to solve the problem then when you do have those resources now.

For example, had a user who wanted a document produced in one specific format. They promised this was what they wanted and it wouldn't change. It changed 5 times as we were nearing release. So I over-engineered it to allow a template to produce the document that can easily be changed (compared to the release process for a code change). It ended up being used numerous times, despite the problem were given clearly states an unchanging template.

Post reply on HN