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

291–300 of 315 posts

Re: No code reviews by default

#291

Post by LenP »

kubanczyk wrote: post #150
jeffbee wrote: post #122

Earlier quoted context omitted.

In Perforce the concepts are different. You have a client, with a view that includes a subset of the repo, and you have changelists which incorporates your related edits. When done with a changelist, you submit it to trunk, usually, because while you can branch a Perforce repo, you mostly don't need to. Basically all the stuff in the first half of the talk where he whines about stepping on toes, conflicting with othe…

Let's say we collaborate on the code below. I'll change line 1, you'll change line 5, we'll merge both changes and then we'll see how Perforce makes the code run successfully "without conflict or even awareness of other person". 1 x := 0 2 x++ 3 x++ 4 x++ 5 if x > 7 { 6 fail_horribly_at_runtime() 7 }

To answer your question, neither Git nor Perforce will cause a merge conflict. You’d need a SCM that isn’t line-based for that.

With large teams, you need code review (which this bug can elide), great test coverage (the tests will likely have a merge conflict) and very importantly, a commit-by-commit build health indicator after merging. You can also do this in batches and bisect to find poisonous commits should a batch be broken - flakes notwithstanding.

Re: No code reviews by default

#292

Post by adinisom »

detaro wrote: post #177
atomashpolskiy wrote: post #120

The problem is not with code reviews but with how adversarial they have become. Instead of making the best attempt to comprehend and embrace the author's style and intention and finding compelling arguments for the question "why this code is probably fine and should be merged as-is?" people nit on all kinds of stuff, mostly highly subjective. Along the way they don't actually catch that many problems or bugs but inst…

> The problem is not with code reviews but with how adversarial they have become. I've never really seen that. What do you think is the reason for people thinking that's the thing to do?

Incentives set up a race to the bottom.

I worked in an environment that was like Raycast and later adopted gatekeeping code reviews. If the code reviewer wasn't happy you couldn't merge. This gives the reviewer power to have the author shape the code the way the reviewer prefers at little cost to the reviewer. And of course the authors often respond by doing likewise when asked to review code. My approach as a reviewer was to provide feedback without blocking merges, but this alone is not enough to incentivize reciprocal behavior.

There was a guy at work who would always complain about numbers that weren't given names as #defines regardless of whether it made sense or not (Magic Numbers!). My co-worker asked him why he was so picky about it. Apparently because someone did that to him.

Re: No code reviews by default

#293

Post by dragonwriter »

mpweiher wrote: post #281
dragonwriter wrote: post #248

Earlier quoted context omitted.

> Books are published once, not through iterations. Incorrect. Bugs (“errata”) are regularly fixed in bugfix versions (“printings”) of books, whereas feature changes are regularly fixed in more major versions (“editions”). And that’s after the equivalent of a “1.0” release. Books are also regularly iteratively released either to a closed group of reviewers or even the public (e.g., Manning MEAPs, PragProg Beta Books,…

> Bugs (“errata”) are regularly fixed in bugfix versions (“printings”) of books, But those fixes don't fix the already printed books.

> But those fixes don't fix the already printed books.

For actual physical books (as opposed to ebooks, where making the complete new version available to purchasers for errata, but usually not new editions, is common), this is usually true for logistical reasons, though issuing errata sheets (usually online now, though the practice is older but notification and distribution was harder in the past) to keep existing users up-to-date is not uncommon.

Re: No code reviews by default

#294

Post by adinisom »

yupper32 wrote: post #65
Abroszka wrote: post #63

Earlier quoted context omitted.

> I think it is unquestionable that code-reviews catch bugs. Yes, it does catch some, but does it catch more than no code review? There is no point in catching bugs if it also creates bugs to catch.

Sorry, are you saying that it's possible that code reviews cause more bugs than they solve? That's absurd...

I've seen it happen.

I was on a team of competent people working on a dense code base before and after introduction of gatekeeping code reviews. The code base was large so no two people would be working in the same area. Before code reviews:

- Team members working on a section of code would find and fix bugs in it.

- Team members would watch the repository history to monitor for any mistakes.

When gatekeeping reviews were introduced, they were not particularly effective at finding bugs because bugs tended to be subtle and required time working on the particular code in question to identify. But the introduction of gatekeeping code review caused bugs because:

- Once reviewed, code was assumed to be good enough because hey, two people agreed on it.

- Any bugs identified would require code reviews to fix which would be an uphill battle and probably involve project management.

- Nobody's looking that closely at history because who wants to deal with finding problems and pushing through fixes? And hey, it's already reviewed.

- Knowledge of code that is being reviewed becomes stale as you start working on other features. This impacts pre-commit testing as you make changes to mollify the reviewer, and thus lead to a bug.

- Lowering velocity lowers the rate of fixing bugs.

- A bug could be fixed but stuck in review, so it's still in the codebase for other developers to run across. You could argue: well, it's documented in JIRA! But bugs can manifest in different ways and affect multiple system.

Re: No code reviews by default

#295

Post by wreath »

LenP wrote: post #290
wreath wrote: post #254

Earlier quoted context omitted.

> At my current startup, we are trying something different: the code reviewer fetches the feature branch and writes tests for the change as part of the review. As ridiculous as that sounds, it’s been working better than we’d imagined. How is this different from the "traditional" QA-writing-tests-for-devs process?

QA writing unit tests for devs is not “traditional” - it’s a signal to leave the place ASAP. As an engineer, you’re expected to write, review, and test code. We simply shuffle responsibilities such that you usually test other engineers’ code and not your own (there is some discretion involved - for very small changes we often write or tweak the test ourselves)

Yeah that's why I put traditional in quotes. I guess, rather outdated? But whatever.. the question is how is this different? The engineer who writes the code still not the one who tests it, am I misunderstanding this?

Re: No code reviews by default

#296

Post by LenP »

wreath wrote: post #295
LenP wrote: post #290

Earlier quoted context omitted.

QA writing unit tests for devs is not “traditional” - it’s a signal to leave the place ASAP. As an engineer, you’re expected to write, review, and test code. We simply shuffle responsibilities such that you usually test other engineers’ code and not your own (there is some discretion involved - for very small changes we often write or tweak the test ourselves)

Yeah that's why I put traditional in quotes. I guess, rather outdated? But whatever.. the question is how is this different? The engineer who writes the code still not the one who tests it, am I misunderstanding this?

Ack. You’re right in that the engineer who writes the code is not the one who writes the test, but recall that all this happens at the code-review phase.

When QA finds holes when writing unit tests (rare), it goes on a backlog. For us, if the reviewer finds a bug or finds the code not very testable, your code isn’t getting merged. We catch both errors and design issues (especially premature optimizations and overly large API surface area) very early. Habitually, our engineers write simple, well-documented code with concise APIs at a well-defined level of abstraction (we also follow some other heuristics to make this obvious) on the first revision, so we rarely (6 total in the last 3 months if I’m querying correctly) have more than two revisions despite the additional ‘write tests’ step from the reviewer.

We found that engineers take great joy in trying to break others’ code, but also enjoy collaborating in a scratch-each-others-backs way when they get to give useful feedback. We’ve been able to channel that into great test coverage (around 99% FWIW). We’re pre-launch, so ultimately we may uncover bugs in production at the same rate as traditional code reviewing shops ¯\_()_/¯, but we’ve had close to zero issues needing manual intervention in months of early user testing.

Re: No code reviews by default

#297

Post by jaredsohn »

brabel wrote: post #257
jolux wrote: post #42

Earlier quoted context omitted.

Sure, I mostly meant automated processes. They won't always save you from yourself but doing blue-green, having an extensive test suite, etc, are all things that will help reduce the risk of a bad deployment.

If there's one thing I've learned over many years writing lots and lots of automated tests for everything is that no matter how far you go with your tests, barring formal proofs or equivalent, the tests cannot save you from, at some point, breaking production badly. One small, tiny thing that slipped through your tests and boom - huge fuck up (even small things can easily cause your whole new feature to fail miserabl…

> to fail miserably on the big launch

What big launch? :) Is useful to feature flag new functionality so that you can release gradually and spot the bugs - often via error monitoring and user feedback - before they affect everyone. Yes, realize it can happen but there are techniques to minimize the chance of it happening.

Re: No code reviews by default

#298

Post by gunapologist99 »

ahtihn wrote: post #256
mpweiher wrote: post #244

Earlier quoted context omitted.

> I want my code peer reviewed TFA: > Engineers [..] request reviews when they think it's necessary. Problemo solved.

If code reviews aren't an expected part of the development process and there's pressure on delivering, it won't happen.

This sounds similar to the peer review process in publishing. Typically, getting other people to actually agree to voluntarily review your work is indeed a significant hurdle if the requested paper/book/chapter/white paper/code review is lengthy.

There is an easy way to resolve this:

     Request peer review for SMALLER chunks of code.
Git makes this exceptionally easy -- it's certainly far easier than reviewing a large document with Word track changes!

Re: No code reviews by default

#299

Post by jolux »

btschaegg wrote: post #279
Ntrails wrote: post #275

Earlier quoted context omitted.

I don't believe that to be true. I believe the expected outcome is that people will still want code reviews on anything vaguely complex as insurance against fuckups . "Shit, WE missed something" is a nicer place to be than "I was sure I was perfect and I'm actually an idiot". It does however mean you don't sit around waiting for someone to approve a three line delete PR that you know is safe. The "Rubber Stamp" revie…

Since I am currently working in an environment that resembles GP's premise, I have to say that I find this take to be way too optimistic. I'd be exactly in the boat you describe (and actually: I've yet to see a situation where someone reviewing my code did not lead to improvements). But: If I want my code reviewed, I have to actually fight for it. Since everybody's workload is too high, even the willing often simply…

This is a fantastic comment, thank you for writing it. I especially appreciate the explanation of how management decisions produce engineering practices.

Re: No code reviews by default

#300

Post by jksmith »

btschaegg wrote: post #279
Ntrails wrote: post #275

Earlier quoted context omitted.

I don't believe that to be true. I believe the expected outcome is that people will still want code reviews on anything vaguely complex as insurance against fuckups . "Shit, WE missed something" is a nicer place to be than "I was sure I was perfect and I'm actually an idiot". It does however mean you don't sit around waiting for someone to approve a three line delete PR that you know is safe. The "Rubber Stamp" revie…

Since I am currently working in an environment that resembles GP's premise, I have to say that I find this take to be way too optimistic. I'd be exactly in the boat you describe (and actually: I've yet to see a situation where someone reviewing my code did not lead to improvements). But: If I want my code reviewed, I have to actually fight for it. Since everybody's workload is too high, even the willing often simply…

This is where properly done scrum shines, provided the nature of the work lends itself to greenfield work (cycle time of one iteration). Good scrum is a dysfunction surfacing machine, so if you have a good system of risk management in place, the team can address in a constructive way. For all the scrum haters out there, come up with some system that codifies certainty, stability, and quality before your leadership starts pointing fingers.
Post reply on HN