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

Changing how I review code

itnext.io

Post reply on HN
Print view View original

21–30 of 80 posts

Re: Changing how I review code

#21

Post by loloquwowndueo »

yjftsjthsd-h wrote: post #4

> Another common critique is that the pull requests encourage reviewers to only look at the code. They never even pull down the changes they are reviewing! You should run it somehow, but if you've got CI running tests, which I think you should have anyways, then does it matter?

A reviewer in my team is fond of changing some of the code in the PR and running the tests. The expectation is that if the code is changed randomly a test should always fail. This helps catch untested code paths.

It’s somewhat similar to the practice of automated code fuzzing, only manual and scoped to changes in a particular PR.

Re: Changing how I review code

#22

Post by convolvatron »

uvesten wrote: post #16
marceloabsousa wrote: post #15

Earlier quoted context omitted.

Code reviews and PRs are different things. Still, PRs can be effective with the right tooling and methodology. We've doing continuous code reviews with PRs and it's been working great to have early feedback. More details at: https://reviewpad.com/blog/continuous-code-reviews-are-the-w...

If we want to argue semantics, sure, but (at least in my experience) a PR is by far the most common way. And from the linked article, they seem to say that pair programming is the ideal, but that continuous code reviews bring most of the same benefits. Sounds good to me. But. I’m still convinced that just doing proper pair programming from the start saves time for the organization, it’s just that the upfront investme…

I personally feel quite comfortable working on something and pinging a peer if I've having an issue figuring out a good way to do something or I want to vet the approach.

then they don't need to spend all afternoon watching me get tea and pick my nose and we still get to have the interesting discussions.

Re: Changing how I review code

#23

Post by avmich »

kasiiaa wrote: post #20
avmich wrote: post #3

Earlier quoted context omitted.

Can somebody post a link which wouldn't start with "One more step Please complete the security check to access"?

> Originally published at https://dangoslen.me . https://dangoslen.me/blog/changing-how-i-review-code/

Thank you.

Re: Changing how I review code

#24

Post by geordimort »

uvesten wrote: post #16
marceloabsousa wrote: post #15

Earlier quoted context omitted.

Code reviews and PRs are different things. Still, PRs can be effective with the right tooling and methodology. We've doing continuous code reviews with PRs and it's been working great to have early feedback. More details at: https://reviewpad.com/blog/continuous-code-reviews-are-the-w...

If we want to argue semantics, sure, but (at least in my experience) a PR is by far the most common way. And from the linked article, they seem to say that pair programming is the ideal, but that continuous code reviews bring most of the same benefits. Sounds good to me. But. I’m still convinced that just doing proper pair programming from the start saves time for the organization, it’s just that the upfront investme…

I find that there’s a lot of bias in pair programming. One takes the role of the lead dev and the other of the ‘copilot’ but not actively reviewing the code. It works great if you want to review later asynchronously without needing to ask for more context. Still pair programming doesn’t replace an asynchronous code review afterwards. Ideally every dev should let the code sink a bit and then review it before distributing it. I see both approaches working together - don’t get this trend of advocating for pair programming as the best of getting the highest quality.

Re: Changing how I review code

#25

Post by miltondts »

Code reviews are not very good:

- Most style issues should be caught by automated tools

- Most functional issues should be caught by tests (written by another person preferably)

- To bring someone up to speed on your chosen style, pair programming is much faster than code reviews

- To have shared knowledge of code (increased bus factor) pair programming, or code walkthroughs are much faster

Not sure why we have fallen so much in love with the idea.

Re: Changing how I review code

#26

Post by t-writescode »

debarshri wrote: post #7
Chernobog wrote: post #6

> Another common critique is that the pull requests encourage reviewers to only look at the code. They never even pull down the changes they are reviewing! Our CI automatically deploys a "review app" when someone creates a pull request. This simplifies code review, QA and demoing!

It sounds cool that you create on-demand review app. In a small scale it works. In a larger scale development is absolute impossible due to resource constraints to create on-demand environment for review app. For instance, in a warehouse automation infrastructure that has 30 devs, working on 8 feature simulataneously, it is next to impossible to create review app with the resources they need.

At a former company I worked for, every branch would create a new environment with those changes (and some shared components that were owned by other teams); and, if multiple repositories had the same new branch name, it would use all the ones with the same name.

Depending on your system, this is entirely doable.

Re: Changing how I review code

#27

Post by travisjungroth »

loloquwowndueo wrote: post #21
yjftsjthsd-h wrote: post #4

> Another common critique is that the pull requests encourage reviewers to only look at the code. They never even pull down the changes they are reviewing! You should run it somehow, but if you've got CI running tests, which I think you should have anyways, then does it matter?

A reviewer in my team is fond of changing some of the code in the PR and running the tests. The expectation is that if the code is changed randomly a test should always fail. This helps catch untested code paths. It’s somewhat similar to the practice of automated code fuzzing, only manual and scoped to changes in a particular PR.

That's mutation testing[1]. For most languages there are tools that can scope the mutations to a diff.

[1]https://en.wikipedia.org/wiki/Mutation_testing

Re: Changing how I review code

#28

Post by the_af »

sfvisser wrote: post #18
yjftsjthsd-h wrote: post #4

> Another common critique is that the pull requests encourage reviewers to only look at the code. They never even pull down the changes they are reviewing! You should run it somehow, but if you've got CI running tests, which I think you should have anyways, then does it matter?

Of course it matters. Otherwise, how do you know if your app works as intended? And don’t tell me your tests tell you, because I guarantee you they don’t!

Isn't that what QA/testers are for? Testing that the feature/release works as intended?

Re: Changing how I review code

#29

Post by elwell »

yjftsjthsd-h wrote: post #4

> Another common critique is that the pull requests encourage reviewers to only look at the code. They never even pull down the changes they are reviewing! You should run it somehow, but if you've got CI running tests, which I think you should have anyways, then does it matter?

In practice, I end up finding at least one bug nearly every time I pull the code down and test it locally.

Re: Changing how I review code

#30

Post by kmtrowbr »

yjftsjthsd-h wrote: post #4

> Another common critique is that the pull requests encourage reviewers to only look at the code. They never even pull down the changes they are reviewing! You should run it somehow, but if you've got CI running tests, which I think you should have anyways, then does it matter?

You should attack it. You should pick it apart and try to break it in as many ways as possible. Of course you should meter your effort depending on how critical the changes are. It amazes me the lackadaisical attitude towards code review. "It will probably be okay" is the worst attitude to have with computer code. It is the most brittle, brutal environment imaginable.
Post reply on HN