undefined | Better HN

0 pointsolliej3y ago0 comments

No, every project I have worked on has review gated commits. It is a /basic/ step in ensuring that a project maintains a high quality codebase. Review does cause delays, because reviewing takes time, but we've generally found that the speed "gained" through poor change control is more than made up for through bad code.

Review also shouldn't be causing ego antagonisms. You're coworkers. You have to be able to work together. That's called having a job.

Every project I have worked on in a professional setting has had mandatory code review, with review acting as a gate. That's been required in order to maintain software quality despite the quality of engineers I've worked with.

0 comments

chiefalchemist3y ago

Agreed. I'd like to add that PRs are not only a means to review code to improve that code. They're also way for new hires and jr devs to learn new things, how the hive mind thinks, etc. This is, ideally, today's PRs help improve tomorrow's code as well.

berkes3y ago

You can have perfectly fine, high quality, peer reviewed code without PRs.

Only if some regulation requires sign offs from e.g.other depts. PRs are inevitable. In all other situations they are at best an inneficient workflow and at worst a Kafkaesque circus.

Peer programming, daily checkups, a rock solid CI, and, above all, trust in the professionalism of your team are some ingredients for high quality, high throughput software development.

This is not an opinion. It's a scientifically proven fact. As laid out in the book Accellarate.

hiptobecubic3y ago

Peer programming is just code review again, except now you're only allowed to write when two people are available.

berkes3y ago

Indeed.

The point was not that "review" is bad, or should be avoided. But that a Pull-Request is a poor way to do that review. Reviewing is crucial, IMO.

jseban3y ago

> Peer programming, daily checkups, a rock solid CI, and, above all, trust in the professionalism of your team are some ingredients for high quality, high throughput software development.

Absolutely agree, the collaboration should happen sooner in the process, and I would add that the team should probably also have made at least a high level proposed solution together before the work even started.

dhzhzjsbevs3y ago

People seem to have forgotten what code was like back in the bad old days.

Fuck that dogpile of bullshit.

You've never seen a 1200 line diy function to parse XML have you?

Nursie3y ago

I apologise if I left you with that one when I left my first job in 2003!

jenscow3y ago

When I joined in 2006, someone had replaced it with 21 regular expressions.

1 more reply

jseban3y ago

> Review also shouldn't be causing ego antagonisms.

Yeah but the tool itself is antagonistic, because it imposes a workflow of open source, a workflow that also is antagonistic with clear and absolute power. So using that tool suddenly brings that antagonism and power into a team which is supposed to be 100% collaborative, and it also only does it periodically and with random and different people in power. It's not hard to see how that becomes somewhat complicated.

actually_a_dog3y ago

Anyone who can't handle having their code reviewed before it goes into production is not welcome on my team. You can call that antagonistic or not, but I have always found that the best engineers are the ones who are able to separate criticism of their code from criticism of themselves. Get that type of people on your team, and code review is no longer antagonistic: it's just an egoless feedback process that improves the end product, which is what all members of an engineering team should be working toward.

jseban3y ago

I'm not criticising the review itself as much as rather the PR workflow and the tools of github, especially the blocking mechanism. It's my opinion that collaboration should happen much sooner rather than being pushed to the end with the review of a PR, and that you should have designated people who have the power to sign something off as production ready.

If a team inside a company want to gather inspiration from the open source world, they should look at how the owner teams of open source projects work internally, not how they work together with outside contributors.

And, it's a very common "solution" in tech to just simply not have any feelings, but people have feeling and that you can't turn off, and that's a very important contributor to work satisfaction.

1 more reply

lovich3y ago

>So using that tool suddenly brings that antagonism and power into a team which is supposed to be 100% collaborative, and it also only does it periodically and with random and different people in power.

Is code review not collaborative in your experience? In every team I've been on/run we've split code review comments between suggestions/questions and actual "I will not let this go into prod if my name is attached to it" blockers. Code review has been 99% the first set of comments and only rarely do I see anyone actually block reviews over things like style and what not that have been mentioned here.

I can't even imagine those topics getting into code review as a blocker as if we have actually strongly held opinions on mechanical issues like that, they are integrated into the various linters used so you don't even need human eyes to catch the issue.

olliejOP3y ago

What tool are you talking about?

I've had patch review as back and forth comments in email, in bugzilla, in myriad other bug databases.

If you can't send out an email with your patch as an attachment, and get feedback, then we have a problem, and the problem is not the adversarial nature of review.

jseban3y ago

But why would you use such a remote asynchronous late stage feedback loop, if you are literally sitting in the same room as your collaborators, during the whole development process?

2 more replies

hedora3y ago

Have you ever worked on a new codebase?

I've found PR's become important approximately when feature momentum starts to drop.

j / k navigate · click thread line to collapse

0 comments

chiefalchemist3y ago

berkes3y ago

You can have perfectly fine, high quality, peer reviewed code without PRs.

Only if some regulation requires sign offs from e.g.other depts. PRs are inevitable. In all other situations they are at best an inneficient workflow and at worst a Kafkaesque circus.

Peer programming, daily checkups, a rock solid CI, and, above all, trust in the professionalism of your team are some ingredients for high quality, high throughput software development.

This is not an opinion. It's a scientifically proven fact. As laid out in the book Accellarate.

hiptobecubic3y ago

Peer programming is just code review again, except now you're only allowed to write when two people are available.

berkes3y ago

Indeed.

The point was not that "review" is bad, or should be avoided. But that a Pull-Request is a poor way to do that review. Reviewing is crucial, IMO.

jseban3y ago

> Peer programming, daily checkups, a rock solid CI, and, above all, trust in the professionalism of your team are some ingredients for high quality, high throughput software development.

dhzhzjsbevs3y ago

People seem to have forgotten what code was like back in the bad old days.

Fuck that dogpile of bullshit.

You've never seen a 1200 line diy function to parse XML have you?

Nursie3y ago

I apologise if I left you with that one when I left my first job in 2003!

jenscow3y ago

When I joined in 2006, someone had replaced it with 21 regular expressions.

1 more reply

jseban3y ago

> Review also shouldn't be causing ego antagonisms.

actually_a_dog3y ago

jseban3y ago

And, it's a very common "solution" in tech to just simply not have any feelings, but people have feeling and that you can't turn off, and that's a very important contributor to work satisfaction.

1 more reply

lovich3y ago

olliejOP3y ago

What tool are you talking about?

I've had patch review as back and forth comments in email, in bugzilla, in myriad other bug databases.

If you can't send out an email with your patch as an attachment, and get feedback, then we have a problem, and the problem is not the adversarial nature of review.

jseban3y ago

But why would you use such a remote asynchronous late stage feedback loop, if you are literally sitting in the same room as your collaborators, during the whole development process?

2 more replies

hedora3y ago

Have you ever worked on a new codebase?

I've found PR's become important approximately when feature momentum starts to drop.

j / k navigate · click thread line to collapse