The Code Review Pyramid(morling.dev)
morling.dev
The Code Review Pyramid
https://www.morling.dev/blog/the-code-review-pyramid/
116 comments
The popularity this sentiment on HN terrifies me.
Please don't do this. I keep finding rubber stamp code reviews (or none at all) while diving into codebases for where bugs were introduced, and 9 times out of 10, it's someone who didn't bother to get proper reviews for their code because "it's just something slowing me down". I can assure you that you have bugs and usability issues in your code, not just "linter issues". Everyone does.
If your code reviews are becoming rubber stamps, that's an indication that you need to slow down and teach other team members about the system, or hire smarter people who can understand your reviews more quickly. Don't respond by getting rid of the review process as a knee-jerk response to these sorts of organizational constraints.
Please don't do this. I keep finding rubber stamp code reviews (or none at all) while diving into codebases for where bugs were introduced, and 9 times out of 10, it's someone who didn't bother to get proper reviews for their code because "it's just something slowing me down". I can assure you that you have bugs and usability issues in your code, not just "linter issues". Everyone does.
If your code reviews are becoming rubber stamps, that's an indication that you need to slow down and teach other team members about the system, or hire smarter people who can understand your reviews more quickly. Don't respond by getting rid of the review process as a knee-jerk response to these sorts of organizational constraints.
I disagree.
If you have a practice that isn't effective (e.g. code reviews that aren't preventing bugs), doubling-down is almost never the correct solution.
Code reviews are like unit tests; they're helpful under the right conditions. Mostly on the tricky bits or when a dev is working in an unfamiliar area.
It's a common fallacy for folks to believe that if a little of something is good, then a lot of it must be better. I've been at places with 100% code test coverage or requiring EVERY SINGLE PULL REQUEST to be reviewed. It's obnoxious, and it doesn't do shit to prevent bugs. Instead it causes folks to burn out on the practice and write crap unit tests or stamp "LGTM" on every PR they're forced to evaluate. People have a limited capacity for cognitive engagement and wasting that on piddly crap means they can't spend it on the important bits.
If you have a practice that isn't effective (e.g. code reviews that aren't preventing bugs), doubling-down is almost never the correct solution.
Code reviews are like unit tests; they're helpful under the right conditions. Mostly on the tricky bits or when a dev is working in an unfamiliar area.
It's a common fallacy for folks to believe that if a little of something is good, then a lot of it must be better. I've been at places with 100% code test coverage or requiring EVERY SINGLE PULL REQUEST to be reviewed. It's obnoxious, and it doesn't do shit to prevent bugs. Instead it causes folks to burn out on the practice and write crap unit tests or stamp "LGTM" on every PR they're forced to evaluate. People have a limited capacity for cognitive engagement and wasting that on piddly crap means they can't spend it on the important bits.
Code reviews are not meant to prevent bugs. They are for making sure an individual's effort is good enough for the team to assume further maintenance of that piece of code. That almost never means that the code must be bug-free, it means that other people than the author can follow the thought processes expressed in the code. If that leads to other people finding shallow bugs, more power to them. But that doesn't mean that bug prevention is (or should be) the primary goal of code reviews.
First and foremost, code reviews are for enforcing team standards, such as testing coverage, documentation, and adhering to scope. "Our code reviews aren't preventing bugs" is not a useful metric in that sense. If you're relying on code reviews to prevent bugs, you have other problems in your development process. If I spot a bug during code review, my first question would be "why wasn't this bug found by the automated test suite?"
First and foremost, code reviews are for enforcing team standards, such as testing coverage, documentation, and adhering to scope. "Our code reviews aren't preventing bugs" is not a useful metric in that sense. If you're relying on code reviews to prevent bugs, you have other problems in your development process. If I spot a bug during code review, my first question would be "why wasn't this bug found by the automated test suite?"
> But that doesn't mean that bug prevention is (or should be) the primary goal of code reviews.
Totally agree and well said. In my experience the mentality that code review should catch bugs puts unnecessary pressure on the reviewer which leads to people not wanting to review code, because what if they missed a bug.
Totally agree and well said. In my experience the mentality that code review should catch bugs puts unnecessary pressure on the reviewer which leads to people not wanting to review code, because what if they missed a bug.
If your code reviews aren't catching bugs, the correct answer is to fix the code reviews, not to stop doing them.
I mean, it's possible that code reviews are worthless. But enough people have found value in them, across enough organizations, enough code styles, and enough decades, that it seems really hard to conclude that. It seems more probable that the team that finds them useless is doing them wrong. (Yeah, I know, No True Scotsman. Still, the first response should be "maybe our team is doing them wrong, and can we fix that?", rather than "let's stop doing them".)
I mean, it's possible that code reviews are worthless. But enough people have found value in them, across enough organizations, enough code styles, and enough decades, that it seems really hard to conclude that. It seems more probable that the team that finds them useless is doing them wrong. (Yeah, I know, No True Scotsman. Still, the first response should be "maybe our team is doing them wrong, and can we fix that?", rather than "let's stop doing them".)
I don't disagree with you at all, but I'm not sure I've been a part of an organization that does it right. Beyond the stuff like style, conventions, and obvious bugs/problems, it's challenging to provide meaningful insight to someone that has many more hours of experience in a particular area of a codebase more than you do. I know I personally hold back from calling out things that I find to be subjective -- even if I think I would have done it differently -- simply because I'm often unsure about the things I don't know.
Plus as a peer reviewer, you're often in the same part of the sprint cycle as they are - meaning your mind is already juggling a lot of things about your own code. It's tough to "come up for air" and then immediately dive into someone else's code changes with enough mental capacity to provide a truly useful review.
It's easy to slip into the rubber stamping mentality when you generally trust the ability of a particular contributor.
Plus as a peer reviewer, you're often in the same part of the sprint cycle as they are - meaning your mind is already juggling a lot of things about your own code. It's tough to "come up for air" and then immediately dive into someone else's code changes with enough mental capacity to provide a truly useful review.
It's easy to slip into the rubber stamping mentality when you generally trust the ability of a particular contributor.
"Beyond the stuff like style, conventions, and obvious bugs/problems ..."
Catching the obvious issues is IMO one of the main benefits of never skipping code reviews. Even trivial changes often have those (at least when I write them) and I've most likely saved many hours of debugging by a reviewer catching stuff like that. But this does require more than simple rubber stamping. I encourage my reviewers to read my code with the assumption that I make errors, basically looking for the bug that's fairly likely to be there.
The other thing I think is easy to do is simply see if you understand the code. If the reviewer struggle to do understand what's going on, chances are that other people will struggle as well. In this case, your lack of knowledge is an asset, the original author is likely to not see how it looks for someone not as well versed with the problem.
The other thing I think is easy to do is simply see if you understand the code. If the reviewer struggle to do understand what's going on, chances are that other people will struggle as well. In this case, your lack of knowledge is an asset, the original author is likely to not see how it looks for someone not as well versed with the problem.
This - last year I broke something because I'd changed a query in a file completely unrelated to the ticket I was working on, and the guy who did the review treated code review as a rubber-stamp - he completely ignored the fact I'd accidentally committed a change to payment processing code in a patch to tighten up company number validation.
Even if code reviewers only made sure the files changed made sense, the reviews would be worth it.
You assume here that people only work on the area of the codebase which they are most familiar with. I think a sign of a healthy team is when people are contributing to other parts of the code and not getting stuck in a rut where each area of code only has one maintainer/gatekeeper.
Also, for less experienced members, reviewing the changes of more experienced people is a great way to learn if you properly sit down and understand the change (why it’s being done, why it’s being done this way) rather than just skimming through. In addition to catching mistakes, this is useful to check that code is actually comprehensible to people other than the author and a good time to request more/better comments if necessary.
Also, for less experienced members, reviewing the changes of more experienced people is a great way to learn if you properly sit down and understand the change (why it’s being done, why it’s being done this way) rather than just skimming through. In addition to catching mistakes, this is useful to check that code is actually comprehensible to people other than the author and a good time to request more/better comments if necessary.
> In addition to catching mistakes, this is useful to check that code is actually comprehensible to people other than the author and a good time to request more/better comments if necessary.
The most convoluted code I ever worked with were in teams that treated code review as secret cow. It happened twice. The best was in teams without code review, but with strong ownership.
The issue is, I think, that code review is myoptic. It ensures surface level readability and does very little for actual structure. And I think it gave people false confidence and made them less willing to refactor.
The most convoluted code I ever worked with were in teams that treated code review as secret cow. It happened twice. The best was in teams without code review, but with strong ownership.
The issue is, I think, that code review is myoptic. It ensures surface level readability and does very little for actual structure. And I think it gave people false confidence and made them less willing to refactor.
> stuck in a rut where each area of code only has one maintainer/gatekeeper.
Why do you assume it's a rut? It might be an organizational liability but it can be a great while it's working. It's common folklore that design by committee leads to poor outcomes, or at best average outcomes completed slowly.
Why assume that we need to build code by consensus or a series of interchangeable cogs?
Why do you assume it's a rut? It might be an organizational liability but it can be a great while it's working. It's common folklore that design by committee leads to poor outcomes, or at best average outcomes completed slowly.
Why assume that we need to build code by consensus or a series of interchangeable cogs?
I’m not assuming that at all. When you’re at a company that has $Xmm lines of code floating around in its repositories, it’s unrealistic to assume you will be able to cross functionally train all developers on all code. Of course you try to aim for that, but you’ll inevitably find yourself in situations where you don’t.
Not all developers on all code, but N developers on any given piece of code, where N should be greater than 1. Why were you asked to do the review? Is N equal to 1 so there is nobody besides the author who has the expertise needed to do the review? That's a good opportunity to start the process of raising N to 2. Is it because N is equal to 2, but the other person is out on vacation? Better start working on getting N to 3 then!
[deleted]
You should ask about the stuff you don't understand or aren't sure about. Code review is a communication tool. If you're unsure about how something works, someone else is going to be unsure about that later. It probably needs to be clarified in code or documentation.
This is where I've found the most value. It's so much more effective to communicate about implementation after the developer has taken a first stab at the problem.
[deleted]
"a lot of organizations do X" doesn't seem like a great reason to do X. A lot of organizations use microservices.
Every place I've worked has required reviews on pull requests. Are there any notable tech companies that don't?
My colleagues sometimes find non-trivial errors in my code, and I find errors in theirs. Maybe we're just not as smart as the people you work with.
My colleagues sometimes find non-trivial errors in my code, and I find errors in theirs. Maybe we're just not as smart as the people you work with.
I don't think this is doubling down so much as singling down.
The problem with unit tests and code reviews is that they ultimately aren't sexy. There's no way to do them that destroys the integrity of the system. Especially when a large percentage of the code written is passable. Yes, reviewing and testing code that works and is bug free is a bit of a waste of time. But you won't know what code isn't working and bug free unless you test and review.
And then there's the fact that most of what we do is not that important. If my code is buggy or inefficient or a third thing, the worst thing that happens is a few people frown while I get it sorted. If you knew that if your code had a bug in it, it would definitely kill 5 - 10 people every time it is run, you'd treat review and testing with the utmost importance because the cost of committing buggy code is way too high.
Testing and review becomes exactly as important as your product.
The problem with unit tests and code reviews is that they ultimately aren't sexy. There's no way to do them that destroys the integrity of the system. Especially when a large percentage of the code written is passable. Yes, reviewing and testing code that works and is bug free is a bit of a waste of time. But you won't know what code isn't working and bug free unless you test and review.
And then there's the fact that most of what we do is not that important. If my code is buggy or inefficient or a third thing, the worst thing that happens is a few people frown while I get it sorted. If you knew that if your code had a bug in it, it would definitely kill 5 - 10 people every time it is run, you'd treat review and testing with the utmost importance because the cost of committing buggy code is way too high.
Testing and review becomes exactly as important as your product.
Been on both camps of this argument. Now tend to favor that responsible seniors should be allowed a fast lane. It depends very much on your team dynamics. Seniors can be very cowboy so some of them actually need even more review than others.
Have been in some teams with very high junior/senior ratio where a junior would need a whole day to review a small thing a senior implemented in an hour, often because understanding the context leads to hundred rabbit holes of more new information, not the small diff itself. Add the one week that PR was waiting for someone to even notice it needed review and you realize the frustration. Vice versa a junior would take a week to implement something a senior can review in 15mins.
Do the math and you realize the senior will quickly get blocked.
Slowing down and teaching others in that situation is a as you say the right thing to do. But that also saturates after a while. You can only pair program x number of hours per day before getting exhausted, especially for a beginner this can feel like a lot of pressure to ingest so much knowledge at that pace. They need to slow down and get in their own zone as well. In the meantime, backlog is on fire with tasks that need to be released yesterday.
Hire smarter people? Yes please, would love to do that, where do I call? “Smarter” doesn’t always help either, the reality is that you often end up with domain experts within some areas of the product, even if you try to keep everyone on board.
Have been in some teams with very high junior/senior ratio where a junior would need a whole day to review a small thing a senior implemented in an hour, often because understanding the context leads to hundred rabbit holes of more new information, not the small diff itself. Add the one week that PR was waiting for someone to even notice it needed review and you realize the frustration. Vice versa a junior would take a week to implement something a senior can review in 15mins.
Do the math and you realize the senior will quickly get blocked.
Slowing down and teaching others in that situation is a as you say the right thing to do. But that also saturates after a while. You can only pair program x number of hours per day before getting exhausted, especially for a beginner this can feel like a lot of pressure to ingest so much knowledge at that pace. They need to slow down and get in their own zone as well. In the meantime, backlog is on fire with tasks that need to be released yesterday.
Hire smarter people? Yes please, would love to do that, where do I call? “Smarter” doesn’t always help either, the reality is that you often end up with domain experts within some areas of the product, even if you try to keep everyone on board.
I have some sad news for you. Code reviews don't eliminate bugs.
Some times, they actually hinder the process of fixing bugs.
Some times, they actually hinder the process of fixing bugs.
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.
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.
I feel I work on a high performance team and code reviews are a joy. When I submit my code for review it's an opportunity to share my work with my peers, potentially explain my thought process and my work process and we get to learn from one another, and when I get code to review it's the opposite, I get to see how my coworkers think and tackle problems.
I have never once felt like a code review did anything to erode trust and I don't see how a code review could slow down a work process. Using git, as soon as I submit code for review I begin work on the next task and if I need anything from the branch I submitted for review, I just merge that into the new branch. If changes are requested I make the changes and then merge again.
Code reviews are among the best ways of ensuring that code remains consistent and that everyone is learning from everyone else. That's why in my team all pull requests are reviewed by two people including one junior developer.
I have never once felt like a code review did anything to erode trust and I don't see how a code review could slow down a work process. Using git, as soon as I submit code for review I begin work on the next task and if I need anything from the branch I submitted for review, I just merge that into the new branch. If changes are requested I make the changes and then merge again.
Code reviews are among the best ways of ensuring that code remains consistent and that everyone is learning from everyone else. That's why in my team all pull requests are reviewed by two people including one junior developer.
On my team, the level of scrutiny a merge request receives depends on the author and the functionality. MRs from a principal or lead engineer tend to get rubber-stamped at a higher rate, though, to your point, many of us will explicitly call out code that we would like to have reviewed in more detail. If a more junior level developer submits a merge request, it is highly likely their code will get a close review.
I'm not sure I agree with your premise that code reviews are the product of a culture with a lack of trust and ownership. I think, in fact, submitting to a review process is a sign of ownership -- it's saying "hey, the quality of the code we produce as a team is important to me, and I know I make mistakes."
I'm not sure I agree with your premise that code reviews are the product of a culture with a lack of trust and ownership. I think, in fact, submitting to a review process is a sign of ownership -- it's saying "hey, the quality of the code we produce as a team is important to me, and I know I make mistakes."
I‘m called a principal engineer in my organization (I know that can mean everything or nothing) I like to use code review requests and sometimes gates also to put a fence in front of my ego. Also this is a great way of sharing knowledge. I had great sessions where questions about X and then answers verbally or in text suddenly put stuff into different perspective. Sometimes I realize that my solution might be to complicated or an more generic, performan, (paste fancy adjective to describe your code here) is easy to achieve.
I trust the people whose code I review to be competent developers. But everyone occasionally makes typos, misreads requirements, has blind spots, etc. Even if the creator is the expert and the reviewer is there to catch only obvious mistakes, that still has value.
> But everyone occasionally makes typos, misreads requirements, has blind spots, etc.
Case in point: OP used the word "effect" when he should have used "affect".
Just like pudding, there's always room for a code review.
Case in point: OP used the word "effect" when he should have used "affect".
Just like pudding, there's always room for a code review.
This is scary to read. We use code reviews for changes and we’ve found that our more junior team members can bring a point of view our more senior team members can’t and vice versa. It’s one of the best parts of working in code for me.
With that said, code reviews only work, in my experience, if team members are strong communicators. Understanding each other, empathizing with issues, and being collaborative rather than tearing people down is critical.
Code reviews are a “we” activity not a “your” and “my” activity.
With that said, code reviews only work, in my experience, if team members are strong communicators. Understanding each other, empathizing with issues, and being collaborative rather than tearing people down is critical.
Code reviews are a “we” activity not a “your” and “my” activity.
Yes on trust. However… I'm pretty darn experienced in what I do these days, and every day I still write a bit of crappy code, usually as I come to an understanding over time of the current problem. WTFs come from code written six months ago as well as yesterday. If someone can point out an issue early, it saves everyone time. So, I welcome it.
This used to be common sense in the few companies I worked at around 2012.
People just pushed code. We occasionally held some review sessions to go over code and exchange comments / ideas about what might be wrong with this or that code and how to prevent this kind of mistake in the future.
But starting from around 2016, I could never ever find a company that does not have mandetory code reviews for every single commit, and this bothers me tremendously.
It's one of the reasons I don't want to work at any programming company anymore.
The really good programmers I know don't look positively at code reviews.
If you need code reviews, ask for it. If not having mandetory reviews results in a huge stream of bugs, maybe you should hire better programmers instead of cheap ones.
People just pushed code. We occasionally held some review sessions to go over code and exchange comments / ideas about what might be wrong with this or that code and how to prevent this kind of mistake in the future.
But starting from around 2016, I could never ever find a company that does not have mandetory code reviews for every single commit, and this bothers me tremendously.
It's one of the reasons I don't want to work at any programming company anymore.
The really good programmers I know don't look positively at code reviews.
If you need code reviews, ask for it. If not having mandetory reviews results in a huge stream of bugs, maybe you should hire better programmers instead of cheap ones.
Where do we find all of those better programmers?
There's a flip between when the feedback is super, super helpful (consensus building, as you say), to a moderate inconvenience to screwing up your estimates because you seem to always post a PR right after the reviewers have started some new thing.
Then farther down that continuum someone is busting your chops for some stupid reason that, after you get back from lunch or coffee, you realize is completely valid (or even gives you an idea for an enhancement) and okay you change how a conditional works or add some more tests.
It makes me wonder if in a corporate setting PRs would be better treated as pseudorandom audits, based on a random factor and a weight for automated linting (so people don't learn to game the linter, which can be some of the worst code you ever see.)
A related issue is that most tools make it pretty difficult to look at and interact with old PRs. Feedback loops have some slop, and while it's better that you get the feedback as you are writing the code, fast follow often works as well, and I think we'd be better as a group if you could request revisions on a recently landed PR after the fact. This is mostly okay but you really need to fix your variable names, conditional block, and you missed a boundary condition over here that might not break today, but there are already stories in the backlog that will break it for sure.
Then farther down that continuum someone is busting your chops for some stupid reason that, after you get back from lunch or coffee, you realize is completely valid (or even gives you an idea for an enhancement) and okay you change how a conditional works or add some more tests.
It makes me wonder if in a corporate setting PRs would be better treated as pseudorandom audits, based on a random factor and a weight for automated linting (so people don't learn to game the linter, which can be some of the worst code you ever see.)
A related issue is that most tools make it pretty difficult to look at and interact with old PRs. Feedback loops have some slop, and while it's better that you get the feedback as you are writing the code, fast follow often works as well, and I think we'd be better as a group if you could request revisions on a recently landed PR after the fact. This is mostly okay but you really need to fix your variable names, conditional block, and you missed a boundary condition over here that might not break today, but there are already stories in the backlog that will break it for sure.
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.
I see 2 clear problems with this ideology:
1) There is too much knowledge to fit inside one person's head. Cooperation is the keystone of success.
2) Your team will grow. You will need to share this knowledge somehow before the trust can be given
But in defense of this view point: code reviews shouldn't be the opposite of what you're talking about.
I trust my teammates to catch any mistakes I've made. I'm human and therefore fallible. It's going to happen.
I trust mistakes they've made are for the same reasons.
What you're trusting is your teammates are top tier 1% every time and bordering on being super human.
I'm trusting that my team has my back and we're in it together.
1) There is too much knowledge to fit inside one person's head. Cooperation is the keystone of success.
2) Your team will grow. You will need to share this knowledge somehow before the trust can be given
But in defense of this view point: code reviews shouldn't be the opposite of what you're talking about.
I trust my teammates to catch any mistakes I've made. I'm human and therefore fallible. It's going to happen.
I trust mistakes they've made are for the same reasons.
What you're trusting is your teammates are top tier 1% every time and bordering on being super human.
I'm trusting that my team has my back and we're in it together.
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.
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.
I think you might be misreading the pyramid? The most important things are at the bottom. It aligns exactly with what you’re saying.
Exactly that. "how hard is it to fix this later" is what drives the ordering in the pyramid.
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.
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.
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
I think you are getting too into the weeds with these complaints. This hierarchy is pretty good. Performance and security are important, but they aren't always a full stop, where a change the breaks the API should always be a full stop. Each higher level is less likely to be a hard stop, and the border between levels is necessarily somewhat grey.
I'm not so concerned with the broad levels, but the specific items within them. So, for example, "is a new API generally useful and not overly specific?" is way too broad a question to elicit only breaking errors in the API, and is an invitation to exactly the sort of loss of focus that the author rightly deprecates.
WRT performance, I specifically wrote "truly inadequate" in an attempt to distinguish between breaking and merely undesirably bad performance, but that is a somewhat vague line - unavoidably, I think.
Off the top of my head, the only sort of security 'problem' that can safely be put aside would be one that cannot lead to serious or irreversible harm, or requires the organization to have already been terminally compromised.
That I have to make these distinctions is, in fact, the basis of my concerns. They follow from two premises: firstly, the purpose of an inspection is to find problems that need to be fixed, and secondly, whether a problem falls into that category is orthogonal to the questions of what functional, structural or operational categories it falls in.
WRT performance, I specifically wrote "truly inadequate" in an attempt to distinguish between breaking and merely undesirably bad performance, but that is a somewhat vague line - unavoidably, I think.
Off the top of my head, the only sort of security 'problem' that can safely be put aside would be one that cannot lead to serious or irreversible harm, or requires the organization to have already been terminally compromised.
That I have to make these distinctions is, in fact, the basis of my concerns. They follow from two premises: firstly, the purpose of an inspection is to find problems that need to be fixed, and secondly, whether a problem falls into that category is orthogonal to the questions of what functional, structural or operational categories it falls in.
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.
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.
If your organization is such that you cannot even discuss these issues until you have working code, then you have other, bigger, problems.
The former, ideally the subjective understanding should be shared before the story/ticket is written and definitely before it is started. Code review is way too late.
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/
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/
> 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.
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.
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.
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.
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.
unfortunately, 'readability' is fairly subjective. I was part of a group recently that spent a good couple hours every week arguing whether 'ctxt' or 'context' was more readable.
when confronted, they explained to me as one would a child, that they cared deeply about code quality
when confronted, they explained to me as one would a child, that they cared deeply about code quality
[deleted]
> when confronted, they explained to me as one would a child, that they cared deeply about code quality
these people sound useless.
Readability as it applies to code is how easy it is to read the code and understand it in relation to how complicated the problem domain is.
these people sound useless.
Readability as it applies to code is how easy it is to read the code and understand it in relation to how complicated the problem domain is.
The best choice is whichever is more consistent with the surrounding codebase. For an outsider to the codebase, you could argue for 'context' being better, But to someone familiar to the codebase, if contexts are always in a variable 'ctxt', then 'ctxt' will instantly parse correctly without a hitch since it'll be interpreted as a single symbol.
There's a simple test for readability. Someone familiar with the codebase reads the code and tries to understand what it does (a.k.a. a code review).
If it takes a low effort, then it's high readability. If it takes a high effort, then it's low readability.
If it takes a low effort, then it's high readability. If it takes a high effort, then it's low readability.
[deleted]
+1, I also focus first on readability while reviewing code.
Software engineering is a collaborative process. I don't write code for the machine; I write it for my colleagues and my future self to read. Code we write 6 months ago looks like someone else code. Readability is important unless you're convinced that it's prototype/throwaway code.
After readability then I'd look for tests. It's a theme: good tests are also meant to be read as documentation. Beside ensuring correctness, it's also the example my colleagues or future self can refer for the module usage.
Software engineering is a collaborative process. I don't write code for the machine; I write it for my colleagues and my future self to read. Code we write 6 months ago looks like someone else code. Readability is important unless you're convinced that it's prototype/throwaway code.
After readability then I'd look for tests. It's a theme: good tests are also meant to be read as documentation. Beside ensuring correctness, it's also the example my colleagues or future self can refer for the module usage.
This is where I have found review to be really tricky. I have reviewed so much unreadable code that I lose faith sometimes. More to the point, readability issues are often perceived as nitpicks. Eventually after a few rounds of readability issues being addressed I can tell what the actual code is doing. At that point the major suggestions and some rework comes out, but now the author feels burned, nitpicked, and now instead of being done "their perfectly functional" code needs more work and the whole thing is not done. Do this a few times and relationships begin to break down.
Hence, I now try really hard to ignore readability until the end.
I don't know if this partly just hostile developers, different understanding of code. Ie, is code for humans to read, or for machines to execute? Or if it's dealing with feature factory developers that just want to keep adding despite how brittle the code is becoming. I presume the problem is somewhere in between, but to another extent the normalcy of the absurd where it is normal to spend 5 minutes per line of code to understand it, it is normal to havk in features and do manual regression testing, etc..
Hence, I now try really hard to ignore readability until the end.
I don't know if this partly just hostile developers, different understanding of code. Ie, is code for humans to read, or for machines to execute? Or if it's dealing with feature factory developers that just want to keep adding despite how brittle the code is becoming. I presume the problem is somewhere in between, but to another extent the normalcy of the absurd where it is normal to spend 5 minutes per line of code to understand it, it is normal to havk in features and do manual regression testing, etc..
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.
First pass: Functional correctness
Second pass: Better ways to do the same thing (optional)
Third pass: your favorite nitpicks.
I think you're missing the point: it's incredibly hard to determine functional correctness of unreadable code.
So the first priority is getting the change to a readable state. I'm not talking about nits, I'm talking about minimizing the cognitive overhead to truly understand what the code is doing.
That being said, it is also easy to identify nits during this phase. In codebases that have collections of best practices, this often helps to ensure that the change is expressed in terms of a common vocabulary. Once this pass is done, the reviewer can usually better understand the intent of the changes.
So the first priority is getting the change to a readable state. I'm not talking about nits, I'm talking about minimizing the cognitive overhead to truly understand what the code is doing.
That being said, it is also easy to identify nits during this phase. In codebases that have collections of best practices, this often helps to ensure that the change is expressed in terms of a common vocabulary. Once this pass is done, the reviewer can usually better understand the intent of the changes.
> I think you're missing the point: it's incredibly hard to determine functional correctness of unreadable code.
There is a spectrum between style differences and absolutely unreadable code.
If it is absolutely unreadable, sure, send it back to be readable. But if it is merely styling issues, I'd encourage you to understand that you are not saving any time by focusing on styling before functionality.
There is a spectrum between style differences and absolutely unreadable code.
If it is absolutely unreadable, sure, send it back to be readable. But if it is merely styling issues, I'd encourage you to understand that you are not saving any time by focusing on styling before functionality.
> I'd encourage you to understand that you are not saving any time by focusing on styling before functionality.
Far more developer time is spent reading code than writing it.
If you can speed up the time required to understand a piece a code by improving the style then it's almost always worth it. For a professional software engineer, just above "absolutely unreadable" is far too low a bar to aim for.
Far more developer time is spent reading code than writing it.
If you can speed up the time required to understand a piece a code by improving the style then it's almost always worth it. For a professional software engineer, just above "absolutely unreadable" is far too low a bar to aim for.
I am still not convinced that readability comes before functionality.
Imagine a developer building a PR for 2 weeks, then reviews back and forth for 2 more weeks. Now 4 weeks have passed and only now the reviewer reviews the functionality - only to find that the entire implementation is wrong/could be done in a better way.
What a waste of 4 weeks of both the author and the reviewer! This could have been short-circuited very early in the process.
On my team, we default to early feedback on functionality and let the CI enforce what it can. Everything else is debatable.
Imagine a developer building a PR for 2 weeks, then reviews back and forth for 2 more weeks. Now 4 weeks have passed and only now the reviewer reviews the functionality - only to find that the entire implementation is wrong/could be done in a better way.
What a waste of 4 weeks of both the author and the reviewer! This could have been short-circuited very early in the process.
On my team, we default to early feedback on functionality and let the CI enforce what it can. Everything else is debatable.
Trying to understand the functionality of code which is hard to read is also a huge waste of time. You should try to keep the code easy to read even in very early drafts instead of leaving that until the end.
Hard to read code is subjective. I am guessing you are working with professionals and not high school programmers. Most of the code I see from professional programmers is passable enough for me to
1. Review functionality and then
2. Point out something about coding style.
In fact, if the functionality is good, I even approve the PR leaving a lot of code styling comments that the author can fix at their own leisure.
1. Review functionality and then
2. Point out something about coding style.
In fact, if the functionality is good, I even approve the PR leaving a lot of code styling comments that the author can fix at their own leisure.
Readability has very little to do with styling.
It's about clearly and concisely expressing ideas.
Leave styling to linters. Reviewers should be focusing on how the code is communicating.
It's about clearly and concisely expressing ideas.
Leave styling to linters. Reviewers should be focusing on how the code is communicating.
It's assumed in your post that readability review is nitpicking. It's actually about making the code idiomatic, consistent, predictable, quickly understandable, and low mental stress while reading. The "nitpicking" parts you referred mostly is about ensuring code consistency and predictability. But there are more to readability than that:
1) did the code use a third party library while a standard lib is sufficient? Is there a way to use native language features (e.g. list comprehension) to make the code more idiomatic to people familiar with the language?
2) are the namings make sense in the business context? Can a new hire read just the public interface and be able to guess the usage?
3) do people have to jump around, full-text-search the code base to understand what's going on? Is automatic dependencies injection being overused? How many things people need to keep in their head to be able to follow this code?
It's all readability there, and I'd argue it's not only about style or formatting.
1) did the code use a third party library while a standard lib is sufficient? Is there a way to use native language features (e.g. list comprehension) to make the code more idiomatic to people familiar with the language?
2) are the namings make sense in the business context? Can a new hire read just the public interface and be able to guess the usage?
3) do people have to jump around, full-text-search the code base to understand what's going on? Is automatic dependencies injection being overused? How many things people need to keep in their head to be able to follow this code?
It's all readability there, and I'd argue it's not only about style or formatting.
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.
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.
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.
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.
Then again, that one is super easy to find and fix if it turns out to actually be problem.
I don't know if I agree that the second pass is optional. I've found that pass is the one I've seen have the most benefit, particularly with newer developers. It serves two purposes when done well.
First it gives you an idea of how well thought out the implementation was (i.e. was this a quick hack to just finish a asked for requirement) or was a best design targeted. It also helps newer developers develop a voice. Often, at the start, newer devs will take what a more senior dev says as gospel, but by striking up a conversation and, in some ways, making them defend their choices it can help build confidence and that voice to speak up when something doesn't seem right.
Second, I've found it a good way to introduce people to new approaches to accomplishing tasks. Not everyone spends their off hours studying patterns and practices and rarely is there time during a work day to do this properly so code reviews are a natural place to bring these things up as there's concrete comparisons and examples to work with. That helps spark a dev's interest to look in to the topics further.
First it gives you an idea of how well thought out the implementation was (i.e. was this a quick hack to just finish a asked for requirement) or was a best design targeted. It also helps newer developers develop a voice. Often, at the start, newer devs will take what a more senior dev says as gospel, but by striking up a conversation and, in some ways, making them defend their choices it can help build confidence and that voice to speak up when something doesn't seem right.
Second, I've found it a good way to introduce people to new approaches to accomplishing tasks. Not everyone spends their off hours studying patterns and practices and rarely is there time during a work day to do this properly so code reviews are a natural place to bring these things up as there's concrete comparisons and examples to work with. That helps spark a dev's interest to look in to the topics further.
As a counter argument, it's much easier for a developer to write code that does the right thing when that code is readable.
Too often I see code that is _maybe_ correct, but the reviewer can't actually be sure of it without manually testing it, and the chances that a future reader in 4 months will have any idea what it does or why are extremely low. Good variable and function names get you 90% of the way there yet for some reason cryptic code is still all too common in the world.
To me, reviewing for readability isn't nitpicking - it's equivalent to a mechanical engineering design review pointing out that design for maintenance or design for assembly has been entirely ignored.
Too often I see code that is _maybe_ correct, but the reviewer can't actually be sure of it without manually testing it, and the chances that a future reader in 4 months will have any idea what it does or why are extremely low. Good variable and function names get you 90% of the way there yet for some reason cryptic code is still all too common in the world.
To me, reviewing for readability isn't nitpicking - it's equivalent to a mechanical engineering design review pointing out that design for maintenance or design for assembly has been entirely ignored.
yes, the strawman of nitpicking has been killed.
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-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.
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-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.
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.
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.
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.
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.
Very easy to forget to change code that isn’t highlighted in the review.
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.
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.
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.
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.
> "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?
Can you give some examples of when not to use ArrayList?
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.
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.
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.
Seems essentially non existent to me, outside blog posts.
> 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.
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.
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.
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.
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.
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
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.
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.
The proper layout is whatever the formatter outputs. That's the point. Everyone uses the formatter and accepts whatever it does, and nobody needs to think about it anymore.
I configure Prettier once on my JS codebases and then never touch it again. Seems about as "solved" as is reasonably possible.
I'm not sure how that's relevant. The fact that it's difficult doesn't make it unsolved.
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.
You're conflating style with formatting. No formatter will tell you if you've picked clear names or need to decompose or combine something for better readability.
As long as the starting point is what an opinionated formatter spits out, then we can talk about the hardest thing in software engineering (naming things). But lets be real, most "code style" conversations are of the formatting variety and they're the purest form of bike shedding.
previous discussion: https://news.ycombinator.com/item?id=30674159
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.
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.
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.
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.
> 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.
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.
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.
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.
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.
[deleted]
If code is getting to the PR stage and doesn't compile, you have serious organizational problems.
In previous jobs, I have "reviewed" code that has clearly never been run (syntax error in a script) and it makes me question my own sanity.
In previous jobs, I have "reviewed" code that has clearly never been run (syntax error in a script) and it makes me question my own sanity.
Or breaks the build in an otherwise non-obvious fashion or fucks the installer?
One of the first things I check on a PR is if it passed CI cleanly. If the build has failed (failing tests, build problems, whatever), I get annoyed I was even asked to look at it.
Arguably too fundamental to mention, at least in one slide. Perhaps covered during the developer-testing period, before peer review.
[deleted]
I think the purpose of code reviews is really information sharing, rather than attempting to identify and remove defects (which might occasionally happen, but is more reliably addressed via testing). On the one hand, the team has the opportunity to see how a new feature works, on the other, the implementer gets feedback on team norms.
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.