12 ms·

The Code Review Pyramid

206 points by adrianomartins 5y ago

sz4kerto 5y ago
Ours is different. Tests (end-to-end and integration) are at the bottom. If there are no tests that prove that the code works, we won't really look further because we don't even know if the code implements the right thing. Then comes interfaces, then implementation, then style.
- sublimefire 5y ago
  What is more the tests will highlight the API and the expectations (see API semantics in the pyramid). By nature, the process of writing tests will usually catch some bugs. I'm not arguing for the tests to cover each possible scenario, but they must exist. If, on the other hand, it is hard to add the tests then it probably means you have bigger problems.
ilovecaching 5y ago
This doesn't seem correct to me. Each of these categories should bear equal weight, I won't spend less time on code style or tests to spend more time on API design. My code review checklist is a checklist. I spend as much time as necessary on each item in the checklist, no more no less. I also think that code style, fuzzing, and benchmark tests are the most important items on my checklist. It keeps the code base from becoming a jenga tower that is brittle to change. It also keeps us honest as to whether a feature is a perf hit or not. Tests are far more important than this graphic portrays them as.
- kolanos 5y ago
  Code style is the first thing I eliminate from a review process. I find the most commonly used code formatter and linter for $LANG (the more opinionated the better) and plug it into pre-commits and CI. Code style is just not worth my time.
  - simonbarker87 5y ago
    I’m at the point now where I don’t want to think about layout and formatting of my code, just let a formatter do it all for me on save please
    - tuyiown 5y ago
      But, code formatting is not a solved problem, right ? You either have to fiddle until the formater outputs proper layout, or let it rest as is and never learn how to format your code, letting the code base leaking tons of probable less than ideal layout.
      - RussianCow 5y ago
        I configure Prettier once on my JS codebases and then never touch it again. Seems about as "solved" as is reasonably possible.
        tuyiown 5y ago
        http://journal.stuffwithstuff.com/2015/09/08/the-hardest-program-ive-ever-written/ http://journal.stuffwithstuff.com/2015/09/08/the-hardest-pro...
        RussianCow 5y ago
        I'm not sure how that's relevant. The fact that it's difficult doesn't make it unsolved.
      - kolanos 5y ago
        I'll take a consistent less than ideal layout over whatever $HUMAN comes up with. But to me an inconsistent layout is the less than ideal one. And humans excel at inconsistency.
        tuyiown 5y ago
simonbarker87 5y ago
I like the attempt at making this process smoother, I guess where to place importance of different aspects relative to each other is always down to opinion and interpretation. I wrote this piece a while ago focussing on using the Must/Should/Could principle to speed up the review process and put a less opinionated framework in place to help keep code review moving. https://careerswitchtocoding.com/blog/moscow-the-best-code-review-technique-youre-not-using https://careerswitchtocoding.com/blog/moscow-the-best-code-r... It’s not an original thought from me but many people hadn’t heard about it and off the back of this post I’ve had a few people get in touch to say they’ve implemented it in their team and it’s helped improve code review speed.
mannykannot 5y ago
While I recognize the problem as real and significant, and I think a hierarchy of concerns is a valid way to mitigate it, I feel there are a few problems in this particular pyramid. Of all the issues that might be raised in a review, which are the most important to fix? I find it difficult to imagine a ranking in which incorrect functioning (including security vulnerabilities and truly inadequate performance) are not at the top (answering the ranking question with "all of them" would just be a way to avoid the issue.) In this pyramid, issues of this nature are to be found at all levels except the top one, mixed in with more-or-less subjective ones, such as "Is a new API generally useful and not overly specific?" - pointless flame wars have erupted over issues such as this (also, orthogonally, code review is late in the game to be asking this one.) For a review to be successful, its participants need to be able to restrain themselves from going down rabbit holes that could not lead to necessary rework - or a strong moderator, if the individual participants cannot act with adequate self-restraint.
- th3iedkid 5y ago
  Another aspect, the pyramid makes code review look like the only thing around the actual value outcome. There’s a lot of other aspects like pair-programming that eliminates most things in the pyramid, taking it out of scope from a review process
- pcmoney 5y ago
  If your team does not share a subjective understanding of what makes their product/API useful none of this matters. Focusing on objective technicalities is the least useful aspect of code review. If you have someone who constantly is like "Well that's subjective" and derails conversation that way you need to coach them or move them off the team.
  - mannykannot 5y ago
    Are you saying that if you enter a code review without having a shared subjective understanding of what makes your product/API useful then the review is not likely to be successful, or that code review is the place to form such a shared subjective understanding? I can agree to the first interpretation, but about the latter I would say that code review is quite a bit later than optimal, and that there are other, better ways of achieving it. If your organization is such that you cannot even discuss these issues until you have working code, then you have other, bigger, problems.
mpalczewski 5y ago
First thing I review is readability. Code should be readable. Once code is readable it makes reviewing the rest much easier. Readable code is more maintainable and problems jump out at you. Stuff other than readability is important, but if you focus on readability it makes the rest of the review go very smoothly.
- nine_zeros 5y ago
  If the code is doing the wrong thing in the first place, no amount of nitpicking on readability will save time. First pass: Functional correctness Second pass: Better ways to do the same thing (optional) Third pass: your favorite nitpicks.
  - pc86 5y ago
    This structure (especially the optionality of the second pass) is how you end up with perfectly readable code iterating over List<T> when a hash map would suffice and be orders of magnitude more performant. Nobody can do it perfectly, but I think trying to keep personal nitpicks out of reviews as much as possible - ideally by codifying team nitpicks in automated formatting/.prettierrc/whatever - lets people focus on things that matter: what the code does and how it does it.
    - watwut 5y ago
      Then again, that one is super easy to find and fix if it turns out to actually be problem.
    - TheCoelacanth 5y ago
      In perfectly readable code, it's easy to notice performance problems. In my experience, unreadable code is usually where the worst performance problems are, because that's where they are the hardest to find and fix. You seem to be conflating readability with formatting, which is not what I mean by readability. Poor and inconsistent formatting adds some friction to reading, but it's a relatively minor part of readability.
  - 5y ago
Quarrelsome 5y ago
aren't we missing something at the base of the pyramid: > does it actually work? I feel like the foundation of any code review has to be a bug analysis to ensure that the feature works, doesn't create regressions or fail in another supported use-case.
- pc86 5y ago
  Potentially unpopular opinion, but I would much rather get well-formatted, performant, readable code that misses the business case a bit than get a ball of spaghetti mess that nails it. Not only will it be easier to update the code in the former, but the business pressure to just deploy the latter and "fix it later" can sometimes be too high to resist. I was just looking at a "proof of concept" with a 2017 commit date in production last week, the danger of that is all too real.
  - Quarrelsome 5y ago
    No, I mean like the primary function of every PR is to ensure the code does what its trying to do and doesn't (for example) fail to compile, or break the build or create an infinite loop or an out of memory exception.
    - digisign 5y ago
      Arguably too fundamental to mention, at least in one slide. Perhaps covered during the developer-testing period, before peer review.
      - deleted 5y ago
        [deleted]
    - b3morales 5y ago
      On a dayjob closed-team project, if my coworkers are putting up PRs without making sure they meet functional requirements, that is a problem that needs to be solved several steps before the PR. If it's a junior person who doesn't know better, more working side-by-side with them to get to the PR. If it's a senior person dealing with a shitty spec, more discussion with PMs/designers before we start coding. If it's a senior person who's putting up broken code because they don't care...that's probably a management conversation. I have to be able to trust that my teammates are doing their best to produce working components. Otherwise code review is a bandaid covering a hole in the hull of a sinking ship.
willcipriano 5y ago
The more experienced I get the more I want to work on teams with a higher degree of trust than ones like this. Don't get me wrong I think code review can be valuable but on high preforming teams it works best on a as needed basis. The way this has worked successfully for me in the past is as a new joiner to a team you submit for code review every time. My first couple of reviews tend to have a good bit of feedback as I settle in to the style of the team and often a new language or framework. After a while though I become a domain expert for the projects I am working on, and code reviews end up becoming a rubber stamp. At that point it's just something slowing me down. I'd request review when I need it, or I'm making a big change that will effect everyone, otherwise there are many cases where there is really only one way to skin this cat and it's totally perfunctory. This sort of arrangement also requires a bit of ownership, if you see reports floating around about an endpoint you just worked on having issues you have to be able to be trusted to jump on it. I feel like trust and ownership are touchy feely people problems so tech people invented code reviews and linters to try to make up for not cultivating the sort of culture where they aren't required on every single little thing.
- edgyquant 5y ago
  On my team we have one other engineer, no matter your or their official position, just do a quick glance over for obvious issues. This does not prevent real bugs from getting merged in, the test cases are supposedly for that but even those have some slippage.
- Cerium 5y ago
  I work on a team that runs following a high trust and domain ownership model. When I joined our process was "write some code, test it, be confident in the code, commit it to trunk". A number of years ago we worked our way through "Clean Code" in a reading group and decided to try out code reviews. Each week one of us would present our recent work and the team would tear it apart line by line, and in the process we developed a better understanding of our group philosophy around style and practices. When we switched to Git we decided that we liked code reviews and wanted to make them part of merging to develop. Depending on the content the reviews are deeper or more superficial. Many times even when I review the code of somebody making changes in their own subject matter domain I have found bugs that simply needed another set of eyes to catch. I like code reviews and in the context I use them find that they are a complement to trust, not a substitute.
neebz 5y ago
previous discussion: https://news.ycombinator.com/item?id=30674159 https://news.ycombinator.com/item?id=30674159
AngeloR 5y ago
I've been thinking about code reviews a lot.. and I think a big problem with existing code reviews is that for most orgs, code reviews were a way to move planning to the end of the development process instead of keeping it up-front. I think the foundation of the pyramid (API and Implementation semantics) is often discussed in code reviews when it should have almost no place. By the time you get to the code review not only should these have been ironed out and shared with the whole team. What you should be looking at is simply adherence to the plan, and calling out deviations. I wrote about a small rat about this: https://xangelo.ca/posts/code-reviews-are-failure/ https://xangelo.ca/posts/code-reviews-are-failure/
- gunnarmorling 5y ago
  > is often discussed in code reviews when it should have almost no place. By the time you get to the code review not only should these have been ironed out and shared with the whole team. That's a fair point. It's my background in open-source and I suppose the specific ways we work in the projects I'm involved with which made me add these things at the bottom. Oftentimes, contributors come up with PRs for new features, and there was no prior discussion about API design etc. (although we highly recommend to have such discussion before starting with the work on a large PR). I.e. oftentimes a PR review will be the first time I see how something is implemented, how the API looks like etc. It may be different with more controlled settings in in-house development teams adhering to more formally defined processes.
  - AngeloR 5y ago
    This is 100% a side-effect of GitHub. Open Source used to have the additional barrier of mailing lists to detract from "drive-by PRs". The side effect, of course, is that it made it much harder to get involved. GitHub really brought a lot of new eyes on open source software and lowered the barrier to contribute exacerbating (an argument could be made for creating) the drive-by PR problem.
VBprogrammer 5y ago
I've noticed a phenomenon where I sometimes have to comment about something near the pointy end of the code review pyramid in order to get my brain into the correct mindset to really start grocking how and why someone has made the changes they have. After I break this barrier it often leads to the deeper questions about whether it is doing what it supposed to be doing.
owlbite 5y ago
Completely different to the order I use, which basically amounts to "how hard is it to fix this later". So most important questions are: (1) API must be as correct as possible, especially if its exposed outside of the immediate team. Changing APIs is a royal pain. (2) Documentation of what's going on, especially for non-obvious stuff. If we come back to this in 3 years time and the author has moved on, do we have any hope of fixing it? (3) Testing. What degree of confidence do we have this is doing the right thing? Will it protect me when I change it in a few years time? Any minor bugs or code style issues can be fixed later as/when they are found and cause problems.
- greymalik 5y ago
  I think you might be misreading the pyramid? The most important things are at the bottom. It aligns exactly with what you’re saying.
  - gunnarmorling 5y ago
    Exactly that. "how hard is it to fix this later" is what drives the ordering in the pyramid.
makecheck 5y ago
An important one often overlooked (due to our tendency to focus on just the “diff”): what else should be changing or could be affected? Very easy to forget to change code that isn’t highlighted in the review.
nhoughto 5y ago
I'll use this I think, very handy. I'd probably emphasis all the text/descriptions more too, lots of important nuggets in there that aren't obvious when glancing, like automating the top portions of the pyramid, humans shouldn't be mis-formatting code and others human comment on it anymore.
BrissyCoder 5y ago
https://en.wikipedia.org/wiki/Law_of_triviality https://en.wikipedia.org/wiki/Law_of_triviality When I was on a larger team that actively participated in sprint demos was regularly a witness to this. Senior types that came along and needed to be heard - not really having a deep understanding of the application - would pick at trivial consistency issues in the UI or whatever. Such eye-rolling behaviour and hard to believe they didn't realise how transparent it was. Probably have been guilty of it myself tbh.
majormajor 5y ago
Across 6 jobs now I've never seen code reviews focus primarily on formatting or style over functionality or future-looking concerns. How common is this, really? At the last couple places formatting has been handled by auto-linters; before that, it was hit or miss, but never a dominant topic of argument.
- sanderjd 5y ago
  Seems essentially non existent to me, outside blog posts.
deschutes 5y ago
I find reviews work best when you place a strong bias towards shipping workable if imperfect code. Quick reviews are also a strongly positive thing. On the other end are fixed processes. Have as little of this as possible, automate it if you can. That means formatting, linters and shallow static analysis. All of this is mainly in service of building trust. If you cant have some measure of success there you are truly doomed.
smokey_circles 5y ago
One thing I wish I knew was coming was what my first code review as the tech lead was gonna be like. I was as unprepared as the grads code was. Things I've learned about my own professional code preferences since 1) I will not sacrifice readability for any reason. That is the hill I'm willing to die on. Reducing code is sometimes hard and that's fine that people like to challenge themselves by doing so, but not at the expense of readability. You will forget the clever trick you employed, I promise 2) Business objectives are second class to system stability. This is because the primary business objective is to be online. Failing a few transaction is bad. Failing all the transactions is much worse. 3) There is no such thing as self documenting code. There is just good nomenclature. Documents always improve someone's experience. On boarding documents for new devs will save you so much trouble. 4) I'm a huge fan of in memory caches. I hate seeing them in code. When they work, they're wonderful. But when there are issues, they're closer to demons than to bugs. 5) Data Structures. Data Structures, Data Structures, Data Structures. And then more Data Structures. Easy pre-optimisation, and these days it usually just means knowing when not to use "the default" data structure (such as ArrayList in java) 7) One day (if you're not there already), you will be on the otherside of this review. You're almost guaranteed to remember the bad times, so pay extra special attention to when the process is working well. A simple trick I've learned is to repeat a process that worked from a previous role at a new role who's process is not working. Works more times than not.
- eorn3452gl 5y ago
  > "knowing when not to use "the default" data structure (such as ArrayList in java)" Can you give some examples of when not to use ArrayList?
dhzhzjsbevs 5y ago
> When it comes to code reviews, it’s a common phenomenon that there is much focus and long-winded discussions around mundane aspects like code formatting and style, whereas important aspects (does the code change do what it is supposed to do, is it performant, is it backwards-compatible for existing clients, and many others) tend to get less attention. Code formatting and style discussions that don't start and end with, hey, our code formatter is broken have absolutely no place in code review. Also, keep your damn nits to yourself. If it's a nit, it's not important.

Ours is different. Tests (end-to-end and integration) are at the bottom. If there are no tests that prove that the code works, we won't really look further because we don't even know if the code implements the right thing. Then comes interfaces, then implementation, then style.

sublimefire 5y ago
What is more the tests will highlight the API and the expectations (see API semantics in the pyramid). By nature, the process of writing tests will usually catch some bugs. I'm not arguing for the tests to cover each possible scenario, but they must exist. If, on the other hand, it is hard to add the tests then it probably means you have bigger problems.

ilovecaching 5y ago

This doesn't seem correct to me. Each of these categories should bear equal weight, I won't spend less time on code style or tests to spend more time on API design. My code review checklist is a checklist. I spend as much time as necessary on each item in the checklist, no more no less. I also think that code style, fuzzing, and benchmark tests are the most important items on my checklist. It keeps the code base from becoming a jenga tower that is brittle to change. It also keeps us honest as to whether a feature is a perf hit or not. Tests are far more important than this graphic portrays them as.

kolanos 5y ago
Code style is the first thing I eliminate from a review process. I find the most commonly used code formatter and linter for $LANG (the more opinionated the better) and plug it into pre-commits and CI. Code style is just not worth my time.
- simonbarker87 5y ago
  I’m at the point now where I don’t want to think about layout and formatting of my code, just let a formatter do it all for me on save please
  - tuyiown 5y ago
    But, code formatting is not a solved problem, right ? You either have to fiddle until the formater outputs proper layout, or let it rest as is and never learn how to format your code, letting the code base leaking tons of probable less than ideal layout.
    - RussianCow 5y ago
      I configure Prettier once on my JS codebases and then never touch it again. Seems about as "solved" as is reasonably possible.
      - tuyiown 5y ago
        http://journal.stuffwithstuff.com/2015/09/08/the-hardest-program-ive-ever-written/ http://journal.stuffwithstuff.com/2015/09/08/the-hardest-pro...
        RussianCow 5y ago
        I'm not sure how that's relevant. The fact that it's difficult doesn't make it unsolved.
    - kolanos 5y ago
      I'll take a consistent less than ideal layout over whatever $HUMAN comes up with. But to me an inconsistent layout is the less than ideal one. And humans excel at inconsistency.
      - tuyiown 5y ago

simonbarker87 5y ago

I like the attempt at making this process smoother, I guess where to place importance of different aspects relative to each other is always down to opinion and interpretation. I wrote this piece a while ago focussing on using the Must/Should/Could principle to speed up the review process and put a less opinionated framework in place to help keep code review moving. https://careerswitchtocoding.com/blog/moscow-the-best-code-review-technique-youre-not-using https://careerswitchtocoding.com/blog/moscow-the-best-code-r... It’s not an original thought from me but many people hadn’t heard about it and off the back of this post I’ve had a few people get in touch to say they’ve implemented it in their team and it’s helped improve code review speed.

mannykannot 5y ago

While I recognize the problem as real and significant, and I think a hierarchy of concerns is a valid way to mitigate it, I feel there are a few problems in this particular pyramid. Of all the issues that might be raised in a review, which are the most important to fix? I find it difficult to imagine a ranking in which incorrect functioning (including security vulnerabilities and truly inadequate performance) are not at the top (answering the ranking question with "all of them" would just be a way to avoid the issue.) In this pyramid, issues of this nature are to be found at all levels except the top one, mixed in with more-or-less subjective ones, such as "Is a new API generally useful and not overly specific?" - pointless flame wars have erupted over issues such as this (also, orthogonally, code review is late in the game to be asking this one.) For a review to be successful, its participants need to be able to restrain themselves from going down rabbit holes that could not lead to necessary rework - or a strong moderator, if the individual participants cannot act with adequate self-restraint.

th3iedkid 5y ago
Another aspect, the pyramid makes code review look like the only thing around the actual value outcome. There’s a lot of other aspects like pair-programming that eliminates most things in the pyramid, taking it out of scope from a review process
pcmoney 5y ago
If your team does not share a subjective understanding of what makes their product/API useful none of this matters. Focusing on objective technicalities is the least useful aspect of code review. If you have someone who constantly is like "Well that's subjective" and derails conversation that way you need to coach them or move them off the team.
- mannykannot 5y ago
  Are you saying that if you enter a code review without having a shared subjective understanding of what makes your product/API useful then the review is not likely to be successful, or that code review is the place to form such a shared subjective understanding? I can agree to the first interpretation, but about the latter I would say that code review is quite a bit later than optimal, and that there are other, better ways of achieving it. If your organization is such that you cannot even discuss these issues until you have working code, then you have other, bigger, problems.

mpalczewski 5y ago

First thing I review is readability. Code should be readable. Once code is readable it makes reviewing the rest much easier. Readable code is more maintainable and problems jump out at you. Stuff other than readability is important, but if you focus on readability it makes the rest of the review go very smoothly.

nine_zeros 5y ago
If the code is doing the wrong thing in the first place, no amount of nitpicking on readability will save time. First pass: Functional correctness Second pass: Better ways to do the same thing (optional) Third pass: your favorite nitpicks.
- pc86 5y ago
  This structure (especially the optionality of the second pass) is how you end up with perfectly readable code iterating over List<T> when a hash map would suffice and be orders of magnitude more performant. Nobody can do it perfectly, but I think trying to keep personal nitpicks out of reviews as much as possible - ideally by codifying team nitpicks in automated formatting/.prettierrc/whatever - lets people focus on things that matter: what the code does and how it does it.
  - watwut 5y ago
    Then again, that one is super easy to find and fix if it turns out to actually be problem.
  - TheCoelacanth 5y ago
    In perfectly readable code, it's easy to notice performance problems. In my experience, unreadable code is usually where the worst performance problems are, because that's where they are the hardest to find and fix. You seem to be conflating readability with formatting, which is not what I mean by readability. Poor and inconsistent formatting adds some friction to reading, but it's a relatively minor part of readability.
- 5y ago

Quarrelsome 5y ago

aren't we missing something at the base of the pyramid: > does it actually work? I feel like the foundation of any code review has to be a bug analysis to ensure that the feature works, doesn't create regressions or fail in another supported use-case.

pc86 5y ago
Potentially unpopular opinion, but I would much rather get well-formatted, performant, readable code that misses the business case a bit than get a ball of spaghetti mess that nails it. Not only will it be easier to update the code in the former, but the business pressure to just deploy the latter and "fix it later" can sometimes be too high to resist. I was just looking at a "proof of concept" with a 2017 commit date in production last week, the danger of that is all too real.
- Quarrelsome 5y ago
  No, I mean like the primary function of every PR is to ensure the code does what its trying to do and doesn't (for example) fail to compile, or break the build or create an infinite loop or an out of memory exception.
  - digisign 5y ago
    Arguably too fundamental to mention, at least in one slide. Perhaps covered during the developer-testing period, before peer review.
    - deleted 5y ago
      [deleted]
  - b3morales 5y ago
    On a dayjob closed-team project, if my coworkers are putting up PRs without making sure they meet functional requirements, that is a problem that needs to be solved several steps before the PR. If it's a junior person who doesn't know better, more working side-by-side with them to get to the PR. If it's a senior person dealing with a shitty spec, more discussion with PMs/designers before we start coding. If it's a senior person who's putting up broken code because they don't care...that's probably a management conversation. I have to be able to trust that my teammates are doing their best to produce working components. Otherwise code review is a bandaid covering a hole in the hull of a sinking ship.

willcipriano 5y ago

The more experienced I get the more I want to work on teams with a higher degree of trust than ones like this. Don't get me wrong I think code review can be valuable but on high preforming teams it works best on a as needed basis. The way this has worked successfully for me in the past is as a new joiner to a team you submit for code review every time. My first couple of reviews tend to have a good bit of feedback as I settle in to the style of the team and often a new language or framework. After a while though I become a domain expert for the projects I am working on, and code reviews end up becoming a rubber stamp. At that point it's just something slowing me down. I'd request review when I need it, or I'm making a big change that will effect everyone, otherwise there are many cases where there is really only one way to skin this cat and it's totally perfunctory. This sort of arrangement also requires a bit of ownership, if you see reports floating around about an endpoint you just worked on having issues you have to be able to be trusted to jump on it. I feel like trust and ownership are touchy feely people problems so tech people invented code reviews and linters to try to make up for not cultivating the sort of culture where they aren't required on every single little thing.

edgyquant 5y ago
On my team we have one other engineer, no matter your or their official position, just do a quick glance over for obvious issues. This does not prevent real bugs from getting merged in, the test cases are supposedly for that but even those have some slippage.
Cerium 5y ago
I work on a team that runs following a high trust and domain ownership model. When I joined our process was "write some code, test it, be confident in the code, commit it to trunk". A number of years ago we worked our way through "Clean Code" in a reading group and decided to try out code reviews. Each week one of us would present our recent work and the team would tear it apart line by line, and in the process we developed a better understanding of our group philosophy around style and practices. When we switched to Git we decided that we liked code reviews and wanted to make them part of merging to develop. Depending on the content the reviews are deeper or more superficial. Many times even when I review the code of somebody making changes in their own subject matter domain I have found bugs that simply needed another set of eyes to catch. I like code reviews and in the context I use them find that they are a complement to trust, not a substitute.

neebz 5y ago

previous discussion: https://news.ycombinator.com/item?id=30674159 https://news.ycombinator.com/item?id=30674159

AngeloR 5y ago

I've been thinking about code reviews a lot.. and I think a big problem with existing code reviews is that for most orgs, code reviews were a way to move planning to the end of the development process instead of keeping it up-front. I think the foundation of the pyramid (API and Implementation semantics) is often discussed in code reviews when it should have almost no place. By the time you get to the code review not only should these have been ironed out and shared with the whole team. What you should be looking at is simply adherence to the plan, and calling out deviations. I wrote about a small rat about this: https://xangelo.ca/posts/code-reviews-are-failure/ https://xangelo.ca/posts/code-reviews-are-failure/

gunnarmorling 5y ago
> is often discussed in code reviews when it should have almost no place. By the time you get to the code review not only should these have been ironed out and shared with the whole team. That's a fair point. It's my background in open-source and I suppose the specific ways we work in the projects I'm involved with which made me add these things at the bottom. Oftentimes, contributors come up with PRs for new features, and there was no prior discussion about API design etc. (although we highly recommend to have such discussion before starting with the work on a large PR). I.e. oftentimes a PR review will be the first time I see how something is implemented, how the API looks like etc. It may be different with more controlled settings in in-house development teams adhering to more formally defined processes.
- AngeloR 5y ago
  This is 100% a side-effect of GitHub. Open Source used to have the additional barrier of mailing lists to detract from "drive-by PRs". The side effect, of course, is that it made it much harder to get involved. GitHub really brought a lot of new eyes on open source software and lowered the barrier to contribute exacerbating (an argument could be made for creating) the drive-by PR problem.

VBprogrammer 5y ago

I've noticed a phenomenon where I sometimes have to comment about something near the pointy end of the code review pyramid in order to get my brain into the correct mindset to really start grocking how and why someone has made the changes they have. After I break this barrier it often leads to the deeper questions about whether it is doing what it supposed to be doing.

owlbite 5y ago

Completely different to the order I use, which basically amounts to "how hard is it to fix this later". So most important questions are: (1) API must be as correct as possible, especially if its exposed outside of the immediate team. Changing APIs is a royal pain. (2) Documentation of what's going on, especially for non-obvious stuff. If we come back to this in 3 years time and the author has moved on, do we have any hope of fixing it? (3) Testing. What degree of confidence do we have this is doing the right thing? Will it protect me when I change it in a few years time? Any minor bugs or code style issues can be fixed later as/when they are found and cause problems.

greymalik 5y ago
I think you might be misreading the pyramid? The most important things are at the bottom. It aligns exactly with what you’re saying.
- gunnarmorling 5y ago
  Exactly that. "how hard is it to fix this later" is what drives the ordering in the pyramid.

makecheck 5y ago

An important one often overlooked (due to our tendency to focus on just the “diff”): what else should be changing or could be affected? Very easy to forget to change code that isn’t highlighted in the review.

nhoughto 5y ago

I'll use this I think, very handy. I'd probably emphasis all the text/descriptions more too, lots of important nuggets in there that aren't obvious when glancing, like automating the top portions of the pyramid, humans shouldn't be mis-formatting code and others human comment on it anymore.

BrissyCoder 5y ago

https://en.wikipedia.org/wiki/Law_of_triviality https://en.wikipedia.org/wiki/Law_of_triviality When I was on a larger team that actively participated in sprint demos was regularly a witness to this. Senior types that came along and needed to be heard - not really having a deep understanding of the application - would pick at trivial consistency issues in the UI or whatever. Such eye-rolling behaviour and hard to believe they didn't realise how transparent it was. Probably have been guilty of it myself tbh.

majormajor 5y ago

Across 6 jobs now I've never seen code reviews focus primarily on formatting or style over functionality or future-looking concerns. How common is this, really? At the last couple places formatting has been handled by auto-linters; before that, it was hit or miss, but never a dominant topic of argument.

sanderjd 5y ago
Seems essentially non existent to me, outside blog posts.

deschutes 5y ago

I find reviews work best when you place a strong bias towards shipping workable if imperfect code. Quick reviews are also a strongly positive thing. On the other end are fixed processes. Have as little of this as possible, automate it if you can. That means formatting, linters and shallow static analysis. All of this is mainly in service of building trust. If you cant have some measure of success there you are truly doomed.

smokey_circles 5y ago

One thing I wish I knew was coming was what my first code review as the tech lead was gonna be like. I was as unprepared as the grads code was. Things I've learned about my own professional code preferences since 1) I will not sacrifice readability for any reason. That is the hill I'm willing to die on. Reducing code is sometimes hard and that's fine that people like to challenge themselves by doing so, but not at the expense of readability. You will forget the clever trick you employed, I promise 2) Business objectives are second class to system stability. This is because the primary business objective is to be online. Failing a few transaction is bad. Failing all the transactions is much worse. 3) There is no such thing as self documenting code. There is just good nomenclature. Documents always improve someone's experience. On boarding documents for new devs will save you so much trouble. 4) I'm a huge fan of in memory caches. I hate seeing them in code. When they work, they're wonderful. But when there are issues, they're closer to demons than to bugs. 5) Data Structures. Data Structures, Data Structures, Data Structures. And then more Data Structures. Easy pre-optimisation, and these days it usually just means knowing when not to use "the default" data structure (such as ArrayList in java) 7) One day (if you're not there already), you will be on the otherside of this review. You're almost guaranteed to remember the bad times, so pay extra special attention to when the process is working well. A simple trick I've learned is to repeat a process that worked from a previous role at a new role who's process is not working. Works more times than not.

eorn3452gl 5y ago
> "knowing when not to use "the default" data structure (such as ArrayList in java)" Can you give some examples of when not to use ArrayList?

dhzhzjsbevs 5y ago

> When it comes to code reviews, it’s a common phenomenon that there is much focus and long-winded discussions around mundane aspects like code formatting and style, whereas important aspects (does the code change do what it is supposed to do, is it performant, is it backwards-compatible for existing clients, and many others) tend to get less attention. Code formatting and style discussions that don't start and end with, hey, our code formatter is broken have absolutely no place in code review. Also, keep your damn nits to yourself. If it's a nit, it's not important.