The primary purpose of code review is to find code that will be hard to maintain

Join the discussion

• Hacker News
• > The primary purpose of code review is to find code that will be _hard to maintain_.

  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.

  by trentnix
• I work with someone who tends to rejects PR suggestions. I also work with someone else who accepts suggestions.

  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.

  by david422
• > 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.

  This is why you leave blocking suggestions and force the conversation if you think it is important enough.

  by acedTrex
• Our team tends to prefix all our comments with one of

  * 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.

  by clates
• Code review isn’t a singular thing. There are many reasons for code review, like knowledge sharing, liability laundering, code quality, regulatory compliance, etc. As usual, what purpose it serves depend on your use case.
  by faitswulff
• Thank you! I find it somewhat ignorant to say, most people do not understand the reason behind a peer review, while obviously being missinformed. To even believe that it only serves a single purpose somewhat tells me, that the author is missing experiences with other teams / people.
  by razemio
• The author is a mathematician, so when he says “it is not in general possible to find bugs by examining the code” he does not mean it is completely impossible to find bugs. He means only that it is not possible to find all bugs or even any particular bug.
  by mjd
• I guess that makes sense. Based on my math lectures during college, mathematicians can often be terrible at communication to other humans, so that would explain why they think what they said is different from how pretty much everyone else reads it.
  by saghm
• Ofc you're correct in that sense.

  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.

  by jonahx
• 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.

  by jibal
• User names match... are you the original author? Why commenting in the third person?
  by d-us-vb
• Sure, ensuring maintainability is one of the benefits of code reviews, but I think it is a bold claim to say that's the solo purpose. For example, code reviews is also a tool that allows teams to get inform of the changes in the code and share responsibility of the whole code base.
  by pmelendez
• True - the biggest thing I want to catch in an MR is "will this change lead us onto a path that is uglier, buggier, less maintenanable".

  People will generally copy and follow existing patterns, so for example if you let somebody add a new internal date time format, then soon your codebase will bifurcate and there'll be multiple inconsistent versions roaming around.

  The other stuff (minor bugs, overly verbose code) can easily be fixed. Paradigm rot cannot.

  by BariumBlue
• It’s probably important to define what sort of code review you are talking about when making broad claims about it.

  GitHub style asynchronous pull request review with inline comments is the norm now, but it’s not the only sort of review there is. I’m old enough to remember processes that include in person reviews that were more like a dissertation defense or conference presentation.

  The literature around this that shows that code review is a useful quality practice (in fact one of the only useful quality practices) comes mostly from much more structured review processes than we see now.

  My personal opinion is that before llms the GitHub style pr review was for making us feel better about our processes (or governance checkbox checking) and the age of llms will sweep them away as the cost/benefit is so much worse now.

  by kasey_junk
• On one of my first jobs, I had printed off change packages which had to be reviewed and signed. There was even a person owning the final copies in filing cabinets. This was more like traditional engineering and everyone had to think of software as more permanent.
  by goalieca
• > GitHub style asynchronous pull request review with inline comments is the norm now, but it’s not the only sort of review there is. I’m old enough to remember processes that include in person reviews that were more like a dissertation defense or conference presentation.

  Synchronous review is still possible today! One of my earliest managers taught me that if a "standard" code review goes back and forth more than once, it's almost always better to just hash it out in person (or on a Zoom call, when at least one person is remote) and then go back and post a comment summarizing what consensus was reached. To use a contorted technical analogy, asynchronous text communication can be lossier in terms of what information it's able to successfully encode than verbal, and the throughput is lower, so sometimes it's worth it to pay the synchronization overhead when you need to exchange a lot of information.

  by saghm
• > it is not in general possible to find bugs by examining the code.

  Oh hell yes it is, at every level of abstraction even. We call those things code smell... A file descriptor that hasn't been closed, a coroutine that hasn't been awaited, a big try/catch block that just falls back to some value without logging the error, wrong type castings, etc.

  As a general rule: Neither type checker, nor compiler, nor runtime should ever be steps that merely want to be satified - work with these steps and treat them as the valuable tools they are, and never work against them.

  by whilenot-dev
• I've been asked to find bugs in PRs during interviews, even!
  by nunez
• Yeah, I have no idea what they're talking about with that one. I've caught bugs when reviewing code without needing to run it before, and I've had the same happen to me in reverse, and I've seen it happen between others on reviews I was observing. I guess they could find some way to define "in general" so that this is technically true, but at that point it's not particularly meaningful.
  by saghm
• This just makes reviewers and authors lazier.

  The purpose of code review is multi-faceted. Hard to maintain? Yes. Might have bugs? Yes. Can be done simpler/cleaner? Yes. Is in line with project code style? Yes. Get someone else to also understand the code? Yes. Onboard junior team member? Yes. Sanity check design decisions? Yes.

  This flippant note is mostly more self-justification for being a lazy code reviewer.

  by titzer
• Totally agree. With the speed at which code can be written and deployed today due to AI, the emphasis should shift onto the review. Does the code actually run properly, are all of our assumptions correct, and are there any unintended side effects?

  I've found the review and debugging process to be much more time consuming than writing/producing code, and just "praying it works" never ends well.

  by connerBown
• Indeed, we worked hard as an industry to move beyond "blame the author" to "blame the process/team".

  Unfortunately this article is just bait, may as well say "People seem to think dinner is about eating food, but it's not about eating at all, actually it's about connecting with family and friends!". It's a specific type of poorly constructed reductionist argument that plays well on HN.

  by andrewvc
• Agreed. There's a whole checklist of what to look out for:

  - Does it functionally achieve what it sets out to (as per tacker issue or PR description)?

  - Does it have extraneous code? Leftover debug prints, private API keys etc...

  - Does it have any obvious defects? Memory leaks, un-handled edge cases, security flaws, obsolete API calls, etc...

  - Could it be more understandable? Add/remove abstractions, better variable/method names, more/less functional etc...

  - Is the style consistent with the codebase and/or style guidelines?

  - Are there obvious performance improvements? Hashset instead of list, lazy evaluations, etc...

  - Is it sufficiently well tested?

  I'm not even sure I agree that if I can't understand the code then it shouldn't go through. Some code is just really hard to understand. The aim is to make it as easy as possible to understand, while being functionally correct.

  by n4r9
• My attitude has always been that code review is best thought of as the gate where code goes from being owned by the author to being owned by the team or project. The code I'm reviewing is not your code, it is code that is about to become our code.

  Maintainability is a major factor in that, of course.

  by sjburt
• Such a luxury, I am envious!

  Our team started using AI, so I switched to a simple method: no comments, and a binary "is this batshit crazy or passable" approval decision rule.

  Saving myself time and sanity.

  by dude250711
• What I find to be maybe the single most important part of code review is knowledge transfer.

  Our entire small team thumbs up a PR before it's merged unless there's a big rush on it, and this gives everyone on the team a rough idea of the state of the codebase at any given time. There's no being blindsided like "this whole system I depend on is gone" like I had happen at far more siloed places I've worked.

  Beyond that, it gives a forum to ask questions about how things work to further build understanding. On a high functioning team, every developer should have at least a modest understanding of the entire system, including parts they never touch.

  Another important feature is just the institutional knowledge check. For instance recently I made a small change to a table and a coworker pointed out that there was a microservice I wasn't considering that wrote to that table that would break (yes, sharing tables is bad design, unrelated). I had no idea this microservice existed let alone had access to this table. The institutional knowledge check here though prevented a larger issue and potential data cleanup situation.

  by donatj
• Decisions by small groups should be the default. Others only need to be involved if the risk/consequences of failure are high.

  I started ignoring all PRs from our large team because we had a similar policy. My teammates can handle, they don't need me to check on each PR.

  by chrisra
• Based on your description, I don't think the process is working.

  You describe a shared database, micro services you aren't aware of using the same database, and a desire for everyone to have a rough idea of the state of the codebase. And things breaking unexpectedly being caught by code review.

  You have a bigger problem here of a system you can't reason about very well. The code review will help here, but I think you have bigger discoverability problems based on what I'm reading.

  As others have said, what is needed is automated testing that would catch this sort of problem automatically without needing a human in the loop saying "Oh, wait...". We still want/need humans in the loop, but they should not be the only safety net.

  by Scubabear68
• Complexity is caused by Dependencies and Obscurities ~ John Ousterhout

  Looks like you had both.

  by abirch
• > Our entire small team thumbs up a PR before it's merged unless there's a big rush on it, and this gives everyone on the team a rough idea of the state of the codebase at any given time.

  For non trivial or chore updates a second pair of eyes is always a good idea. But it’s not possible to scale out “everybody reads everything” to a large N. The problem is that nobody could keep up with that ad the reader when there are some huge number of things to read. That’s why we delegate, create docs, and have overview sessions.

  by koolba