4 ms·

HN moment. I’ve never seen in practice that someone says ”I don’t understand it” and the author says ”good point, I will simplify it”. Rather, the opposite. I

by klabb3 2y ago

HN moment. I’ve never seen in practice that someone says ”I don’t understand it” and the author says ”good point, I will simplify it”. Rather, the opposite. I often saw people make unnecessary complex or large PRs that were too much workload to review, leading the reviewer to approve, on the grounds of ”seems like you know what you’re doing and tbh I don’t have half a day to review this properly”. Code review is a ritual. If you ask why we have it people will give you hypothetical answers more often than concrete examples. Personally I’m a proponent of opt-in CRs, ie ask for a second pair of eyes when your spidey senses tell you.

Capricorn2481 2y ago
> I often saw people make unnecessary complex or large PRs that were too much workload to review, leading the reviewer to approve, on the grounds of ”seems like you know what you’re doing and tbh I don’t have half a day to review this properly” That just seems like company wide apathy to me. Obviously you have to make an effort to read the code, but there are lots of ways developers can overcomplicate things because they were excited to try a pattern or clever solution. It doesn't make them bad devs, it's just an easy trap to fall into. These should not pass a code review just because the code "works." It's totally acceptable to say "we're not gonna understand this in 3 months the way it's written, we need to make this simpler" and give some suggestions. And usually (if you're working with people that care about the workload they make for others) they will stop after a few reviews that point this out. We've done this at our company and it's helped us immensely. Recognizing whether the code is unnecessarily complex or the problem is inherently complex is part of it, though.
throw-qqqqq 2y ago
Our juniors write horribly complex code that senior devs have to ask to simplify. This happens all the time. And the juniors simplify and thank us for teaching and mentoring. It’s a big reason we do reviews. So we can control how dirty the code is before merging and so we can grow each other with constructive feedback. Sometimes it’s also just “LGTM” if nothing smells. 90% of comments in my team’s PRs come with suggestions that can be applied with a click (we use GitLab). It requires almost no effort to apply suggestions and it’s often not much extra work for reviewers to explain and suggest a concrete change. I agree that reviews should be used pragmatically.
delusional 2y ago
Get (or create) better colleagues. It's usually pretty easy to identify if people are approving pull requests that they don't understand. Pull them aside and have a professional talk about what a pull request review is. People want to do good, but you have to make it clear that you value their opinion. If you treat the people around you as valuable collaborators instead of pawns to be played to fulfill your processes, your appreciation for reviews will transform. Remember that it's their work too.

by klabb3 2y ago

Capricorn2481 2y ago

> I often saw people make unnecessary complex or large PRs that were too much workload to review, leading the reviewer to approve, on the grounds of ”seems like you know what you’re doing and tbh I don’t have half a day to review this properly” That just seems like company wide apathy to me. Obviously you have to make an effort to read the code, but there are lots of ways developers can overcomplicate things because they were excited to try a pattern or clever solution. It doesn't make them bad devs, it's just an easy trap to fall into. These should not pass a code review just because the code "works." It's totally acceptable to say "we're not gonna understand this in 3 months the way it's written, we need to make this simpler" and give some suggestions. And usually (if you're working with people that care about the workload they make for others) they will stop after a few reviews that point this out. We've done this at our company and it's helped us immensely. Recognizing whether the code is unnecessarily complex or the problem is inherently complex is part of it, though.

throw-qqqqq 2y ago

Our juniors write horribly complex code that senior devs have to ask to simplify. This happens all the time. And the juniors simplify and thank us for teaching and mentoring. It’s a big reason we do reviews. So we can control how dirty the code is before merging and so we can grow each other with constructive feedback. Sometimes it’s also just “LGTM” if nothing smells. 90% of comments in my team’s PRs come with suggestions that can be applied with a click (we use GitLab). It requires almost no effort to apply suggestions and it’s often not much extra work for reviewers to explain and suggest a concrete change. I agree that reviews should be used pragmatically.

delusional 2y ago

Get (or create) better colleagues. It's usually pretty easy to identify if people are approving pull requests that they don't understand. Pull them aside and have a professional talk about what a pull request review is. People want to do good, but you have to make it clear that you value their opinion. If you treat the people around you as valuable collaborators instead of pawns to be played to fulfill your processes, your appreciation for reviews will transform. Remember that it's their work too.