10 ms·

The Code Review Pyramid (2022)

214 points by rainhacker 3y ago

blt 3y ago
handwriting font replaced by sans-serif: https://svgur.com/s/v6n https://svgur.com/s/v6n
mtreis86 3y ago
Some quick chart review: don't use acronyms, they add cognitive load and require the reader to take action just to read your work.
Bellend 3y ago
When does code review kick in? Is it based on say a 10 person team or a 100 person team? Somewhere in-between?
- sidlls 3y ago
  There’s no good reason for any team of 2 or more to not do code reviews.
  - junofan 3y ago
    I even do it solo. Why not proofread?
    - mathgeek 3y ago
      Code review generally requires a peer to do the reviewing, so most folks would likely say that doesn’t count. I was curious if it would, but the first couple pages of googled results confirm the peer aspect.
      - thfuran 3y ago
        That's certainly better than reviewing your own code, but tossing your code into a merge request to run tests and then taking another look over it tomorrow if it's all green is still better than just pushing straight to prod because there's not another pair of eyes.
        mathgeek 3y ago
        Naturally. That’s a different process from a code review though, which is what this thread is discussing.
        thfuran 3y ago
        It's the same process as code review. It is code review.
        mathgeek 3y ago
        Agree to disagree then, per wikipedia and several other sources that pop up when googling.
        Izkata 3y ago
        > but tossing your code into a merge request to run tests and then taking another look over it tomorrow Or even just immediately. I often find minor issues just by looking at it in a different context (the merge request vs my editor).
    - Bellend 3y ago
      That seems almost ridiculous? You commit code that you wrote and then Code Review your commit and then verify it? Doesn't that happen as part of the original commit by common sense?
      - recursive 3y ago
        You might have never seen the diff all at once.
        Bellend 3y ago
        But you wouldn't commit that as there are multiple tools to review a commit before you literally stamp your name on that commit? Otherwise you are generating noise for your teammates.
      - Klaster_1 3y ago
        In practice, reviewing your own code in a different context really makes a difference, at least for me. Not sure why, but I always notice things I wouldn't before. The same effects holds true even for regular text - as much as I try, proofreading my writing in editor yields subpar results compared to reading a submitted message, I simply fail to notice even the most blatant mistakes. I wonder if this effect has a name.
- Thaxll 3y ago
  You should never submit and merge code that was not reviewed, no matter the size of the team.
  - deleted 3y ago
    [deleted]
  - oxfordmale 3y ago
    Why? Does this apply to prototypes too? If you are a team of two, and once of them is on holiday, do you wait until they come back?
    - tkiolp4 3y ago
      My take: if you submit code without the approval of your peers, and that code causes a noticeable issue in production… well, everyone will think “if only they have added me as a reviewer we wouldn’t have this issue!”. Sure thing, the bug could still exists in prod even after peer review, but then at least that would have meant that the bug wasn’t easily discoverable (i.e., the author and the reviewers missed it). If you think you’re smart enough to submit code without bugs and without peer review, that’s fine. Take into account that your peers may not think you are that smart if you do that. Software engineering is mostly about dealing with people (well, almost everything in this world is about that). Obviously if there’s no one to review your code, you may as well go ahead and push it without approval (unless your company has policies against that).
RangerScience 3y ago
Ehhhhh yeah kinda? Yes, you should spend most of your time on "API Semantics" (what does it look like using this code?), and you should spend a lot less time on "how are the tests?" but, for example, Writing good tests massively contributes to good implementation details and API semantics; and test code is also code that needs to be reviewed under more-or-less the same criteria as the rest. Also - documentation (or, legibility, if you'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. I might say instead: - What will it be like reviewing this code? (style, test coverage, etc - do I have to worry about spotting typos, and other stupid stuff?) - What will be like debugging this code? (patterns, logging, etc - which might handled by a framework) - What will it be like altering this code? (documentation/legibility, implementation details, etc - when the business needs change or grow) - What will it be like using this code? (API semantics, API docs - when I go to build something on top of this) And then yeah; the top should be entirely automated, and you should (generally) spend most of your time on the bottom.
- TeMPOraL 3y ago
  My own "but" is shorter: the idea is fine, except code review also "should", per the prevailing wisdom, happen on small, focused changes. But that deep in the woods, style and testing is pretty much all you can talk about. There isn't much use in starting a discussion about API semantics on a commit that implements a stub of one of its endpoints or sth.
  - zerodensity 3y ago
    I might hold an unpopular view here but I do not like reviewing small focused changes. When a feature is implemented I preferably want the entire thing before me when reviewing. Otherwise I find it hard to keep track of everything that has happened. Not to mention that the small focused changes might be reviewed by different people leaving only the implementer in full knowledge of everything that was done. Peer/Assembly programming help but if you do peer/assembly programming reviews are mostly a waste of time (especially for assembly programming).
LadyCailin 3y ago
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. https://kentcdodds.com/blog/write-tests https://kentcdodds.com/blog/write-tests
- makeitdouble 3y ago
  I had that mindset for a while, just to fall into enough issues that I had to balance it again. Integration tests are important for features, unit tests stay important for code blocks. My realization was that a ton of what we wrote aren't features, but blocks. For instance, let's imagine an API that build an invoice. Where do you put the VAT calculation test ? Do you jam your 100+ VAT test cases in the integrations tests, or do you unit test the VAT calculation in isolation and only cover the main cases in the integration ones ? Same for name display, invoice number generation, Invoice items fetching etc. If you want a decent coverage and also document/properly test the edge cases, unit test will easily be half or more of your whole test volume. And of course you'll want to reuse there blocks, so make sure they're rock solid, so pay even more attention to tests.
- imiric 3y ago
  Eh, I disagree. I find the traditional test pyramid a suitable model still. Sure, unit tests require much more maintenance than higher level tests, but they give you confidence that the smallest parts of the codebase work as intended. They're the ones that test all sorts of failure scenarios and edge cases, which is typically not the purpose of integration tests. They also should be inexpensive to run, simple to write, and require minimal setup. Unit tests should also give you immediate feedback that something went wrong, by pinpointing the exact component that failed. In contrast, integration tests might happily pass, as they're not granular enough to cover all code paths. Integration tests focus on, well, that the integration between components is working as expected. So I still insist on having mostly unit tests, many integration tests, and some E2E tests. Doing it otherwise because it saves you maintenance efforts will haunt you in the long run.
alexjurkiewicz 3y ago
If you are reviewing implementation semantics after someone has already coded a feature, I'd say it's too late. You might catch issues, but the loss of goodwill and wasted productivity will make everyone hate code review. I think this pyramid is accurate but better framed as a "review pyramid". Catch semantic issues in an earlier phase (informal discussion or rfc-style proposal document).
- dbish 3y ago
  That really depends on the size of the feature, but imho we should be reducing the need for meetings/reviews, not adding more. Forcing design reviews just slows people down. Jeffy B's "communication is terrible" are words to live by if you want fast moving engineers.
  - tkiolp4 3y ago
    > Forcing design reviews just slows people down Sure, but the price is worth it. If we have a design session before you start coding, it definitely will make the feature go “late” by an amount of time proportional to the design session… which is less time than the required to fix the design after the PR is open. “Fast moving engineers”: I can only imagine managers advocating for that. As an engineer, I can only but express disdain for such a mentality.
    - dbish 3y ago
      I disagree, as someone who has been both an engineer and manager, if you want feedback one on one from someone who you know would be valuable, always go for it, but official design reviews end up including many people who don’t matter once it becomes a required process. The people who most love taking your time for design reviews tend to be architecture astronauts, not fully grokking the real nutty gritty or caring about things that don’t matter.
Pannoniae 3y ago
From what I've seen so far, people love debating code style. The main problem is that those programmers mostly care about superficial syntax and don'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't talk about the object graph or the event lifecycle of the program, because that's hard. A very common thing I see nowadays is a forced adoption of gofmt/black/whatever, hoping it would solve the obsession with the formatting. However, this just locks in a (often substandard) coding style and removes any 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't stop obsession with style, it just forces a specific style on everyone which most of the time, everyone slightly hates. This is a really good article which summarises the problem way better than me. https://luminousmen.com/post/my-unpopular-opinion-about-black-code-formatter https://luminousmen.com/post/my-unpopular-opinion-about-blac...
- eyelidlessness 3y ago
  > From what I've seen so far, people love debating code style. In my experience with people who seemed to confirm this, the opposite turned out to be true. I spent an inordinate amount of time configuring linting and formatting to suit their tastes, to minimize any formatting subjectivity in code review. Their reviews got a lot more substantive and valuable pretty much immediately because their formatting concerns were evidently close enough to solved that they could pay attention to what the code actually did. I’ve had similar success adopting formatting that no one likes just because it’s opinionated and no one gets to dispute it because they couldn’t if they wanted to without diverting resources to a new tool. Now we’re all unhappy with the formatting and still happy to be reviewing actual substance. When the bad formatting gets in the way, it’s trivial to ask the question “is this just a whitespace change?” and everyone groans for a second, agrees that it is, and gets on with life. And when there’s any remaining style considerations (oh you like, or don’t like, reduce?) that’s a much smaller space of style concerns to negotiate and settle.
  - Pannoniae 3y ago
holaworld12 3y ago
I recently received feedback from my manager to take time out to review teammate's PR and design docs. For context, I've always been under-confident and scared to review and feel like I fail to add quality feedback to other people's code and design. I'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?
- alphazard 3y ago
  You may just have a keen level of self-awareness. If you don't have any suggestions, you can just say so. As you gain experience this will likely change, and you will have more to offer. Don't be fooled by your peers that look for superficial things to comment on. There are many people with the same amount of knowledge as you, but who are less self-aware and are adding noise the system, because they are scared of being perceived as incompetent. Find someone who has given you meaningful feedback during code/design review, and see what kind of comments they leave on other people's work. If you don't understand one of their suggestions ask them about it.
- recursivecaveat 3y ago
  One of the most valuable feedback comments you can leave on a review is "hey I found this part confusing, can you add a comment?". I think sometimes people are afraid to admit something like that, but obviously if you're confused by it now, in-context, it will not be easier to understand in 16 months. In general you don't have to provide super clever bug catches, often a second set of eyes is all that's needed. Its surprisingly easy to leave really stupid stuff in your diffs, because you gloss over them, thinking you remember exactly what's in there. As long as you don't make any comments about style, or make a ton of work for them, people are usually very receptive. People enjoy shipping things that are quality, so if you give them an excuse they don't mind adding some comments or trying to merge 2 samey segments.
29athrowaway 3y ago
Code style: Use an opinionated style and formatter like black, rustfmt, etc. Add it to continuous integration. Never discuss styling again.
alphazard 3y ago
Code review is just the last line of defense to make sure people don't merge things that will create problems for everyone else. Good engineers figure out the important stuff before anyone writes a line of code. The APIs, the architecture, high-level component names. It's all been talked over. When a good engineer reviews another good engineer's pull request, they're just checking that what is delivered is an implementation of what is expected. Code review is a road block for people who don't do the above. It causes them to bump into someone who knows what's up. Everything else that gets argued about: local variable names, formatting, equivalent ways to express the same thing. It's not important. I don't know if it's actually confusion about what matters, or if it's a desire to make people jump through hoops, or maybe boredom?
- bee_rider 3y ago
  Could be a sort of social signal that you paid attention to the review?
- imiric 3y ago
  I mostly agree with you, but often, especially with greenfield projects and features, the implementation details are not clear until someone spends the time to dig into the codebase, and deliver a proof-of-concept. This can then drive the design discussion to get everyone on the same page. And even then, the design might need to be changed if it happens that some things weren't taken into consideration, or new specs are communicated and priorities change. It's also worth not spending too much time on creating the perfect design upfront, since it might change during development. The whole waterfall vs. agile situation. An initial proposal document helps with avoiding lengthy discussions and friction during code reviews, but it also takes a lot of effort to produce, and even then, reviews can be difficult for many reasons. > I don't know if it's actually confusion about what matters, or if it's a desire to make people jump through hoops, or maybe boredom? My theory is that sometimes it's a need to say _something_ to prove that you've read the code. Also, egos are often involved, and some reviewers have the need to demonstrate their superior intellect/knowledge/whatever by insisting on minutia. Software engineers are often proud and take code reviews as a chance to flaunt their intellect, while at the same time being defensive of their opinions, and argumentative to the point of arrogance. Few senior engineers are humble, and consider code reviews as a collaborative effort to deliver the best possible solution.
nathants 3y ago
reviewing code is obviously fine, we all do it constantly. mandatory code review is a symptom of an adversarial environment involving bad faith actors. there is no quick fix for organizational disfunction, and in that environment mandatory code review will have a negligible effect.
- e28eta 3y ago
  > mandatory code review is a symptom of an adversarial environment involving bad faith actors. and is required (afaik) for PCI compliance
dav 3y ago
Wow. Great idea and overall goal, but I’m disappointed in some of the placement choices. Tests very much in particular deserve more weight. Being DRY is nowhere on the same level as bike shedding code style.
dang 3y ago
Discussed at the time: The Code Review Pyramid - https://news.ycombinator.com/item?id=30757206 https://news.ycombinator.com/item?id=30757206 - March 2022 (110 comments) The Code Review Pyramid - https://news.ycombinator.com/item?id=30674159 https://news.ycombinator.com/item?id=30674159 - March 2022 (4 comments)
benjbrooks 3y ago
helpful context, just sent for discussion in our team slack
barbariangrunge 3y ago
Is this just a way of saying, “stop wasting all your review time on the style guide and look at the system design”? Although, the style guide should just be followed and fixed before you get to code review phase. That’s just a matter of professionalism
- int0x2e 3y ago
  At a former team, we went from spending quite a bit of time on code style comments and disagreements to spending no time at all on it, with the simple act of making the code linter a breaking step in our CI build, and deciding no review will start until the build is green. We had to adjust our linter settings here and there - but it was still super efficient for everyone's time compared to what we had before... I can't recommend this more.
jimmaswell 3y ago
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 /too well commented/). They weren'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're doing as the person in the trenches. I'm going to paste a monologue I gave recently: 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'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. 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, "== true", things of that nature. I'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's that made it through have ever come back to bite us in any way. everyone's time was better spent continuing to work on providing value to the user instead. I hold myself to a higher standard, but when reading others' code, I'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'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. if we'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's reasonably maintainable, but I'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.
- tkiolp4 3y ago
  It’s all about taste. Some engineers have it, some do not. And yes, taste is subjective, but the Mona Lisa is the Mona Lisa regardless of how many people don’t like it. The most important thing about code is probably its correctness… but that doesn’t mean other less important things are not worth looking at (whether to do so when reviewing a PR is a different topic, though). It also depends on whether one feels too attached to the code in question. If I just landed in a codebase and someone asks me to review a PR, well I guess I don’t mind too much (yet) about things besides correctness/performance/etc. But if I have been maintaining a codebase for years and suddenly some newcomer opens a PR with correct code but that doesn’t have “nice” variable names (at least in accordance to the rest of the codebase), well, I do care about that.
iovrthoughtthis 3y ago
I find this too concrete to be useful. I have a more principals / tradeoffs approach to code review: Areas: - 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 All of the above areas need to be considered within the context of the project and individual team members. ## Readability Code is readable if it meets your expectations and surprises you only when there's something you don'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. ## Maintainability Maintainability is the code'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. ## Risk Code is low risk if it accounts for, and where possible, mitigates the various risks to a project. ## Correctness Correct code does as you expect and surprises you only when there something about the system you didn'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. ## Robustness 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. ## Performance Performant code is as resource efficient as the resources are expensive, but also as efficient as the development costs are cheap.
- epgui 3y ago
  Principles, not principals.
rotifer 3y ago
I write and review medical software. When I'm reviewing code the most important question is, "Is it correct?" If it's not, then the review simply isn't approved (obviously). 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're implementing something that has to conform to the DICOM standard, then you should consider following their nomenclature, even if it may not be what you'd normally use in other contexts. Aside from correctness issues, I'll make anything from "strong recommendations" to "mild suggestions". (In theory, any of these could cause me not to approve a change, but the closer we get to "mild suggestion" the less likely that is.) Examples might include: - You're trying to do something with X. I'm a domain expert in X and while your code is technically correct it's not idiomatic. I suggest doing it this other way instead. - You added a public function to a library, but it will fail if the String argument isn't a valid Photometric Interpretation. Yes, I see that you only call it with valid values, but since it's public anybody can now call it, and they may not be so diligent. How about making it an Enum? - You have a loop that, superficially, looks like it's calculating Bar, but upon closer examination it's (correctly) calculating Bar'. How about adding a comment stating this, so that no one is tempted to (incorrectly) "fix" it? - You'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? - Your log message makes sense if you're reading the surrounding code, but someone from support seeing it in the log file won't know what to make of it. How about including this contextual information in it? - In the loop that you added, your indentation is inconsistent with the rest of the function. How about making it the same? - Why do you have four blanks lines in the middle of your function? In my own code I'm pretty anal about formatting, comments, log messages, use of whitespace, etc. I'm definitely less harsh when reviewing other people's code, but I have to admit that when I see sloppy formatting, grammatical errors or gratuitous use of whitespace, I'm on the alert for sloppiness in other areas, such as design and implementation.
nlnn 3y ago
One goal that I think often gets missed out of code review guides is education. To me, code review is not just there to produce better software, but also better developers. It's a great place to talk about tradeoffs, alternative approaches, reasoning behind changes, etc. Maybe less useful in huge projects with many contractors you're never going to work with again, but I've seen the mentoring aspect have great benefits in small teams.
- butterNaN 3y ago
  Additionally, the Education is both ways - the reviewer is also getting educated along the way, especially from new developers who have a more fresh knowledge of things.
- andelink 3y ago
  Couldn’t agree more. I am lucky to have worked with two individuals early in my career who would take the time to really help develop team members by way of thoughtful, detailed PR comments. I attribute much of my own growth due to this, and as such I always try to do the same for others. I care deeply about it and consider it to be one of the most important parts of the job. It saddens me that most people (IME) don’t feel the same.
  - beerpls 3y ago
    On the other side of the spectrum - code reviews have never, not even one single time, given me any real opportunity to learn something of value. They have only ever been someone else shoving their preferences onto my coding style (for better or worse) I was shocked at my first job to realize the code reviewer didn’t even read my code, merely glanced over and demanded style changes (which were not consistent from review to review in the slightest) It really felt more like how you hand your advisor your thesis with obvious easy to fix errors so he doesn’t compulsively decide “well something must need improvement” and make you fix something difficult
    - toiletduck 3y ago
      From your multiple comments on threads it seems like your ego is strongly attached to your code - if you truly never, not even one time, derived value from a code review, either you always choose to work in toxic environment/s or you're not willing to accept feedback or defend your "coding style" well enough to align colleagues with your opinion. I desperately miss code review feedback at this point in my career after excellent engineers made me question my strong beliefs and empathise with theirs helped me build my path.
ozim 3y ago
Code style and “are tests passing” should not even be there. Code style should be automated same as running tests when someone creates PR.
- hakre 3y ago
  Your relatively short comment caught my interest and I'd like to learn more about your thoughts. As, IMHO it depends (as so often). Code review is commonly the place that puts the current style under test (hence automated otherwise you don't have the results during review) and under review. E.g. Code review finds out if a code-style is missing, incomplete or even outright wrong. If that approach is taken, passing tests and code-style must be in there, otherwise it is not useful to argue about violations. From my understanding I would therefore see those results be available during review. It may also be that there is code-style for automated test-code, and that code part of the review. Without taken this into context of the code-review, I miss a bit the boundary of your comment. Could you elaborate a bit where you draw the line and why? E.g. I can imagine there is a benefit to keep a distinct context for code-reviews so that they are still practically feasible and those parts that need further adoption are put into a different phase, like steps before (preparation of the current increment) or after (preparation of next increments) the code-review itself.
  - ozim 3y ago
    Style formatting should be automated on level of IDE or other CI tooling. Discussing or fixing code style is huge waste of time and styling can be automated. Yes it will be broken in some edge cases but for me that is acceptable because my goal is to deliver the feature and not to make "perfectly aligned code". Aligning code by hand is waste of time, thinking about aligning code is waste of time. Formatting should be in configuration and you format whole file at once and never do stuff by hand. Passing tests is also easy to automate when creating PR, CI should run tests and tell that to person creating PR - hey tests failed, fix it and then after they pass you can open PR, not a fellow developer because that is waste of time for everyone. I did not write "tests should not be there" only "are all tests passing?" is not part of code review for me, it is something that is there before code review even begins. So checking if there are proper tests and if tests make sense is part of code review.
juliangmp 3y ago
That's pretty neat, I think I'll print it and put it up in the office
deleted 3y ago
[deleted]
raygelogic 3y ago
overall this is great. but I'm surprised that meeting acceptance criteria is under "implementation" 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. it makes me wonder if the product/engineering split has grown wider than it should. we really should be pretending we're product managers more.
johnnyAghands 3y ago
I find this kind of backward. IMO it'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't be hard to extend/migrate. Whereas changing code style or practices around it require a lot more work, both in terms of coding and culture.
somsak2 3y ago
IMO code review is way too late to be talking about API semantics unless you'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's not so simple and doing it up front will save you tons of time.
gilbetron 3y ago
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's not my job, that's the developer's job. I know developers that will spend days on a code review, which is a horrible waste of time. CI/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't, you need to spend time there. My main concern as a reviewer is to make sure future people can understand the code when the inevitable modification comes along. 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're messing up the mentorship!
- beerpls 3y ago
  “ 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's not my job, that's the developer's job.” And I personally refuse to work with this style of workflow anymore Does the code do what it needs? That’s objective and it either does or doesn’t. I can manage that kind of review. Does the code look pretty enough? This is subjective and guarantees i’ll waste hours of my life every month bc someone else felt anxious/neurotic/masochistic and wanted to dump a chore on me. No thanks, if you have style guidelines i’ll follow them. I’m not your whipping boy that will ask how high when you say jump.
  - bnjm 3y ago
    > Does the code look pretty enough? ‘…readable, maintainable [and tested]’, ie. if someone else needs to modify or remove this code in the future, will there be friction?
  - smokel 3y ago
    This is a bit harsh. The fact that something is subjective does not mean that it is useless. IMHO, writing maintainable code requires a lot of experience, which one cannot expect from all junior developers. Requiring style guidelines for everything is a bit pedantic, and does not even make sense when you stride in to new territories. The latter is increasingly becoming the only part of software development where I would like to go. If all code and requirements would be so objective and so simple, then I'm afraid you'll soon be replaced by an AI agent :)
osigurdson 3y ago
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”. 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. A quick spot check type review involves rapidly scanning the code for code standards or anything egregiously incorrect. There doesn’t seem to be much of a sweet spot as either the cost is too high or the value is low. 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. 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. 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.
LindaMadison 3y ago
[dead]
taylorsindle 3y ago
[dead]
Everly345 3y ago
[dead]
hubertjeannine 3y ago
[dead]
Nellyz 3y ago
[dead]

214 points by rainhacker 3y ago

blt 3y ago

handwriting font replaced by sans-serif: https://svgur.com/s/v6n https://svgur.com/s/v6n

Some quick chart review: don't use acronyms, they add cognitive load and require the reader to take action just to read your work.

Bellend 3y ago

When does code review kick in? Is it based on say a 10 person team or a 100 person team? Somewhere in-between?

sidlls 3y ago
There’s no good reason for any team of 2 or more to not do code reviews.
- junofan 3y ago
  I even do it solo. Why not proofread?
  - mathgeek 3y ago
    Code review generally requires a peer to do the reviewing, so most folks would likely say that doesn’t count. I was curious if it would, but the first couple pages of googled results confirm the peer aspect.
    - thfuran 3y ago
      That's certainly better than reviewing your own code, but tossing your code into a merge request to run tests and then taking another look over it tomorrow if it's all green is still better than just pushing straight to prod because there's not another pair of eyes.
      - mathgeek 3y ago
        Naturally. That’s a different process from a code review though, which is what this thread is discussing.
        thfuran 3y ago
        It's the same process as code review. It is code review.
        mathgeek 3y ago
        Agree to disagree then, per wikipedia and several other sources that pop up when googling.
      - Izkata 3y ago
        > but tossing your code into a merge request to run tests and then taking another look over it tomorrow Or even just immediately. I often find minor issues just by looking at it in a different context (the merge request vs my editor).
  - Bellend 3y ago
    That seems almost ridiculous? You commit code that you wrote and then Code Review your commit and then verify it? Doesn't that happen as part of the original commit by common sense?
    - recursive 3y ago
      You might have never seen the diff all at once.
      - Bellend 3y ago
        But you wouldn't commit that as there are multiple tools to review a commit before you literally stamp your name on that commit? Otherwise you are generating noise for your teammates.
    - Klaster_1 3y ago
      In practice, reviewing your own code in a different context really makes a difference, at least for me. Not sure why, but I always notice things I wouldn't before. The same effects holds true even for regular text - as much as I try, proofreading my writing in editor yields subpar results compared to reading a submitted message, I simply fail to notice even the most blatant mistakes. I wonder if this effect has a name.
Thaxll 3y ago
You should never submit and merge code that was not reviewed, no matter the size of the team.
- deleted 3y ago
  [deleted]
- oxfordmale 3y ago
  Why? Does this apply to prototypes too? If you are a team of two, and once of them is on holiday, do you wait until they come back?
  - tkiolp4 3y ago
    My take: if you submit code without the approval of your peers, and that code causes a noticeable issue in production… well, everyone will think “if only they have added me as a reviewer we wouldn’t have this issue!”. Sure thing, the bug could still exists in prod even after peer review, but then at least that would have meant that the bug wasn’t easily discoverable (i.e., the author and the reviewers missed it). If you think you’re smart enough to submit code without bugs and without peer review, that’s fine. Take into account that your peers may not think you are that smart if you do that. Software engineering is mostly about dealing with people (well, almost everything in this world is about that). Obviously if there’s no one to review your code, you may as well go ahead and push it without approval (unless your company has policies against that).

RangerScience 3y ago

Ehhhhh yeah kinda? Yes, you should spend most of your time on "API Semantics" (what does it look like using this code?), and you should spend a lot less time on "how are the tests?" but, for example, Writing good tests massively contributes to good implementation details and API semantics; and test code is also code that needs to be reviewed under more-or-less the same criteria as the rest. Also - documentation (or, legibility, if you'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. I might say instead: - What will it be like reviewing this code? (style, test coverage, etc - do I have to worry about spotting typos, and other stupid stuff?) - What will be like debugging this code? (patterns, logging, etc - which might handled by a framework) - What will it be like altering this code? (documentation/legibility, implementation details, etc - when the business needs change or grow) - What will it be like using this code? (API semantics, API docs - when I go to build something on top of this) And then yeah; the top should be entirely automated, and you should (generally) spend most of your time on the bottom.

TeMPOraL 3y ago
My own "but" is shorter: the idea is fine, except code review also "should", per the prevailing wisdom, happen on small, focused changes. But that deep in the woods, style and testing is pretty much all you can talk about. There isn't much use in starting a discussion about API semantics on a commit that implements a stub of one of its endpoints or sth.
- zerodensity 3y ago
  I might hold an unpopular view here but I do not like reviewing small focused changes. When a feature is implemented I preferably want the entire thing before me when reviewing. Otherwise I find it hard to keep track of everything that has happened. Not to mention that the small focused changes might be reviewed by different people leaving only the implementer in full knowledge of everything that was done. Peer/Assembly programming help but if you do peer/assembly programming reviews are mostly a waste of time (especially for assembly programming).

LadyCailin 3y ago

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. https://kentcdodds.com/blog/write-tests https://kentcdodds.com/blog/write-tests

makeitdouble 3y ago
I had that mindset for a while, just to fall into enough issues that I had to balance it again. Integration tests are important for features, unit tests stay important for code blocks. My realization was that a ton of what we wrote aren't features, but blocks. For instance, let's imagine an API that build an invoice. Where do you put the VAT calculation test ? Do you jam your 100+ VAT test cases in the integrations tests, or do you unit test the VAT calculation in isolation and only cover the main cases in the integration ones ? Same for name display, invoice number generation, Invoice items fetching etc. If you want a decent coverage and also document/properly test the edge cases, unit test will easily be half or more of your whole test volume. And of course you'll want to reuse there blocks, so make sure they're rock solid, so pay even more attention to tests.
imiric 3y ago
Eh, I disagree. I find the traditional test pyramid a suitable model still. Sure, unit tests require much more maintenance than higher level tests, but they give you confidence that the smallest parts of the codebase work as intended. They're the ones that test all sorts of failure scenarios and edge cases, which is typically not the purpose of integration tests. They also should be inexpensive to run, simple to write, and require minimal setup. Unit tests should also give you immediate feedback that something went wrong, by pinpointing the exact component that failed. In contrast, integration tests might happily pass, as they're not granular enough to cover all code paths. Integration tests focus on, well, that the integration between components is working as expected. So I still insist on having mostly unit tests, many integration tests, and some E2E tests. Doing it otherwise because it saves you maintenance efforts will haunt you in the long run.

alexjurkiewicz 3y ago

If you are reviewing implementation semantics after someone has already coded a feature, I'd say it's too late. You might catch issues, but the loss of goodwill and wasted productivity will make everyone hate code review. I think this pyramid is accurate but better framed as a "review pyramid". Catch semantic issues in an earlier phase (informal discussion or rfc-style proposal document).

dbish 3y ago
That really depends on the size of the feature, but imho we should be reducing the need for meetings/reviews, not adding more. Forcing design reviews just slows people down. Jeffy B's "communication is terrible" are words to live by if you want fast moving engineers.
- tkiolp4 3y ago
  > Forcing design reviews just slows people down Sure, but the price is worth it. If we have a design session before you start coding, it definitely will make the feature go “late” by an amount of time proportional to the design session… which is less time than the required to fix the design after the PR is open. “Fast moving engineers”: I can only imagine managers advocating for that. As an engineer, I can only but express disdain for such a mentality.
  - dbish 3y ago
    I disagree, as someone who has been both an engineer and manager, if you want feedback one on one from someone who you know would be valuable, always go for it, but official design reviews end up including many people who don’t matter once it becomes a required process. The people who most love taking your time for design reviews tend to be architecture astronauts, not fully grokking the real nutty gritty or caring about things that don’t matter.

Pannoniae 3y ago

From what I've seen so far, people love debating code style. The main problem is that those programmers mostly care about superficial syntax and don'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't talk about the object graph or the event lifecycle of the program, because that's hard. A very common thing I see nowadays is a forced adoption of gofmt/black/whatever, hoping it would solve the obsession with the formatting. However, this just locks in a (often substandard) coding style and removes any 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't stop obsession with style, it just forces a specific style on everyone which most of the time, everyone slightly hates. This is a really good article which summarises the problem way better than me. https://luminousmen.com/post/my-unpopular-opinion-about-black-code-formatter https://luminousmen.com/post/my-unpopular-opinion-about-blac...

eyelidlessness 3y ago
> From what I've seen so far, people love debating code style. In my experience with people who seemed to confirm this, the opposite turned out to be true. I spent an inordinate amount of time configuring linting and formatting to suit their tastes, to minimize any formatting subjectivity in code review. Their reviews got a lot more substantive and valuable pretty much immediately because their formatting concerns were evidently close enough to solved that they could pay attention to what the code actually did. I’ve had similar success adopting formatting that no one likes just because it’s opinionated and no one gets to dispute it because they couldn’t if they wanted to without diverting resources to a new tool. Now we’re all unhappy with the formatting and still happy to be reviewing actual substance. When the bad formatting gets in the way, it’s trivial to ask the question “is this just a whitespace change?” and everyone groans for a second, agrees that it is, and gets on with life. And when there’s any remaining style considerations (oh you like, or don’t like, reduce?) that’s a much smaller space of style concerns to negotiate and settle.
- Pannoniae 3y ago

holaworld12 3y ago

I recently received feedback from my manager to take time out to review teammate's PR and design docs. For context, I've always been under-confident and scared to review and feel like I fail to add quality feedback to other people's code and design. I'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?

alphazard 3y ago
You may just have a keen level of self-awareness. If you don't have any suggestions, you can just say so. As you gain experience this will likely change, and you will have more to offer. Don't be fooled by your peers that look for superficial things to comment on. There are many people with the same amount of knowledge as you, but who are less self-aware and are adding noise the system, because they are scared of being perceived as incompetent. Find someone who has given you meaningful feedback during code/design review, and see what kind of comments they leave on other people's work. If you don't understand one of their suggestions ask them about it.
recursivecaveat 3y ago
One of the most valuable feedback comments you can leave on a review is "hey I found this part confusing, can you add a comment?". I think sometimes people are afraid to admit something like that, but obviously if you're confused by it now, in-context, it will not be easier to understand in 16 months. In general you don't have to provide super clever bug catches, often a second set of eyes is all that's needed. Its surprisingly easy to leave really stupid stuff in your diffs, because you gloss over them, thinking you remember exactly what's in there. As long as you don't make any comments about style, or make a ton of work for them, people are usually very receptive. People enjoy shipping things that are quality, so if you give them an excuse they don't mind adding some comments or trying to merge 2 samey segments.

29athrowaway 3y ago

Code style: Use an opinionated style and formatter like black, rustfmt, etc. Add it to continuous integration. Never discuss styling again.

alphazard 3y ago

Code review is just the last line of defense to make sure people don't merge things that will create problems for everyone else. Good engineers figure out the important stuff before anyone writes a line of code. The APIs, the architecture, high-level component names. It's all been talked over. When a good engineer reviews another good engineer's pull request, they're just checking that what is delivered is an implementation of what is expected. Code review is a road block for people who don't do the above. It causes them to bump into someone who knows what's up. Everything else that gets argued about: local variable names, formatting, equivalent ways to express the same thing. It's not important. I don't know if it's actually confusion about what matters, or if it's a desire to make people jump through hoops, or maybe boredom?

bee_rider 3y ago
Could be a sort of social signal that you paid attention to the review?
imiric 3y ago
I mostly agree with you, but often, especially with greenfield projects and features, the implementation details are not clear until someone spends the time to dig into the codebase, and deliver a proof-of-concept. This can then drive the design discussion to get everyone on the same page. And even then, the design might need to be changed if it happens that some things weren't taken into consideration, or new specs are communicated and priorities change. It's also worth not spending too much time on creating the perfect design upfront, since it might change during development. The whole waterfall vs. agile situation. An initial proposal document helps with avoiding lengthy discussions and friction during code reviews, but it also takes a lot of effort to produce, and even then, reviews can be difficult for many reasons. > I don't know if it's actually confusion about what matters, or if it's a desire to make people jump through hoops, or maybe boredom? My theory is that sometimes it's a need to say _something_ to prove that you've read the code. Also, egos are often involved, and some reviewers have the need to demonstrate their superior intellect/knowledge/whatever by insisting on minutia. Software engineers are often proud and take code reviews as a chance to flaunt their intellect, while at the same time being defensive of their opinions, and argumentative to the point of arrogance. Few senior engineers are humble, and consider code reviews as a collaborative effort to deliver the best possible solution.

nathants 3y ago

reviewing code is obviously fine, we all do it constantly. mandatory code review is a symptom of an adversarial environment involving bad faith actors. there is no quick fix for organizational disfunction, and in that environment mandatory code review will have a negligible effect.

e28eta 3y ago
> mandatory code review is a symptom of an adversarial environment involving bad faith actors. and is required (afaik) for PCI compliance

dav 3y ago

Wow. Great idea and overall goal, but I’m disappointed in some of the placement choices. Tests very much in particular deserve more weight. Being DRY is nowhere on the same level as bike shedding code style.

dang 3y ago

Discussed at the time: The Code Review Pyramid - https://news.ycombinator.com/item?id=30757206 https://news.ycombinator.com/item?id=30757206 - March 2022 (110 comments) The Code Review Pyramid - https://news.ycombinator.com/item?id=30674159 https://news.ycombinator.com/item?id=30674159 - March 2022 (4 comments)

benjbrooks 3y ago

helpful context, just sent for discussion in our team slack

barbariangrunge 3y ago

Is this just a way of saying, “stop wasting all your review time on the style guide and look at the system design”? Although, the style guide should just be followed and fixed before you get to code review phase. That’s just a matter of professionalism

int0x2e 3y ago
At a former team, we went from spending quite a bit of time on code style comments and disagreements to spending no time at all on it, with the simple act of making the code linter a breaking step in our CI build, and deciding no review will start until the build is green. We had to adjust our linter settings here and there - but it was still super efficient for everyone's time compared to what we had before... I can't recommend this more.

jimmaswell 3y ago

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 /too well commented/). They weren'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're doing as the person in the trenches. I'm going to paste a monologue I gave recently: 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'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. 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, "== true", things of that nature. I'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's that made it through have ever come back to bite us in any way. everyone's time was better spent continuing to work on providing value to the user instead. I hold myself to a higher standard, but when reading others' code, I'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'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. if we'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's reasonably maintainable, but I'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.

tkiolp4 3y ago
It’s all about taste. Some engineers have it, some do not. And yes, taste is subjective, but the Mona Lisa is the Mona Lisa regardless of how many people don’t like it. The most important thing about code is probably its correctness… but that doesn’t mean other less important things are not worth looking at (whether to do so when reviewing a PR is a different topic, though). It also depends on whether one feels too attached to the code in question. If I just landed in a codebase and someone asks me to review a PR, well I guess I don’t mind too much (yet) about things besides correctness/performance/etc. But if I have been maintaining a codebase for years and suddenly some newcomer opens a PR with correct code but that doesn’t have “nice” variable names (at least in accordance to the rest of the codebase), well, I do care about that.

iovrthoughtthis 3y ago

I find this too concrete to be useful. I have a more principals / tradeoffs approach to code review: Areas: - 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 All of the above areas need to be considered within the context of the project and individual team members. ## Readability Code is readable if it meets your expectations and surprises you only when there's something you don'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. ## Maintainability Maintainability is the code'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. ## Risk Code is low risk if it accounts for, and where possible, mitigates the various risks to a project. ## Correctness Correct code does as you expect and surprises you only when there something about the system you didn'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. ## Robustness 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. ## Performance Performant code is as resource efficient as the resources are expensive, but also as efficient as the development costs are cheap.

epgui 3y ago
Principles, not principals.

rotifer 3y ago

I write and review medical software. When I'm reviewing code the most important question is, "Is it correct?" If it's not, then the review simply isn't approved (obviously). 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're implementing something that has to conform to the DICOM standard, then you should consider following their nomenclature, even if it may not be what you'd normally use in other contexts. Aside from correctness issues, I'll make anything from "strong recommendations" to "mild suggestions". (In theory, any of these could cause me not to approve a change, but the closer we get to "mild suggestion" the less likely that is.) Examples might include: - You're trying to do something with X. I'm a domain expert in X and while your code is technically correct it's not idiomatic. I suggest doing it this other way instead. - You added a public function to a library, but it will fail if the String argument isn't a valid Photometric Interpretation. Yes, I see that you only call it with valid values, but since it's public anybody can now call it, and they may not be so diligent. How about making it an Enum? - You have a loop that, superficially, looks like it's calculating Bar, but upon closer examination it's (correctly) calculating Bar'. How about adding a comment stating this, so that no one is tempted to (incorrectly) "fix" it? - You'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? - Your log message makes sense if you're reading the surrounding code, but someone from support seeing it in the log file won't know what to make of it. How about including this contextual information in it? - In the loop that you added, your indentation is inconsistent with the rest of the function. How about making it the same? - Why do you have four blanks lines in the middle of your function? In my own code I'm pretty anal about formatting, comments, log messages, use of whitespace, etc. I'm definitely less harsh when reviewing other people's code, but I have to admit that when I see sloppy formatting, grammatical errors or gratuitous use of whitespace, I'm on the alert for sloppiness in other areas, such as design and implementation.

nlnn 3y ago

One goal that I think often gets missed out of code review guides is education. To me, code review is not just there to produce better software, but also better developers. It's a great place to talk about tradeoffs, alternative approaches, reasoning behind changes, etc. Maybe less useful in huge projects with many contractors you're never going to work with again, but I've seen the mentoring aspect have great benefits in small teams.

butterNaN 3y ago
Additionally, the Education is both ways - the reviewer is also getting educated along the way, especially from new developers who have a more fresh knowledge of things.
andelink 3y ago
Couldn’t agree more. I am lucky to have worked with two individuals early in my career who would take the time to really help develop team members by way of thoughtful, detailed PR comments. I attribute much of my own growth due to this, and as such I always try to do the same for others. I care deeply about it and consider it to be one of the most important parts of the job. It saddens me that most people (IME) don’t feel the same.
- beerpls 3y ago
  On the other side of the spectrum - code reviews have never, not even one single time, given me any real opportunity to learn something of value. They have only ever been someone else shoving their preferences onto my coding style (for better or worse) I was shocked at my first job to realize the code reviewer didn’t even read my code, merely glanced over and demanded style changes (which were not consistent from review to review in the slightest) It really felt more like how you hand your advisor your thesis with obvious easy to fix errors so he doesn’t compulsively decide “well something must need improvement” and make you fix something difficult
  - toiletduck 3y ago
    From your multiple comments on threads it seems like your ego is strongly attached to your code - if you truly never, not even one time, derived value from a code review, either you always choose to work in toxic environment/s or you're not willing to accept feedback or defend your "coding style" well enough to align colleagues with your opinion. I desperately miss code review feedback at this point in my career after excellent engineers made me question my strong beliefs and empathise with theirs helped me build my path.

ozim 3y ago

Code style and “are tests passing” should not even be there. Code style should be automated same as running tests when someone creates PR.

hakre 3y ago
Your relatively short comment caught my interest and I'd like to learn more about your thoughts. As, IMHO it depends (as so often). Code review is commonly the place that puts the current style under test (hence automated otherwise you don't have the results during review) and under review. E.g. Code review finds out if a code-style is missing, incomplete or even outright wrong. If that approach is taken, passing tests and code-style must be in there, otherwise it is not useful to argue about violations. From my understanding I would therefore see those results be available during review. It may also be that there is code-style for automated test-code, and that code part of the review. Without taken this into context of the code-review, I miss a bit the boundary of your comment. Could you elaborate a bit where you draw the line and why? E.g. I can imagine there is a benefit to keep a distinct context for code-reviews so that they are still practically feasible and those parts that need further adoption are put into a different phase, like steps before (preparation of the current increment) or after (preparation of next increments) the code-review itself.
- ozim 3y ago
  Style formatting should be automated on level of IDE or other CI tooling. Discussing or fixing code style is huge waste of time and styling can be automated. Yes it will be broken in some edge cases but for me that is acceptable because my goal is to deliver the feature and not to make "perfectly aligned code". Aligning code by hand is waste of time, thinking about aligning code is waste of time. Formatting should be in configuration and you format whole file at once and never do stuff by hand. Passing tests is also easy to automate when creating PR, CI should run tests and tell that to person creating PR - hey tests failed, fix it and then after they pass you can open PR, not a fellow developer because that is waste of time for everyone. I did not write "tests should not be there" only "are all tests passing?" is not part of code review for me, it is something that is there before code review even begins. So checking if there are proper tests and if tests make sense is part of code review.

juliangmp 3y ago

That's pretty neat, I think I'll print it and put it up in the office

deleted 3y ago

[deleted]

raygelogic 3y ago

overall this is great. but I'm surprised that meeting acceptance criteria is under "implementation" 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. it makes me wonder if the product/engineering split has grown wider than it should. we really should be pretending we're product managers more.

johnnyAghands 3y ago

I find this kind of backward. IMO it'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't be hard to extend/migrate. Whereas changing code style or practices around it require a lot more work, both in terms of coding and culture.

somsak2 3y ago

IMO code review is way too late to be talking about API semantics unless you'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's not so simple and doing it up front will save you tons of time.

gilbetron 3y ago

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's not my job, that's the developer's job. I know developers that will spend days on a code review, which is a horrible waste of time. CI/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't, you need to spend time there. My main concern as a reviewer is to make sure future people can understand the code when the inevitable modification comes along. 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're messing up the mentorship!

beerpls 3y ago
“ 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's not my job, that's the developer's job.” And I personally refuse to work with this style of workflow anymore Does the code do what it needs? That’s objective and it either does or doesn’t. I can manage that kind of review. Does the code look pretty enough? This is subjective and guarantees i’ll waste hours of my life every month bc someone else felt anxious/neurotic/masochistic and wanted to dump a chore on me. No thanks, if you have style guidelines i’ll follow them. I’m not your whipping boy that will ask how high when you say jump.
- bnjm 3y ago
  > Does the code look pretty enough? ‘…readable, maintainable [and tested]’, ie. if someone else needs to modify or remove this code in the future, will there be friction?
- smokel 3y ago
  This is a bit harsh. The fact that something is subjective does not mean that it is useless. IMHO, writing maintainable code requires a lot of experience, which one cannot expect from all junior developers. Requiring style guidelines for everything is a bit pedantic, and does not even make sense when you stride in to new territories. The latter is increasingly becoming the only part of software development where I would like to go. If all code and requirements would be so objective and so simple, then I'm afraid you'll soon be replaced by an AI agent :)

osigurdson 3y ago

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”. 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. A quick spot check type review involves rapidly scanning the code for code standards or anything egregiously incorrect. There doesn’t seem to be much of a sweet spot as either the cost is too high or the value is low. 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. 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. 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.

LindaMadison 3y ago

[dead]

taylorsindle 3y ago

[dead]

Everly345 3y ago

[dead]

hubertjeannine 3y ago

[dead]

Nellyz 3y ago

[dead]