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

31–40 of 58 posts

Re: On Code Review

#31

Post by tsimionescu »

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

Earlier quoted context omitted.

"I find post-coding, pre-merge code reviews ineffective, for a change. The code was already written. Time/money spent." This implies the need for smaller pull requests maybe? If work is broken into smaller chunks, addressing peer feedback isn't as costly, because there's time to back out of poor decisions if the peers get alerted to them early on. Changing the course once the implementer has indeed invested lots of t…

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

While I think this would be nice to have in principle, I think the devil is really in the details. For example, the structure you show for these sub-commits is one I generally hate to review - the data models may look OK when I look at them, but then when I see the implementation code, I realize that the data models were inadequate etc.

Re: On Code Review

#32

Post by CraigJPerry »

monster_group wrote: post #30

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…

In those environments i'd expect a tech lead to take the initiative to stand up and explain / defend the team's quality control processes.

If the management rebukes, you move employer.

Quality is not something the customer or your manager is responsible for. It's entirely on us as professional developers.

That said - code review is not the only way to ensure quality. Do what works. Pairing generally works even better for getting quality review time in.

Re: On Code Review

#33

Post by [deleted] »

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.

[deleted]

Re: On Code Review

#34

Post by andrewingram »

freddref wrote: post #29
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?

I'll see what I can dig up. Some of it is from Allen Holub, who I appreciate is often met with mixed responses here; but his angle is mostly that if you have sufficient up-front collaboration (he favours mobbing), code review don't offer you anything on top of what automated tools provide, and can actually erode team trust/morale because they're perceived as a barrier often introduced as a way to say "no" to people.

I don't agree 100%, but I am on-board with the sentiment. I've seen systemic problems of under-communication pre-review which results in me looking at a PR and seeing a fundamental issue, but not knowing how to approach tackling it without hurting feelings. I don't think eliminating reviews is the solution, but I think it's much less of a critical step than it's often made out to be. My take is that if reviews are actually preventing major issues with released code, they're almost certainly including work that should've happened much earlier in the development process.

Re: On Code Review

#35

Post by siva7 »

lhnz wrote: post #27
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 practic…

It’s a matter of efficient role separation. Most teams i’m aware of do not manually test a PR in the same cycle as a code review. It is absolutely expected that the PR author already tested his changes sufficiently. The manual test is usually reserved for the product or QA team.

Re: On Code Review

#36

Post by [deleted] »

fredrb wrote: post #14
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. I'm 100% on-board with this. However, I understand were OP's mindset comes from. A few years ago I was working at a development shop in a fairly large project, where we had little to no automated tests. We frequently had PRs that would break main features. At some point developers wer…

[deleted]

Re: On Code Review

#37

Post by pfdietz »

Just asking the person who wrote the code to explain it better so I could properly review it can be helpful. I've had cases where this led to "yeah, that's not right, I'll redo it" even if I had no clue what was going on.

Re: On Code Review

#38

Post by detaro »

monster_group wrote: post #30

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…

These things obviously are to a degree a matter of team-level process and policy. In other teams it might very well be "good luck getting that promotion if you constantly refuse to participate properly in code reviews".

Re: On Code Review

#39

Post by pixel_tracing »

siva7 wrote: post #35
lhnz wrote: post #27

Earlier quoted context omitted.

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

It’s a matter of efficient role separation. Most teams i’m aware of do not manually test a PR in the same cycle as a code review. It is absolutely expected that the PR author already tested his changes sufficiently. The manual test is usually reserved for the product or QA team.

Good luck getting junior or mid level engineers to do this properly each and every single time :) hell I’ve seen even seniors do this improperly

Re: On Code Review

#40

Post by BobbyJo »

pixel_tracing wrote: post #39
siva7 wrote: post #35

Earlier quoted context omitted.

It’s a matter of efficient role separation. Most teams i’m aware of do not manually test a PR in the same cycle as a code review. It is absolutely expected that the PR author already tested his changes sufficiently. The manual test is usually reserved for the product or QA team.

Good luck getting junior or mid level engineers to do this properly each and every single time :) hell I’ve seen even seniors do this improperly

If you can get people to test in review, I don't see why you couldn't get them to test before review. If you can't get your developers to test appropriately, then you've hired bad developers.
Post reply on HN