51–60 of 80 posts
Oh man I'm so guilty of this. If there's a spec, I basically say "YEP LOOKS GOOD!"
But shamefully, I've been bitten by this when the spec itself is written with the wrong logic. Argh!
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 falle…
Sure, reviews are a time sink. No one likes them. We are still better off having done them.
I had the pleasure of being at a startup with a very talented SRE named M. Harrison. His minimum-level review criteria were that when you were planning to approve a PR, you should summarize everything the PR does in the approval comment. Responses like ":+1:" or "LGTM" were not allowed. When I would read a PR intending to understand and then summarize, the errors or omissions (eventually) started to just jump out at…
Writing a summary document really forces you to get to the nitty gritty and catch things that a casual review would miss.
> 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.
There's a human instinct towards laziness. If there are no outwardly-visible reasons to distrust something, it doesn't get evaluated. It's why a hard-hat and a clipboard can get you backstage, and why you don't spend 2 minutes scanning every bush for crouching predators before you walk past it. In everyday life, a "lazy" tendency saves time by avoiding needless work. In engineering, academia, etc., it can create dangerously low standards.
Earlier quoted context omitted.
Writing the summary is a higher bar than just deciding that you could.
Wouldn’t most people just restate the ticket? Or do you mean, they write about the actually implementation. Eg, “this fixes the race condition by removing the timeout and waiting until the initialization task finishes (line #55) before instantiating the chart widget”
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…
What kind of PR process do you have where reviewing the PR takes as long as writing the code in the first place? Because that's the only way pair programming is as efficient as code reviewing.
Earlier quoted context omitted.
Writing the summary is a higher bar than just deciding that you could.
Wouldn’t most people just restate the ticket? Or do you mean, they write about the actually implementation. Eg, “this fixes the race condition by removing the timeout and waiting until the initialization task finishes (line #55) before instantiating the chart widget”
How often is this even feasible though? sure if there's little difference between the PR branch and whatever the last build I've done is, it wont take too long to build but then I have to install our product and get it set up to test. We have a ton of legacy code that isn't easily testable. We do have a CI build but it takes like ~6 hours to build.
> They never even pull down the changes they are reviewing! To be completely transparent, I’m guilty of that myself. How often is this even feasible though? sure if there's little difference between the PR branch and whatever the last build I've done is, it wont take too long to build but then I have to install our product and get it set up to test. We have a ton of legacy code that isn't easily testable. We do have…
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 falle…
Hence, a combination of intense pair programming and knowledge sharing sessions in combination with quick & lightweight PR reviews as well as regular refactorings has worked best in my experience.