The point of code reviews isn’t to catch bugs. It’s for someone else on the team to read your code and make sure they can understand it. If no one else on your team can understand your code, you shouldn’t be committing it to the repository.
Maybe. But then, sure, I can understand the code you wrote - on a syntactic/operational level. This adds Foos to bar instead of baz, and makes Quux do extra Frob() call. Whatever, that's stupid stuff below junior level. What would actually matter is for me to understand why you're doing this, what it all means. Which I won't, because you're doing some code for symbolic transformation of equations for optimizing some process, and I'm doing data exchange between our backend and a million one-off proprietary industrial formats, and we only see each other on a team call once a week.
I'm exaggerating, but only a little. Point is, in a deep project you may have domain-specialized parts, and those specialties don't overlap well. Like, ideally I'd take you aside for an hour to explain the 101 of the math you're doing and the context surrounding the change, but if neither you nor me have the time, that PR is getting a +2 from me on the "no stupid shit being done, looks legit code-wise; assuming you know your domain and this makes sense" basis.
To do a good job on code review, reviewer must understand the challenge and how it is best to address it better than the programmer who implemented it. Necessarily!
To do an adequate job, reviewer must understand the task at least equivalently well.
The above is possible, and probably even desirable, but it adds a massive overhead in developer time for any non-trivial change. Enforcing good code review can approximately double development costs for the company, and inevitably creates a lot of programming & architecture work—nearly equivalent to any other programming & architecture work, minus the time spent hitting keyboard keys—for engineers, who might not exactly enjoy it.
As a result, it is very rare, and most reviews just tick boxes and enforce taste.
I personally hate to review substantial implementations. It is work minus the fun part. During work, I get to come up with a solution and bring it to life. Instead, during review I have to devise a solution only to use it as a reference point to assess another’s solution. This can also cause review suggestions that are difficult to enact (if my solution is sufficiently different and I think better than the original, that’s a lot of work to redo).
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.
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.
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.
> 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.
I watched pre merge code reviews become a requirement in the industry and catching bugs was almost always the #1 reason given.
The times I've seen a 2nd set of eyes really help with the understandability of code, it was almost always collaboration before or while the code was being written.
I would estimate something like 1 out of 100 PR reviews I've seen in my life were really focussed on improving understandability.
