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

No code reviews by default

raycast.com

Post reply on HN
Print view View original

231–240 of 315 posts

Re: No code reviews by default

#231

Post by kqr »

XorNot wrote: post #219
sharken wrote: post #171

Earlier quoted context omitted.

It's the same in the financial sector, you can't just push code without a review. However, the review process is far from perfect and can create a false sense of security. To review a piece of code, that code should ideally be small in scope. For larger pieces it's rather common that the reviewer don't have enough time to do a good job. If there were better incentives for doing a review, then it would greatly improve…

I've had a lot of trouble getting people to do proper reviews. A minimum to me is that you actually compile and execute the code in some way to check its sane. Better would be the reviewer actually adds to the test suite for the code to prove their expectations of how it works. In almost all cases it's very hard to get people to look outside the web browser for the diffs. Diffs show you something, but never the whole…

Why should it be on the reviewer to compile and execute the code, rather than the author?

Or are you saying that one can actually find more bugs when two people compile and execute the same code rather than just one?

When I review code, I want the author to tell me how they have verified that the code has the desired effect -- but if they do that, I'm going to trust that they did the actual verification and I'm not going to simply retrace their steps. That seems like a waste of time to me.

Re: No code reviews by default

#232

Post by kqr »

mike_d wrote: post #164
mumblemumble wrote: post #24

Earlier quoted context omitted.

We (at least should) also do code reviews in order to make sure everyone understands the code. On my team, the majority of code review comments are not discussing potential bugs, they're making sure everybody knows how the new thing works, why it was designed that way, implementation tradeoffs, etc. All that discussion is extremely valuable over the long run. For example, it means that nobody has to pay attention to…

> do code reviews in order to make sure everyone understands the code. I like a hybrid "the person who wrote it can't deploy it" model. You get an informal code review and two sets of eyes/brains to the extent that the deploying engineer feels like they understand it enough to shepherd it into prod weighted against how important the component and the scope of the change is. It self scales from "copy change on the web…

Ooh, I really like this. Do you have practical experience with this or do you like it in principle?

Re: No code reviews by default

#233

Post by asiachick »

tus666 wrote: post #7

He mentions "trust" a lot. Is that why we do code reviews, because we don't trust each other? I find this attitude problematic. We do code reviews because humans make mistakes. Requirements can be misinterpreted. Different work in progress can be in conflict with each other. Reviews are a good way to learn from each other and keep abreast of what work is occurring outside your own bubble. Not doing code reviews becau…

Agreed, so many times reviewers have saved me from myself as well as passed on solutions I didn't know, pointed out functions in our giant codebase I wasn't aware of, taught me new language concepts I wasn't familiar/comfortable with (ie, C++11->14->17->20) and generally helped limit our technical debt.

I'm a fan of code review

For me, code review is collaboration. We're helping each other.

The one thing I'd suggest is, as much as possible, try to phrase reviews like a question: "Do you mean to do X here?", "Would using fooTheFob here work?", "Is this a race condition?". At least it feels better to me.

Re: No code reviews by default

#234

Post by kqr »

hsn915 wrote: post #222
alecbz wrote: post #27

Earlier quoted context omitted.

Yeah, IMO this is like a publisher saying "we don't have editors: we trust our authors". The intended ethos of editors and code review is that mistakes and imperfections are the norm, and you need a second pair of eyes to iron them out. That said, I do sometimes encounter individuals or cultures that seem to view code review more as a mechanism for catching abnormal/unexpected mistakes than as a normal part of the pr…

Books are published once, not through iterations. The equivalent in book writing would be that you are not allowed to write more than one page at a time, and someone must approve it before you start working on the next page.

Another difference between books and code is that books are read and interpreted by humans (who are very tolerant of grammar bad, in the grand scheme of things) and code is read and interpreted by computers (who will swallow any ambiguity whole and just steam on with their interpretation.)

If books were read by machines rather than humans, and were published incrementally, then it would make sense to ask someone to edit each iteration.

Re: No code reviews by default

#235

Post by asiachick »

hsn915 wrote: post #223
ljm wrote: post #14

Earlier quoted context omitted.

If something hit production and caused a major fuck-up because there was no peer review process, then in all the places I've worked at the first action item in the post-mortem would be "we should introduce peer review." Otherwise someone would ask how we could ensure it wouldn't happen again, and no one would be able to say "because we trust them," and leave it at that. It would sound more like "I trust you will neve…

This strikes me as weird excuse for code reviews. To spot quality problems before going to production .. what you need is testing. Not "unit testing" - which is just another pointless process that does not actually provide the purported benefits. You need a separate QA team that handles testing - manual and automated.

Our tests have caught 10s of thousands of bugs. We couldn't move forward without them. Maybe they just fit our use case more than yours.

In our case we have a public API and several backends. The tests run against the public API. To know that a new backend is working it has to pass all the tests. Getting any new backend working correctly without the tests would be nearly impossible.

Re: No code reviews by default

#236

Post by kqr »

tshaddox wrote: post #210
detaro wrote: post #121

Earlier quoted context omitted.

> Only for a drive-by take-down by someone with none of the context If that's a regular issue that's a culture problem. Starting with "if you've been talking with colleagues about your problem, why is someone with no context reviewing the result?", people not investing time in reviews, people doing "take-downs" instead of asking questions if they don't understand things, hold up merge for non-urgent concerns ... > Ev…

> If that's a regular issue that's a culture problem. Couldn’t one also argue that if code reviews are routinely catching problems mentioned earlier in the thread (misinterpreted requirements, conflicts with other WIP, etc.) then that’s a cultural problem? It just seems odd to assume that the person assigned the task couldn’t possibly be expected to routinely avoid those problems, but that adding rigorous code review…

Yup, definitely. Code review discovers both issues stemming from cultural problems and silly bugs and understandability issues. Peer review in general is a very powerful tool for things that should be designed to satisfy customer needs.

Re: No code reviews by default

#237

Post by kqr »

tombert wrote: post #221
dsjoerg wrote: post #197

Earlier quoted context omitted.

Sure. Seems like a classic "seven whys" kind of situation... coming down hard on the person who wrote the bug is counterproductive in a way that the whole tech industry understands, except for your former bosses at Apple.

To be clear, I’m not absolving myself on this. The bug was still my fault at the end of the day, I just feel like my managers handled it poorly.

Did you purposefully introduce this bug? Then it was your fault. Otherwise, from all the evidence we have, you couldn't predict it from where you were standing at the time. You can't be at fault for that.

Any other conclusion is a cultural problem that hints at a "shoot the messenger" philosophy ruling.

If we want a Westrum generative culture (and we do!) we can't go around assigning fault to ourselves or others when we end up in bad situations.

Focus on whether the process is good or bad, not where individuals ended up due to random variance around the mean.

End rant. Sorry, this is one of the major things that upset me about how other people run their organisations.

Re: No code reviews by default

#238

Post by bawolff »

I think code review is like any other quality control: they come at a cost, but its worth it if implemented well. If the culture around code review is bad then its utter hell.

Re: No code reviews by default

#239

Post by fjfaase »

kgeist wrote: post #226

It probably makes sense when all the developers are experienced seniors with deep knowledge of the codebase, but often it's not the case. I don't think it scales beyond a small team of senior founders who only start making their first MVP. According to Crunchbase, they were founded 2 years ago and have below 10 employees. At our company, code reviews help: 1) onboard new devs and teach them about the codebase, catch…

The article is not saying that no code reviews are done, but that it is not manditory. For on boarding code reviews are useful, but you could also do some pair programming or work together on the code.

Re: No code reviews by default

#240

Post by loosescrews »

If you find code reviews are significantly slowing down your process, the problem might be your code review tool. My employer switched from Github to Gerrit and I have for the most part stopped feeling like code reviews are slowing me down. I just keep stacking my changes and the reviewers can get to them when they have time. This also allows me to review others' changes when I have time and not feel like I need to drop everything in order to unblock people.
Post reply on HN