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

51–60 of 117 posts

Re: The Code Review Pyramid

#51

Post by greymalik »

owlbite wrote: post #40

Completely different to the order I use, which basically amounts to "how hard is it to fix this later". So most important questions are: (1) API must be as correct as possible, especially if its exposed outside of the immediate team. Changing APIs is a royal pain. (2) Documentation of what's going on, especially for non-obvious stuff. If we come back to this in 3 years time and the author has moved on, do we have any…

I think you might be misreading the pyramid? The most important things are at the bottom. It aligns exactly with what you’re saying.

Re: The Code Review Pyramid

#52

Post by ryandvm »

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 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 better. I've been at places with 100% code test coverage or requiring EVERY SINGLE PULL REQUEST to be reviewed. It's obnoxious, and it doesn't do shit to prevent bugs. Instead it causes folks to burn out on the practice and write crap unit tests or stamp "LGTM" on every PR they're forced to evaluate. People have a limited capacity for cognitive engagement and wasting that on piddly crap means they can't spend it on the important bits.

Re: The Code Review Pyramid

#53

Post by gunnarmorling »

greymalik wrote: post #51
owlbite wrote: post #40

Completely different to the order I use, which basically amounts to "how hard is it to fix this later". So most important questions are: (1) API must be as correct as possible, especially if its exposed outside of the immediate team. Changing APIs is a royal pain. (2) Documentation of what's going on, especially for non-obvious stuff. If we come back to this in 3 years time and the author has moved on, do we have any…

I think you might be misreading the pyramid? The most important things are at the bottom. It aligns exactly with what you’re saying.

Exactly that. "how hard is it to fix this later" is what drives the ordering in the pyramid.

Re: The Code Review Pyramid

#54

Post by gunnarmorling »

AngeloR wrote: post #29

I've been thinking about code reviews a lot.. and I think a big problem with existing code reviews is that for most orgs, code reviews were a way to move planning to the end of the development process instead of keeping it up-front. I think the foundation of the pyramid (API and Implementation semantics) is often discussed in code reviews when it should have almost no place. By the time you get to the code review not…

> is often discussed in code reviews when it should have almost no place. By the time you get to the code review not only should these have been ironed out and shared with the whole team.

That's a fair point. It's my background in open-source and I suppose the specific ways we work in the projects I'm involved with which made me add these things at the bottom. Oftentimes, contributors come up with PRs for new features, and there was no prior discussion about API design etc. (although we highly recommend to have such discussion before starting with the work on a large PR).

I.e. oftentimes a PR review will be the first time I see how something is implemented, how the API looks like etc. It may be different with more controlled settings in in-house development teams adhering to more formally defined processes.

Re: The Code Review Pyramid

#55

Post by makecheck »

An important one often overlooked (due to our tendency to focus on just the “diff”): what else should be changing or could be affected?

Very easy to forget to change code that isn’t highlighted in the review.

Re: The Code Review Pyramid

#56

Post by AnimalMuppet »

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…

If your code reviews aren't catching bugs, the correct answer is to fix the code reviews, not to stop doing them.

I mean, it's possible that code reviews are worthless. But enough people have found value in them, across enough organizations, enough code styles, and enough decades, that it seems really hard to conclude that. It seems more probable that the team that finds them useless is doing them wrong. (Yeah, I know, No True Scotsman. Still, the first response should be "maybe our team is doing them wrong, and can we fix that?", rather than "let's stop doing them".)

Re: The Code Review Pyramid

#57

Post by [deleted] »

AnimalMuppet wrote: post #56
ryandvm wrote: post #52

Earlier quoted context omitted.

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…

If your code reviews aren't catching bugs, the correct answer is to fix the code reviews , not to stop doing them. I mean, it's possible that code reviews are worthless. But enough people have found value in them, across enough organizations, enough code styles, and enough decades, that it seems really hard to conclude that. It seems more probable that the team that finds them useless is doing them wrong. (Yeah, I kn…

[deleted]

Re: The Code Review Pyramid

#58

Post by bena »

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…

I don't think this is doubling down so much as singling down.

The problem with unit tests and code reviews is that they ultimately aren't sexy. There's no way to do them that destroys the integrity of the system. Especially when a large percentage of the code written is passable. Yes, reviewing and testing code that works and is bug free is a bit of a waste of time. But you won't know what code isn't working and bug free unless you test and review.

And then there's the fact that most of what we do is not that important. If my code is buggy or inefficient or a third thing, the worst thing that happens is a few people frown while I get it sorted. If you knew that if your code had a bug in it, it would definitely kill 5 - 10 people every time it is run, you'd treat review and testing with the utmost importance because the cost of committing buggy code is way too high.

Testing and review becomes exactly as important as your product.

Re: The Code Review Pyramid

#59

Post by boredtofears »

AnimalMuppet wrote: post #56
ryandvm wrote: post #52

Earlier quoted context omitted.

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…

If your code reviews aren't catching bugs, the correct answer is to fix the code reviews , not to stop doing them. I mean, it's possible that code reviews are worthless. But enough people have found value in them, across enough organizations, enough code styles, and enough decades, that it seems really hard to conclude that. It seems more probable that the team that finds them useless is doing them wrong. (Yeah, I kn…

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 I think I would have done it differently -- simply because I'm often unsure about the things I don't know.

Plus as a peer reviewer, you're often in the same part of the sprint cycle as they are - meaning your mind is already juggling a lot of things about your own code. It's tough to "come up for air" and then immediately dive into someone else's code changes with enough mental capacity to provide a truly useful review.

It's easy to slip into the rubber stamping mentality when you generally trust the ability of a particular contributor.

Re: The Code Review Pyramid

#60

Post by twblalock »

tuyiown wrote: post #11
simonbarker87 wrote: post #7

Earlier quoted context omitted.

I’m at the point now where I don’t want to think about layout and formatting of my code, just let a formatter do it all for me on save please

But, code formatting is not a solved problem, right ? You either have to fiddle until the formater outputs proper layout, or let it rest as is and never learn how to format your code, letting the code base leaking tons of probable less than ideal layout.

The proper layout is whatever the formatter outputs. That's the point. Everyone uses the formatter and accepts whatever it does, and nobody needs to think about it anymore.
Post reply on HN