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

41–50 of 58 posts

Re: On Code Review

#41

Post by siva7 »

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 there are engineers on the team wo assign PR for review that aren’t tested by themselves or don’t contain automated software tests (if the project has such standard) it’s time for the manager to have a 1on1 discussion about professional standards. Sometimes juniors aren’t aware of these shortcomings or are coming from a shop with bad development practices. They just need sometimes to learn professional practices and there is nothing lost.

Re: On Code Review

#42

Post by ozim »

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.

Efficient role separation?

When devs would only write code and never click anything around in the system how can they implement anything good?

I don't believe in "efficient role separation" that is 100%. Devs still need to click the system around and still need to attend meetings that give business context. The same for QA, they should understand basic exceptions and know where to find logs to make good reports and not just make screenshot and say "not passed" and drop all troubleshooting on the devs or ops.

Best outcomes I see are when you have multidisciplinary team where team members work together. Of course you have Dev or QA specialists but you have to have people who don't throw stuff over the wall and are focusing on delivering togheter.

Re: On Code Review

#43

Post by ozim »

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.

I think the problem here is that people who thought CR will be some "silver bullet" see it is not working for them.

Code reviews are in my opinion still a good tool. It works well in other industries as well. You want to bounce stuff from the other people before pushing it further.

Where it goes wrong is people - so it is people problem not the tool problem. If you do not have good people you won't fix them with code reviews.

It is the same with agile, managers tend to think they can just follow some agile coach and can hire anyone to the team. But the truth is, I see bad teams going bad with or without agile and good teams agile or not going strong.

Re: On Code Review

#44

Post by [deleted] »

ozim wrote: post #42
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.

Efficient role separation? When devs would only write code and never click anything around in the system how can they implement anything good? I don't believe in "efficient role separation" that is 100%. Devs still need to click the system around and still need to attend meetings that give business context. The same for QA, they should understand basic exceptions and know where to find logs to make good reports and n…

[deleted]

Re: On Code Review

#45

Post by [deleted] »

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

[deleted]

Re: On Code Review

#46

Post by ljm »

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

It's a lot easier to do in an application that's easy to boot up locally. A lot of software in web-startup world grows out of that stage once the k8s/distributed architecture earworm burrows in.

Re: On Code Review

#47

Post by barrkel »

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…

FWIW, that series of small separate requests, reviewed separately, is pretty much how code review works in Google. Each change is attached to the same tracking issue which is what tracks completion, rather than any single or group change being committed.

Yes, alone they don't provide actual value, but they are small (easy to review) and can be trivially / automatically rolled back (database migrations aside) and can be deployed (without being used) safely, and even when they are exercised, it may be via a switch, itself toggled with a pull request which itself can be rolled back in isolation.

Whatever you think is a small change, think even smaller. Think creating a class and stubbing out many of the methods with TODOs.

Delaying the merge until all commits required for the feature wouldn't work in a large monorepo - just keeping small commits up to date can be work when there's enough people working around the clock. There's no feasible way to sustain continuously rebasing in a feature branch model in a large monorepo.

Re: On Code Review

#48

Post by lhnz »

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.

It doesn't seem possible to me that "role separation" could be more efficient.

If you wait until code is merged into `master` before doing further testing, you're saving yourself 15 minutes of time, but when a problem is found everybody will be disconnected from the source of the bug.

If you, as a software engineer, pull down the branch and try running the code then you'll be able to find bugs, edge-cases, misunderstandings of the design (etc) and then literally point out where they occurred in the source code to your colleagues at the point of the review.

It saves a bunch of time to do it this way, because it only takes (I think the only reason that people rest on "role separation" here is because they've never actually tried doing anything deeper so can't comprehend that it helps to find deeper problems.)

Re: On Code Review

#49

Post by wokwokwok »

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…

Bluntly, this is the problem with code reviews: at the review stage it’s too late.

Changes from this type of review are usually fine details (“use a constant for this”, “consider this edge case”, “prefer this style…”) that can be done easily by the reviewer with no context.

If you’re lucky, you might get meaningful feedback (“consider using this approach instead…”), but many people just use code reviews as a gateway to merging code (1).

This is the pragmatic approach; just trust the other developers and doing their jobs and do a light pass check to ensure that everyone is aligned on approach and style.

..but, that’s not what you were tasked with.

You were asked to review the code, not the syntax.

You cannot review code you do not understand.

I’m sorry, but your mental parser does not run code. It does not render ui, update databases or generate concurrent race conditions. You may be able to approximate some of those, but most people can only do all of those things by actually running the code.

So… you may get some value from looking at code; but I question, in almost all cases, if teams contributed significant value by doing so.

[1] - see https://blog.jetbrains.com/upsource/2017/01/18/code-review-a..., etc. google “code review as gateway”.

Re: On Code Review

#50

Post by frank_bin »

gocartStatue wrote: post #10

I find post-coding, pre-merge code reviews ineffective, for a change. The code was already written. Time/money spent. Finding out that solution is subpar at this stage is costly, and delaying integration makes ineffective teams. If you want to enforce standards, automate it (there are multiple highly configurable tools for that). If you want good solutions, pair program. If you want to maintain good codebase, review…

I tend to agree. I prefer design drafts (documents) instead of pair programming, though.
Post reply on HN