TE
科技回声
首页24小时热榜最新最佳问答展示工作
GitHubTwitter
首页

科技回声

基于 Next.js 构建的科技新闻平台,提供全球科技新闻和讨论内容。

GitHubTwitter

首页

首页最新最佳问答展示工作

资源链接

HackerNews API原版 HackerNewsNext.js

© 2025 科技回声. 版权所有。

The Code Review Pyramid (2022)

214 点作者 rainhacker将近 2 年前

26 条评论

nlnn将近 2 年前
One goal that I think often gets missed out of code review guides is education.<p>To me, code review is not just there to produce better software, but also better developers.<p>It&#x27;s a great place to talk about tradeoffs, alternative approaches, reasoning behind changes, etc.<p>Maybe less useful in huge projects with many contractors you&#x27;re never going to work with again, but I&#x27;ve seen the mentoring aspect have great benefits in small teams.
评论 #36653448 未加载
评论 #36654624 未加载
评论 #36653979 未加载
评论 #36661830 未加载
评论 #36653255 未加载
gilbetron将近 2 年前
After 30+ years, I kind of reverse the pyramid, honestly. I look for readable, maintainable code, and try to make sure the writer is doing appropriate testing. But whether or not it works and is correct, that&#x27;s not my job, that&#x27;s the developer&#x27;s job. I know developers that will spend days on a code review, which is a horrible waste of time. CI&#x2F;CD and your deployment followup structure (canaries, monitors, gates, etc) should be catching any significant issues the vast majority of the time, and if it isn&#x27;t, you need to spend time there.<p>My main concern as a reviewer is to make sure future people can understand the code when the inevitable modification comes along.<p>Things are different if the writer and reviewer are in a mentorship relationship, but even still, if you are only engaging with the code as a mentor when the PR hits, you&#x27;re messing up the mentorship!
评论 #36657708 未加载
Pannoniae将近 2 年前
From what I&#x27;ve seen so far, people <i>love</i> debating code style.<p>The main problem is that those programmers mostly care about superficial syntax and don&#x27;t actually care about code readability or design. They just go for the lowest hanging fruit and criticise variable naming or whitespace because it is easy; they don&#x27;t talk about the object graph or the event lifecycle of the program, because that&#x27;s hard.<p>A very common thing I see nowadays is a forced adoption of gofmt&#x2F;black&#x2F;whatever, hoping it would solve the obsession with the formatting. However, this just locks in a (often substandard) coding style and removes <i>any</i> kind of personality from the code. This is good if you are a manager trying to treat your employees as fungible units of work, but is bad for actually maintaining a codebase. Also, it doesn&#x27;t stop obsession with style, it just forces a specific style on everyone which most of the time, everyone slightly hates.<p>This is a really good article which summarises the problem way better than me. <a href="https:&#x2F;&#x2F;luminousmen.com&#x2F;post&#x2F;my-unpopular-opinion-about-black-code-formatter" rel="nofollow noreferrer">https:&#x2F;&#x2F;luminousmen.com&#x2F;post&#x2F;my-unpopular-opinion-about-blac...</a>
评论 #36650380 未加载
评论 #36650360 未加载
评论 #36651982 未加载
评论 #36652863 未加载
评论 #36652976 未加载
评论 #36655222 未加载
评论 #36651202 未加载
评论 #36652562 未加载
评论 #36650444 未加载
评论 #36654662 未加载
评论 #36654364 未加载
iovrthoughtthis将近 2 年前
I find this too concrete to be useful. I have a more principals &#x2F; tradeoffs approach to code review:<p>Areas:<p><pre><code> - Readability - how understandable the code is - Maintainability - how the code enables the project to evolve - Risk - security, regulatory and other vulnerabilities - Correctness - whether the code does what you intended - Robustness - how well the code handles unintended circumstances - Performance - how resource efficient the code is </code></pre> All of the above areas need to be considered within the context of the project and individual team members.<p>## Readability<p>Code is readable if it meets your expectations and surprises you only when there&#x27;s something you don&#x27;t yet know about the technology. We expect code to look like the code around it. To follow most, if not all of the technologies idioms. To use clear and informative names. To explain why deviance exists. To be as abstract as the concepts being abstracted are crystallised.<p>## Maintainability<p>Maintainability is the code&#x27;s relationship with the wider team and time. You should be able to learn all you need to know to change, test and deploy code. You should be confident that any changes you make will not cause unintended changes elsewhere in the code. To make a change, you should have to touch as little code as the concepts being changed are crystallised. You should be able to debug an issue as easily as, the system the issue is in, is old. You should be able to track down and alter the changes that introduce a bug quickly. You should be able to learn why a change exists.<p>## Risk<p>Code is low risk if it accounts for, and where possible, mitigates the various risks to a project.<p>## Correctness<p>Correct code does as you expect and surprises you only when there something about the system you didn&#x27;t yet understand. Validating correct code is as automated as the validations are frequent. Correct code is only as complicated as the concepts it models. Correct code is only as abstract as the concepts it models are crystallised.<p>## Robustness<p>Robust code handles unexpected circumstances safely, quickly and predictably. Validating robust code is as automated as the validations are frequent and as comprehensive as the failures are risky.<p>## Performance<p>Performant code is as resource efficient as the resources are expensive, but also as efficient as the development costs are cheap.
评论 #36653735 未加载
RangerScience将近 2 年前
Ehhhhh yeah kinda? Yes, you should spend most of your time on &quot;API Semantics&quot; (what does it look like using this code?), and you should spend a lot less time on &quot;how are the tests?&quot;<p><i>but</i>, for example,<p>Writing good tests massively contributes to good implementation details and API semantics; and test code is <i>also</i> code that needs to be reviewed under more-or-less the same criteria as the rest.<p>Also - documentation (or, legibility, if you&#x27;re onboard with self-documenting code) can be more important than either implementation details, and even API semantics, as it can define whether the entire work is useable or maintainable.<p>I might say instead:<p>- What will it be like <i>reviewing</i> this code? (style, test coverage, etc - do I have to worry about spotting typos, and other stupid stuff?)<p>- What will be like <i>debugging</i> this code? (patterns, logging, etc - which might handled by a framework)<p>- What will it be like <i>altering</i> this code? (documentation&#x2F;legibility, implementation details, etc - when the business needs change or grow)<p>- What will it be like <i>using</i> this code? (API semantics, API docs - when I go to build something on top of this)<p>And then yeah; the top <i>should be</i> entirely automated, and you should (generally) spend most of your time on the bottom.
评论 #36649860 未加载
barbariangrunge将近 2 年前
Is this just a way of saying, “stop wasting all your review time on the style guide and look at the system design”?<p>Although, the style guide should just be followed and fixed before you get to code review phase. That’s just a matter of professionalism
评论 #36651761 未加载
dang将近 2 年前
Discussed at the time:<p><i>The Code Review Pyramid</i> - <a href="https:&#x2F;&#x2F;news.ycombinator.com&#x2F;item?id=30757206">https:&#x2F;&#x2F;news.ycombinator.com&#x2F;item?id=30757206</a> - March 2022 (110 comments)<p><i>The Code Review Pyramid</i> - <a href="https:&#x2F;&#x2F;news.ycombinator.com&#x2F;item?id=30674159">https:&#x2F;&#x2F;news.ycombinator.com&#x2F;item?id=30674159</a> - March 2022 (4 comments)
alexjurkiewicz将近 2 年前
If you are reviewing implementation semantics after someone has already coded a feature, I&#x27;d say it&#x27;s too late. You might catch issues, but the loss of goodwill and wasted productivity will make everyone hate code review.<p>I think this pyramid is accurate but better framed as a &quot;review pyramid&quot;. Catch semantic issues in an earlier phase (informal discussion or rfc-style proposal document).
评论 #36652022 未加载
ozim将近 2 年前
Code style and “are tests passing” should not even be there.<p>Code style should be automated same as running tests when someone creates PR.
评论 #36652872 未加载
评论 #36652893 未加载
评论 #36653940 未加载
rotifer将近 2 年前
I write and review medical software. When I&#x27;m reviewing code the most important question is, &quot;Is it correct?&quot; If it&#x27;s not, then the review simply isn&#x27;t approved (obviously).<p>Anything that makes it harder to determine correctness is a strike against the code. This can include anything from high level organization, to comments, to the naming of variables, to the use of whitespace. For example, with respect to variable names, if you&#x27;re implementing something that has to conform to the DICOM standard, then you should <i>consider</i> following their nomenclature, even if it may not be what you&#x27;d normally use in other contexts.<p>Aside from correctness issues, I&#x27;ll make anything from &quot;strong recommendations&quot; to &quot;mild suggestions&quot;. (In theory, any of these could cause me not to approve a change, but the closer we get to &quot;mild suggestion&quot; the less likely that is.) Examples might include:<p>- You&#x27;re trying to do something with X. I&#x27;m a domain expert in X and while your code is technically correct it&#x27;s not idiomatic. I suggest doing it this other way instead.<p>- You added a public function to a library, but it will fail if the String argument isn&#x27;t a valid Photometric Interpretation. Yes, I see that you only call it with valid values, but since it&#x27;s public anybody can now call it, and they may not be so diligent. How about making it an Enum?<p>- You have a loop that, superficially, looks like it&#x27;s calculating Bar, but upon closer examination it&#x27;s (correctly) calculating Bar&#x27;. How about adding a comment stating this, so that no one is tempted to (incorrectly) &quot;fix&quot; it?<p>- You&#x27;ve implemented a function that does Y. All our code links with the Foo library, which happens to have a function that does Y. How about using that instead?<p>- Your log message makes sense if you&#x27;re reading the surrounding code, but someone from support seeing it in the log file won&#x27;t know what to make of it. How about including this contextual information in it?<p>- In the loop that you added, your indentation is inconsistent with the rest of the function. How about making it the same?<p>- Why do you have four blanks lines in the middle of your function?<p>In my own code I&#x27;m pretty anal about formatting, comments, log messages, use of whitespace, etc. I&#x27;m definitely less harsh when reviewing other people&#x27;s code, but I have to admit that when I see sloppy formatting, grammatical errors or gratuitous use of whitespace, I&#x27;m on the alert for sloppiness in other areas, such as design and implementation.
LadyCailin将近 2 年前
Tests should mostly be integration, not unit. Unit tests tie you down to the specific implementation, which means changing the implementation becomes much more onerous. Integration tests have much more value, give you higher confidence that the code does what it claims, and is easier to code review. <a href="https:&#x2F;&#x2F;kentcdodds.com&#x2F;blog&#x2F;write-tests" rel="nofollow noreferrer">https:&#x2F;&#x2F;kentcdodds.com&#x2F;blog&#x2F;write-tests</a>
评论 #36650107 未加载
评论 #36650083 未加载
评论 #36650249 未加载
评论 #36650195 未加载
评论 #36650751 未加载
holaworld12将近 2 年前
I recently received feedback from my manager to take time out to review teammate&#x27;s PR and design docs. For context, I&#x27;ve always been under-confident and scared to review and feel like I fail to add quality feedback to other people&#x27;s code and design. I&#x27;ll use this article as a guideline for my next PR review! Has anyone else been in the same situation as I? How did you overcome it?
评论 #36650614 未加载
评论 #36652793 未加载
blt将近 2 年前
handwriting font replaced by sans-serif: <a href="https:&#x2F;&#x2F;svgur.com&#x2F;s&#x2F;v6n" rel="nofollow noreferrer">https:&#x2F;&#x2F;svgur.com&#x2F;s&#x2F;v6n</a>
alphazard将近 2 年前
Code review is just the last line of defense to make sure people don&#x27;t merge things that will create problems for everyone else.<p>Good engineers figure out the important stuff before anyone writes a line of code. The APIs, the architecture, high-level component names. It&#x27;s all been talked over. When a good engineer reviews another good engineer&#x27;s pull request, they&#x27;re just checking that what is delivered is an implementation of what is expected.<p>Code review is a road block for people who don&#x27;t do the above. It causes them to bump into someone who knows what&#x27;s up.<p>Everything else that gets argued about: local variable names, formatting, equivalent ways to express the same thing. It&#x27;s not important. I don&#x27;t know if it&#x27;s actually confusion about what matters, or if it&#x27;s a desire to make people jump through hoops, or maybe boredom?
评论 #36651199 未加载
评论 #36651165 未加载
jimmaswell将近 2 年前
In code review I absolutely hate armhair architects and backseat programmers who nitpick and theorize over every little architectural decision you made, every variable name, every loop, every comment (even complaining something is &#x2F;too well commented&#x2F;). They weren&#x27;t the one to dive into the problem and consider it and fix it but they think their knee jerk reactions are always more valid than the remote possibility you know what you&#x27;re doing as the person in the trenches. I&#x27;m going to paste a monologue I gave recently:<p>sometimes as people who care about our craft, we need to step back and keep the big picture in mind. the goal is to make a functioning website, or video game, or text editor. the user couldn&#x27;t care less about your code formatting, composition vs inheritance decisions, or commit messages. these are useful tools to the extent that they make it easier for yourself or others in the future to provide more value to the user.<p>at my job, people (mostly external contractors) might make pull requests with formatting annoyances, pointless null checks, getters and setters that have no reason to exist, useless comments, &quot;== true&quot;, things of that nature. I&#x27;d ask these to be fixed and sometimes they would be, but other times someone else would just come in and approve the PR themselves. I realized one day that not a single one of those ugly PR&#x27;s that made it through have ever come back to bite us in any way. everyone&#x27;s time was better spent continuing to work on providing value to the user instead.<p>I hold myself to a higher standard, but when reading others&#x27; code, I&#x27;ve found it really means nothing to me where they put the brackets or what naming convention they use, or even if they change these up randomly. I care that it does the thing, has no glaring red flags, looks appropriately performant for the problem at hand, and won&#x27;t be a maintenance nightmare (will spending 30 minutes changing something right now save at least 1 hour dealing with it later? will those code even be around by the time we get to that point, or replaced by something else? in many cases probably not, of course consider how foundational or isolated the code is.) in all my experience, any bikeshedding over aesthetics has proven to be a total waste of time.<p>if we&#x27;re making a thing that lets the people designing a website make a thing for the visitors, I care that the experience is good for the designer and the visitor, it does what it should, etc. and it&#x27;s <i>reasonably</i> maintainable, but I&#x27;m not going to drag someone into the bikeshed and beat them with the tire pump of subjective aesthetics until they rename their variables to my whimsies.
评论 #36653535 未加载
评论 #36653317 未加载
评论 #36653624 未加载
osigurdson将近 2 年前
I occasionally question the value of the pre-commit code review. In general, there are two options - a quick spot check or a “deep review”.<p>A “deep review” involves completely understanding the code, verifying that it works, refactoring sections or even completely rewriting the original. This type of review can easily take 30 - 70% of the original effort.<p>A quick spot check type review involves rapidly scanning the code for code standards or anything egregiously incorrect.<p>There doesn’t seem to be much of a sweet spot as either the cost is too high or the value is low.<p>I think most reviews are closer to “spot check” type territory. Maybe this can just be done by AI and deep reviews can be done on an as needed basis.<p>Perhaps we can eschew the code review and simply expect that developers do high quality work. When this doesn’t happen, processes could be in place for needed fixes post commit - obviously needed anyway since most reviews just scratch the surface.<p>This is a bit of a strawman, but I expect most have felt that in-depth reviews require too much time while quick spot check type reviews add little value and interrupt everyone’s flow considerably.
johnnyAghands将近 2 年前
I find this kind of backward. IMO it&#x27;s more important to establlish code standards (code style, etc) up front, so I would actually place this as the base. API design is important, but if designed well (e.g. versioned), it shouldn&#x27;t be hard to extend&#x2F;migrate. Whereas changing code style or practices around it require a lot more work, both in terms of coding and culture.
somsak2将近 2 年前
IMO code review is way too late to be talking about API semantics unless you&#x27;re at a really tiny startup. Once you get to even 100 engineers, this stuff needs to be hashed out with stakeholders before you start writing code. this can be as easy as a 10 minute conversation, but sometimes it&#x27;s not so simple and doing it up front will save you tons of time.
raygelogic将近 2 年前
overall this is great. but I&#x27;m surprised that meeting acceptance criteria is under &quot;implementation&quot; and considered less important than api design. I agree that api design is harder to change later on, but the most important question is, does the PR solve a problem that currently exists? if it does, then you can think about whether it does it well.<p>it makes me wonder if the product&#x2F;engineering split has grown wider than it should. we really should be pretending we&#x27;re product managers more.
dav将近 2 年前
Wow. Great idea and overall goal, but I’m disappointed in some of the placement choices.<p>Tests very much in particular deserve more weight. Being DRY is nowhere on the same level as bike shedding code style.
29athrowaway将近 2 年前
Code style: Use an opinionated style and formatter like black, rustfmt, etc. Add it to continuous integration. Never discuss styling again.
mtreis86将近 2 年前
Some quick chart review: don&#x27;t use acronyms, they add cognitive load and require the reader to take action just to read your work.
Bellend将近 2 年前
When does code review kick in? Is it based on say a 10 person team or a 100 person team? Somewhere in-between?
评论 #36649848 未加载
评论 #36650037 未加载
juliangmp将近 2 年前
That&#x27;s pretty neat, I think I&#x27;ll print it and put it up in the office
benjbrooks将近 2 年前
helpful context, just sent for discussion in our team slack
nathants将近 2 年前
reviewing code is obviously fine, we all do it constantly.<p>mandatory code review is a symptom of an adversarial environment involving bad faith actors.<p>there is no quick fix for organizational disfunction, and in that environment mandatory code review will have a negligible effect.
评论 #36650880 未加载