Changing how I review code | Hacker News Reader

Changing how I review code

itnext.io

88 pointsdeletionist78 comments

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 me. I'm not sure if I'm articulating it properly...but that expectation of a small book report-ish paragraph helped me become much better at critically reviewing PRs.

disclaimer: the majority(not all) PR's were in a very very large terraform codebase.

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.

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”

The latter would be a very useful code commit message, (the line #55 may be excessive), both for the author and the reviewer.

I long for tickets that descriptive

The ticket would probably just say “this thing doesn’t load correctly sometimes”, my description was for the code review

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

Here's a contrived example... Let's say a PR arrived that appeared to create a new S3 bucket and a new IAM role with an attached policy that grants read-only permissions to that bucket.

The old me would read it, and write something like "adds S3 bucket and IAM role".

The newer me would write it as:

1. Creates S3 bucket <XYZ> in account <DEF>.

2. Creates IAM role <ABC>, with an attached policy granting read-only access to S3 bucket <XYZ>.

It seems like a small change, but when I'm actually planning to type out those details - I'm more likely to notice typos in the name/account/role/etc, or just outright errors. One of the mental shifts from my old reviews to newer ones is that I open the PR assuming I don't know what it does yet. So I have to read it for myself and interpret it, then summarize it for someone else.

Thanks, this helped clarify for me what your experience was like. It's interesting.

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.

Writing a summary document really forces you to get to the nitty gritty and catch things that a casual review would miss.

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?

Furthermore, wouldn't a Markdown document be a bit better as opposed to Word? That way, you or anyone else could easily search through its contents alongside the rest of the codebase, were it to be included in the repository, whereas that wouldn't work with Word, because it's a proprietary and binary format (unless you open that file in particular).

> 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 :)

You are looking at a problem from developer's perspective. From infrastructure perspective, it is a terrible pattern if you own the feature-flow or review app flow. It is really wasteful with no guarantee that when all the features that are written concurrently are merged, it your feature will still work as expected.

Let me put it in another way, would it have been if you would have kept, development, test, staging and production environments up to date all the time and had meaningful integration tests?

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.

Get more resources

Depends on the app/arch. If you're doing a stateless SPA, the cost is trivial. For example, my org deploys _every_ commit as a preview env. That's thousands of preview envs each month, which we then run E2E automation tests against, to validate each commit.

Using serverless hosting, the hosting bill is < $10/mo.

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.

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

Because organisations obsess about risk minimisation.

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.

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.

This is a deeply unpopular opinion, but I find myself aligning with this more every day. I think code reviews are a fundamentally broken process. Design reviews, before any (non-prototype) code even gets written, are vastly more important than code reviews in my opinion.

Upvoted because these are all great points that I agree with but code review is still worth the trouble in many situations. On my team it helps us keep a consistent style and forces more people on the team to stay up to date with changes. Code quality is improved because of our review process. Pair programming would accomplish some of that, but not all team members would have a voice. Further, reviews are asynchronous.

Sure, reviews are a time sink. No one likes them. We are still better off having done them.

I agree. Code reviews are still a nice "sanity check" where I don't have to understand in full detail what happens and I don't criticize style problems and minor issues. However, I check if the change fits into the bigger picture (in terms of architecture), that the intention of the changes is correct and that tests are added if needed.

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.

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.

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

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

Nope. People have biases, and if you are teaching coding practices via pair programming you will just propagate those biases. By having a new team member receive feedback from multiple members, these biases are more likely to average out. For style specifically, it's also wise to get reviews from other teams for new people.

> - Most functional issues should be caught by tests (written by another person preferably) > - Most style issues should be caught by automated tools

I do a lot of Typescript coding. Using Prettier/eslint rules, a host of (e.g. hundreds) of common stylistic and functional issues are caught in CI/CD automatically.

Better yet, I've made a `npm run fix` command that will fix 99% of them instantly.

If your tooling can automatically bring the code into compliance, then folks won't push back and will be highly likely to comply :)

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

At least with UI code, the tests / code don't really express the experience for the user. (But it's getting better with time!)

In that practice, it's better to run it locally! For each UI file / function, try to get it to execute from a user's POV.

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

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.

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

In a well oiled machine, that's what the QA department does. They have close working relationships with the engineering and customer support departments so they develop testing processes and systems tailored to the "quirks" of engineers and users. They spend extra time on the places they know the engineers have blind spots and work on automated regression tests based on customer support workloads.

Whether they go as low level as unit tests depends on the organization but one place I worked at, the domain was complicated enough that "I see no red squiggles in my IDE, pinky swear" was the threshold to go from engineering to QA. It was an ongoing joke that whenever there was a major dependency update, 10-25% of the work sent to QA that day contained obvious syntax errors and wouldn't compile because the IDE everyone used often took an hour or more to reindex the project without any clear indication that it was in a useless intermediate state. Running the compiler would slow it and the indexer to a crawl, so the engineers just YOLOd it so they could move on to other work, and the QA department set up a static analyzer with an empty rule set on all PRs to catch dumb errors caused by immutable process/purchasing decisions.

IME these kinds of QA departments are the #1 springboard into software engineering roles because they work so closely with engineering and are exposed to code in the form of automated tests.

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?

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

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

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.

Can you expand on this?

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's harder to remove properly than it is to add new things. Say they changed the validation on a model: is this going to work well with the production database? Do we need to write a migration to catch the database up to the new reality of the code? Automated tests are just automated warning flags that give you confidence that the codebase is basically hanging together. You still need to be very careful & thoughtful as you're making changes.

I like to actually reset the branch so I can see all the raw changes in my IDE, this helps me to think it through, in git you can do this via:

git pull feature-branch; git checkout develop; git checkout -b my_initials/feature-branch; git merge feature-branch --squash --no-commit

An hour is way too much time. If you need that long the pr is too big and needs to be broken up into smaller parts

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?

If your code and tests are so opaque that the reviewer can't understand them without physically running and modifying the code, then as a reviewer you should ask the author to make it clearer.

Because otherwise that's how you get write-only code. Things like code style and documentation can make a big difference, and review time is when you can help enforce them and build team culture and knowledge.

It's something I've noticed in modern peer-review practices, too. And some have gone as far as creating fake papers to study whether or not they get published: https://www.theatlantic.com/ideas/archive/2018/10/new-sokal-...

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.

Just because there are tests doesn't mean the tests are any good. I've only started my quest in TDD and I've discovered it is rather trivial to write tests that don't really mean anything.

I find myself asking "what is this supposed to do?" and "does this actually do what it is supposed to do?". I tend to drift into a design mindset during test writing (more-so than in code review) that highlights data flow and the basic bits of logic. Even then, I find that I can miss test cases rather easily.

That's what code coverage tooling + human code reviewers are for. Look at the change in product, look at the amount of changes in tests - do they make sense? E.g., product change adds a non-trivial function with 10 if/else branches, but UTs touch only happy path - that's a legitimate blocker on the PR.

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.

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

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.

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.

Sure it is

PRs are perfect in open source . In companies they’re just an illusion of minimising risk.

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

I think there is a middle ground.

If I'm doing a PR (e.g. for a new dev), and it's becoming a real bloodbath, I stop doing the PR and do a 1:1 pair programming session w/ the dev. This helps save face and keeps the advice private and inculcation.

Don't let PR's turn into public floggings.

https://archive.is/jeDmL

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.

I'm doing a lot of code reviews and I never run the code myself. PR owner does that, PR validation suite (essentially reduced CI) does that.

"This code works when you run it" is the last thing I care about when I'm reading the code - it's a given, we won't allow change to merge if it wasn't the case. Code reviews are there to catch mistakes in design, copypasta, how the change fits into the product overall.

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

> 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 a CI build but it takes like ~6 hours to build.

This is why fast-to-compile languages have become critical for me. Working in Go means that I can make reviews that I can feel confident in.

My biggest issue with code reviews/PRs is that it runs counter to trunk-based development, of which I am a fan.

I much prefer to go in and check people's small commits to master than look at a huge PR before it gets merged in, but it requires you (and the whole team) to be really diligent and do it on a daily basis, and when it comes to big features, you lose a lot of the context. OTOH, the amount of times I've been presented with a huge PR of an entire feature where the author begins by saying "sorry for the huge PR" and my predictable reply "I'm having trouble getting the whole picture here, but LGTM?" are countless, so I'm not sure that's better either.

Not sure how to reconcile the two models into something great.

As for the style of code review, I ask the team and adjust accordingly. I can be "big picture guy", or I can do that and simultaneously be "Mr. Nitpick" if people so wish (some people actually do, me included).

I’d be curious to see a survey of how different companies do code review or it’s equivalent. I’d like something about the motivations they had for setting up the systems in certain ways and how they changed them over time. At my employer, code review was once done periodically as a big Herculean effort at each “release” (development stopped for code review) involving printing out the code/diffs on physical paper to read it and a senior person reading basically all the critical code. Over time the system became more automated and digital, and the requirements for review were also reduced. But I have no idea how that compares to other places apart from the first thing sounding very silly.

> They never even pull down the changes they are reviewing! To be completely transparent, I’m guilty of that myself.

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!

I wanted to read the article but it's behind a paywall. Is there a way to access it?

I didn't hit a paywall but the article was originally published at https://dangoslen.me/blog/changing-how-i-review-code/ ; maybe that will work for you.

Reply on news.ycombinator.com