No code reviews by default • Hacker News Board

Re: No code reviews by default

#191

Post by strken » Wed, Jan 05, 2022, 1:52 AM UTC

joombaga wrote: post #145

snarf21 wrote: post #43

Earlier quoted context omitted.

I think it is trust in the trust and verify sense. Meaning, we trust each other to do our best but let us verify that we are all on the same page and aren't breaking things or doing something harmful.

What is the "trust and verify sense"? I've been told this by managers before but I've honestly never understood it. "Trust" to me means "be willing to take action without further examination of the facts at hand". If I trust your code I wouldn't review it. But I don't trust your code, and I don't trust you to have taken all of the appropriate considerations when you were writing it. What's the difference between "tru…

I think it's differentiating total lack of trust ("all your PRs suck while mine are perfect, so I have to read yours") from universal fallibility ("your PRs are great, but all code has bugs, so we all review each other's code to catch some of them").

Re: No code reviews by default

#192

Post by codeapprove » Wed, Jan 05, 2022, 1:53 AM UTC

If you view code reviews as expressing a lack of trust, you're probably doing them wrong. Code reviews should be a conversation among people who trust each other. You're saying "hey I'd like to know your thoughts about this, and I'd also like you to learn a bit about what I am doing"

When done right code reviews spread knowledge and build trust among team members. Of course there are certainly situations which call for a push without a code review, but those should be exceptions not the rule.

While a lot of the issues around code review are cultural, I also think there's a lack of good tooling. That's why I'm building CodeApprove which makes reviewing code on GitHub faster and more enjoyable: https://codeapprove.com/

Re: No code reviews by default

#193

Post by [deleted] » Wed, Jan 05, 2022, 1:56 AM UTC

baskethead wrote: post #114

tombert wrote: post #104

I think I largely agree with this article. When I worked for Apple, we had a strict "all code needs to be reviewed" policy, which I had no problem with. Then, after being there for about 1.5 years, I make a PR, ask two different people to approve it, which they do, and it gets merged. A week later, the code is released, it looks like my code caused a fairly major bug. My manager's manager and my manager had a meeting…

I don't know what your point is. You would have gotten in trouble whether or not code reviews were present. If you're saying that code reviews should not be default, then the entire blame would rest on your shoulders anyway. Are you saying you wish you got in less trouble at Apple and the code reviewers should have gotten in more trouble for missing YOUR bug?

[deleted]

Re: No code reviews by default

#194

Post by Nursie » Wed, Jan 05, 2022, 2:03 AM UTC

erpellan wrote: post #38

tus666 wrote: post #7

He mentions "trust" a lot. Is that why we do code reviews, because we don't trust each other? I find this attitude problematic. We do code reviews because humans make mistakes. Requirements can be misinterpreted. Different work in progress can be in conflict with each other. Reviews are a good way to learn from each other and keep abreast of what work is occurring outside your own bubble. Not doing code reviews becau…

That final, 1 word sentence is one of the major problems of PRs. An engineer might spend several hours really thinking through a problem, talking with colleagues, whiteboarding options and coming up with a workable solution that addresses all the obvious issues and a bunch of non-obvious ones. Only for a drive-by take-down by someone with none of the context. It's a totally asymmetric investment of time. Even helpful…

> Only for a drive-by take-down

Code reviews aren't a "take-down", they're a process of helping each other produce better solutions and better code.

> Even helpful review comments might only take a minute to write but a day to incorporate.

Then like all pieces of work, a decision must be taken whether that day is worth it, no?

Re: No code reviews by default

#195

Post by jkingsbery » Wed, Jan 05, 2022, 2:10 AM UTC

> They can request a review if they want somebody else to take another look at their code.

This can work sometimes and with some groups of engineers, but the engineers I worked with in my career who most needed this (1) would never ask for another set of eyes and (2) were the exact type of person who think a code review equals "people don't trust me."

When I started at my current large tech company as a senior software engineer, I learned a ton from having "more junior" engineers review my code. I also learned a ton from reviewing other people's code. Even now that I have the fancy title of Principal Software Engineer, if I want to make a change to our team's code, I need two ship-its from my teammates, same as everyone else. It's not about "not being trusted," it's about earning trust of others by not pretending your poop doesn't smell.

> Code reviews aren't high priority for engineers.

Well there's your problem. On teams I've been on that do code reviews well, code reviews are considered a high priority. When code reviews are given high priority, and when engineers take the time to plan their work so that changes are small enough to understand, code reviews really don't take that long and are not a blocker to productivity.

> It's up to you and your colleagues to set the rules for your team. Don't adopt best practices in a dogmatic fashion. Rather, ask yourself if the circumstances of others apply to you.

This is true - companies shouldn't blindly copy practices from other companies. I have been a part of teams before that didn't have required code reviews and were still effective, but this required a unique set of circumstances in which all engineers were pretty senior, and so we were doing a lot of owning features end-to-end. It is fun to be part of a team like that, but it doesn't scale very well.

What this article also doesn't address is that reviewing code is its own skill, and many engineers are not naturally good at it. Just as it is bad to blindly copy other companies, it's also not great to blindly equate "we're not good at a thing" with "this thing isn't useful."

Re: No code reviews by default

#196

Post by da39a3ee » Wed, Jan 05, 2022, 2:14 AM UTC

> PRs are an afterthought and not what you want to spend your time on as an engineer.

Couldn't disagree more, and don't even understand where it's coming from. When you've dome a substantial body of work, it certainly makes sense -- and is satisfying, and enjoyable -- to explain what it is and how it works and present it to people. And it makes sense for those reviewers, given that they work on the same codebase, to learn about the code change.

I get the impression that the authors of TFA maybe just aren't writing any interesting code.

Re: No code reviews by default

#197

Post by dsjoerg » Wed, Jan 05, 2022, 2:20 AM UTC

tombert wrote: post #131

dsjoerg wrote: post #110

Earlier quoted context omitted.

> test it quickly Yes, in your scenario the lack of testing stands out as the major opportunity for improvement. If a bug is so important that it's mentioned in a performance review, then it's important enough that there should be tests that would have caught it. Automated preferably, or manual if necessary. And everyone involved with the software, from you on up and sideways, should be calling for this testing. It's…

There actually were unit tests for it, there was just an edge case I didn’t think to test that caused things to crash, which caused a cascading error.

Sure. Seems like a classic "seven whys" kind of situation... coming down hard on the person who wrote the bug is counterproductive in a way that the whole tech industry understands, except for your former bosses at Apple.

Re: No code reviews by default

#198

Post by autarch » Wed, Jan 05, 2022, 2:27 AM UTC

I think the word "trust" is covering a lot of ground in this piece and the discussion here. There are a lot of different things we may or may not trust about our colleagues and ourselves.

Here's how I think about it:

* I trust my colleagues and myself to do good work to the best of their ability. * I trust my colleagues and myself to care about the quality of their code. * I trust my colleagues and myself to follow coding standards.

* I trust my colleagues and myself to write tests and to do manual testing when needed. * I trust my colleagues and myself to pay attention to CI and code quality tool output and to act appropriately on that output. * I _do not trust_ my colleagues or myself to do perfectly bug-free work all the time. * I _do not trust_ my colleagues or myself to be able to model the entire system we are working on in their heads. * I _do not trust_ my colleagues or myself to always come up with a complete set of tests, including all corner cases and paths through the code. * And finally, I _do_ trust my colleagues and myself to be human, which means we are not perfect, we get tired, we get distracted, we forget things, and we make mistakes. But I _also_ trust my colleagues and myself to try to help each other avoid problems in the face of this imperfection, and one of the best ways to do this is with code review.

Re: No code reviews by default

#199

Post by autarch » Wed, Jan 05, 2022, 2:31 AM UTC

baskethead wrote: post #118

that_guy_iain wrote: post #86

> Pull requests don't prevent bugs. I remember once I was giving a talk on code review and I said that code review doesn't prevent bugs and people were shaking their heads. Everyone hangs on to this one thing, but if you look at the majority of pull requests that have been approved you'll see a lack of comments about pontential bugs. Also, if you do see a comment about bugs they'll often be disregarded. This drives m…

It sounds like your company has a shitty culture. I've never had issues with code reviews in the companies I worked at. One company was particularly rigorous, where we had to print it ou on paper, and a room of engineers would go over things line by line. But that also had the best engineered software I had seen in my career. Any issues of style should be in the style guide. Anything not in the style guide can be ign…

> Any issues of style should be in the style guide. Anything not in the style guide can be ignored. Or the style guide should be updated.

I'd say any issues of style should be caught by required code formatting and linting tools, which optionally run pre-commit and _must_ pass in CI. This should all be handled _before_ code review. Then review can focus on substance, not style.

Re: No code reviews by default

#200

Post by ec109685 » Wed, Jan 05, 2022, 2:39 AM UTC

I wonder how much refactoring is _not_ done because of the need for a code review. In the era before code reviews, I used to go to town on my codebase when I saw something I wanted to fix. Are folks more hesitant to make such changes now because they are afraid of the burden on the code reviewer?