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

Code review can be better

tigerbeetle.com

Post reply on HN
Print view View original

201–210 of 253 posts

Re: Code review can be better

#201

Post by pm90 »

jbmsf wrote: post #61

Recently, I've been wondering about the point of code review as a whole. When I started my career, no one did code review. I'm old. At some point, my first company grew; we hired new people and started to offshore. Suddenly, you couldn't rely on developers having good judgement... or at least being responsible for fixing their own mess. Code review was a tool I discovered and made mandatory. A few years later, everyo…

My rule of thumb is that if you have an OnCall rotation for a codebase, you should require reviews. Besides all the benefits you've mentioned, its important to spread know-how of the code so that people on the rotation don't need to be pulled in e.g. over the weekends/on vacation because they're the only ones familiar with the code.

(There should be breakglass mechanisms to bypass code reviews, sure. Just the default should always be to require reviews)

Re: Code review can be better

#202

Post by ozim »

motorest wrote: post #132
ozim wrote: post #102

Earlier quoted context omitted.

It still is engineering you only mistake design phase. Writing code is the design phase. You don’t need design phase for doing design. Will drop link to relevant video later.

> Writing code is the design phase. No, it really isn't. I don't know which amateur operation you've been involved with, but that is really not how things work in the real world. In companies that are not entirely dysfunctional, each significant change to the system's involve a design phase, which often includes reviews from stakeholders and involved parties such as security reviews and data protection reviews. These…

Operation that uses software developers not as code monkeys but actual business problem solvers that have also business knowledge.

Operation that delivers features instead of burning budget on discussions.

Operation that uses test/acceptance environments where you deploy and validate the design so people actually see the outcome.

Obviously you have to write down the requirements - but writing down requirements is not design phase.

Design starts with idea, is written down to couple sentences or paragraphs then turned into code and while it is still on test/acceptance it still is design phase. Once feature goes to production in a release "design phase" is done, implementation and changes are part of design and finding out issues, limitations.

Re: Code review can be better

#203

Post by actinium226 »

liampulles wrote: post #105

Here's an alternative I've wondered about: Instead of one person writing code, and another reviewing it - instead you have one person write the first pass and then have another person adjust it and merge it in. And vice-versa; the roles rotate. Anyone tried something like this? How did it go?

I would want the first person to write 90+% of the code, and really more like ~98% of it, because at some point you need to just do your job. But I like the idea of having the reviewer make the relevant changes themselves and merge it in. That's more or less what we did at the first place I worked, and the expectation was that both of you were responsible for the code. If it was more than minor changes the second person could send you notes for you to implement, but they were always the person to merge. I prefer it to the alternative of chasing someone down so that they hit "approve" so that you can go back to your desk and hit "merge."

Re: Code review can be better

#204

Post by vlovich123 »

thinkindie wrote: post #113
tomasreimers wrote: post #16

Just taking a step back, it is SO COOL to me to be reading about stacked pull requests on HN. When we started graphite.dev years ago that was a workflow most developers had never heard of unless they had previously been at FB / Google. Fun to see how fast code review can change over 3-4yrs :)

Given the security incident that happened to CodeRabbit I’m a bit less enthusiastic about testing out new tools that have LLMs and my codebase under the same tool. What can be a very nice experiment to try something new can easily become a security headache to deal with.

I don’t understand. By LLMs you’re referring to the optional LLM review graphite offers as an additional purchase on product? I’m not sure I understand the risk concern.

Re: Code review can be better

#205

Post by jFriedensreich »

Its pretty clear to a growing number of devs what a review tool should look like. It is more a matter of what needs to happen so this becomes a usable and sustainable reality and what shape of organisation/ players can make this happen in the right way.

- git itself wont go much further than the change-id which is already a huge win (thanks to jj, git butler, gerrit and other teams)

- graphite and github clearly showed they are not interested in solving this for anyone but their userslaves and have obviously opposing incentives.

- there are dozens of semi abandoned cli tools trying this without any traction, a cli can be a part of a solution but is just a small part

What we need:

- usable fully local

- core team support for vscode not just a broken afterthought by someone from the broader community

- web UI for usecases where vscode does not fit (possibly via vscode web or other ways to reuse as much of the interface work that went into the vscode integration)

- the core needs to be usable from a cli or library with clear boundaries so other editor teams can build as great integrations as the reference but fitting their native ui concepts

- it needs to work for commits, branches, stacked commits and any snapshot an agent creates as well as reviewing a devs own work before pushing

- it needs to incorporate CI/CD signals natively, meta did great UI work on this and its crucial to not ignore all that progress but build on top of it

- it needs to be as fine grained as the situation requires and with editability at every step. Why can i just accept one line in cursor but there is nothing like that when reviewing a humans code? Why can i fix a typo without any effort when reviewing in cursor when i have to go through at least 5 clicks to do the same when fixing a typo of a human.

- It needs to by fully incremental, when a pr is fixed there needs to be a simple way to review just the fix and not re-review the whole pr or the full file

Re: Code review can be better

#206

Post by [deleted] »

liampulles wrote: post #105

Here's an alternative I've wondered about: Instead of one person writing code, and another reviewing it - instead you have one person write the first pass and then have another person adjust it and merge it in. And vice-versa; the roles rotate. Anyone tried something like this? How did it go?

[deleted]

Re: Code review can be better

#207

Post by vlovich123 »

mytailorisrich wrote: post #114
tomasreimers wrote: post #16

Just taking a step back, it is SO COOL to me to be reading about stacked pull requests on HN. When we started graphite.dev years ago that was a workflow most developers had never heard of unless they had previously been at FB / Google. Fun to see how fast code review can change over 3-4yrs :)

Stacked pull requests seem to add a layer of complexity to solve a problem that should and can be avoided in the first place. Frequent, small changes are really a good practice. Then we have things like trunk-based development and continuous integration.

I’m confused. How do you do frequent small changes and avoid stacked PRs. Do you just do a small commit, wait for a review, merge, do another small commit? Or do you make a bunch of small commits locally and only put up the next one for review when the previous one is reviewed and merged?

That’s the only models I can think of and it’s weird to advocate to have a variable time asynchronous process in the middle of your code or review loops. Seems like you’re just handicapping your velocity for no reason.

Stacked PRs are precisely about factoring out small changes into individually reviewable commits that can be reviewed and landed independently, decoupling reviewer and developer while retaining good properties like small commits that the reviewer is going to do a better job on, larger single purpose commits that the reviewer knows to spend more time on without getting overwhelmed dealing with unrelated noise, and the ability to see relationships between smaller commits and the bigger picture. Meanwhile the developer gets to land unobtrusive cleanups that serve a broader goal faster to avoid merge conflicts while getting feedback quicker on work while working towards a larger goal.

The only time stacked commits aren’t as useful is for junior devs who cants organize themselves well enough to understand how to do this well (it’s an art you have to intentionally practice at) and don’t generally have a good handle on the broader scope of what they’re working towards.

Re: Code review can be better

#208

Post by vlovich123 »

mytailorisrich wrote: post #130
radlad wrote: post #120

Earlier quoted context omitted.

Stacked PRs allow me to post frequent, small changes without waiting for a review between each one.

Well, you don't need stacked PRs for that... I think stacked PRs are a symptoms of the issues the underlying workflow (feature branches with blocking reviews) has.

What’s your counter proposal on the workflow? No code reviews?

Re: Code review can be better

#209

Post by fogzen »

romanovcode wrote: post #129
liampulles wrote: post #105

Here's an alternative I've wondered about: Instead of one person writing code, and another reviewing it - instead you have one person write the first pass and then have another person adjust it and merge it in. And vice-versa; the roles rotate. Anyone tried something like this? How did it go?

Great idea, if you're fine with development time to take twice as long.

It might be worth it if it reduces bugs. If you’re running a payments company maybe working software is more important than saving a buck and shipping a day earlier.

Braintree was a pair programming company for example.

Re: Code review can be better

#210

Post by vlovich123 »

mytailorisrich wrote: post #174
ncruces wrote: post #172

Earlier quoted context omitted.

“What you'll do next and in what way” is often an important tool to put the small changes into context. Stacked pull requests can be an important tool to enable “frequent, small changes” IMO. Sure, I can use a single pull request and a branch on top of that, but then it's harder for others to leave notes on the future, WIP, steps. A common situation is that during code review I create a few alternative WIP changes to…

From a continuous integration perspective my understanding is that stacked pulled requests do not make change more frequent if we define a "change" as being committed on the master branch. They only split the feature branch into smaller chunks. On the other hand, I do take your point about context over a number of consecutive changes. But, to me, "creating a few alternative WIP changes to communicate to a reviewer" i…

> From a continuous integration perspective my understanding is that stacked pulled requests do not make change more frequent if we define a "change" as being committed on the master branch

Empirically this is not true if you also control for review quality. If your code review is a rubber stamp then sure mega PRs win because you put up a PR and then merge. But why review then?

However, code review quality goes up when you break things down into smaller commits because the code reviewer can sanity check a refactor without going over each line (pattern matching) while spending more time on other PRs that do other things.

And if you are breaking things down, then stacked PRs are definitely faster at merged to master/unit of time. I introduced graphite to my team and whereas before we struggled to land a broken down PR of ~5 commits in one week, we’d regularly land ~10+ commit stacks every few days because most of the changes of a larger body of work got approved and merged (since often times the commit order isn’t even important, you can reorder the small commits), conditional approvals (ie cleanups needed) didn’t require further follow ups from the reviewer, and longer discussion PRs could stay open for longer without blocking progress and both developer and reviewer could focus their attention there.

Additionally, graphite is good about automatically merging a group of approved small individual commits from a larger set of changes automatically without you babysitting which is infinitely easier than managing this in GitHub and merging 1 commit, rebasing other PRs after a merge etc.

Post reply on HN