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

131–140 of 193 posts

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

#131

Post by giancarlostoro »

threethirtytwo wrote: post #120
giancarlostoro wrote: post #93

The other issue with code review, and I'm glad I've not worked with people like this anymore, for the person being reviewed: NOBODY IS ATTACKING YOU, nobody is saying your code is bad, the goal is to do a once-over for quality. Another goal people often miss: It's okay to ask "stupid questions" and I would argue as a Junior, ask away, even if no code changes happen, ASK. Kind of follows the spirit of the original pos…

Not true. I’ve attacked people before in the sense I was disgusted with the short cuts they took and the things they tried to get away with in the code. I’ve seen the malicious deletion of features in order to stay hidden and I’ve called it out.

That's not always an attack though, at least in a constructive critical review, maybe the approach could be different, but I'm also talking about when people are leaving comments that are genuine, professional, and tone is fine. In your case, yes you are attacking them I suppose, even though you don't really need to, you just need to rejec the PR and comment "hey, are you sure this is right?" and substantiate it. Sometimes people don't realize.

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

#132

Post by boie0025 »

boie0025 wrote: post #127
thayne wrote: post #109

Code review doesn't have a single purpose. Finding code that is hard to maintain is one of those, and and an important one, but certainly not the only one, and I'm not sure it is even the most important one. Other purposes include: - a safety check to ensure that if a developer (or AI) goes rogue, it is more difficult to merge malicious code - a second perspective from someone who isn't as close to the problem and mi…

I agree with all of this, I read into the "single purpose" of understanding the code and complaining about what you don't understand implying that if you understand it, you will be able to point out and comment on things that are wrong /foolish/unsafe/etc after understanding it. From that perspective on the OP, it makes sense to me. Particularly with regard to modularity and factoring; once I understand all of a giga…

After a re-read, I realized the claim wasn't "single purpose", it was "primary purpose", in which case this makes even more sense to me. I guess everything else comes from understanding what's in a given PR. It is difficult to find bugs in code that you don't understand, and it's difficult to understand code that doesn't follow convention, etc. I think I've worked this way and just not thought about it from this perspective. I review a lot of code, and what I generally do is fire up my editor in the relevant repo and follow along. If there's a method call to outside of the PR, depending on what it is and what I know about it, I'll pull that up in my editor and review there to make sure I understand what's happening. That understanding is where the comments come from. Maybe "I understand this and it's right" or "I understand this, and it seems wrong because [something]" or "I do not understand this because [whatever]" etc. Maybe "primary purpose" isn't perfect.. perhaps "overarching goal" or similar.. :shipit:

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

#133

Post by morganherlocker »

laybak wrote: post #51
jonahx wrote: post #40

Earlier quoted context omitted.

It's a good practice. Worth mentioning also: the same can be done with ordinary git log, assuming everyone is using git well. A proper git log of yesterday's work can be like your work newspaper with coffee.

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.

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

#134

Post by cat_plus_plus »

SketchySeaBeast wrote: post #24
cat_plus_plus wrote: post #19

Earlier quoted context omitted.

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.

Hehe you think L9s do oncall? Senior not understanding is a problem all right. It's not necessarily junior's problem.

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

#135

Post by morganherlocker »

acedTrex wrote: post #64
dude250711 wrote: post #43

Earlier quoted context omitted.

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.

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.

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

#136

Post by nacozarina »

munksbeer wrote: post #101

Look at the state of the comments on that thread. "This is completely false! Code is review is to ..." proceeds to state an opinion. Sometimes, some days, I just look forward to not having to deal with programmer hubris ever again.

I remember a time when the sole point of code review was to answer the question 'is this change going to break any other work currently in-progress?' That was it.

it was expected to be a short-duration fixed-scope activity with a clear Go-NoGo answer, and you would have gotten roasted for digressing into navel-gazing and distracted conversations about armchair philosophy. if the change worked as-advertised and didn't break your work, then it was a 'go', get back to work...

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

#139

Post by skydhash »

dotancohen wrote: post #123
a4isms wrote: post #115

Earlier quoted context omitted.

Another—very old—rationale: People write code differently when they know that it will be reviewed by people who will not only comment on it, but also form long-term impressions of the submitter's competence and fit based on the code that is reviewed.

I've always felt that this is and advantage to open source software. The vast majority of open source software that I've bothered to look at the code for used best practices, was reasonably secure, and was above all maintainable. The bespoke projects that I've worked on at various companies? Complete spaghetti messes almost all of them.

Pretty much. I use OpenBSD and the basic stance is that you need to look at the code of the system and the various software in ports. Because the only way to get timely support is you helping yourself and then the community will help you. And if you find some hackish code, there’s generally a good reason it’s that way.

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

#140

Post by SketchySeaBeast »

cat_plus_plus wrote: post #134
SketchySeaBeast wrote: post #24

Earlier quoted context omitted.

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.

Hehe you think L9s do oncall? Senior not understanding is a problem all right. It's not necessarily junior's problem.

I feel like you keep missing the point and are more interested in gotcha's.
Post reply on HN