Posted by ColinWright 14 hours ago
If the primary purpose of code review is to assess maintainability, there is no need for review, that can be done by automated tooling (formatting, bad naming, cyclomatic complexity etc.)
What's the worth of this comment if you've decided to keep this purpose a secret?
The second sentence is just nonsense. There are no automated maintainability devices (quite yet.) You seem to be literally declaring that linting is the only worry that any software project would have when it comes to maintainability. Everybody is linting, the conversation begins after that.
You're not reviewing the code to confirm that the code is bug free... you're reviewing the additional code that confirms that the feature-code is bug free.
Any process that has a step of "we'll get to that later" is a failure. That includes testing. Until there is some provided content that will be able to provide evidence that that code is safe to merge, it's not done.
But yeah, I need to be able to understand what every line does.
Lost me here. I would agree that it’s not possible to find every bug by examining code, but in real code reviews bugs and errors are identified by reviewers all the time. Reviewers lend their past experience to the situation, identify some unnoticed interaction, think of an edge case that the author hadn’t considered, or some times just notice simple logic errors.
Code review is a fresh set of eyes. When we write code eventually parts of it get accepted in our minds as done or correct and we can start missing things that are obvious to a reviewer.
I’m not a fan of these blanket declarations that code review isn’t about reviewing code. I’ve read countless hot takes like this that code review is about some other thing (finding unmaintainable code, knowledge transfer, etc) that all miss the point that code review isn’t about one thing. These reductions and exclusions can be really misleading.
I think that this would be better directed at the person who posted "no one ever finds bugs during code review" rather than at the person who said that just re-reading code is (obviously) not an effective way of debugging, and is better thought of as a time to make code clear enough that bugs will be more apparent or less likely to be introduced by later authors.
> I’m not a fan of these blanket declarations that code review isn’t about reviewing code.
Using a shovel to dig a hole isn't about using a shovel, it's about digging a hole. Reviewing code is a necessary prerequisite to finding code that will be hard to maintain (and finding any number of other things, and knowledge transfer, and getting acquainted with coworkers' coding styles and domains, etc.) It is not a purpose in itself, but a tool.
> code review isn’t about one thing.
But you just said that code review is about reviewing code, and explicitly not about "some other thing" in the beginning of the sentence, right before listing two other things that code review could be about, and then insisting that it could be about many other things. The author is saying that it is primarily about one thing, likely because in their opinion it is most effective at that one thing, and that one thing goes a long way into solving other issues.
It isn't like they said "ignore bugs during code review, never learn anything from it."
I agree with the OP. Code reviews are finishing steps where things are polished, and polished code should be correct code, but more importantly intelligible code. If your code is clear, the mistakes will be obvious to more people than if your code is not readable. To make it simple: you've written something and it feels done, now you want someone to read it to see if it makes sense to them. No different than any other type of writing.
Oooh, I bet including the author? Yeah, right there, he fails to make any qualifications for his statement, making it factually incorrect.
There are plenty of reasons to do code review. If you force me to, I'll define it as information transfer. The point is to have a conversation about the code. To expand both people's understanding about the codebase. Everything on top of that is extra. I've found real and significant bugs doing code review. In large part because I understand the codebase better than the author. That's finding and preventing bugs.
I also read PRs looking for malicious code trying to sneak in, you never know, the person I've called my best friend, someone with opsec better than mine, may suddenly have turned evil and this is the PR where they're finally sneaking in the back door.... that's never happened yet, but fingers crossed!
> As everyone should know by now, it is not in general possible to find bugs by examining the code.
... I'd love to know what the author really meant to write here, because it certainly wasn't this.
this. was a lead in small businesses working with very inexperienced people in one team -- literally having to teach them git -- and another with outsourced devs who had one day a week for us and could barely remember cos they context switched onto other projects for the other 4 days a week etc.
my job was to always know the most about the systems, what we're trying to achieve with the systems and what is in the codebases. answer questions. send links to people. know which devs to ask for more detail etc. i never necessarily knew everything about frameworks, libraries, package updates, new tools etc., but i knew our system/devs/goals/products inside and out.
as i said to several people, your first PR submission is just your first public draft. i will find bugs. i will find things that need changing. it's not personal. it's just about making your PR better than it was. this is an opportunity to learn from me.
the author of the article seems to be considering code review from an idealised situation -- this never occurs in reality, most of us have to make do.
and that means changing focus of code review for what is needed, rather than what is ideal.
"in general" quantifies over all occasions, not over all bugs on one occasion.
To quote my own response:
===
Apparently the mathematician author doesn't understand the meaning of his own natural language quantifiers. “it is not in general possible to find bugs by examining the code” means “it is not in general possible to find ANY bugs by examining the code”, not “it is not in general possible to find ALL bugs by examining the code”.
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.
===