Hacker News Reader

Comment by emodendroket | Hacker News Reader

It's also easy for code review to devolve into "this isn't quite the way I'd implement it; scrap all this work and start over"

View on HN

In a lot of cases, I see variations of this when there was not enough up front thought put into the design (before writing code).

Slight tangent, but a good way to tackle that is to outline what you want to do (sketch the API you're going to fill in, etc), and run that by a reviewer

In a dysfunctional enough culture there's always someone you didn't include who has a show-stopping objection.

That's not a code review. That's either a suggestion that the initial commit is 'wrong' or the reviewer is a jerk. Either way, this is the PM's fault - not enough information to finish it right or too reviewer nitpicky on useless stuff.

There's a really good code review guide on the plaid blog that addressed this: https://blog.plaid.com/building-an-inclusive-code-review-cul....

Money quote: "Nothing should be surprising to a reviewer in a code review: discuss any significant design decisions before code is written. Code reviews are a time to iron out implementation details, not discuss major design or architecture decisions."

What if you use pull requests to prototype architectural change? My take is your pull request should include the context of the change and the sort of feedback you want.

I think that explicitly noting “this isn’t ready, but I’d like to discuss x,y,z” ought to cover it.

It was pretty common at my last job; seemed like every third PR got at least one request for a total or near-total rewrite, usually for completely non-functional changes without obvious benefits over the solution implemented.

I've been asked to make my solution more complicated and to add usage of things like the friend keyword to unit tests instead of testing a public static function.

And that's just one of the differences between a good coder and a good senior engineer.

Reply on news.ycombinator.com