Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

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

mathstodon.xyz

Post reply on HN
Print view View original

171–180 of 193 posts

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

#171

Post by jonahx »

morganherlocker wrote: post #133
laybak wrote: post #51

Earlier quoted context omitted.

this seems like a chain of good practices. though I find it hard to stay disciplined about keeping commits well scoped and well described

You can setup most PR systems to squash on merge using the first commit's message, then enforce that the top commit message is prefixed with a ticket ID. This practice has many benefits: readable git log, easier git bisect for tracking down regressions, it becomes easy to find all commits associated with a block of work, more useful git blame.

I strongly recommend against auto-squashing. It creates large commits without the semantic intent of the author. In a large PR, you want clean, small, semantic commits, which makes it much easier to review and understand.

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

#172

Post by acedTrex »

morganherlocker wrote: post #135
acedTrex wrote: post #64

Earlier quoted context omitted.

Yep, without a decent team culture this is what LLMs force, the slop deluge is just overwhelming without leadership asserting "no, stop" Ultimately you just let bugs through because the alternative is spend an inordinate amount of time communicating with someones claude through PR comments about what the shape should be. Career was fun while it lasted. I suppose its a blessing a to get to do a job that you enjoyed fo…

Disapprove and ask for a call where the author must verbally explain the changes to receive approval? This seems like a solvable problem, and one that already existed in repos with many contributors of varying skill (open source, bigco with lots of interns). Letting bugs through is an even bigger time sink.

It comes down to culture on the team, if you think your leadership will allow that/you won't get a bad perf review for doing it go for it. That is sadly not a lot of peoples reality.

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

#174

Post by layer8 »

janalsncm wrote: post #170
saghm wrote: post #145

Earlier quoted context omitted.

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.

I read it as a reference to some theoretical limitation like the Halting Problem. Fine, in general it is not possible to tell if code will halt. But if I see a while(1) I’m going to check if the loop can break.

Arguably, if you can’t tell if the code will halt, the code is not acceptable. Code is acceptable if it can be properly reasoned about. If it can’t, it is not.

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

#175

Post by saghm »

layer8 wrote: post #174
janalsncm wrote: post #170

Earlier quoted context omitted.

I read it as a reference to some theoretical limitation like the Halting Problem. Fine, in general it is not possible to tell if code will halt. But if I see a while(1) I’m going to check if the loop can break.

Arguably, if you can’t tell if the code will halt, the code is not acceptable. Code is acceptable if it can be properly reasoned about. If it can’t, it is not.

Yeah, this seems like a misunderstanding of the halting problem. You can prove that a program halts will halt; you just can't prove that it won't halt if it doesn't.

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

#176

Post by xeonmc »

dijksterhuis wrote: post #106
dolmen wrote: post #73

Earlier quoted context omitted.

Because it was him... 10 months ago?

it's still weird. if i'm talking about something i wrote 10 months ago i'd say something like, "when i originally wrote this i was trying to say ...". not "when the author wrote this, he was trying to say"

Maybe it's because the author is a mathematician.

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

#177

Post by furyofantares »

jibal wrote: post #153
mjd wrote: post #13

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.

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 tr…

"in general" in mathematics means "in all cases, without exception", rather different than the normal usage where it means "usually, but not always".

If a mathematician using the mathematical sense of the word general says "it is not in general possible to tell if a program will halt by inspecting it", they're talking about the halting problem, even if you've looked at lots of programs where you can tell if they'll halt or not, and your experience might be correctly described using the normal usage of the word with "in general I can tell if a program will halt or not".

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

#178

Post by d0liver »

d-us-vb wrote: post #38
d0liver wrote: post #23

What if I told you that understanding what it is doing and finding bugs is actually the same problem?

Personally, I would tell you that whatever understanding you gain may still have bugs. Unless your understanding is as complete as a formal treatment of the code, then there may still be bugs in the code due to shared misunderstandings between author and reviewer. The biggest one is both having an incomplete understanding of what a library function does. So while there may be some overlap, particularly if each person…

If, by your definition of understanding, you can understand something without knowing how it works, then what would be the point in understanding it?

Your argument underscores the fact that understanding exists on a spectrum and that deep understanding can be difficult, or even impossible to achieve. Even formal verification seeks to verify certain properties of a system, and doesn't represent anything close to comprehensive understanding, and the process of formal verification is also guided by a fallible human. In a lot of ways, formal verification is just pushing up to review of a different "programming language."

So, good review is really hard, but ignoring the difficulty doesn't make it go away.

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

#179

Post by nunez »

6LLvveMx2koXfwn wrote: post #54
donatj wrote: post #16

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…

> 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 If code reviews are important, where does testing sit? Presumably if the coworker had not been part of the code review something would have stopped the breaking change making its way to prod?

Before the PR is cut, before the PR is sent to review (so that time isn't wasted on low-valuw bugs and problems --- engineers are expensive!), pre-merge and pre-release.

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

#180

Post by nunez »

connerBown wrote: post #108
titzer wrote: post #14

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.

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.

The problem with this is that so many diffs created by AI are gigantic. Thousands and thousands of lines of code, per PR. Of course this is fine if AI is creating, reviewing AND merging, but it's an exercise in futiliy for human reviewers, as there is no way for them to catch up.
Post reply on HN