3 ms·

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 thi

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 at the top (answering the ranking question with "all of them" would just be a way to avoid the issue.) In this pyramid, issues of this nature are to be found at all levels except the top one, mixed in with more-or-less subjective ones, such as "Is a new API generally useful and not overly specific?" - pointless flame wars have erupted over issues such as this (also, orthogonally, code review is late in the game to be asking this one.) For a review to be successful, its participants need to be able to restrain themselves from going down rabbit holes that could not lead to necessary rework - or a strong moderator, if the individual participants cannot act with adequate self-restraint.

th3iedkid 5y ago
Another aspect, the pyramid makes code review look like the only thing around the actual value outcome. There’s a lot of other aspects like pair-programming that eliminates most things in the pyramid, taking it out of scope from a review process
pcmoney 5y ago
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.
- mannykannot 5y ago
  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, better ways of achieving it. If your organization is such that you cannot even discuss these issues until you have working code, then you have other, bigger, problems.
  - pcmoney 5y ago
    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.
ryathal 5y ago
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.
- mannykannot 5y ago
  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.

th3iedkid 5y ago

Another aspect, the pyramid makes code review look like the only thing around the actual value outcome. There’s a lot of other aspects like pair-programming that eliminates most things in the pyramid, taking it out of scope from a review process

pcmoney 5y ago

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.

mannykannot 5y ago
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, better ways of achieving it. If your organization is such that you cannot even discuss these issues until you have working code, then you have other, bigger, problems.
- pcmoney 5y ago
  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.

ryathal 5y ago

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.

mannykannot 5y ago
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.