Posted by ColinWright 8 hours ago
not "when the author wrote this, he was trying to say"
And the first interpretation is relevant but wrong, whereas the second interpretation is true but irrelevant.
P.S. It seems that the author meant to say “it is not in general possible to find a given bug by examining the code”, i.e., "not (for all bugs B it is possible to find B)", which again is true but not relevant.
I would add that (related to your "maintainability" point) ensuring the code is as simple as possible, and thus much more likely to be "debuggable by review", is a goal of review. Even that won't prevent bugs in the absolute sense, as you rightly say, but it boosts your probabilities.
In order words, the purpose of code review is to or not ask for changes on the code.
The rest is not part of the definition; critics don't review movies in order to change them.
But just accepting that at face value, making code more readable is a "change." I don't know what you're trying to say here.
> making code more readable is a "change." I don't know what you're trying to say here.
Yes and this is exactly what I am trying to say. The author limited what is the purpose of a code review like it's has really this limitation but, in practice, the code review is widely about what you think it should changed on the code by multiple reasons: readability, manutenibility, performance, security, bugs, use cases, test coverage etc.
Making code more readable is just one of the reasons why you want the code to be changed on a pull request, it's not the primary purpose.
This is why the solutions for high-trust environments (small teams) and low-trust environments (big companies, open source projects) will be different.
And I agree to some extent with what the OP's tweeter said. these days, bugs can look perfectly fine on the surface, but when combined with the existing system, entirely new types of bugs emerge. This is an especially common pattern in the AI era: the added code itself isn't the problem, but it becomes a bug once it interacts with the existing codebase.
There are many peripheral nice-to-haves you can get from a code review. Bugs, security, performance, correctness issues are all possible bonus findings you may get sometimes.
But there is only one _must_ in reviews: another person reading and understanding the code, possibly suggesting architectural improvements, or asking questions that should be answered by rephrasing the code for clarity, or by adding code comments. In other words: maintainability. That's the one thing that's not a bonus point, and is a constant for all code reviews.
In some organizations, maintainability may be the biggest risk being mitigated in a code review. But for me, that's selling code reviews short.
In my experience, code reviews are the single most important quality control process in the entire development life cycle. Engineers often don't have a lot of influence over the quality of requirements. Engineers often don't have a lot of influence over the competence and thoroughness of the QA process (and it often doesn't exist at all). But engineers frequently have total control over code reviews.
If I can't depend on the rest of the organization for QC, code reviews are the first place I look to mitigate that risk. That means code reviews find bugs. That means code reviews identify code smells. That means code reviews pressure test requirements and whether the implementation matches the assignment. That means code reviews transfer knowledge and serve as a teacher for both the PR author and the reviewer. And so on.
Thorough and pedantic code reviews are challenging and tedious, at least at first, but the team adapts and both the code and the review process gets better.
Well kinda - code review needs to identify any missing tests? And without the tests more likely a bug could exist.
I think that the for the person who accepts suggestions, it's made me wonder if they accept them in part to share ownership with me. I feel like we both maintain and understand the code, and are on the same page.
For the person who rejects PR suggestions, it makes me less inclined to participate in those PRs. Why spend the time doing a thorough review if it's going to get rejected anyways.
* thought: Maybe foo'ing is more common in the future - we can refactor if that happens.
* change: This is a leaky abstraction, would prefer to see this modeled like bar instead.
* nit: Naming seems a little unintuitive, consider "Baz", "Boo" maybe?
* fix: This unit test is validating the wrong field.
* chat: This is a big decision and would dictate how solutions of this category look like going forward. Let's bring this to the team first.
----
With the idea that some of those prefixes are stopping the PR until they are changed, and some are just a "take it or leave it" type comments. It makes it unambiguous to the opener that you consider these X things as "We've gotta get on the same page" and these Y things as "Stated preferences" or "just an observation".
word of warning - don't feel bad if you leave a nit, the other person disagrees and ignores it. If you felt strongly about it, it shouldn't have been a nit.
This is why you leave blocking suggestions and force the conversation if you think it is important enough.