Changing how I review code

https://itnext.io/im-changing-how-i-review-code-dbf212507e5f

by deletionist • 5 years ago

88 78 5 years ago

itnext.io

deletionist• 5 years ago

https://archive.is/jeDmL

avmich2• 5 years ago

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

kasiiaa• 5 years ago

Originally published at https://dangoslen.me.

https://dangoslen.me/blog/changing-how-i-review-code/

yjftsjthsd-h• 5 years ago

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?

juanca• 5 years ago

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.

watwut• 5 years ago

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

bern4444• 5 years ago

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

sfvisser• 5 years ago

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!

the_af• 5 years ago

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

loloquwowndueo• 5 years ago

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.

travisjungroth• 5 years ago

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

elwell• 5 years ago

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

kmtrowbr2• 5 years ago

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.

all22• 5 years ago

Can you expand on this?

LeftHandPath2• 5 years ago

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.

all22• 5 years ago

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.

jimbobimbo2• 5 years ago

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.

Chernobog2• 5 years ago

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!

debarshri2• 5 years ago

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.

Chernobog2• 5 years ago

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

t-writescode• 5 years ago

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.

tantalor• 5 years ago

Get more resources

twistedpair3• 5 years ago

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.

clipradiowallet2• 5 years ago

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.

cbsmith5• 5 years ago

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

mandelbrotwurst• 5 years ago

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

clipradiowallet2• 5 years ago

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.

LeftHandPath2• 5 years ago

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.

chii• 5 years ago

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?

uvesten2• 5 years ago

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.

dave_sid2• 5 years ago

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

marceloabsousa2• 5 years ago

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

the_gipsy2• 5 years ago

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

stavros• 5 years ago

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.

presentation• 5 years ago

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.

twistedpair3• 5 years ago

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.

marceloabsousa2• 5 years ago

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

IvyMike• 5 years ago

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.

miltondts2• 5 years ago

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.

the_gipsy2• 5 years ago

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

Because organisations obsess about risk minimisation.

cbsmith5• 5 years ago

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.

miltondts2• 5 years ago

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.

beebmam• 5 years ago

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.

01100011• 5 years ago

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.

valenterry• 5 years ago

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.

TheCoelacanth• 5 years ago

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?

rixed• 5 years ago

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

dragonwriter• 5 years ago

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.

omegalulw• 5 years ago

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.

twistedpair3• 5 years ago

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

jimbobimbo2• 5 years ago

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.

dan-robertson• 5 years ago

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.

irjustin• 5 years ago

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!

vncecartersknee• 5 years ago

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.

ferdowsi• 5 years ago

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.

epage• 5 years ago

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.

moogly• 5 years ago

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

Changing how I review code

Related Stories