Hacker News Reader

Comment by montroser | Hacker News Reader

That workbook is all about anxiety in general, which is fine, but there's almost nothing specifically to do with anxiety as it relates to code review.

Here's my advice: Focus on whether the change actually solves the problem it's trying to solve. Run the code; does it work? What regressions could it cause? Is there any way it could be simpler?

Along the way, you may notice style guide offences, or minor nitpicks. It's good to take note of those things, but only raise those issues once you've addressed the fundamentals.

At the end of the day, confidence comes from understanding. So dig in and understand the context, understand the effects of the change, and give the best feedback you can to help get to the best solution.

View on HN

Good code review method, but that doesn't address the anxiety aspect, which is real.

I always tried to treat and see it as a teaching and learning exercise and not make it accusatory or negative. As a reviewer, see it as an opportunity to teach others in good code practices. As a reviewee, see it as an opportunity to learn, not as a slam on you personally.

Doesn't solve it completely, but it helps.

Yeah, agreed that it is a good mindset to approach as a teaching and learning opportunity.

On the anxiety aspect, I think my point is that -- okay, exercises and workbooks can have their place, but the real way forward is to have a thorough understanding of the task at hand and provide your constructive criticism and help your team make their way.

If you need more context, ask questions. If you need to poke and prod to understand how the pieces fit together, then poke and prod. With all of that knowledge you can have a productive conversation about the proposed changes.

On the other hand, without all of that, then it is guaranteed to be anxiety inducing, since with your review, you are taking some amount of responsibility for a thing you may not sufficiently understand.

IMHO the syntax & other nitpickable matters should be caught by the linter. The regressions should be caught by the tests. And the fact the code runs and works should be a given. The discussion should be around the architecture of the solution & the long terms consequences of solving it that way. I also like to do the reviews in person, where we just go over the code together and talk about it. Small remarks can be immediately fixed

Also I think work in progress should be pushed often in a wip PR/MR, that way people can give early feedback

Completely agree.

It's a peer review, Not a control layer.

> It's a peer review, Not a control layer.

Peer review is absolutely a control layer. Having a review process in place means it requires coordination or at least negligence for a developer to introduce malicious changes into a project. At the very least, peer review can act as a quality gate for production changes.

Reply on news.ycombinator.com