Changing how I review code • Hacker News Board

Re: Changing how I review code

#61

Post by epage » Thu, Jul 01, 2021, 2:01 AM UTC

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

I see some people pulling down code to check for "correctness" but I don't think reviewers should be testers. I see the focus being on architectural feedback, sharing of knowledge, etc.

I can see within some specific fields pulling the code to do a UX run through but that isn't all fields of software development that that is even needed (99.9% of the time I've never felt the need) and mostly I see authors posting screenshots or screencasts to help reviewers with this.

> There is a sense that pull requests actually might make it too easy to comment on a line of code. This ease might be leading to the conflicts and hyper-zealous commenting that frustrates many in our industry.

If there are a lot of comments, I usually find its because of either a disconnect between author and reviewer that should have been addressed before authoring the code or there is something that needs to be automated.

Re: Changing how I review code

#62

Post by miltondts » Thu, Jul 01, 2021, 2:12 AM UTC

cbsmith wrote: post #39

miltondts wrote: post #25

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…

That's interesting to consider a code walkthrough as different than a code review. How would you describe the differences? To me code reviews can catch style and functional problems in code that get by your automation systems, but the primary impact of a code review is on the team, not the code. It imparts not just an understanding and ownership of the code, but also establishes/negotiates how the team works together…

> That's interesting to consider a code walkthrough as different than a code review. How would you describe the differences?

The code review process I'm familiar with is: open a PR, wait for a couple of reviewers to comment on the work. Then normally what happens is:

- The comments are trivial, so I implement the changes (these are almost always things that could be automated, but for one reason or another haven't been).

- The comments need to be discussed/clarified, so I need to call the person.

The code by itself doesn't have all the context, all the things that were tried etc. A code walkthrough is a session with other coders, where it is explained how and why we arrived at the solution, what considerations were made and so on. Walkthrough sessions provide much faster feedback and all improvements can be discussed immediately with much greater clarity.

Re: Changing how I review code

#63

Post by cbsmith » Thu, Jul 01, 2021, 2:40 AM UTC

miltondts wrote: post #62

cbsmith wrote: post #39

Earlier quoted context omitted.

That's interesting to consider a code walkthrough as different than a code review. How would you describe the differences? To me code reviews can catch style and functional problems in code that get by your automation systems, but the primary impact of a code review is on the team, not the code. It imparts not just an understanding and ownership of the code, but also establishes/negotiates how the team works together…

> That's interesting to consider a code walkthrough as different than a code review. How would you describe the differences? The code review process I'm familiar with is: open a PR, wait for a couple of reviewers to comment on the work. Then normally what happens is: - The comments are trivial, so I implement the changes (these are almost always things that could be automated, but for one reason or another haven't be…

Got it. So for my semantics, your review process for PR's sucks, lacking engagement, and you have to have meetings to get it done right.

Re: Changing how I review code

#64

Post by cbsmith » Thu, Jul 01, 2021, 2:41 AM UTC

mandelbrotwurst wrote: post #48

cbsmith wrote: post #36

Earlier quoted context omitted.

Interesting. If before you weren't intending to understand and be able to summarize before, what were you doing when you reviewed code?

Writing the summary is a higher bar than just deciding that you could.

I agree. I just was curious if that was the only difference. The way it was phrased it sounded like it wasn't.

Re: Changing how I review code

#65

Post by chii » Thu, Jul 01, 2021, 2:44 AM UTC

LeftHandPath wrote: post #53

clipradiowallet wrote: post #9

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…

At work, I've been writing a word document to explain every part of a program I've written and how each element interacts with the rest. It's been very helpful to find things that don't make as much sense as I thought they did when I wrote them, and revealing old code that is no longer used anywhere in the application. And I've caught a few places where outdated .txt / .md / inline-doxygen documentation lies, too. Wr…

But wouldn't this summary document be better as a form of comment in the source code rather than a separate document that will become out of date the next time someone else makes changes?

Re: Changing how I review code

#66

Post by TheCoelacanth » Thu, Jul 01, 2021, 3:01 AM UTC

miltondts wrote: post #25

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…

None of those are the type of things that code reviews should be focused on catching. They should be focused on things like:

- Does the code make sense to a human?

- Is this the best way to achieve the objective?

- Is the thing that the code is trying to do even something that should be done?

Re: Changing how I review code

#67

Post by dragonwriter » Thu, Jul 01, 2021, 3:06 AM UTC

miltondts wrote: post #25

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…

> Most style issues should be caught by automated tools

Some style issues can be, others (“do variables, functions, etc. have meaningful, accurate, relevant names?”) can’t be, until the automated tools are backed by AGI, but at that point “code review” and “automated analysis” become...not so different.

Re: Changing how I review code

#68

Post by presentation » Thu, Jul 01, 2021, 3:17 AM UTC

stavros wrote: post #56

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…

> Just have two people do the work together, and lose the need for constant context switches and wasted time. 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.

I find I work more efficiently when I pair program, and am usually more likely to choose an approach that is good for the future of the product, so I don’t think your statement holds.

Re: Changing how I review code

#69

Post by rixed » Thu, Jul 01, 2021, 5:31 AM UTC

TheCoelacanth wrote: post #66

miltondts wrote: post #25

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…

None of those are the type of things that code reviews should be focused on catching. They should be focused on things like: - Does the code make sense to a human? - Is this the best way to achieve the objective? - Is the thing that the code is trying to do even something that should be done?

Those last two are the most important (and less subjective), and should definitively happen before the code is even written, using design docs.

Re: Changing how I review code

#70

Post by Dunedan » Thu, Jul 01, 2021, 5:43 AM UTC

QuercusMax wrote: post #46

kmtrowbr wrote: post #35

Earlier quoted context omitted.

You should pull the PR you're reviewing down into your IDE and you should spend like, an hour reviewing it carefully and trying to break it. Have a mental checklist: say they renamed a function ... is the file named the same as the function? Did they remember to rename the file? Did they delete parts of the code? Did they remember to delete everything related to that? Often the most challenging parts are negative: it…

How big are these changes? That sounds horrendously slow. If you don't trust your tests, write better tests. If you don't trust your migrations, write better tests.

In probably most cases the person who does the code changes does write tests for it as well. So as a reviewer without understanding what the code and the tests do, how do you want to assess if the tests actually do something useful?