It’s also really important to define defect during these reviews. I worked at a company where code reviews would identify lots of superficial errors like indention, improper documentation, and missing metadata. You want these in quality code based, but if this counts as a defect, it’s not as important as serious bugs being identified and corrected.
Have you ever actually been constrained in how much time you can spend on code review?
I generally try to get my developers to spend more time on it than they think is necessary -- if it comes at the short-term pains of their productivity, I am totally ok with that and will work to revise their individual contributor expectations.
In most companies I have seen code reviews were scheduled in addition to the regular workload. This means you have to do find extra time to do them. There are also only a few people who can give a meaningful review of complex code.
I am now a believer in pair programming. Code review is already done during development time.
We just do code reviews on every commit, it’s just part of our flow. My experience is it’s far more productive than paired programming, our team doesn’t care about style issues and most code is clearly correct. So review of 4 hours of work can usually be done in less than 20 mins.
As far as cutting in the schedule, we get done what we get done. The schedulers can bite me, i wasn’t the one who mislead them to think dev estimates had any accuracy or usefulness.
My teams also assign code review "in addition" to the regular workload, but this is always in the context of an agile team effort: we get done what we get done [1]. A developer takes a story and works on it until it is solved (or splits it appropriately), then another developer (or a few) are expected to do review.
--
[1] I might be measuring productivity with story points in the background, but there is no "scheduled workload," this is merely an averaging/planning exercise.
The (correct, IMO) managerial effort to improve individual productivity is much softer and more understanding than that and comes with the understanding that your output takes many forms. Some of my best developers directly complete almost zero stories in a sprint because their time is spent on code review, pair programming, direct assistance and architecture discussion.
It depends a bit on context, I suppose, but if you spend most of your time/brainpower in code reviews on style issues, then I think you're probably not making a very good use of time. I think it's much more important to look at the problem being solved and the solution and to think through whether it makes sense. Often times I prefer to first have a conversation with the submitter where we talk through the structure and the decisions made, then I'll go through and leave inline comments for things like style as I see them.
I would put much more trust in a codebase built with careful engineering solutions than a codebase with careful formatting, and I think I would find it to be more pleasant to work with as well. Ideally you have both, of course.
I totally agree with you. However, in my experience the time you get for reviews is often not much and the developer already has moved on to something else and doesn't want to deal with the review. Obviously this is an organizational problem but one I have seen a lot.
On my team all code has to be reviewed before it goes to QA. We strive to do it same day, to make it more productive for reviewers and writers of the code.
It’s not too much overhead. Even though most reviews are satisfactory, occasionally it catches something that’s a good learning opportunity, and that’s golden.
Not sure why a review would take so much time, mine typically take 20 minutes and I do two a day.
We ignore formatting (which can be formalized with a linter should we choose). Basically i check every line for reasonable error handling, review the whole for reasonable algorithm/data structures and i’m done.
It probably depends on the work you are doing. We do test harnesses for hardware and most reviews are the result of a lot of experimentation and learning about protocols. In order to understand that code you need to know the characteristics of the device you are interfacing with. This may take weeks for a reviewer.
> When a reviewer or consensus of reviewers determines that code must be changed before it is acceptable, it is a “defect.” If the algorithm is wrong, it’s a defect. If the code is right but unintelligible due to poor documentation, it’s a defect. If the code is right but there’s a better way to do it, it’s a defect. A simple conversation is not a defect nor is a conversation where a reviewer believed he found a defect but later agreed that it wasn’t one. In any event a defect is an improvement to the code that would not have occurred without review.
I'd say that's a bit looser of a definition than I'd prefer, but it's at least good that the study didn't try to use some metric like "number of inline comments", since often times I use inline comments for praise or to mention possible alternatives that may or may not be any better than the code.
In all seriousness I have found it's not the number of reviews or that they are done consistently. I have found that the most helpful reviews are all the ones where really good code is reviewed by lots of people that has all of the things that organization considers a good practice. 1 Review like that does a lot more good than 25 reviews by lesser skilled coders.
I don't like code reviews, but it has a few benefits:
Pros:
1. Developers pay more attention to what they throw over the wall as a completed tasks because they will get grilled on it.
2. Reviewing other people's code is good for learning a new system.
3. It keeps creating of methods that already exists to a minimum and boosts proper code reuse (of existing, difficult to find code).
Cons:
1. It takes a lot of time.
2. Petty arguments over style rather than technique. It can devolve into minutia / trivia.
3. Once a developer "gets the idea" of how the organization wants coding done, it becomes more of a seemingly pointless routine.
I suggest code reviews for a developers first 3-6 months, or when working with new (to them) modules / subsystems. After that, skip it (unless they are slow to conform).
Con 2 has historically been the biggest problem for me. If any experienced people have good methods to help me mitigate that problem on my teams, I want to hear it.
We've done a few strategies at places I've managed code review:
1. Insist on linting (in Javascript, eslint-config-airbnb) and opinionated style guides/automation (in JS, Prettier), and have the team buy in on "let's just not even debate this and just do what it says."
2. Have programmers highlight problem areas (structural/architectural/clarity) in their own code before submitting the PR. This is draws the discussion to the high ROI locations quickly, while not prohibiting the reviewers from catching more minor issues.
3. Clearly indicate that the goal of code review is to, yes, impart style and feel on the rest of the team, but largely to avoid duplication, resolve bugs and improve everyone's skill. It is a within-team visibility and knowledge sharing process that also happens to catch meaningful defects.
I found that the best coded reviews are always from small change sets. They tend to have more comments on what the code is doing rather than what the code looks like. Sometimes this can’t be done though.
I’d also say that if your team is commenting only on style in code reviews then they don’t understand that part of the code base very well. It isn’t a bad thing but presents a signal that they could spend some of their work day understanding that part of the code. Keep in mind the culture of the team needs to allow this to happen. It pays off in the long term.
One of the best reasons for code review, hammered into me by two very experienced developers, is high visibility of changes and the opportunity to learn more about what other developers are working on. Don’t underestimate this benefit.
Ultimately every code review doesn’t need to have a high net benefit for it to be good for your team.
On a side note we didn’t have any style guide at my old workplace but found that our code reviews were incredibly high quality for core components. Part of this is culture and the other part is the people you work with.
With some reflection, I think you're correct on the understanding problem.
The problem has always been inconsistent - some code reviews are worth thousands of times more than others - and a determining factor may be how invested the particular reviewer is in that piece of code, how well they understand the surrounding ecosystem and things of that nature.
As a concrete example, it has always been rare that a backend API change PR doesn't get great feedback from the frontend team that is dependent on it. The visibility is a core benefit here and the clear process to collaborate (especially if done early, low cost with individual small changes) is essential to the return on time investment.
For the record, I agree, I think a strong culture of code review is the most valuable process I've added to any development team. In my grandparent post, I was merely drawing attention to a potential spot for improvement, I hope no one took it as against code review as a process.
That’s very true. I’ve seen code review when it’s a bit more relaxed and most of the reviews are comments about style - only a handful of people provide useful comments about what the code does.
In your defence, it’s hard to find out a way to fix a bad code review process with a team. I’m unsure how to really tackle that.
My main strategy is what you call (3). I've found that if I take a strong stance on code style - namely, "I don't care about this style matter, I'm going to approve your reviews regardless of it, and will call people out for blocking code on purely style matters" - then I haven't gotten much pushback, and the teams naturally loosely find a happy medium shared style that includes automated linting type stuff so that there's an objective criteria everyone's on board with but little personal opinion-based quibbling.
I like code reviews to end up talking about "does this functionality belong in this file/block/class/whatever?" because that's what I find turns into tech debt if the whole team isn't on board with the strategy, and so I don't think anyone ever outgrows peer review on that. Those are critically important decisions, and lord knows I've inadvertently gotten it wrong as often as anyone else I've worked with. It's not that any particular dev needs oversight, it's just a really hard problem as shown by the fact that pretty much every codebase at every company turns into a big mess over time.
It's possible one bad actor could still mess things up for a team with that, but then that's a managerial problem. If someone's causing the whole team to slow down, that's a problem that's fairly easy to communicate to them (and if they aren't listening, it's easy to communicate to the rest of the team in terms of "this is why we had to do something, you don't need to be worried yourself").
The automatic formatter, extensive style guide, and linter helps.
We also have a (team local) culture where it's ok to make (logically justified) style suggestions, but they aren't ever blockers and can be safely ignored if you disagree with them or are just in a hurry or whatever else.
Basically, if you don't see any logical flaws or duplicated code or whatnot, you hit accept, but it's fine to make further comments.
Another thing to add, is that for "obvious" style suggestions (i.e., changes that everyone is nearly certain to agree on, though they might not agree on the importance), I encourage the reviewer to simply make the change.
This is only for stuff like missing whitespace or a typo in a comment. It is not worth the roundtrip to call that stuff out and I've never seen it cause a meaningful conflict on my teams to simply fix it. I've only directly managed teams <10 local people or <5 remote people, I can imagine more structure is necessary on considerably larger or more disconnected teams.
While not a problem I've ever really handled, this approach may have the side-effect of reducing dysfunctional codebase ownership that I've heard about from other team leads.
Out of curiosity, what's the context in which you think developers should stop doing code reviews after 3-6 months? Every place I've worked has required code reviews for all changes (except in emergencies or when the author thinks the change is trivial). For example, how big of a company are you talking about, and how critical is correctness and maintainability? My impression is that skipping code reviews mostly makes sense for early startups, internal tools, prototypes, and other situations where velocity is more important than quality, but that most companies should have pretty much all code go through review.
I can’t imagine any reason to skip code reviews in any professional development. Someone is paying the team to produce professional code, finding out well after the fact that one section/contributed failed that test is unacceptable.
Code reviews shouldn’t be lots of overhead. They shouldn’t devolve into style critiques. They should be the best tool you have for teaching and communicating best practices. Reviewing a more senior devs code can teach a junior a lot, and occasionally catch significant errors.
Preferably in the most efficient way possible (which is of course subjective). I believe that after 3-6 months in a particular subsystem, the ROI of code reviews increasingly decreases over time.
>Bugs going in testing are far more expensive to fix than those found in code review.
How so? Particularly if you don't do your code review right after development. That holds true for production vs. QA, but I'm not sure it does for QA vs. code review, unless your QA deployment and ticket bounce back process is overly cumbersome and you don't have time planned for it.
We always do code review before QA. But our QA process is also multilayered, first layer is QA engineer, next layer is first level business acceptance testing, then another higher level business acceptance testing. Gets pretty expensive once the SR marketing person rejects.
"They should be the best tool you have for teaching and communicating best practices. "
For christ sake, no. If you want communicate new best practice, do so. And if team agrees, we will follow. Don't ambush me with new expectations during code review and don't keep my task hostage in order to push for your new best practice.
Same with teaching. Teach proactively instead of pretending juniors can solve it and then rewrite all their decisions in code rewiew.
If you have expectations, tell them in advance. It is just unfair to use code review for that.
That reminds me of a SQL code reviewer at my last company. He'd have a standard that we would follow and finally get stuff passed most of the time. After a few weeks, he would read a new article and completely change his passing criteria without telling anyone, so all of our previously passing submissions would then fail. This happened several times over the course of a year. It was very frustrating and rarely helped the overall quality of the SQL.
> If you want communicate new best practice, do so.
I think the quote your replying to means existing best practices. IMO, things like "we tend to follow this naming convention" and "here's a useful language feature you may not know about" seem completely fine to say at code review time, and it's not like you can teach someone absolutely everything before they get started.
Having a reviewer hold a review hostage is a big cultural red flag, IMO. If there's a real significant disagreement, you shouldn't try to solve it via code review comments; you should move to in-person communication if possible and maybe pull other people in to take a look. I also think that in most situations, reviewers should bias toward suggestions and ideas rather than demands ("strong opinions, weakly held"), and authors should feel comfortable pushing back or proposing to merge the commit as-is and leave some change for a follow-up review.
Saying code reviews are "the best tool" for teaching and communicating best practices is maybe a stretch; I find things like pair programming to be much better for stuff like that. But I still think code reviews can be a great mechanism for teaching code-level details like code style.
I have zero expectations on formatting preferences. But i have high expectations on writing correct, maintainable code. If i see code that can fail in ways you haven’t anticipated, or is a duplication of other code in the project i’m going to point that out. If you implement something in a way i wouldn’t, or can’t understand, it may be an opportunity for you to teach me something.
And if I tell you to re-implement working code my way simply because of some personal preference, i would expect you to tell me to fuck off. Cause that’s what i’d say if you did it to me.
To give you an idea of how our code reviews are perceived, we have one junior dev, at last retrospective she spoke up about how much she valued and appreciated them.
Obviously some places don’t get it, but i think we do.
I don't mind formatting requirements - when they are known in advance and same for everybody.
But last time there were two "seniors", each with different conflicting preferences and each insisting that your code is "bad" and "mess". (And inexperienced project manager buying their bs because they were confident). And it changed based on what blogs they read last week. Juniors were more fine with that then me, because they tended to belive seniors know what they are doing. Plus juniors worked only on one module and I did on two, so I could see how what one called crap on the meeting is demanded by the other.
And the rules were different for different people.
I think it is the ability of coder to tell "fuck off" on changes he disagree with that makes all the difference. I was comiting code I disagreed with and had to come Sunday to make changes I considered useless.
I used to be for code reviews, but now I think that it requires people with strong self awareness, knowing difference between preference/feeling and objectively betters, and many programers out there don't have that. The seniors I had trouble with were on spectrum, one a little bit other a lot. They could not tell the difference and it cause a lot of problems beyond code reviews.
I think they lose their value in any organization. It's not a hard rule, because there are a lot of variables that could increase or decrease their value. I'll give you an example of each:
1. Decreased value: If a person is the primary (only) developer on a subsystem, any outside code reviewer will be less informed of the subsystem he is reviewing. I don't see any value in that other than get the other reviewer up to date on a subsystem he doesn't actively develop / maintain. I guess you could argue redundancies, but if you have a big system, having every developer understand every part of the system is unrealistic.
2. Increased value: if a developer is working on a new subsystem they aren't familiar with, then the 3-6 month window restarts. The 3-6 month window isn't the time of employment, it's really the time of understanding a particular subsystem.
My point is that doing code reviews in and of itself has decreasing value after 3-6 months. The 3-6 months is the time I'm estimating for a competent developer to familiarize himself with an existing subsystem codebase.
Also, any developer that still has issues after the 3-6 window of the same subsystem is a huge red flag. Code reviews will then just be a crutch for him and the entire organization when the real solution would be to get a better developer.
Also, I'm not saying any communication about code loses value. If I want a guy to look at my code and help me work though something (usually by just listening to me explain it), that has value. I'm generally speaking of the formalized code review that must happen after any coding change. That loses value after a developer is acclimated to the subsystem he has been working on, especially if the reviewer doesn't know as much about the subsystem he is reviewing (and has no need to know). I'm a very anti-hard-process guy. A process must have strong value, otherwise it's just another speed bump.
One of the biggest pros is that reviewing code keeps you up to date on the current state of the code base. I have, at least briefly, read and understood every line of code in our teams code base. I might not remember it perfectly, but I still feel like it is fairly easy to dive into and change any part of it, even the parts I haven't worked on in a while. This, plus occasional informal architecture standups, means we have a lot of flexibility in terms of developer allocation for tickets.
> 2. Petty arguments over style rather than technique. It can devolve into minutia / trivia.
This can be mostly addressed by using a linter. In our organization, we have a rule that if there is a glaring style issue, we ask whether the submitter had actually run the code through the linter before submitting it for review.
On the other hand, if the linter does not complain about some code, but the style it's written in is not agreed upon, we update the linter configuration to handle that case if the rest of the team agrees. Otherwise we leave the code as is (assuming it works as its supposed to).
> 3. Once a developer "gets the idea" of how the organization wants coding done, it becomes more of a seemingly pointless routine.
I find that there's always room for improvement in terms of how things are done and those things could be done in a code review. The other thing is that its always a good thing to have at least one other person look over a change just to make sure that its correct and something obvious wasn't missed.
>I find that there's always room for improvement in terms of how things are done and those things could be done in a code review.
I agree that there is room for improvement, but after 3-6 months, it starts violating the 80/20 rule, meaning you are spending a hell of a lot of time & energy (money for business guys) for increasingly depreciating value.
>The other thing is that its always a good thing to have at least one other person look over a change just to make sure that its correct and something obvious wasn't missed.
Eh, developer should be reviewing their own code fairly thoroughly anyway. If a developer can self review their code, then the second person doing it costs a lot and doesn't get a lot of value. I wouldn't expect glaring mistakes from a developer that has lasted 3-6 months. (it's a requirement for employment, IMO).
Again, what you are saying is right, but I believe you get decreasing values over time as the developer starts to "get" how the org wants things done. Like I said, I think code reviews have great value the first 3-6 months, then ever decreasing value after that. I'm also assuming competent developers. If an org is hiring cheap coders, they don't really value development efficiency in the first place and my theory doesn't really apply.
> developer should be reviewing their own code fairly thoroughly anyway
I agree, but sometimes, when one has been looking through the code for a while, they can miss things that will immediately stand out to someone else who hasn't looked at the code recently.
For example, we had one developer on our team add a item to a JSON config file list and, since it was thought to be trivial, it didn't go through review. It turns out that they forgot to add a comma to the end of the quoted string. When it was time to roll out the change, we discovered the issue.
We have since enabled post-push linting on that repo, but I think that had someone else looked over it, they may have spotted that issue before it was merged.
> Again, what you are saying is right, but I believe you get decreasing values over time as the developer starts to "get" how the org wants things done.
To some extent, you're correct, but only if the code base is relatively static and conventions don't change. That usually doesn't hold true over time.
>when one has been looking through the code for a while, they can miss things that will immediately stand out to someone else who hasn't looked at the code recently.
I agree with you, but it would seem rare. Maybe 1 out of 50 code reviews.
>For example, we had one developer on our team add a item to a JSON config file list and, since it was thought to be trivial, it didn't go through review. It turns out that they forgot to add a comma to the end of the quoted string. When it was time to roll out the change, we discovered the issue.
I would think in this example, the developer didn't bother to test his own code and he is using the code review as a replacement for that, which seems lazy. I would imagine the config error would pop up when he was running the application and that the config line he wrote was directly used by his changes. It would certainly pop up during QA.
In terms of the cons, I find that if the output code is good and the team as good practices regarding style (a linter and styleguide), reviews for unobjectionable code are a breeze, and often take minutes for reasonably sized changes that have no issues.
A style guide and linter also remove the arguments about minutia. Google's python style guide is a lot less strict than the go or java styles, for example, and even still most of the style comments I make when reviewing code amount to "be internally consistent" about something, and those are nits.
And that whole "reviews take 5 minutes" doesn't mean that they aren't useful. I often find something that is either confusing, or a typo, or dumb mistake/overlooked thing or a non-idiomatic way of doing something, and leave a comment expressing that. The review still takes 5 minutes, but I've caught a bug or a bit of technical debt later.
As a code-writer, I have more or less the same opinion. I would flatly refuse to work somewhere that doesn't do code review. It makes working in the codebase difficult, and makes personal development more difficult.
Our code reviews are line-by-line and can take several hours because the reviewers feel they must completely understand everything that is going on to properly review changes (which I would agree with). I just don't see how you can do a useful code review in 5 minutes, unless it's just for posterity's sake, in which case, why bother?
Can you explain your code review process? Maybe we're just doing it wrong, or maybe we're just talking about different types of changes. We typically do it at the final merge, after developer and unit testing, before QA, and most of my code is new systems, new subsystems or substantial features.
I work at Google. I do code review in two contexts: one within my team, and I also volunteer to review python code for teams across google.
The code review I do is subtly different in those two contexts. Within my team, my goals are to keep us to a very high standard of committed code, it should be clear, simple, and well documented. We want our output to be relatively reusable, well tested and testable, and well designed. This goes beyond our newly written code, we are stewards of libraries and tools, and we should maintain them as well as we can.
On the other hand when reviewing google-wide code, I am, yes, looking for bugs, but mostly looking for good "mastery" of python. That is, I'm explicitly looking for idiomatic python from people who are not as experienced with it. These reviews also are often larger and longer.
Within my team, we have a number of practices that help make code reviews fast:
- Most changes are small: within my team I reviewed 3 changes of more than 250 lines this quarter, compared to a dozen or so of <10 lines
- Most of the code written is really good. I rarely, if ever, have to mention lint errors, nor do I often come across bugs.
- My comments mostly revolve around idiomatic ways of solving problems and, as a team python expert, "pythonic" approaches, and better approaches using the python standard library and known tooling (ie. "You're reinventing the wheel here, just use XYZ", or "this is confusing, use pathlib.path instead.")
- The other big thing I comment about is clarity and readability, as a reviewer, I want to be able to follow your code, code is written more than it is read, and so most of my comments are things that amount to "This is difficult to follow as someone reading, can you refactor this to make it easier to read." These comments vary in both necessity and type, they can range from "hey maybe reorder your conditionals here and see if it reads better" to "holy wow it took me 10 minutes with a repl open and asking for help to figure out what the heck this function is doing, this needs to be refactored."
- And the last thing I suggest is future work to be done: "We should refactor this method in the future, we're having to work around it a lot", or a very common one: "We can parameterize these tests".
- The other big thing we do to make code reviews quick is that everyone does them. They don't just fall on the TL or manager or whatever. I review code for everyone, and everyone reviews my code, and as a result the code is not mine but the team's. Nitpicks aren't really a thing, because if someone is picky just to be picky, they'll get the same treatment in turn (and also because that's a dick move and we don't do that).
Note that for the most part, the things I'm looking for are pretty easy to notice, unidiomatic code is obvious, and "this is difficult or tricky" is also very clear. I still notice bugs and such, but they're rare, and we write code to make reviewing easy and errors obvious, and as a result have fewer of them.
The across-google reviews are on average bigger than within my team, and often result in more comments, because the writers are less good at python (everyone on my team has python readability, whereas the people I'm reviewing for python do not, otherwise they wouldn't need my review). They take longer. Many of them are still relatively small, but yeah the big ones that cross 500 or 1000 lines, and are in an unfamiliar part of the codebase, those do take a significant chunk of time to review (normally ~1-2 hours), but they're rare, I get one of those every couple of weeks.
In general I'd say the answer to how to make code reviews go faster is
1. Encourage small changes when possible
2. Use code review to teach best practices
3. Encourage writing code for readability and testability
4. Have everyone review code
I'm not the person you replied to, but I'm a big advocate of code reviews, and happy to tell you about my process:
Someone sends me something to review and I see a notification pop up with a summary, regardless of what I'm doing at the time. More than 90% of the time (maybe more like 98%), it is from one of my team members, who has sent out something I knew they were working on because we had chatted (in a meeting, or at our desks, or on IM) about the need for it, and often also the approach they're planning on taking.
A lot of times (maybe around 50%?) this is configuration, documentation, or a very small code or test change. These I immediately skim for obvious problems like typos or confusing phrasing, check a diff of the config for those changes to make sure it does what it thinks it does, and almost always immediately approve the change with no comments. These often take less than 5 minutes.
Maybe 10% of the time, it's a fairly small functionality-preserving refactor. I defer these until I have some free time, because they aren't critical. I review them with an eye toward making sure I think they actually preserve functionality and decrease complexity and/or confusion. These can take 15 minutes to an hour, but aren't very disruptive because I can do them when I'm not otherwise busy.
Maybe 30-40% of the time, these are medium to large functionality changes to the part of the codebase I am intimately familiar with (again: the vast majority of these are coming from members of my team). Depending on what I'm doing at the time, I'll either start reviewing it immediately, or take a note reminding myself to take a look when I get a chance. I actually start by spending some time reading the description and thinking about whether I think what it's claiming to do is even the right thing to do; if not, I comment on that before even reading the code.
After that, I dig into the code. I start by reading the whole thing at a pretty high level to see if it seems to do what the description says, focusing on method names and signatures and especially on documentation.
If all looks well at the high level, I like to start going through the changes "outside-in", that is, starting with the higher-level methods and digging into the lower-level ones over time. Mostly what I'm looking for is whether everything makes sense to me when I read it, if not I ask for renaming, restructuring, or just more documentation if it seems fundamentally complex or confusing. In other words, I try to read it from the perspective of someone coming in cold and trying to figure out what's going on. This will be the case for everyone reading this code in the future, including me and the author in a few months. I also try to think hard about what assumptions are being made, whether they are right, and what should be done about it if I'm unsure.
After all this, I look at the tests. I start by checking for good coverage. If some things appear uncovered, I ask for tests, or suggest ways it could be tested if it seems tricky. If the coverage is good, I'm honestly kind of lazy about reading the tests themselves, looking for obvious ugliness, but mostly assuming that if they're passing and covering the branches, they're probably fine. Sometimes I suggest integration tests that I think might be useful.
If the change is in code that has lots of downstream clients, I'll usually do another pass focused on whether we might be breaking them somehow. Sometimes I have to do research on libraries that are being used or could be used instead of something else.
This can all take 15 minutes to maybe two or three hours of work time, sometimes spread across a few days of wall time, including any back and forth. I wouldn't say I go "line-by-line", though I do think I end up reading all the (non-test) lines at least once. I do feel I should completely understand what's going on, but that's usually built into my experience with the code ...
It is a fallacy that it takes a lot of time, like saying QA is worthless because it too takes a lot of time. Just like it's easier to fix a bug when QA finds it before a customer does, it's even easier to fix when a developer spots it in code.
If you think you stop making coding mistakes after a few months (well, or ever), you're kidding yourself.
I've never got into an argument over code style in a PR, and it seems like if this did happen I'd be looking to fix deeper problems in the team. I've had comments in open source PRs asking me to change something, but that's part of getting into a new code base. The right style is there one that's there, so unless your PR has the sole purpose of specifically changing the style, you should comply with existing style, no arguing.
Anyway, there are lots of other pros:
A reviewer might suggest a better way of doing something, helping others learn, and likewise will pick up new techniques from the code they're reviewing. This applies to even the most senior, experienced person in the team.
If a new library is brought in, it can be checked for conflicting versions or overlap/redundancy with an existing library, and license compliance.
Noticing simple things like hardcoded parameters/constants, unchecked config or input values, spots with potential unhanded errors, among dozens of other problems.
Unrelated changes are usually caught. Even if it's fixing another bug, I'd argue it's better to do separately with its own issue, because otherwise it gets lost when you go to build release notes or a change log.
>It is a fallacy that it takes a lot of time, like saying QA is worthless because it too takes a lot of time.
If you think it's so valuable, why not have every developer in the team review every merge? That's like 15 code reviews for each piece of code checked in. Obviously, it's because at some point you get diminishing returns. We have developer testing, unit testing, QA and UAT, all to catch defects before production. That's 4 layers of testing. Code reviews make it 5 layers. Not only is it a fifth layer, it's expensive because the most expensive people (in both wages and opportunity cost) are doing it. Reviews are just another iteration of essentially the same thing. At some point, you are spending a lot of money on little return. For me that is mandatory code review after 3-6 months.
I've read this and similar posts on this study a dozen times in the last few weeks. I think the data is poorly interpreted and what you're really seeing is that shorter pull requests elicit better feedback (in this case "more defects per line").
In my experience running code reviews, shorter pull requests, presumably due to their reduced effort necessary to understand, tend to get better review, review that is more than just superficial style/linting errors.
In light of that, I always encourage developers to aim for the shortest reasonable changeset - and make it very clear to them that 2 line PRs are totally acceptable - on my current team, we go as far as to encourage tricks like rewriting (local) Git history and cherrypicking to ensure that is the case. Continuous integration and good unit tests help to ensure that strange states generated from that process are still good and I find the costs are far outweighed by the better reviews.
Another little lesson I've learned managing that process is to encourage the submitting developer to highlight problem areas in his own code: when I submit my own code for review, I'll actually do the review first, line-level highlighting areas I wish the reviewer to pay attention to. This will only work if you can trust your developers to not try to "sneak something by," but if you can't do that, you've probably already lost. Developers generally know what they weren't sure about during the process, where things are going to be difficult to understand and where someone else on the team is going to have helpful contributions to the quality.
I think it is important for developer-managers to remember that programming is largely a craft and developers largely want to be proud of their output. People _like_ producing "good code" and it is easy to align the goals by having the process help improve their craft.
This matches my experience, and also has implications for date-driven projects. People have two typical responses to long code reviews: skim and ignore, or go in depth and ask a lot of questions because there's a lot going on that's new. If it's a critical feature, I try to push the team to not just ignore, so the longer someone works in a silo, the longer it takes to merge those changes back in with everything else, the higher the risk of getting major "why not do it this other way instead" late feedback throwing a wrench in everything last minute.
What I've taken away from recognizing this pattern is to ensure everyone's checking in frequently, even in the early design and idea phase, so that nothing ever turns into a bad surprise in a code review. Depending on the seniority of the dev driving the feature, there isn't a single one-size-fits-all prescription for what's a "too big" code review, but the rest of the team's job is to make sure that whoever is driving the feature never ends up in a position where their technical choices put the delivery at significant risk - nobody wants to feel like their team left them to sink or swim and then only offered criticism when they finally submitted their code for review.
I'm also a huge fan of the described "identify what I dislike about my own code when submitting it for review" practice, too. I'll come straight out and ask "I don't like how I did this, anyone got better ideas?" and I've seen other people start picking this up on the team too.
I directly manage one team, and oversee another, both use gerrit (the code review from Google, used on Android).
I 100% agree that the shorter the changes, the better the reviews.
I am 100% sold on code reviews, because of:
1. the number of bugs found before they were pushed,
2. everyone writes code for other people now, whereas before we wrote the code for ourselves, and
3. improvement to APIs before they hit the code base and documentation.
Every single one of these things was a surprise to me, and I've been doing this a very long time. I never would have believed how happy I would be with the result.
Regarding other comments on annoying, small comments on style, etc: this is a problem with your people and your team leader. If this happened with my people, I'd have a conversation offline with them and tell this to not use code reviews in this petty way (to push their own styles). One thing it made me realize, every organization needs a style guide, but you need to make sure it doesn't go too far. Give people some latitude, just put the important stuff in it.
In regards to your last sentence - as a developer, I _like_ solving business problems, and coding is just a tool used to get there. What makes me proud is when end users are able to use the application and it solves some problem they have, and as a side effect, produces revenue for the company.
Religious adherence to processes gets in the way of that and if they block me, I'm happy to shove them to the side and get executive support to do so. My experience with code reviews is that they are 95% style nitpicks, 4% ways to make code cleaner and simpler, and 1% real bugs which impact end users. I only value the 5% myself. From a cost/value perspective, I haven't been sold on the value of doing code review on everything.
Not all developers feel that way, sure. But it is a mistake to assume all developers are driven by the same things.
Reviewer must be able to explain and demonstrate why his remark is right. The petty code rewiew problem tend to happen in system where reviewer is assumed right and programmer is assumed wrong.
When original programmer has right to refuse review, the percentage of useless code review comments goes down. Most of them are power trip anyway, if you remove power from it people cease to make them.
"Most of them are power trip anyway" is not my experience, and I think author-right-to-refuse is usually the wrong default. It always seems super critical to an author to get their changes committed as soon as possible, but it rarely is.
What you need is a respected conflict mediator, probably a team lead. This person should chat offline with both authors and reviewers who are frequently involved in conflicts, for instance because they often refuse to adopt the team's idioms as an author, or because they are often on a counter-productive power trip as a reviewer. If the team, or the company as a whole, doesn't have a person who can effectively mediate this sort of thing, that's a major problem, and something the management structure needs to be keeping an eye on as a major risk to the company.
I'm not sure I assumed developers are driven by the same things. A careful reading of my last sentence did not define what it meant for code to be good or by what process you would be proud of the output, I think that is a product for you, your team and leadership to decide. Perhaps my use of the word pride was too strong, the core of my philosophizing there is that software development on a team is a social endeavour and that code review can be a very effective alignment mechanism.
Someone further down the page suggested that the style nitpicks arise from a lack of understanding and familiarity with that piece of the codebase. I think that is often true and a factor to be considered both when allocating code review and when setting expectations regarding code ownership and cross-functional understanding.
I've also provided some suggestions for how to tune down the "95% style nitpicks" elsewhere on this page - it is a problem, but with appropriate tooling and expectation setting, it can be reduced to a small fraction of total code review output. It would be dysfunctional for a team to spend time on that stuff when we've all agreed it isn't high ROI. I agree that code review can be done very poorly, but your observed ratios are not a fixed property of the world of code review.
Let me be clear: I am absolutely not advocating for a process that motivates nerding for the sake of nerding - I push back on a whole wealth of that sort of behavior. I am a business person motivated by producing sustainable teams that produce real value in the form of solutions to problems. I just feel that is a high-dimensional problem and certain kinds of technical debt can come at extreme cost and can, at times, if done correctly, be mitigated with processes like code review.
That is a fair rebuttal - perhaps my opinions are shaded by the fact that I've worked for several companies and none of them have a code review process which isn't 95% style nitpicks - the idea of one which lacks those has always been a hypothetical thing in my mind, and not something which I've seen in the real world.
Consistent style is useful because it reduces cognitive load when reading unfamiliar code, but everyone should have automated tools that do that "95%", and most people don't. This is something that Go got very right by building it straight into the compiler but automatic linter-formatters exist for most languages, and it's useful for someone high up in the early technical team to just pick some flags they like for one of those and integrate it into the review process.
Once you eliminate that 95% overhead (which I think is an exaggeration, but I'll allow it can be high), the value proposition of code review is an absolute no-brainer.
linters and auto formatters add a lot of value, but some style changes don't fall into that. e.g.: naming things, or just when the language provides more than one way to do something, which one you choose.
I don't consider either of those to be matters of "style". Naming is more akin to documentation, and as such, it is a very useful thing to review: if names don't make sense to a reviewer now, they're even less likely to make sense to someone reading them in the future.
Picking which among a few ways of doing something is idiom, not rote style. I agree that this is one of the least valuable parts of review, but people ramp up quickly on it in my experience, so it isn't constantly coming up. If an idiom actually matters, for instance if it is protecting against some commonly problematic pattern, it may be possible for tooling to catch that too. If it really doesn't matter which way its done, the reviewer should just let the author do it their way, and if they don't, the author should just change it without making a fuss. But yeah, arguing over idioms can be a problem. It helps to write down decisions the team has made in the past, so that you can just point dissenters to the documentation.
A lot of the comments here imply that style can be fixed with a linter, but what about the style of things other than syntax, like choices in variable names,
DRY-ing through mixins vs composition, grammatical style in code comments, etc.
60 comments
[ 3.0 ms ] story [ 119 ms ] threadI would trust someone with a sharp eye like that to review my code much more than I would trust the author of the original article.
The PDF that requires an email can also be found here without giving one: https://pdfs.semanticscholar.org/feb0/0dd349ad701e1e4045282b...
I generally try to get my developers to spend more time on it than they think is necessary -- if it comes at the short-term pains of their productivity, I am totally ok with that and will work to revise their individual contributor expectations.
I am now a believer in pair programming. Code review is already done during development time.
As far as cutting in the schedule, we get done what we get done. The schedulers can bite me, i wasn’t the one who mislead them to think dev estimates had any accuracy or usefulness.
--
[1] I might be measuring productivity with story points in the background, but there is no "scheduled workload," this is merely an averaging/planning exercise.
The (correct, IMO) managerial effort to improve individual productivity is much softer and more understanding than that and comes with the understanding that your output takes many forms. Some of my best developers directly complete almost zero stories in a sprint because their time is spent on code review, pair programming, direct assistance and architecture discussion.
I would put much more trust in a codebase built with careful engineering solutions than a codebase with careful formatting, and I think I would find it to be more pleasant to work with as well. Ideally you have both, of course.
It’s not too much overhead. Even though most reviews are satisfactory, occasionally it catches something that’s a good learning opportunity, and that’s golden.
We ignore formatting (which can be formalized with a linter should we choose). Basically i check every line for reasonable error handling, review the whole for reasonable algorithm/data structures and i’m done.
What am i missing?
> When a reviewer or consensus of reviewers determines that code must be changed before it is acceptable, it is a “defect.” If the algorithm is wrong, it’s a defect. If the code is right but unintelligible due to poor documentation, it’s a defect. If the code is right but there’s a better way to do it, it’s a defect. A simple conversation is not a defect nor is a conversation where a reviewer believed he found a defect but later agreed that it wasn’t one. In any event a defect is an improvement to the code that would not have occurred without review.
I'd say that's a bit looser of a definition than I'd prefer, but it's at least good that the study didn't try to use some metric like "number of inline comments", since often times I use inline comments for praise or to mention possible alternatives that may or may not be any better than the code.
In all seriousness I have found it's not the number of reviews or that they are done consistently. I have found that the most helpful reviews are all the ones where really good code is reviewed by lots of people that has all of the things that organization considers a good practice. 1 Review like that does a lot more good than 25 reviews by lesser skilled coders.
Pros:
1. Developers pay more attention to what they throw over the wall as a completed tasks because they will get grilled on it.
2. Reviewing other people's code is good for learning a new system.
3. It keeps creating of methods that already exists to a minimum and boosts proper code reuse (of existing, difficult to find code).
Cons:
1. It takes a lot of time.
2. Petty arguments over style rather than technique. It can devolve into minutia / trivia.
3. Once a developer "gets the idea" of how the organization wants coding done, it becomes more of a seemingly pointless routine.
I suggest code reviews for a developers first 3-6 months, or when working with new (to them) modules / subsystems. After that, skip it (unless they are slow to conform).
We've done a few strategies at places I've managed code review:
1. Insist on linting (in Javascript, eslint-config-airbnb) and opinionated style guides/automation (in JS, Prettier), and have the team buy in on "let's just not even debate this and just do what it says."
2. Have programmers highlight problem areas (structural/architectural/clarity) in their own code before submitting the PR. This is draws the discussion to the high ROI locations quickly, while not prohibiting the reviewers from catching more minor issues.
3. Clearly indicate that the goal of code review is to, yes, impart style and feel on the rest of the team, but largely to avoid duplication, resolve bugs and improve everyone's skill. It is a within-team visibility and knowledge sharing process that also happens to catch meaningful defects.
I’d also say that if your team is commenting only on style in code reviews then they don’t understand that part of the code base very well. It isn’t a bad thing but presents a signal that they could spend some of their work day understanding that part of the code. Keep in mind the culture of the team needs to allow this to happen. It pays off in the long term.
One of the best reasons for code review, hammered into me by two very experienced developers, is high visibility of changes and the opportunity to learn more about what other developers are working on. Don’t underestimate this benefit.
Ultimately every code review doesn’t need to have a high net benefit for it to be good for your team.
On a side note we didn’t have any style guide at my old workplace but found that our code reviews were incredibly high quality for core components. Part of this is culture and the other part is the people you work with.
The problem has always been inconsistent - some code reviews are worth thousands of times more than others - and a determining factor may be how invested the particular reviewer is in that piece of code, how well they understand the surrounding ecosystem and things of that nature.
As a concrete example, it has always been rare that a backend API change PR doesn't get great feedback from the frontend team that is dependent on it. The visibility is a core benefit here and the clear process to collaborate (especially if done early, low cost with individual small changes) is essential to the return on time investment.
For the record, I agree, I think a strong culture of code review is the most valuable process I've added to any development team. In my grandparent post, I was merely drawing attention to a potential spot for improvement, I hope no one took it as against code review as a process.
In your defence, it’s hard to find out a way to fix a bad code review process with a team. I’m unsure how to really tackle that.
I like code reviews to end up talking about "does this functionality belong in this file/block/class/whatever?" because that's what I find turns into tech debt if the whole team isn't on board with the strategy, and so I don't think anyone ever outgrows peer review on that. Those are critically important decisions, and lord knows I've inadvertently gotten it wrong as often as anyone else I've worked with. It's not that any particular dev needs oversight, it's just a really hard problem as shown by the fact that pretty much every codebase at every company turns into a big mess over time.
It's possible one bad actor could still mess things up for a team with that, but then that's a managerial problem. If someone's causing the whole team to slow down, that's a problem that's fairly easy to communicate to them (and if they aren't listening, it's easy to communicate to the rest of the team in terms of "this is why we had to do something, you don't need to be worried yourself").
We also have a (team local) culture where it's ok to make (logically justified) style suggestions, but they aren't ever blockers and can be safely ignored if you disagree with them or are just in a hurry or whatever else.
Basically, if you don't see any logical flaws or duplicated code or whatnot, you hit accept, but it's fine to make further comments.
This is only for stuff like missing whitespace or a typo in a comment. It is not worth the roundtrip to call that stuff out and I've never seen it cause a meaningful conflict on my teams to simply fix it. I've only directly managed teams <10 local people or <5 remote people, I can imagine more structure is necessary on considerably larger or more disconnected teams.
While not a problem I've ever really handled, this approach may have the side-effect of reducing dysfunctional codebase ownership that I've heard about from other team leads.
Code reviews shouldn’t be lots of overhead. They shouldn’t devolve into style critiques. They should be the best tool you have for teaching and communicating best practices. Reviewing a more senior devs code can teach a junior a lot, and occasionally catch significant errors.
How so? Particularly if you don't do your code review right after development. That holds true for production vs. QA, but I'm not sure it does for QA vs. code review, unless your QA deployment and ticket bounce back process is overly cumbersome and you don't have time planned for it.
No i didn’t design this process.
For christ sake, no. If you want communicate new best practice, do so. And if team agrees, we will follow. Don't ambush me with new expectations during code review and don't keep my task hostage in order to push for your new best practice.
Same with teaching. Teach proactively instead of pretending juniors can solve it and then rewrite all their decisions in code rewiew.
If you have expectations, tell them in advance. It is just unfair to use code review for that.
I think the quote your replying to means existing best practices. IMO, things like "we tend to follow this naming convention" and "here's a useful language feature you may not know about" seem completely fine to say at code review time, and it's not like you can teach someone absolutely everything before they get started.
Having a reviewer hold a review hostage is a big cultural red flag, IMO. If there's a real significant disagreement, you shouldn't try to solve it via code review comments; you should move to in-person communication if possible and maybe pull other people in to take a look. I also think that in most situations, reviewers should bias toward suggestions and ideas rather than demands ("strong opinions, weakly held"), and authors should feel comfortable pushing back or proposing to merge the commit as-is and leave some change for a follow-up review.
Saying code reviews are "the best tool" for teaching and communicating best practices is maybe a stretch; I find things like pair programming to be much better for stuff like that. But I still think code reviews can be a great mechanism for teaching code-level details like code style.
And if I tell you to re-implement working code my way simply because of some personal preference, i would expect you to tell me to fuck off. Cause that’s what i’d say if you did it to me.
To give you an idea of how our code reviews are perceived, we have one junior dev, at last retrospective she spoke up about how much she valued and appreciated them.
Obviously some places don’t get it, but i think we do.
But last time there were two "seniors", each with different conflicting preferences and each insisting that your code is "bad" and "mess". (And inexperienced project manager buying their bs because they were confident). And it changed based on what blogs they read last week. Juniors were more fine with that then me, because they tended to belive seniors know what they are doing. Plus juniors worked only on one module and I did on two, so I could see how what one called crap on the meeting is demanded by the other.
And the rules were different for different people.
I think it is the ability of coder to tell "fuck off" on changes he disagree with that makes all the difference. I was comiting code I disagreed with and had to come Sunday to make changes I considered useless.
I used to be for code reviews, but now I think that it requires people with strong self awareness, knowing difference between preference/feeling and objectively betters, and many programers out there don't have that. The seniors I had trouble with were on spectrum, one a little bit other a lot. They could not tell the difference and it cause a lot of problems beyond code reviews.
1. Decreased value: If a person is the primary (only) developer on a subsystem, any outside code reviewer will be less informed of the subsystem he is reviewing. I don't see any value in that other than get the other reviewer up to date on a subsystem he doesn't actively develop / maintain. I guess you could argue redundancies, but if you have a big system, having every developer understand every part of the system is unrealistic.
2. Increased value: if a developer is working on a new subsystem they aren't familiar with, then the 3-6 month window restarts. The 3-6 month window isn't the time of employment, it's really the time of understanding a particular subsystem.
My point is that doing code reviews in and of itself has decreasing value after 3-6 months. The 3-6 months is the time I'm estimating for a competent developer to familiarize himself with an existing subsystem codebase.
Also, any developer that still has issues after the 3-6 window of the same subsystem is a huge red flag. Code reviews will then just be a crutch for him and the entire organization when the real solution would be to get a better developer.
Also, I'm not saying any communication about code loses value. If I want a guy to look at my code and help me work though something (usually by just listening to me explain it), that has value. I'm generally speaking of the formalized code review that must happen after any coding change. That loses value after a developer is acclimated to the subsystem he has been working on, especially if the reviewer doesn't know as much about the subsystem he is reviewing (and has no need to know). I'm a very anti-hard-process guy. A process must have strong value, otherwise it's just another speed bump.
2) So lay down the law: no bitching over style, add a style verification step to your CI
3) you are still stuck on style.
I suggest you never skip code review, stick to functionality and make sure everybody's stuff (including the gurus) gets reviewed.
> 2. Petty arguments over style rather than technique. It can devolve into minutia / trivia.
This can be mostly addressed by using a linter. In our organization, we have a rule that if there is a glaring style issue, we ask whether the submitter had actually run the code through the linter before submitting it for review.
On the other hand, if the linter does not complain about some code, but the style it's written in is not agreed upon, we update the linter configuration to handle that case if the rest of the team agrees. Otherwise we leave the code as is (assuming it works as its supposed to).
> 3. Once a developer "gets the idea" of how the organization wants coding done, it becomes more of a seemingly pointless routine.
I find that there's always room for improvement in terms of how things are done and those things could be done in a code review. The other thing is that its always a good thing to have at least one other person look over a change just to make sure that its correct and something obvious wasn't missed.
Edit: s/submitted/submitter/
I agree that there is room for improvement, but after 3-6 months, it starts violating the 80/20 rule, meaning you are spending a hell of a lot of time & energy (money for business guys) for increasingly depreciating value.
>The other thing is that its always a good thing to have at least one other person look over a change just to make sure that its correct and something obvious wasn't missed.
Eh, developer should be reviewing their own code fairly thoroughly anyway. If a developer can self review their code, then the second person doing it costs a lot and doesn't get a lot of value. I wouldn't expect glaring mistakes from a developer that has lasted 3-6 months. (it's a requirement for employment, IMO).
Again, what you are saying is right, but I believe you get decreasing values over time as the developer starts to "get" how the org wants things done. Like I said, I think code reviews have great value the first 3-6 months, then ever decreasing value after that. I'm also assuming competent developers. If an org is hiring cheap coders, they don't really value development efficiency in the first place and my theory doesn't really apply.
I agree, but sometimes, when one has been looking through the code for a while, they can miss things that will immediately stand out to someone else who hasn't looked at the code recently.
For example, we had one developer on our team add a item to a JSON config file list and, since it was thought to be trivial, it didn't go through review. It turns out that they forgot to add a comma to the end of the quoted string. When it was time to roll out the change, we discovered the issue.
We have since enabled post-push linting on that repo, but I think that had someone else looked over it, they may have spotted that issue before it was merged.
> Again, what you are saying is right, but I believe you get decreasing values over time as the developer starts to "get" how the org wants things done.
To some extent, you're correct, but only if the code base is relatively static and conventions don't change. That usually doesn't hold true over time.
I agree with you, but it would seem rare. Maybe 1 out of 50 code reviews.
>For example, we had one developer on our team add a item to a JSON config file list and, since it was thought to be trivial, it didn't go through review. It turns out that they forgot to add a comma to the end of the quoted string. When it was time to roll out the change, we discovered the issue.
I would think in this example, the developer didn't bother to test his own code and he is using the code review as a replacement for that, which seems lazy. I would imagine the config error would pop up when he was running the application and that the config line he wrote was directly used by his changes. It would certainly pop up during QA.
A style guide and linter also remove the arguments about minutia. Google's python style guide is a lot less strict than the go or java styles, for example, and even still most of the style comments I make when reviewing code amount to "be internally consistent" about something, and those are nits.
And that whole "reviews take 5 minutes" doesn't mean that they aren't useful. I often find something that is either confusing, or a typo, or dumb mistake/overlooked thing or a non-idiomatic way of doing something, and leave a comment expressing that. The review still takes 5 minutes, but I've caught a bug or a bit of technical debt later.
As a code-writer, I have more or less the same opinion. I would flatly refuse to work somewhere that doesn't do code review. It makes working in the codebase difficult, and makes personal development more difficult.
Can you explain your code review process? Maybe we're just doing it wrong, or maybe we're just talking about different types of changes. We typically do it at the final merge, after developer and unit testing, before QA, and most of my code is new systems, new subsystems or substantial features.
The code review I do is subtly different in those two contexts. Within my team, my goals are to keep us to a very high standard of committed code, it should be clear, simple, and well documented. We want our output to be relatively reusable, well tested and testable, and well designed. This goes beyond our newly written code, we are stewards of libraries and tools, and we should maintain them as well as we can.
On the other hand when reviewing google-wide code, I am, yes, looking for bugs, but mostly looking for good "mastery" of python. That is, I'm explicitly looking for idiomatic python from people who are not as experienced with it. These reviews also are often larger and longer.
Within my team, we have a number of practices that help make code reviews fast:
- Most changes are small: within my team I reviewed 3 changes of more than 250 lines this quarter, compared to a dozen or so of <10 lines
- Most of the code written is really good. I rarely, if ever, have to mention lint errors, nor do I often come across bugs.
- My comments mostly revolve around idiomatic ways of solving problems and, as a team python expert, "pythonic" approaches, and better approaches using the python standard library and known tooling (ie. "You're reinventing the wheel here, just use XYZ", or "this is confusing, use pathlib.path instead.")
- The other big thing I comment about is clarity and readability, as a reviewer, I want to be able to follow your code, code is written more than it is read, and so most of my comments are things that amount to "This is difficult to follow as someone reading, can you refactor this to make it easier to read." These comments vary in both necessity and type, they can range from "hey maybe reorder your conditionals here and see if it reads better" to "holy wow it took me 10 minutes with a repl open and asking for help to figure out what the heck this function is doing, this needs to be refactored."
- And the last thing I suggest is future work to be done: "We should refactor this method in the future, we're having to work around it a lot", or a very common one: "We can parameterize these tests".
- The other big thing we do to make code reviews quick is that everyone does them. They don't just fall on the TL or manager or whatever. I review code for everyone, and everyone reviews my code, and as a result the code is not mine but the team's. Nitpicks aren't really a thing, because if someone is picky just to be picky, they'll get the same treatment in turn (and also because that's a dick move and we don't do that).
Note that for the most part, the things I'm looking for are pretty easy to notice, unidiomatic code is obvious, and "this is difficult or tricky" is also very clear. I still notice bugs and such, but they're rare, and we write code to make reviewing easy and errors obvious, and as a result have fewer of them.
The across-google reviews are on average bigger than within my team, and often result in more comments, because the writers are less good at python (everyone on my team has python readability, whereas the people I'm reviewing for python do not, otherwise they wouldn't need my review). They take longer. Many of them are still relatively small, but yeah the big ones that cross 500 or 1000 lines, and are in an unfamiliar part of the codebase, those do take a significant chunk of time to review (normally ~1-2 hours), but they're rare, I get one of those every couple of weeks.
In general I'd say the answer to how to make code reviews go faster is
These t...Someone sends me something to review and I see a notification pop up with a summary, regardless of what I'm doing at the time. More than 90% of the time (maybe more like 98%), it is from one of my team members, who has sent out something I knew they were working on because we had chatted (in a meeting, or at our desks, or on IM) about the need for it, and often also the approach they're planning on taking.
A lot of times (maybe around 50%?) this is configuration, documentation, or a very small code or test change. These I immediately skim for obvious problems like typos or confusing phrasing, check a diff of the config for those changes to make sure it does what it thinks it does, and almost always immediately approve the change with no comments. These often take less than 5 minutes.
Maybe 10% of the time, it's a fairly small functionality-preserving refactor. I defer these until I have some free time, because they aren't critical. I review them with an eye toward making sure I think they actually preserve functionality and decrease complexity and/or confusion. These can take 15 minutes to an hour, but aren't very disruptive because I can do them when I'm not otherwise busy.
Maybe 30-40% of the time, these are medium to large functionality changes to the part of the codebase I am intimately familiar with (again: the vast majority of these are coming from members of my team). Depending on what I'm doing at the time, I'll either start reviewing it immediately, or take a note reminding myself to take a look when I get a chance. I actually start by spending some time reading the description and thinking about whether I think what it's claiming to do is even the right thing to do; if not, I comment on that before even reading the code.
After that, I dig into the code. I start by reading the whole thing at a pretty high level to see if it seems to do what the description says, focusing on method names and signatures and especially on documentation.
If all looks well at the high level, I like to start going through the changes "outside-in", that is, starting with the higher-level methods and digging into the lower-level ones over time. Mostly what I'm looking for is whether everything makes sense to me when I read it, if not I ask for renaming, restructuring, or just more documentation if it seems fundamentally complex or confusing. In other words, I try to read it from the perspective of someone coming in cold and trying to figure out what's going on. This will be the case for everyone reading this code in the future, including me and the author in a few months. I also try to think hard about what assumptions are being made, whether they are right, and what should be done about it if I'm unsure.
After all this, I look at the tests. I start by checking for good coverage. If some things appear uncovered, I ask for tests, or suggest ways it could be tested if it seems tricky. If the coverage is good, I'm honestly kind of lazy about reading the tests themselves, looking for obvious ugliness, but mostly assuming that if they're passing and covering the branches, they're probably fine. Sometimes I suggest integration tests that I think might be useful.
If the change is in code that has lots of downstream clients, I'll usually do another pass focused on whether we might be breaking them somehow. Sometimes I have to do research on libraries that are being used or could be used instead of something else.
This can all take 15 minutes to maybe two or three hours of work time, sometimes spread across a few days of wall time, including any back and forth. I wouldn't say I go "line-by-line", though I do think I end up reading all the (non-test) lines at least once. I do feel I should completely understand what's going on, but that's usually built into my experience with the code ...
If you think you stop making coding mistakes after a few months (well, or ever), you're kidding yourself.
I've never got into an argument over code style in a PR, and it seems like if this did happen I'd be looking to fix deeper problems in the team. I've had comments in open source PRs asking me to change something, but that's part of getting into a new code base. The right style is there one that's there, so unless your PR has the sole purpose of specifically changing the style, you should comply with existing style, no arguing.
Anyway, there are lots of other pros:
A reviewer might suggest a better way of doing something, helping others learn, and likewise will pick up new techniques from the code they're reviewing. This applies to even the most senior, experienced person in the team.
If a new library is brought in, it can be checked for conflicting versions or overlap/redundancy with an existing library, and license compliance.
Noticing simple things like hardcoded parameters/constants, unchecked config or input values, spots with potential unhanded errors, among dozens of other problems.
Unrelated changes are usually caught. Even if it's fixing another bug, I'd argue it's better to do separately with its own issue, because otherwise it gets lost when you go to build release notes or a change log.
If you think it's so valuable, why not have every developer in the team review every merge? That's like 15 code reviews for each piece of code checked in. Obviously, it's because at some point you get diminishing returns. We have developer testing, unit testing, QA and UAT, all to catch defects before production. That's 4 layers of testing. Code reviews make it 5 layers. Not only is it a fifth layer, it's expensive because the most expensive people (in both wages and opportunity cost) are doing it. Reviews are just another iteration of essentially the same thing. At some point, you are spending a lot of money on little return. For me that is mandatory code review after 3-6 months.
In my experience running code reviews, shorter pull requests, presumably due to their reduced effort necessary to understand, tend to get better review, review that is more than just superficial style/linting errors.
In light of that, I always encourage developers to aim for the shortest reasonable changeset - and make it very clear to them that 2 line PRs are totally acceptable - on my current team, we go as far as to encourage tricks like rewriting (local) Git history and cherrypicking to ensure that is the case. Continuous integration and good unit tests help to ensure that strange states generated from that process are still good and I find the costs are far outweighed by the better reviews.
Another little lesson I've learned managing that process is to encourage the submitting developer to highlight problem areas in his own code: when I submit my own code for review, I'll actually do the review first, line-level highlighting areas I wish the reviewer to pay attention to. This will only work if you can trust your developers to not try to "sneak something by," but if you can't do that, you've probably already lost. Developers generally know what they weren't sure about during the process, where things are going to be difficult to understand and where someone else on the team is going to have helpful contributions to the quality.
I think it is important for developer-managers to remember that programming is largely a craft and developers largely want to be proud of their output. People _like_ producing "good code" and it is easy to align the goals by having the process help improve their craft.
What I've taken away from recognizing this pattern is to ensure everyone's checking in frequently, even in the early design and idea phase, so that nothing ever turns into a bad surprise in a code review. Depending on the seniority of the dev driving the feature, there isn't a single one-size-fits-all prescription for what's a "too big" code review, but the rest of the team's job is to make sure that whoever is driving the feature never ends up in a position where their technical choices put the delivery at significant risk - nobody wants to feel like their team left them to sink or swim and then only offered criticism when they finally submitted their code for review.
I'm also a huge fan of the described "identify what I dislike about my own code when submitting it for review" practice, too. I'll come straight out and ask "I don't like how I did this, anyone got better ideas?" and I've seen other people start picking this up on the team too.
I 100% agree that the shorter the changes, the better the reviews.
I am 100% sold on code reviews, because of:
1. the number of bugs found before they were pushed,
2. everyone writes code for other people now, whereas before we wrote the code for ourselves, and
3. improvement to APIs before they hit the code base and documentation.
Every single one of these things was a surprise to me, and I've been doing this a very long time. I never would have believed how happy I would be with the result.
Regarding other comments on annoying, small comments on style, etc: this is a problem with your people and your team leader. If this happened with my people, I'd have a conversation offline with them and tell this to not use code reviews in this petty way (to push their own styles). One thing it made me realize, every organization needs a style guide, but you need to make sure it doesn't go too far. Give people some latitude, just put the important stuff in it.
Religious adherence to processes gets in the way of that and if they block me, I'm happy to shove them to the side and get executive support to do so. My experience with code reviews is that they are 95% style nitpicks, 4% ways to make code cleaner and simpler, and 1% real bugs which impact end users. I only value the 5% myself. From a cost/value perspective, I haven't been sold on the value of doing code review on everything.
Not all developers feel that way, sure. But it is a mistake to assume all developers are driven by the same things.
When original programmer has right to refuse review, the percentage of useless code review comments goes down. Most of them are power trip anyway, if you remove power from it people cease to make them.
What you need is a respected conflict mediator, probably a team lead. This person should chat offline with both authors and reviewers who are frequently involved in conflicts, for instance because they often refuse to adopt the team's idioms as an author, or because they are often on a counter-productive power trip as a reviewer. If the team, or the company as a whole, doesn't have a person who can effectively mediate this sort of thing, that's a major problem, and something the management structure needs to be keeping an eye on as a major risk to the company.
Someone further down the page suggested that the style nitpicks arise from a lack of understanding and familiarity with that piece of the codebase. I think that is often true and a factor to be considered both when allocating code review and when setting expectations regarding code ownership and cross-functional understanding.
I've also provided some suggestions for how to tune down the "95% style nitpicks" elsewhere on this page - it is a problem, but with appropriate tooling and expectation setting, it can be reduced to a small fraction of total code review output. It would be dysfunctional for a team to spend time on that stuff when we've all agreed it isn't high ROI. I agree that code review can be done very poorly, but your observed ratios are not a fixed property of the world of code review.
Let me be clear: I am absolutely not advocating for a process that motivates nerding for the sake of nerding - I push back on a whole wealth of that sort of behavior. I am a business person motivated by producing sustainable teams that produce real value in the form of solutions to problems. I just feel that is a high-dimensional problem and certain kinds of technical debt can come at extreme cost and can, at times, if done correctly, be mitigated with processes like code review.
Once you eliminate that 95% overhead (which I think is an exaggeration, but I'll allow it can be high), the value proposition of code review is an absolute no-brainer.
Picking which among a few ways of doing something is idiom, not rote style. I agree that this is one of the least valuable parts of review, but people ramp up quickly on it in my experience, so it isn't constantly coming up. If an idiom actually matters, for instance if it is protecting against some commonly problematic pattern, it may be possible for tooling to catch that too. If it really doesn't matter which way its done, the reviewer should just let the author do it their way, and if they don't, the author should just change it without making a fuss. But yeah, arguing over idioms can be a problem. It helps to write down decisions the team has made in the past, so that you can just point dissenters to the documentation.