[HN Gopher] Maslow's Pyramid of Code Review (2015)
___________________________________________________________________
Maslow's Pyramid of Code Review (2015)
Author : nkjoep
Score : 88 points
Date : 2021-04-06 13:35 UTC (9 hours ago)
(HTM) web link (www.dein.fr)
(TXT) w3m dump (www.dein.fr)
| agentultra wrote:
| _Correct with respect to what?_
|
| The answer to, _does your code do what you expect it to?_ is
| pernicious and difficult to answer if you 're not writing
| specifications, at a certain scope of complexity.
| hyperpallium2 wrote:
| No code gets past the "correct" layer, because all software has
| bugs. Maybe, "correct enough"?
|
| How can you review it if it's not "readable"?
| yellowyacht wrote:
| Instead of "correct", the layer should be "Performs" or "Works
| for me". Then maybe the next layer should be "Works for others
| without crashing"
| throwawayboise wrote:
| If nobody in the code review can make sense of the code because
| of readability issues, that's the result of the review. Send it
| back for rewrite.
| legerdemain wrote:
| Almost all code review fails right out the gate at hurdle #1, "is
| the code correct." When you're looking at an interleaved diff on
| Github, you have little ability to see whether the code is
| broken, especially when the diffing heuristic awkwardly mashes
| together unrelated functions and declarations.
|
| Over a decade of experience with merging enthusiastically
| approved, hopelessly broken code, I've come to believe that code
| review won't, and often can't, and isn't really designed to find
| bugs, only nits. And this pertains to both local bugs (this
| function is wrong) and systemic bugs (this function makes false
| assumptions about surrounding code).
|
| What visual code review can uncover:
|
| - style nits
|
| - a lack of unit tests
|
| - feelings of dread when the diff touches a critical part of the
| codebase
|
| What actually uncovers bugs:
|
| - thorough tests
|
| - in-person code walkthroughs
|
| - design documents
| pydave wrote:
| Why do you think in-person code walkthroughs are effective but
| tool-mediated reviews are not?
|
| Most of my code review comments are along the lines of "what
| does this do" -- the primary question I'm asking in an in-
| person review. My goal for a review is to pre-emptively answer
| the questions I'd ask if I had to fix bugs in the code. This
| frequently exposes bugs because it requires the author to
| consider their code from a different perspective.
|
| Maybe it's the code under review: I work in games and was
| reviewing a lot of code from juniors, so I'd often be asking
| questions intended to get them thinking about their technical
| design and we had very little rigor.
|
| In my eyes, the biggest upside of in-person is how it reduces
| mental drain from back-and-forth. A conversation is comfortable
| instead of the digital equivalent of repeated red ink all over
| your work.
|
| > Over a decade of experience with merging enthusiastically
| approved, hopelessly broken code
|
| Or maybe you're commenting on the code reviewers you've worked
| with rather than your own experience as a reviewer?
| legerdemain wrote:
| When a sample is large enough, it becomes representative of
| the whole and you can start drawing inferences from it.
| rmk wrote:
| This is very much on the money. Code reviews often fall into
| the trap of people discussing and dissecting things that they
| are comfortable with, while ignoring the difficult aspects. For
| instance, reviewers often dissect the elegance and variable
| naming to death while totally ignoring how the change has been
| tested (unit tests can show some issues, but if the tests
| themselves are not covering the core aspects of the problem
| being solved, then they are not hugely useful, particularly in
| duck-typed languages like python).
|
| Also, changesets that are large but can't be broken down into
| smaller ones because of practical constraints cause 'code
| review fatigue': people will review the first few commits/files
| with gusto and lose steam by the time they are reading the
| critical code which may arrive in a later changeset or file!)
| datagram wrote:
| > I've come to believe that code review won't, and often can't,
| and isn't really designed to find bugs
|
| > What actually uncovers bugs: > - thorough tests
|
| How do you know that the tests don't have bugs?
|
| > - in-person code walkthroughs
|
| How is this different from reading the code in a code review,
| or checking it out on your own machine and reading it there?
| imoverclocked wrote:
| Thorough tests can also be in the form of good static analysis
| tools. eg: in Java codebases, errorprone+NullAway can block
| some really obvious issues leading to NPEs.
|
| In my experience, the issue is often getting the codebase to a
| point where the tools are useful. The simpler (more nit-picky)
| rules in such system are usually easier to enforce while the
| more useful ones build on the simpler rules and flag design
| issues.
| legerdemain wrote:
| NPEs are a red herring. My immediate read on people who
| complain about NPEs is that they haven't done any enterprise
| Java development.
|
| Let's say you have the potential for some reference to be
| null. And you cleverly wrap it in an Optional or add some
| @NotNull annotation to it. Great, now how do you handle the
| null? Maybe it's some query result set that unexpectedly
| contains zero results or an empty list of live service
| shards. What do you do to recover? Print an error to the
| application log and continue running in a corrupted state? Is
| that really an improvement?
|
| Bugs don't get fixed by adding Optional everywhere or "null
| coalescing" or whatever, they get fixed by actually
| understanding how the bug arises during operation. It's a bug
| in the mental model of how the application works, not at the
| syntactic level of null references.
| fiddlerwoaroof wrote:
| I agree, but I think the benefit of things like Optional is
| a bit different: it's not really about eliminating NPE-type
| bugs, but limiting the scope in which they can occur. If
| you use them consistently, you only have to check the
| behavior of places where the optional is unwrapped, and you
| push those places upwards towards the entry-point rather
| than weaving null-checks into your business logic.
|
| All that being said, the complaints about NPEs have always
| felt to me like focus on symptoms (null references) rather
| than the underlying causes (badly-designed application
| code)
| eikenberry wrote:
| I always thought the primary value of code reviews was
| informational, not bug finding. That is by reviewing code you
| have a better understanding of the changes that went into it,
| give you a chance to ask why something was done and will
| generally lead to better future interactions with it.
| ufmace wrote:
| I'm currently at a place with a culture of thorough code
| reviews. IMO, it entirely depends on the people and the
| culture. It's on the reviewer to actually thoroughly review the
| code and ensure it does what it's supposed to do, including
| local checkout and testing if needed. And of course on the
| management structure to provide senior enough reviewers with
| enough time to do that. It certainly is easier and less time-
| consuming to just skim it and comment on some style nitpicks
| and unit tests, but there's no reason it has to be.
|
| The flip side of course is that the original coder does still
| need to be reasonably competent. Relying on even exhaustively
| thorough and repeated code reviews to turn basically garbage
| into reasonable quality code is a bad bet.
| legerdemain wrote:
| At the moderately large, very successful companies where I've
| worked, code is mostly disposable. It's written and read, on
| average, once. Code craftsmanship doesn't make any sense in
| these environments. We treat code review as a formality.
| Like, we just kind of skim the diff and click the approve
| button, but most of the time none of us really have the time
| to understand the code on a deep level.
|
| Correctness is one competent dev banging on the code until it
| works, and then getting git-blames and bug reports sent
| directly to them when it stops working. We don't blame the
| reviewer for our own bugs. If the dev leaves, it's faster for
| the next dev to paper over the bug or rewrite the feature
| than it is to do a close reading of the existing codebase.
| That's what "rapid iteration" and "agile development" is all
| about.
| BobbyJo wrote:
| I am vehemently against making reviewers checkout and test
| code. You should have tests that demonstrate correctness with
| very high confidence, and a release process that can find
| bugs early.
|
| Expecting reviewers to checkout and run/test things is such a
| time sink, and over the course of a year of doing it at a new
| work place, I've never seen it be the thing that caught a
| bug. If anything I feel like it caused more bugs because
| people end up getting laxed with tests.
| dragonwriter wrote:
| > Over a decade of experience with merging enthusiastically
| approved, hopelessly broken code, I've come to believe that
| code review won't, and often can't, and isn't really designed
| to find bugs, only nits.
|
| Nits are, both literally and figuratively, bugs that just
| haven't hatched yet.
|
| (Relevantly, the things you've pointed out that code review can
| identify are all issues that impact maintenance of the code,
| including the ability to find bugs and avoid creating them.)
| bcanzanella wrote:
| Yeah context plays a big part here. We wanted to make the
| experience of looking at "interleaved diff[s] on Github" a lot
| better. So we made https://www.codestream.com/ which lets
| reviewers look at pull requests (github) and soon merge
| requests (gitlab) right in their editor, leveraging the entire
| context of the file and all the things they love about their
| IDEs.
| hackeraccount wrote:
| I'd call #1 minimum viable product and I'd switch 4 and 5 but
| it's interesting as I thought experiment no matter how much you
| agree or disagree with it.
| 1-6 wrote:
| DOTADIW. It's easier to climb that triangle when the project is
| small.
| auslegung wrote:
| > Code should be 1) correct 2) secure 3) readable 4) elegant 5)
| altruist
|
| This is a great companion to the adage "Make it work, make it
| right, make it fast." I would map "Make it work" to #1 and "make
| it right" to #2 - #4.
|
| I don't quite see a clear mapping with "make it fast" to any
| levels of the pyramid, nor #5 with any statement in the adage,
| though that doesn't mean there are any problems with either.
| nvader wrote:
| I am partial to the formulation, "Make it correct, make it
| clear, make it concise, make it fast. In that order." - Wes
| Dyer
| auslegung wrote:
| When I think of "make it work", I think, "do any hackey thing
| you need to to get it to work". Then in the next step, "make
| it right", I refactor and clean up. It's much like writing,
| where the advice is to write without censoring, then edit
| later. I think the idea is it's always easier to edit
| something that exists, rather than write something correct
| the first time, or at least try to make it correct the first
| time.
|
| That being said, when I read your quote where it first says
| "make it correct", I would skip the first step I mentioned
| above, which is a very important step. What are your
| thoughts?
| bironran wrote:
| > Correct: ... Is it performant enough for this use case?
|
| I completely agree with that definition of correct. Code should
| be performant _enough_ for the use case. It shouldn't strive
| for the unachievable "infinite performance" or "endless
| scalability". It should do well now and in the next performance
| / growth cycle (usually measured in months to a low number of
| years unless you're in hyper growth).
|
| However I disagree with "Secure" being on a different level
| than "Correct". Or rather, the "release" line passes over the
| secure. I may not be happy with the structure of the code, but
| I will never knowingly release insecure code or allow such code
| to be released if I can help it. The impact of security issues
| to the bottom line is usually far greater (in both immediate
| and future terms) than the impact of any non-data-loss inducing
| functional issue.
|
| -- addendum ---
|
| Also I miss "being evolution ready" (future proofing).
| Sometimes you give up on some of the other aspects to make sure
| your code (and the data it governs) can be evolved should the
| need arise.
| barbazoo wrote:
| Maybe "make it fast" could fall under #4 Make it elegant, as an
| elegant way to solve a problem could be a way that solves it
| more efficiently (faster) than others.
| snidane wrote:
| It very much depends on which context the pyramid is applied. I
| see two types of code review typically done
|
| 1. mandatory code review as part of automated workflow. Eg.
| ticket in jira won't be closed or git branch won't be merged to
| master before someone code reviews. The reviewer does the code
| review asynchronously from the code author on his own his
| schedule. Because of time pressure and pressure of being accused
| of blocking the team's progress, this kind of code review tends
| to do the bare minimum, focusing only the correctness part of the
| review maslow pyramid, if at all. Most often the reviewer just
| points out some cosmetics so as to appear that he actually looked
| at it.
|
| 2. two people sit side by side (or virtually over zoom) and walk
| through the code together while having a synchronous
| conversation, asking questions when not understanding something.
| Higher levels of the code review pyramid can be accessed using
| this code review style as well as achieving higher level
| understanding of author's thinking process and proliferation of
| good practices.
|
| I've seen code review type #1 pushed in manager dominated
| environments, ending up as a formality and a tool of blame. I've
| experienced code review type #2 among very senior engineers,
| often organized informally with no managers involved.
| ac42 wrote:
| Except for the "Elegant" thing, which I would drop entirely, I
| think the pyramid either has it backwards or should be read from
| top to bottom for priority.
|
| Usually nobody cares for unreliable code because the "correct"
| bit can't be figured in most cases. And people messing with the
| code base kind of just failed the fizzbuzz test.
| ac42 wrote:
| s/unreliable/unreadable/
|
| (autocorrection got me wrong)
| heisenbit wrote:
| > As in Maslow's pyramid, each layer requires the previous one.
| It is useless for code that is charging the wrong customer to be
| readable.
|
| Now would you rather have code that is having a bug but is
| readable or having code that is incomprehensible but afaik. was
| giving the right answer when last run? The latter is
| unfortunately just literally a bit away from being wrong and
| incomprehensible and a total write-off.
| the_af wrote:
| > _Now would you rather have code that is having a bug but is
| readable or having code that is incomprehensible but afaik_
|
| I don't know the answer myself, but the "Practical C
| Programming" book argues that clear code that doesn't work is
| preferable to unclear code which works but is hard to
| understand (because of its messiness). This is because you
| understand what's wrong about the clear but non-working code
| and therefore you can fix it.
|
| I don't know if this maxim works for every situation, but as a
| general rule it seems ok.
| rmk wrote:
| The trouble with the 'pyramid' is it doesn't take into account
| interdependencies. For example, it is very important for code to
| be clean and understandable in order to ascertain correctness (to
| the extent possible via code inspection). And the whole notion of
| 'value', which is subjective, typically dominates code reviews.
| The 'value' question is basically whether the change must be
| rethought from the ground up to pass muster as a net value add to
| the customer. For example, if the code is extremely dense and
| complicated but essentially correct, then the value may be very
| high for an earlier stage product, versus for a larger,
| established product where long-term maintainability costs can
| overshadow the value derived from the change.
| notacoward wrote:
| The most important part is easy to miss in the intro.
|
| > each layer requires the previous one
|
| In my experience (several projects and companies) most code
| reviewers pay scant attention to correctness, ignore security
| altogether, and spend _all_ of their time on readability
| /elegance. Time after time after time, I've seen several people
| have lengthy exchanges about these "higher level" concerns during
| code review, the code gets merged, and then _multiple_ bugs end
| up tracing back to fairly basic logic errors that they all
| overlooked.
|
| Why? Because it's _easier_ for people to talk about the
| superficial structure of the code. It 's almost easy to argue
| about various micro-optimizations (which usually don't even
| matter). Making sure that each path leads to a reasonable result
| and/or gets tested is _much_ harder. Identifying the paths /cases
| that are missing altogether is harder still, as it requires
| context about the rest of the system as well as the bits under
| review.
|
| Most code reviews are looking for the keys under the lamp post.
| IMO the only way to fix that is to add some accountability, but
| that usually gets mistaken for adding hierarchy and process so
| engineers (particularly the "move fast" variety) strongly resist
| it.
| 6gvONxR4sf7o wrote:
| I think all the time on readability is worthwhile. It sucks to
| go into an unfamiliar codebase, and that includes your own code
| you haven't touched in six months. Making maintenance and
| usability easy comes first. Once the code is understandable,
| you can start working on correctness. If you can't reason about
| it easily, you can't get trustworthy correctness. Just like a
| pyramid implies with readability/understandability at the
| bottom.
|
| That's different from style nits, and also why automated
| stylers and linters are so worthwhile, so your tools handle the
| stuff you don't want people wasting their time on.
|
| Unfortunately, most reviews stop there, but at least it's the
| right order in a Maslow-esque pyramid.
| firebaze wrote:
| Sorry, but I can't confirm your experience at all. Maybe I'm
| just lucky with my past jobs, but, aside from very junior
| reviewers, I never observed the ,,focus on style" effect.
|
| Sometimes it comes as an addendum (like ,,and please fix the
| order of imports"), but it is very rarely the core message.
| why_Mr_Anderson wrote:
| I'm pretty sure what my response would be if someone rejected
| my commit with message 'fix the order of imports'...
| rzzzt wrote:
| Using the menu item for automatically organizing imports,
| or the equivalent keyboard shortcut, surely.
| BlargMcLarg wrote:
| It depends on the individuals and the places. I can attest to
| GP's situation being predominantly prevalent in my life.
| Stylistic issues can be as simple as passing different data
| on the backend rather than cleaning them up on the frontend,
| without any mention why one is preferred over the other. I've
| had cases where people wanted me to make a new variable Y in
| a model to pass 1 - X, where X was another float variable
| being passed at the same time (so we passed both Y=1-X and X
| without intuitive or clear reason). Or saying "this needs a
| comment" (notice the tone too, it's not "can you add a
| comment here", or better, "this reads kind of iffy, can you
| improve it somehow?"). Things a linter or guidelines won't
| pick up.
| notacoward wrote:
| The fact that you mention "style" when I never did suggests
| that we're working from different definitions. I had
| originally typed a sentence about _not_ meaning stuff like
| variable names or indentation or (as you mention) import
| order, but I deleted it for the sake of brevity. What I mean
| by focus on readability /elegance is stuff like moving code
| into separate functions, using structs/enums/optionals
| instead of long argument lists and opaque booleans, etc. It's
| still valuable, but it's not correctness.
|
| In the process of addressing this kind of feedback it's
| pretty common for very local correctness issues to be
| discovered. Oops, used greater-than instead of greater-or-
| equal. Oops, didn't check for the right error code (or any).
| Oops, now we need to free this object at a
| different/additional point. But these discoveries often seem
| accidental. A lot of low-level errors still slip through, and
| higher-level logic errors almost never get caught. A
| perfectly "correct" piece of code for handling disk errors is
| useless if it's not in the path we reach when a disk error
| actually occurs. A perfectly "correct" message handler can
| still invalidate the distributed algorithm of which it's only
| one part. And so on.
|
| In 30 years, across a dozen companies and half a dozen
| specialties, I've found that maybe one in ten engineers at
| "senior" level or above will _systematically_ review others '
| code for actual correctness - identifying missing cases,
| making sure the expectations on one end of an API line up
| with the expectations on the other, finding possible race
| conditions, etc. They're like gold when you find them. The
| other nine out of ten typically spend their review time
| around the periphery instead of addressing the core issue of
| whether the code _does what it 's supposed to_.
| kerblang wrote:
| I suppose what you want is possible provided the reviewer gets
| as deeply invested in the process as the author. Most of the
| time we're lucky if we even know enough about the project to
| understand what the author is trying to do - in fact I noticed
| the article claimed that the requirements should be self-
| evident in the programming, which I think is ridiculous.
| yoz-y wrote:
| Which is why I am partial to mandatory coding rules, preferably
| enforced by tooling. Once the conversation about style is
| eliminated altogether, people can focus on the important
| things.
|
| For me the important thing about code review is bringing in
| people who may be more familiar with other components with
| which the code interacts.
___________________________________________________________________
(page generated 2021-04-06 23:01 UTC)