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

311–320 of 376 posts

Re: How to do a code review

#311

Post by Nimitz14 »

kccqzy wrote: post #200
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…

Does Google really practice what they preach though? Just recently I was looking at Angular, Google's web frontend framework. Services, modules, directives, angular-specific markup. Coming from React, I find this grossly over-engineered.

Yeah, based on my experience with android and tensorflow they fail spectacularly at it.

Re: How to do a code review

#313

Post by 0x445442 »

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…

I agree with you. This "Agile" cancer that has infected the industry needs to die now. Someone here called it collectivized micromanagement and that's the best term I have heard for what's become of "Agile".

Re: How to do a code review

#314

Post by BeetleB »

molodec wrote: post #296
BeetleB wrote: post #158

Earlier quoted context omitted.

bt848 already said it, but you picked a really poor example. In many real world scenarios, a linear search beats a binary search due to a lack of overhead. Along those lines, in C++, a vector very often performs better than a theoretically better data structure. I learned C++ fairly well from a very experienced guy in the company, and he said that you should always benchmark against a vector. I suspect if I was in hi…

Yes, the size of the data matters, and linear search may be the right choice for many use cases and may outperform more advanced data structures. You learn it on the job as it is not a topic usually studied in universities. But I don't agree that we need to completely throw away theory, O complexity analysis where N - input size is the main variable. Microbenchmarks are hard to write correctly. Recreating the scenari…

>But I don't agree that we need to completely throw away theory, O complexity analysis where N - input size is the main variable.

No one here is agreeing with that.

Re: How to do a code review

#315

Post by omnimike »

SkyBelow wrote: post #310
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 do…

I would argue that as soon as they asked to change the format the first time then the template solution is no long over engineering as it has been demonstrated that the format can and will change. In general I wouldn’t call it over engineering if the problem you are trying to solve occurred in he past and you have reason to suspect it will occur again. The problem with over engineering is solving problems that have never (and may never) occur.

Re: How to do a code review

#316

Post by aledalgrande »

UK-Al05 wrote: post #204
Glyptodon wrote: post #27

Earlier quoted context omitted.

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

This implies they don't have very good automated deploy, monitoring and automated rollback solution. The test of a good cd is that your ok with releasing on Friday at end of the day.

You are assuming that monitoring and rollback solutions are bug free, always on and have 100% knowledge of what's going on. That is never true.

Re: How to do a code review

#317

Post by jf22 »

SkyBelow wrote: post #310
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 do…

If the user did not have that problem wouldn't you have wasted engineering time?

Re: How to do a code review

#318

Post by jsolson »

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…

I disagree, with a caveat. Assuming your review tool supports "Resolved" or "No Action Required" comments (ours does), it's rather easy to distinguish something that is informative from something that is actionable. I am now recalling that some review tools don't distinguish open/unresolved comments from resolved or informative comments, which would make this more of a trade-off than an obvious win.

Re: How to do a code review

#319

Post by bt848 »

flukus wrote: post #207
bt848 wrote: post #88

Earlier quoted context omitted.

You can draw bright lines about some of these things. If a change claims to fix some bug, a test must demonstrate that. If I patch just the test into HEAD and run it, it should fail. If this is not the case then the change "needs more tests". "Not readable" is why Google also has the "readability" process. A person without readability needs the pre-submit approval of someone with readability in that language. After a…

How would I go about writing a test for an extremely rare race condition between two components 10 levels of indirection away covering 10s of thousands of lines of code? Because I've fixed bugs like that but can't fathom a way to write a test for it, I only even found the cause by grepping 6 months worth of logs and finding a recurring pattern. I briefly considered writing a debugger script to pause at the right spot…

There are tools for that stuff. You can run the unit tests a million times and count the failures for starters. You can also run tests under TSAN which often just points out races. You can also inject clocks or other dependencies and simply create the precondition necessary and advance time manually to show the problem.

Re: How to do a code review

#320

Post by jsolson »

mytailorisrich wrote: post #308
afarrell wrote: post #301

Earlier quoted context omitted.

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

> This is a professional procedure in a professional setting, not warm words from an encouraging teacher at school...

Citing a good practice in someone's code as "yes, please do more of this" alongside "don't do this please" is not, in my opinion, fluff.

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

So only pay attention to unresolved comments?

Post reply on HN