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

On Code Review

hugodias.substack.com

Post reply on HN
Print view View original

21–30 of 58 posts

Re: On Code Review

#21

Post by KronisLV »

V-2 wrote: post #19
KronisLV wrote: post #16

Earlier quoted context omitted.

> This implies the need for smaller pull requests maybe? This makes me feel like perhaps pull requests shouldn't always pull/merge the changes directly, but instead there should be something like the GitLab "Start a review" functionality, where you can queue up code comments, before submitting all of them at once, but for pull requests. As a Jira analogy, imagine the totality of the changes that you want to merge as…

"You don't actually want these changes to end up in the main branch before they are done." True - and it sounds like a textbook scenario for introducing a feature branch... this approach has worked well for me. The idea of queueing up "sub-requests" is interesting, but it comes at the cost of creating another layer of complexity (in my view).

Agreed! I guess what i'm trying to say (in an awfully wordy way) is that we could probably improve the UI for pull/merge requests and gain a lot from it.

I mean, GitHub and GitLab already improved many development workflows with their inline comments (as well as other features, like code recommendations) and it feels like sooner or later more improvements are bound to come!

Re: On Code Review

#22

Post by redis_mlc »

andrewingram wrote: post #15

I've observed a small (yet growing) sentiment emerging lately that code reviews aren't all they're cracked up to be, and can actually be a net negative. I'm somewhat on the fence and can see all the arguments, though still err on the side of code review. But I am somewhat convinced that the way most teams (at least those i've seen) practice them isn't ideal.

> can actually be a net negative

Gen Z gets perpetually triggered, so there is a move to not do code reviews.

I just make an effort to be first to review and keep the mood light.

Re: On Code Review

#23

Post by pm215 »

eertami wrote: post #18
raducu wrote: post #11

I must be the exception, but I've only had bad or really "meh" experiences with code reviews: Doing the reviews myself: A lot of simple code splintered in 100 files -- I and all others that I know only review the diffs. if(THIS_COERNER_CASE){do this ; do that}; xml configs relating to this or that business case There's nothing to review, really, I don't know the business case, the code looks okish. I really don't car…

> because I care about the convenience of the person that is actually interested in investigating a feature/bug 5 years from now and not of the lazy code reviewer now Doesn't seem like such a big problem to make a PR with multiple commits, and then squash and merge into 1 commit after the review.

Personally I think that is pessimal, because if I am the person investigating in 5 years time I want to see the change split up into multiple logical commits. So lots-of-commits-plus-squash-and-merge requires the developer to do the work of splitting up the feature implementation but doesn't give the benefit of having a usefully split set of commits to the future-debugging-person...

Re: On Code Review

#24

Post by chromanoid »

raducu wrote: post #11

I must be the exception, but I've only had bad or really "meh" experiences with code reviews: Doing the reviews myself: A lot of simple code splintered in 100 files -- I and all others that I know only review the diffs. if(THIS_COERNER_CASE){do this ; do that}; xml configs relating to this or that business case There's nothing to review, really, I don't know the business case, the code looks okish. I really don't car…

The most efficient peer reviews I participated in were early code walkthroughs (prototyping/implementation phase) combined with code reviews. The actual code reviews were much more interesting because you could see what and how the programmer implemented points of critique that came up in the walkthrough. Navigating through the code was also much easier (with bigger changesets), because you already knew to some extent what was changed.

Re: On Code Review

#25

Post by HugoDaniel »

raducu wrote: post #11

I must be the exception, but I've only had bad or really "meh" experiences with code reviews: Doing the reviews myself: A lot of simple code splintered in 100 files -- I and all others that I know only review the diffs. if(THIS_COERNER_CASE){do this ; do that}; xml configs relating to this or that business case There's nothing to review, really, I don't know the business case, the code looks okish. I really don't car…

you forgot the one where you go like this

- "you are right, i completely forgot about that. thank you for catching this, i am changing the code and asking you to review it again after the fix."

Re: On Code Review

#26

Post by maccard »

eertami wrote: post #18
raducu wrote: post #11

I must be the exception, but I've only had bad or really "meh" experiences with code reviews: Doing the reviews myself: A lot of simple code splintered in 100 files -- I and all others that I know only review the diffs. if(THIS_COERNER_CASE){do this ; do that}; xml configs relating to this or that business case There's nothing to review, really, I don't know the business case, the code looks okish. I really don't car…

> because I care about the convenience of the person that is actually interested in investigating a feature/bug 5 years from now and not of the lazy code reviewer now Doesn't seem like such a big problem to make a PR with multiple commits, and then squash and merge into 1 commit after the review.

> Doesn't seem like such a big problem to make a PR with multiple commits, and then squash and merge into 1 commit after the review.

I fundamentally disagree here. If you're reviewing code, the code that is pushed should be what is reviewed, not a draft of what is reviewed..

Re: On Code Review

#27

Post by lhnz »

V-2 wrote: post #5

"Approving a pull request without even testing the code it’s very dangerous" Well; code review is not QA. My approval means - I'm OK with how the code layer is knitted. It doesn't mean I've tested the changes. Of course, if there's some logical problem with the implementation (such as the author seemingly failing to handle an edge case), any careful reviewer should catch this out. Still, this is a situation where the…

  > Well; code review is not QA. My approval means - I'm OK
  > with how the code layer is knitted. It doesn't mean 
  > I've tested the changes.
You will be able to do more substantiative comments if you pull down their code and test it. It will improve the quality of your review, since it can be situated in the context of how the program works instead of how the code is abstracted. Your reviews should consider the practical workings of a program, and not just consistency with patterns or superficial bugs.

I know that more experienced people are able to notice bugs or edge cases due to having a lot of experience and understanding of prior art, but noticing incorrectly designed/implemented logic/behaviour is much easier if your review includes execution of their code.

If you leave this to some later QA stage or QA team then you're just making your reviews less effective and increasing the size of the feedback loop.

Re: On Code Review

#28

Post by somebodythere »

maccard wrote: post #26
eertami wrote: post #18

Earlier quoted context omitted.

> because I care about the convenience of the person that is actually interested in investigating a feature/bug 5 years from now and not of the lazy code reviewer now Doesn't seem like such a big problem to make a PR with multiple commits, and then squash and merge into 1 commit after the review.

> Doesn't seem like such a big problem to make a PR with multiple commits, and then squash and merge into 1 commit after the review. I fundamentally disagree here. If you're reviewing code, the code that is pushed should be what is reviewed, not a draft of what is reviewed..

If you squash and merge shouldn't the final pushed code be exactly the same as what was reviewed?

Re: On Code Review

#29

Post by freddref »

andrewingram wrote: post #15

I've observed a small (yet growing) sentiment emerging lately that code reviews aren't all they're cracked up to be, and can actually be a net negative. I'm somewhat on the fence and can see all the arguments, though still err on the side of code review. But I am somewhat convinced that the way most teams (at least those i've seen) practice them isn't ideal.

Do you have links to reading on the arguments against code review?

Re: On Code Review

#30

Post by monster_group »

Just to be clear - I am strongly in favor of code reviews and do them every day. However, the article advocates spending 20-40% time on doing code reviews. Good luck convincing your manager why your productivity is so low when others are churning out high quality features (because you spent 20-40% of your time reviewing others' code but they didn't spend commensurate time reviewing yours). Net result - other people's code is higher quality, they get more done. Your code is lower quality and you get less done. Good luck getting that promotion or even retaining your job.
Post reply on HN