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

111–117 of 117 posts

Re: The Code Review Pyramid

#111

Post by Quarrelsome »

icedchai wrote: post #80
Quarrelsome wrote: post #33

Earlier quoted context omitted.

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.

Or breaks the build in an otherwise non-obvious fashion or fucks the installer?

Re: The Code Review Pyramid

#112

Post by [deleted] »

digisign wrote: post #37
Quarrelsome wrote: post #33

Earlier quoted context omitted.

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.

Arguably too fundamental to mention, at least in one slide. Perhaps covered during the developer-testing period, before peer review.

[deleted]

Re: The Code Review Pyramid

#113

Post by icedchai »

Quarrelsome wrote: post #111
icedchai wrote: post #80

Earlier quoted context omitted.

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.

Or breaks the build in an otherwise non-obvious fashion or fucks the installer?

One of the first things I check on a PR is if it passed CI cleanly. If the build has failed (failing tests, build problems, whatever), I get annoyed I was even asked to look at it.

Re: The Code Review Pyramid

#114

Post by gizmogwai »

librish wrote: post #61
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…

Velocity vs correctness is a trade-off. Stringent review, testing, and ownership requirements can easily slow down developers by an order of magnitude. Which is often the right call, but not always.

Velocity is only measured by the value you introduce. Incorrect code decreases value.

Re: The Code Review Pyramid

#115

Post by mannykannot »

ryathal wrote: post #105
mannykannot wrote: post #6

While I recognize the problem as real and significant, and I think a hierarchy of concerns is a valid way to mitigate it, I feel there are a few problems in this particular pyramid. Of all the issues that might be raised in a review, which are the most important to fix? I find it difficult to imagine a ranking in which incorrect functioning (including security vulnerabilities and truly inadequate performance) are not…

I think you are getting too into the weeds with these complaints. This hierarchy is pretty good. Performance and security are important, but they aren't always a full stop, where a change the breaks the API should always be a full stop. Each higher level is less likely to be a hard stop, and the border between levels is necessarily somewhat grey.

I'm not so concerned with the broad levels, but the specific items within them. So, for example, "is a new API generally useful and not overly specific?" is way too broad a question to elicit only breaking errors in the API, and is an invitation to exactly the sort of loss of focus that the author rightly deprecates.

WRT performance, I specifically wrote "truly inadequate" in an attempt to distinguish between breaking and merely undesirably bad performance, but that is a somewhat vague line - unavoidably, I think.

Off the top of my head, the only sort of security 'problem' that can safely be put aside would be one that cannot lead to serious or irreversible harm, or requires the organization to have already been terminally compromised.

That I have to make these distinctions is, in fact, the basis of my concerns. They follow from two premises: firstly, the purpose of an inspection is to find problems that need to be fixed, and secondly, whether a problem falls into that category is orthogonal to the questions of what functional, structural or operational categories it falls in.

Re: The Code Review Pyramid

#116

Post by RussianCow »

tuyiown wrote: post #102
RussianCow wrote: post #30

Earlier quoted context omitted.

I configure Prettier once on my JS codebases and then never touch it again. Seems about as "solved" as is reasonably possible.

http://journal.stuffwithstuff.com/2015/09/08/the-hardest-pro...

I'm not sure how that's relevant. The fact that it's difficult doesn't make it unsolved.

Re: The Code Review Pyramid

#117

Post by pcmoney »

mannykannot wrote: post #50
pcmoney wrote: post #32

Earlier quoted context omitted.

If your team does not share a subjective understanding of what makes their product/API useful none of this matters. Focusing on objective technicalities is the least useful aspect of code review. If you have someone who constantly is like "Well that's subjective" and derails conversation that way you need to coach them or move them off the team.

Are you saying that if you enter a code review without having a shared subjective understanding of what makes your product/API useful then the review is not likely to be successful, or that code review is the place to form such a shared subjective understanding? I can agree to the first interpretation, but about the latter I would say that code review is quite a bit later than optimal, and that there are other, bette…

The former, ideally the subjective understanding should be shared before the story/ticket is written and definitely before it is started. Code review is way too late.
Post reply on HN