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

11–20 of 80 posts

Re: Changing how I review code

#11

Post by Chernobog »

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.

I'm not going to dismiss your experience, or your knowledge about the environment you describe, but we are most definitively not doing small scale development. Our company has a large software stack comprised of several backends and frontends, maintained by 8+ teams.

I maintain one frontend in this stack, and the review app produced will only contain my version of the front-end, and the API calls will go to the backends in the development environment.

Review apps for backend services will also point to the development environment if this particular backend needs to communicate with other services.

When a feature touches a frontend and a backend simultaneously, we can solve that in various ways. Sometimes the backend can be finished first, but if that's not feasible we can supply URL overrides for endpoints to the review app of the frontend, so it communicates with the review app of the backend (and uses the rest of the development environment for the other calls).

This works for us and might not be feasible for all projects/stacks/companies for various reasons. YMMV I guess :)

Re: Changing how I review code

#12

Post by uvesten »

Pair programming is sooo much better than PR’s. I really loathe reviewing pull requests, no matter if you do it absolutely correct, and don’t miss anything, it’s extremely wasteful of the time of both the coder and the reviewer, even if there are no changes that need to be made. There is the context switch for the reviewer, ditto for the coder, waiting for feedback. Ugh. Just have two people do the work together, and lose the need for constant context switches and wasted time.

Re: Changing how I review code

#13

Post by dave_sid »

uvesten wrote: post #12

Pair programming is sooo much better than PR’s. I really loathe reviewing pull requests, no matter if you do it absolutely correct, and don’t miss anything, it’s extremely wasteful of the time of both the coder and the reviewer, even if there are no changes that need to be made. There is the context switch for the reviewer, ditto for the coder, waiting for feedback. Ugh. Just have two people do the work together, and…

Yeah I’ve always felt that PRs are ineffective. It’s to late a stage in the development process to give feedback.

Re: Changing how I review code

#15

Post by marceloabsousa »

dave_sid wrote: post #13
uvesten wrote: post #12

Pair programming is sooo much better than PR’s. I really loathe reviewing pull requests, no matter if you do it absolutely correct, and don’t miss anything, it’s extremely wasteful of the time of both the coder and the reviewer, even if there are no changes that need to be made. There is the context switch for the reviewer, ditto for the coder, waiting for feedback. Ugh. Just have two people do the work together, and…

Yeah I’ve always felt that PRs are ineffective. It’s to late a stage in the development process to give feedback.

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

Re: Changing how I review code

#16

Post by uvesten »

marceloabsousa wrote: post #15
dave_sid wrote: post #13

Earlier quoted context omitted.

Yeah I’ve always felt that PRs are ineffective. It’s to late a stage in the development process to give feedback.

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 investment seems too great. (I’d argue that spending 2x programmer time is easily the cheapest way to get a feature done, and if you think you are saving costs/being more productive doing solo dev + code review you’re not accurately measuring time spent. (Or that the review is very superficial.))

Re: Changing how I review code

#17

Post by shinryuu »

bern4444 wrote: post #10
watwut wrote: post #8

Earlier quoted context omitted.

That is what testers are for. Testing is great thing.

/s/testers/users Just kidding and in reality UI tests exists and aren't super difficult to set up. The front end has plenty of tools like Cypress and its pretty simple to automate running a bunch of tests that screenshot and diff compare your site. I never really pulled down UI code to test them out. There has to be a certain level of trust between engineers. Sometimes I'd ask or post screenshots of changes but that…

Still, getting a second set of fresh eyes using the code that you wrote is pretty invaluable. This can then be fed back as improvements.

Re: Changing how I review code

#18

Post by sfvisser »

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!

Post reply on HN