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

21–30 of 193 posts

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

#21

Post by rowanseymour »

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…

We even find ourselves creating PRs in situations where the code is going to be merged immediately anyway, and tagging other devs, just so they have a convenient way to see what got merged and why. So people don't lose track of what is in the codebase.

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

#22

Post by brimtown »

The best writing on this is the "agent principal-agent" problem, which correctly frames the problem of agents and code review in terms of trust.

This is why the solutions for high-trust environments (small teams) and low-trust environments (big companies, open source projects) will be different.

https://crawshaw.io/blog/agent-principal-agent

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

#24

Post by SketchySeaBeast »

cat_plus_plus wrote: post #19
SketchySeaBeast wrote: post #7

Earlier quoted context omitted.

If the code is so smart that it's not easily understandable, it's not easily fixable. My transition from junior to senior was accompanied by the realization that simpler is nearly always better.

Write me simple AI inference code that has high performance in big batches.

I don't know enough to speak about that particular domain, but if the junior is writing something the senior can't understand, that's always going to be a problem. That code becomes the team's responsibility, and that code needs to be able to be maintained by the entire team, not only by the junior with something to prove.

Who is getting called at 2 AM when something breaks? Not the junior.

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

#25

Post by phendrenad2 »

Whatever purpose code review served pre-2002, post-2002 it serves as corporate audit coverage. A common (mis?) interpretation of SOX and SOC2 is essentially that the company must have a two-person sign-off on any system change that could damage the company or its reputation.

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

#26

Post by Pxtl »

Well, the code review should also be reviewing the provided test code or test plan or whatever that will prove it does not have bugs.

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.

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

#27

Post by AlotOfReading »

cat_plus_plus wrote: post #19
SketchySeaBeast wrote: post #7

Earlier quoted context omitted.

If the code is so smart that it's not easily understandable, it's not easily fixable. My transition from junior to senior was accompanied by the realization that simpler is nearly always better.

Write me simple AI inference code that has high performance in big batches.

Does tinygrad not count for some reason?

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

#28

Post by sjducb »

SketchySeaBeast wrote: post #7
cat_plus_plus wrote: post #4

The primary purpose of code review is to maintain existing hierarchy by preventing junior SWEs from getting promoted by committing code that is smarter than what the senior architect can understand.

If the code is so smart that it's not easily understandable, it's not easily fixable. My transition from junior to senior was accompanied by the realization that simpler is nearly always better.

Good programmers write code that’s so simple and obvious it looks like anyone could have written it.

Bad programmers make the simplest things really complicated.

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

#30

Post by Pxtl »

high_na_euv wrote: post #8

Uh.. why not both?

Because the reviewer is not magical. If there was something in the code the author couldn't see, the reviewer probably won't see it either.

The way to confirm that code does not have bugs is testing. So the reviewer is not looking at the code saying "this will work", they're looking at the code saying "I understand how this works, it makes sense."

Evidence that the code is safe is something that also should be provided in the PR, but it is not the main code. It is ideally test automation that is just as understandable as the feature code, but failing that ad-hoc test evidence or a specific step-by-step plan with evidence of execution is good too.

Post reply on HN