Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

The Code Review Pyramid

morling.dev

Post reply on HN
Print view View original

71–80 of 117 posts

Re: The Code Review Pyramid

#71

Post by nine_zeros »

jeltz wrote: post #70
nine_zeros wrote: post #63

Earlier quoted context omitted.

I am still not convinced that readability comes before functionality. Imagine a developer building a PR for 2 weeks, then reviews back and forth for 2 more weeks. Now 4 weeks have passed and only now the reviewer reviews the functionality - only to find that the entire implementation is wrong/could be done in a better way. What a waste of 4 weeks of both the author and the reviewer! This could have been short-circuit…

Trying to understand the functionality of code which is hard to read is also a huge waste of time. You should try to keep the code easy to read even in very early drafts instead of leaving that until the end.

Hard to read code is subjective. I am guessing you are working with professionals and not high school programmers. Most of the code I see from professional programmers is passable enough for me to

1. Review functionality and then

2. Point out something about coding style.

In fact, if the functionality is good, I even approve the PR leaving a lot of code styling comments that the author can fix at their own leisure.

Re: The Code Review Pyramid

#72

Post by throwawaythekey »

cillian64 wrote: post #65
boredtofears wrote: post #59

Earlier quoted context omitted.

I don't disagree with you at all, but I'm not sure I've been a part of an organization that does it right. Beyond the stuff like style, conventions, and obvious bugs/problems, it's challenging to provide meaningful insight to someone that has many more hours of experience in a particular area of a codebase more than you do. I know I personally hold back from calling out things that I find to be subjective -- even if…

You assume here that people only work on the area of the codebase which they are most familiar with. I think a sign of a healthy team is when people are contributing to other parts of the code and not getting stuck in a rut where each area of code only has one maintainer/gatekeeper. Also, for less experienced members, reviewing the changes of more experienced people is a great way to learn if you properly sit down an…

> stuck in a rut where each area of code only has one maintainer/gatekeeper.

Why do you assume it's a rut? It might be an organizational liability but it can be a great while it's working. It's common folklore that design by committee leads to poor outcomes, or at best average outcomes completed slowly.

Why assume that we need to build code by consensus or a series of interchangeable cogs?

Re: The Code Review Pyramid

#73

Post by hsn915 »

quadrifoliate wrote: post #46
willcipriano wrote: post #18

The more experienced I get the more I want to work on teams with a higher degree of trust than ones like this. Don't get me wrong I think code review can be valuable but on high preforming teams it works best on a as needed basis. The way this has worked successfully for me in the past is as a new joiner to a team you submit for code review every time. My first couple of reviews tend to have a good bit of feedback as…

The popularity this sentiment on HN terrifies me. Please don't do this. I keep finding rubber stamp code reviews (or none at all) while diving into codebases for where bugs were introduced, and 9 times out of 10, it's someone who didn't bother to get proper reviews for their code because "it's just something slowing me down". I can assure you that you have bugs and usability issues in your code, not just "linter issu…

I have some sad news for you. Code reviews don't eliminate bugs.

Some times, they actually hinder the process of fixing bugs.

Re: The Code Review Pyramid

#74

Post by hsn915 »

willcipriano wrote: post #18

The more experienced I get the more I want to work on teams with a higher degree of trust than ones like this. Don't get me wrong I think code review can be valuable but on high preforming teams it works best on a as needed basis. The way this has worked successfully for me in the past is as a new joiner to a team you submit for code review every time. My first couple of reviews tend to have a good bit of feedback as…

This used to be common sense in the few companies I worked at around 2012.

People just pushed code. We occasionally held some review sessions to go over code and exchange comments / ideas about what might be wrong with this or that code and how to prevent this kind of mistake in the future.

But starting from around 2016, I could never ever find a company that does not have mandetory code reviews for every single commit, and this bothers me tremendously.

It's one of the reasons I don't want to work at any programming company anymore.

The really good programmers I know don't look positively at code reviews.

If you need code reviews, ask for it. If not having mandetory reviews results in a huge stream of bugs, maybe you should hire better programmers instead of cheap ones.

Re: The Code Review Pyramid

#75

Post by BrissyCoder »

https://en.wikipedia.org/wiki/Law_of_triviality

When I was on a larger team that actively participated in sprint demos was regularly a witness to this. Senior types that came along and needed to be heard - not really having a deep understanding of the application - would pick at trivial consistency issues in the UI or whatever. Such eye-rolling behaviour and hard to believe they didn't realise how transparent it was. Probably have been guilty of it myself tbh.

Re: The Code Review Pyramid

#76

Post by jamesfinlayson »

jonex wrote: post #62
boredtofears wrote: post #59

Earlier quoted context omitted.

I don't disagree with you at all, but I'm not sure I've been a part of an organization that does it right. Beyond the stuff like style, conventions, and obvious bugs/problems, it's challenging to provide meaningful insight to someone that has many more hours of experience in a particular area of a codebase more than you do. I know I personally hold back from calling out things that I find to be subjective -- even if…

"Beyond the stuff like style, conventions, and obvious bugs/problems ..." Catching the obvious issues is IMO one of the main benefits of never skipping code reviews. Even trivial changes often have those (at least when I write them) and I've most likely saved many hours of debugging by a reviewer catching stuff like that. But this does require more than simple rubber stamping. I encourage my reviewers to read my code…

This - last year I broke something because I'd changed a query in a file completely unrelated to the ticket I was working on, and the guy who did the review treated code review as a rubber-stamp - he completely ignored the fact I'd accidentally committed a change to payment processing code in a patch to tighten up company number validation.

Re: The Code Review Pyramid

#77

Post by tremon »

ryandvm wrote: post #52
quadrifoliate wrote: post #46

Earlier quoted context omitted.

The popularity this sentiment on HN terrifies me. Please don't do this. I keep finding rubber stamp code reviews (or none at all) while diving into codebases for where bugs were introduced, and 9 times out of 10, it's someone who didn't bother to get proper reviews for their code because "it's just something slowing me down". I can assure you that you have bugs and usability issues in your code, not just "linter issu…

I disagree. If you have a practice that isn't effective (e.g. code reviews that aren't preventing bugs), doubling-down is almost never the correct solution. Code reviews are like unit tests; they're helpful under the right conditions. Mostly on the tricky bits or when a dev is working in an unfamiliar area. It's a common fallacy for folks to believe that if a little of something is good, then a lot of it must be bett…

Code reviews are not meant to prevent bugs. They are for making sure an individual's effort is good enough for the team to assume further maintenance of that piece of code. That almost never means that the code must be bug-free, it means that other people than the author can follow the thought processes expressed in the code. If that leads to other people finding shallow bugs, more power to them. But that doesn't mean that bug prevention is (or should be) the primary goal of code reviews.

First and foremost, code reviews are for enforcing team standards, such as testing coverage, documentation, and adhering to scope. "Our code reviews aren't preventing bugs" is not a useful metric in that sense. If you're relying on code reviews to prevent bugs, you have other problems in your development process. If I spot a bug during code review, my first question would be "why wasn't this bug found by the automated test suite?"

Re: The Code Review Pyramid

#78

Post by majormajor »

Across 6 jobs now I've never seen code reviews focus primarily on formatting or style over functionality or future-looking concerns. How common is this, really?

At the last couple places formatting has been handled by auto-linters; before that, it was hit or miss, but never a dominant topic of argument.

Re: The Code Review Pyramid

#79

Post by sanderjd »

majormajor wrote: post #78

Across 6 jobs now I've never seen code reviews focus primarily on formatting or style over functionality or future-looking concerns. How common is this, really? At the last couple places formatting has been handled by auto-linters; before that, it was hit or miss, but never a dominant topic of argument.

Seems essentially non existent to me, outside blog posts.

Re: The Code Review Pyramid

#80

Post by icedchai »

Quarrelsome wrote: post #33
pc86 wrote: post #16

Earlier quoted context omitted.

Potentially unpopular opinion, but I would much rather get well-formatted, performant, readable code that misses the business case a bit than get a ball of spaghetti mess that nails it. Not only will it be easier to update the code in the former, but the business pressure to just deploy the latter and "fix it later" can sometimes be too high to resist. I was just looking at a "proof of concept" with a 2017 commit dat…

No, I mean like the primary function of every PR is to ensure the code does what its trying to do and doesn't (for example) fail to compile, or break the build or create an infinite loop or an out of memory exception.

If code is getting to the PR stage and doesn't compile, you have serious organizational problems.

In previous jobs, I have "reviewed" code that has clearly never been run (syntax error in a script) and it makes me question my own sanity.

Post reply on HN