What I find more though is code that isn't thought through. Tech debt and code smell are real, and they affect the performance of a team. Nipping that in the bud takes quality PR reviews and time to meet with submitters around issues you find.
Knock on wood but working at the company I do now where I, along with my team, have made quality PR reviews normal.. our codebase is now enjoyable and fun to work on. I highly recommend it!
One key aspect is being “kind, not nice”. Be helpful when leaving comments in PRs, but don’t be nice for the sake of avoiding conflict.
Also if you find code reviews to be a waste of time I can reccomend one thing I do often - give warnings. I approve and give comments around things I’d like to be fixed in the future for similar PRs. I don’t hold up the merge for little things, but at the same time I won’t let the little things slide forever
I think what I struggle most with is that often times there's a valid business reason to "just ship it ASAP", but the missing piece is the accountability around the conditions attached to that. Like, okay, if we don't want to fix this now because it needs to be in the next release then we can merge it as-is, but you can't document this externally, it can't because part of an API, and there can't be any further development in this direction until X, Y, and Z have been rewritten to be in like with ABC.
I find it profoundly hard to get buy-in for those types of discussions. Everyone is happy to smile and nod and the appropriate tickets are filed with deadlines attached, but then the next release rolls around and there's new business-imperative stuff that's the focus and the cleanup tickets are quietly moved to backlog with the deadlines removed.
Seeing this repeated over a number of years has left me with kind of a cynicism about the process, where it feels like code review is at least partly an exercise in frustration; I don't have the backing required to insist on doing it right upfront, so instead I'm really just getting a preview of what is going to land in my lap a year or two from now.
1. A lot of problems arise from too few people working on too many things. If it's one-two devs and backlog is growing, the problem is not that you have no time to fix things, but that you're understaffed. If you have enough people, then from the business perspective it shouldn't even be that noticeable that someone is refining previous work, while someone else is building the next thing.
2. If you're not understaffed, then the best time to clean up new code is during or immediately after writing it. A phrase I like to use is "while it's still fresh in memory". You're saving time and not adding new bugs, by not having to remember everything again, load all that context back into your head.
Once I did a final review of a backlog of a project that got decommissioned after a decade of development and use. Most of the "fix tech debt" tickets were still there in the backlog. And how could it be otherwise if the tech debt tickets always got assigned priority 2, and everything the management wanted done got assigned priority 1, and there were always more priority 1 tasks than we could fit into a spring?
Next time, I will have no confusion about what "priority 2" actually means. It's in the backlog to make you feel happy, but you are not supposed to actually ever do it. (If by some miracle all current "priority 1" tickets get done at some moment, some of your team members will be assigned to a different project.)
- The same people leave detailed comments on others' merge requests, but get discouraged when nobody else puts in the same amount of effort for theirs.
- People blindly accept suggestions with no resistance or discussion to get the review over with.
- People send their MRs to side channels or other teams to get their changes merged in without resistance or back and forth. (I've had to revert several of these).
1. PRs are supposed to wait for 2 acceptances, can be shipped with 1, and can be emergency shipped with 0. So the barrier is low, but the culture supports more. We are expected to get 2 reviewers from our team to okay.
2. Depending on the code project, we have to fill out a template for the PR, what is in it, what it changes, what to look for when we test the code, etc.
3. Some areas have code owners that might require an additional review from a specific team.
4. We are expected to check out, and test branches when we review them. So a quick read and LGTM is really discouraged outside of a few small cases.
I have seen a lot of places that do the blind PR acceptance, and its tough because without this really being enforced and encouraged that culture is hard to change.
There's always going to be pushback from adding more process, but if there's an understanding amongst the team that keeping things working is P0 then these processes will slowly/naturally come up as the team realizes that investing in them proactively will save them time down the road.
For a team not used to code reviews, they might seem more trouble than they're worth at first. Most likely they will be more trouble than they're worth for the first few months. Keep doing them and eventually your smart developers will figure "if we have to do this anyway, we may as well find something useful to say" :)
A few things you can do to make it smoother:
- Manage expectations. Initially it may be as simple as "we just want to have a second pair of eyes on every change" or "be aware what other team members are up to" - i.e. communication first, improving code second.
- Set up your tooling to make the process smooth. If somebody wants to just get it over with - it should be easier for them to use your official review process then to use some side channel. A vicious alternative is to make using side channels harder ;)
- Leverage automation. Run tests, linters, static checkers, etc. on your PRs so that developers get something useful even if no human leaves interesting comments.
- If some team members already have experience with code reviews - adjust their workload so that they can do more reviews. They are effectively training others by example.
- Make sure that code changes under review are reasonably sized. Encourage submitting changes for review early and often. "Here is the feature I worked on for 3 months, and it's due on Friday, please review" won't make anybody happy.
- Make it less intimidating. Code reviews are not just for finding bugs and flaws, encourage reviewers to say positive things as well.
I would raise all these issues and more in group meetings. Try to get people to understand the many different benefits review brings to both the committer and reviewer - by having them state the benefits they want or could see getting. Talk about various kinds of comments (clear bugs, performance, style, robustness, factoring and organization, etc.), and the various priorities from no-action-required to nits to blockers. Talk about the priority of reviews themselves. Reducing the latency of reviews has a huge positive effect, ime.
I ask because I had this one job, where the tech team was a few nerdy programmers in one office, before COVID, and a bunch of people in a friend group I wasn't part of, after COVID.
By that I mean, before COVID it was common for the founder to take us out for lunch or tennis as like official team building time. I loved this because I'm a picky eater and it's hard for me to make friends, so if the company makes official initiatives, it's easier for me to fit in.
After COVID, the official initiatives weakened. The team was too big to take everyone out, and I didn't join the friend groups who naturally found ways to socialize.
In that new environment I no longer felt like an equal member of the team, I felt like an outsider who had authority on paper but didn't have any of the camaraderie needed to get things done and survive a work day.
Even though everyone repeatedly said I was respected and valued as the most senior programmer, I found it impossible to be a good teammate in that new environment, I felt like I was just spending all day being mean and nobody got a chance to see me as human. That was part of why I quit.
In that environment my code reviews sucked.
Now I'm at a remote company where once again it feels like everyone is equally non-social, and I'm just gonna ride that as far as it goes. If they get an office I'll probably cash out and go on vacation for a year
Edit: almost forgot, the other woman who was part of the original "nerdy programmer" team, ended up also burning out and quitting about the same time as I did. She also didn't really make friends in the new environment, and seems much happier pursuing her hobbies and taking it easy between jobs
This is always a precarious situation. Because as soon as these people become jaded, your ability to make good PR culture will also vanish. And they can become jaded for many reasons. If these people are not explicitly or implicitly valued, they will know. If people who are doing the incorrect things are getting promoted first (or even at the same rate!), the same raises/bonuses, and on all accounts are treated equally, the employee will almost always converge to "well why am I putting in all this extra hard work if it's not benefiting me in any way?" And I don't think promises of early promotion or similar have a good effect because there's many employees who've had those promises made to them and it not follow through[0]. So there needs to be some, even if incredibly minor reward in the shorter term.
Also, do not underestimate the value of explicitly saying "good job." There's often a huge bias in communication where it is only made when something is wrong and when good work is done that it is left unsaid. You don't have to say it for everything, but I think you'll be surprised by how many people have never heard this from management.
[0] I wanted to share a story of an instance I had with this. I was a green (mechanical) engineer working at a startup. I had a physics degree instead of a ME, but have always been hands on. But because of this I was paid less and not valued as much. I asked my manager what I would need to do to get promoted and to be on par with everyone else. I got it in writing so I could refer back to it. At my next performance review I was just talked down to. Complaining about how I didn't do this or that (sometimes things that were impossible and sometimes they were weird like "your code may have been 20% faster but X couldn't understand it so we can't use it" -- X was a manager who had only been writing in C++ for < a year and I __heavily__ documented my use of functors). I asked about the things I did and the promises. They admitted I did all of them and even more. One of these being getting a contract (I think they put that there not expecting me to get it), and I was the only non-manager with one, bringing in 20% of company revenue while being the only person on that project. You can imagine I walked out of that meeting polishing up my resume and I was strictly a 9-to-5er doing the bare minimum from that point on. But the next manager I had, was liberal with complements and would critique instead of complain. Understood that there were unknown unknowns and all that and would actually tell me to go home when I was putting in overtime. I never worked harder in my life AND it was the happiest I had been. A manager can make or break an employee. And to part of this is that there may be ways to get back those broken employees, but you might need to figure out why they became broken in the first place. And if it is something you can fix or not. I believe environment has a big impact on employee attitudes and thus, efficiency/productivity. If passion is worth 10 IQ points, then happiness is at least a big factor in making an employee productive. Everyone can win because it isn't a zero sum game.
- The team is understaffed, there is not enough time to do things properly. I can write the code in a day, but it would take two days to make it really good (to do the code review carefully, to address the comments, to review the rewrites), and at the end I would get scolded because my measured productivity got too low. In other words, in theory the company wants code reviews, but in practice it rewards those who skip them, and punishes those who actually do them. Also, the more busy people are, the longer I probably need to wait until someone finally reviews my code.
- Big ego. Some people take a suggestion to improve their code as a personal offense; as if the other person was telling them "I am a better programmer and indeed a better person than you". (Often the same people are quite happy to give a lot of comments to their colleagues, some of them genuine improvements, many of them merely "I would have done it differently" except they insist that their way is always better.) If you have such people on your team, you are going to have problems. In my experience, about 10-20% of men are like that. (No idea about women; I would expect the fraction to be smaller, but I didn't have enough female colleagues to verify this.) I prefer teams where no one is like that, because one person like that can ruin the entire team's dynamic.
- Lack of experience. A junior developer reviewing another person's code sometimes simply doesn't know what is good and what is bad. "If the code compiles, it's probably good? Wow, there are even unit tests, and those pass too; what else is there to complain about?" But I think that even having the code reviewed by a junior is a good thing; at least this gives them an opportunity to ask why a particular approach was chosen.
- If you only have one person on a project, if someone working on a different project is supposed to review their code, they probably feel like "I don't know much about this project, I don't know what their constraints are, how the rest of the project looks like... I guess, unless I see an obvious horrible error, I will just approve it".
So basically it's either the ego or external pressure (or both). Depending on your position in the company, you may be unable to do anything about that.
Some people have an ego that you can't fix. You can avoid hiring them, but if they already are on the team and they otherwise do their job well... You just can give more attention to this in the future. For example, if you have a group of people who cooperate well, do not just randomly redistribute them to other projects once their project is over; instead maybe try giving a new project to the same group. When interviewing new team members, always ask for feedback from the existing team members.
Some companies create toxic environment by comparing developers against each other, sometimes with the explicit goal to fire the least performing 20% each year. In such environment, obviously the code reviews also become adversarial, and people will try to avoid having their code reviewed, and some will use the code review as an opportunity to harm their competitors. In a cooperative environment, code reviews work much better; it's people working together to achieve a common goal. To create a cooperative environment, you need to treat the team as a whole. (For example, never measure individual velocity, because people will obviously soon notice that helping their team members may hurt their personal metric. You don't want to fire a person just because they spend too much time helping their colleagues with their tasks.)
EDIT:
If you use some automatic system to highlight problems with code, it will probably hurt the ego less than a similar comment made by another human. Just make sure to configure the tool properly, for example to write comments but never actually block the commit, otherwise developers will get really angry about the false positives.
I would work for that man again in a heart beat. Because for as much as he was apt to yell, or dress me down, he was also willing to give good advice, to elevate, to teach.
The office is not a safe space. You seem to know what's wrong, IM sure you have asked nicely. I am sure you offered the carrot, but does your team know you have a stick?
> The same people leave detailed comments on others' merge requests
Call these people out, in public, for doing good work. Tell everyone they are setting the bar and others are not living up to it.
> People blindly accept suggestions
Coaching, lots of one on one coaching about finding and having a voice. Lots of "team building" where you level out the playing field with the strong vs weak voices. Figure out what those quiet ones excel at and do a fun activity around that. Let them find legs...
> People send their MRs to side channels or other teams
Stick. Harshly worded emails. Down dressing in public. Telling your team that in no uncertain terms that "this is unacceptable behavior"
As for the chair thrower... He was always fair, he always had his team first, I grew as a person, a manager and an engineer working for him. Its not growing happy go lucky good times while I get a pay check, its Growing pains, spreading that (pain) around is part of your job.
He encouraged his team to discuss an approach beforehand or to work on something together.
Other than that they had a lot of tests and a very structured codebase, I guess it worked for them.
I found it utterly exhausting. I was somehow working at 100% capacity but producing output at 50% because so much of my cognitive bandwidth was taken up with the pairing process.
CR slows things down & often unnecessarily. Overly time consuming
Time lost with needless explanations.
Somewhat impossible when reviewer pool is one or two people. A bigger reviewer pool is probably not going to have to have context.
Creates a bias to not ship. "waiting for CR" can be a big impediment for shipping. Perhaps that tonight was the good time for you to get something into prod, not tomorrow - but you're waiting for a CR.
It's an example of process over people (AKA: cargo-culting). The context of when/where/how/why CR is important and is situational. CR best practices are going to have different results in different situations. Often CR is just done because it is a best practice, blindly. It would be preferable to think deeply about what is important & why - rather than just a "oh yeah, here is another hoop we jump through because #AmazonDidIt"
Stratifies the team. The senior/elite/favored people invariably get easier times during review.
CR can get political. Scrum masters & managers scrambling to unblock a team member and get something reviewed. Which is great for that one time, but reveals an utterly broken process otherwise. When a CR is "escalated", what's the chance the reviewer will actually spend the time needed and the back-and-forths to get things "properly" reviewed?
Conducive to nitting, which are are useless and counter-productive. Time spent on nits is waste and draining to keep coming back to something to then tweak "is it good enough yet?"
Drive by reviews without context
Coming to agreement during CR is difficult. Not everyone is able to observe/experience and/or resolve conflict.
CR is late in the code development process; it's one of the worst times to get feedback after something has been done, polished, tested, made production ready - and suddenly then someone thinks it should be done differently (which is a lot easier to see when something is already done and working). It's akin to writing code 3 times, once to get it to right, a second time to get it nice, and a third time for whatever the hell the reviewer wants.
Shows lack of trust in team. It is gatekeeping.
Does not scale well. I was once told by a reviewer that I write code faster than they could read it. (At the time I reviewed about 6 PRs every day, and sent out about 3. I was doing 80% of reviews, it was a shit-show; the reviews I received were slow and useless - the team members were focused on trying to deliver their over-stretched projects; I was too streched to give proper feedback and not work 100 hours a week).
Better options exist, namely the "ship/show/ask" strategy: https://martinfowler.com/articles/ship-show-ask.html
That latter branching strategy puts it up to the code author to decide, "does this comment typo fix need a review? Does this fix in this code that I authored originally and know everything about - really need a blocking review?" In that last case, if a person can just merge a bunch of precursor refactors right away, they can get that knocked out - the PR they send out for review is then clean & interesting; and there is no time lost managing branches or pestering someone
A second option to help make things better is to allow "after-merge" reviews too. A few teams I've been on, we did that enough where we learned what kinds of things are good to ship on their own. To another extent, there wasn't the bandwidth to review everything. It was not a bad thing.
Always when joining a team the first thing I tell devs is "I don't care how critical you are, just be honest" I think setting expectations early on is very critical. I think people not feeling attacked / too defensive of code is a good step forward. People who vehemently defend their code are bad developers imho.
I just move on. At some time he will realize how bad his code is.
If it's fine for now, it's fine. If it's not fine - then speak up.
If you do have such concerns, "fine for now" - that is something for an offline conversation - outside of the context of a CR. Which is also a good litmus test for how important a comment is. Would you schedule a 15 minute meeting to discuss the issue with your colleague? If not, then why is it okay to do that to someone during a CR?
What! I would be livid if someone scheduled a meeting with me about a PR. We have way too many meetings already, this is one of the only processes that is mercifully async.
Personally I like it when teams start with in-person CRs if they are not used to them. Only after healthy relationships and conflict resolution mechanisms established, do the PRs go async. I also like a rule that there should never be more than 2 round trips on communications in a CR, otherwise it should be done at the same time.
Delaying small changes to allow for such round-trip times is not feasible in a lot of environments. There's too much to be done. Not worth it. When projects do fail, it's very rarely that some manager slaps a big "failed" label on it. I bring that up to say that projects fail all the damn time because the team is moving too slow - and individuals often don't appreciate when they actually did need to move faster (the feedback loop is often missing, of, "oh, we actually were too slow, we needed to ship faster than we actually did. The JIRA, planning, estimation, CR, best practices, time spent fixing linting - it all doesn't matter cause this project actually failed - it took too long.")
But statistics are tricky. With the example given in the article "15% of smokers get lung cancer" compared to "80% of people with lung cancer smoke." These two are not in contradiction with one another but are just different ways to view the same thing. In fact, this is often how people will mislead you (or how you may unintentionally mislead yourself!) with statistics.
Another famous example is one that hits HN every once in awhile: "Despite just 5.8% sales, over 38% of bug reports come from the Linux community"[1]. In short this one is about how linux users are just more trained to make bug reports and how most bugs are not system specific. So if you just classify bugs by the architecture of those submitting them, you'll actually miss out on a lot of valuable information. And because how statistics work, if the architecture dependence rate was as low as even 50% (I'd be surprised!) then that's still a huge amount of useful bug reports. As a linux user, I've seen these types of bugs, and they aren't uncommon. But I've frequently seen them dismissed because I report from a linux system. Or worse, support sends you to their page that requests you to "upvote" a "feature" or bug issue. One you have to login to. I can't take a company like that seriously but hell, Spotify did that to me and I've sent them the line of code that was wrong. And Netflix did it to me saying "We don't block firefox" but switching user agents gave me access. Sometimes we got to just think a bit more than surface level.
So I guess I wanted to say, there's a general lesson here that can be abstracted out.
[0] Everyone jokes that stats are made up, but this is equally bad.
Microsoft does this for enterprise products where customers might be paying $100K/mo or even millions.
“We hear you, but your complaint is just not popular enough so go away.”
“Sure it’s a catastrophic data loss bug that ate your finance transactions, but if other people can’t identify that their seemingly unrelated crash is the exact same issue then no fix for you.”
“Now that you did get ten thousand votes on an issue titled ‘Consiser doing your job’, we’ve decided to improve your experience by wiping out the bug forum and starting a new one from scratch that has fewer scathing comments from upset users.”
That's a tremendous improvement when compared to the time I interacted with them.
That's why looking at % is dangerous. You could be finding 5 bugs per code review, which is a lot, but if you also make 30 other non-bug comments, suddenly "only 15% of comments are bugs".
Code reviews can also do more than just find bugs. You can point out a better way of doing things. Maybe this SQL could be more efficient. Maybe you can refactor some bit of code to make it more robust. Maybe you should put a logging statement here. This method name is confusing, may I suggest renaming it to xyz?
Reviews _from_ juniors or new team members are also really valuable, as they don't have the history or tribal knowledge that others may have. They'll often spot things that have gone overlooked because "that's how it is".
I have definitely found bugs [ETA during code review] in code written by very senior developers in code they've been familiar with for over a decade.
CR does give others a chance to study code, and become familiar with it - but that is different from "keeping up to date."
The abstract of the paper:
> Because of its many uses and benefits, code reviews are a standard part of the modern software engineering workflow. Since they require involvement of people, code reviewing is often the longest part of the code integration activities. Using experience gained at Microsoft and with support of data, we posit (1) that code reviews often do not find functionality issues that should block a code submission; (2) that effective code reviews should be performed by people with specific set of skills; and (3) that the social aspect of code reviews cannot be ignored. We find that we need to be more sophisticated with our guidelines for the code review workflow. We show how our findings from code reviewing practice influence our code review tools at Microsoft. Finally, we assert that, due to its costs, code reviewing practice is a topic deserving to be better understood, systematized and applied to software engineering workflow with more precision than the best practice currently prescribes.
“Code Reviews Do Not Find Bugs: How the Current Code Review Best Practice Slows Us Down”
https://www.microsoft.com/en-us/research/wp-content/uploads/...
The same is true of "code smell" issues: Everyone who asks you to change things is a pedant who's slowing down the project for pointless aesthetics, and everyone who pushes back against changes you've requested is a cowboy who is going to make the code harder to maintain in the future.
So in the paper, how did they decide whether a non-bug change "should block submission" or not?
If you think about it, as bugs/defects are removed, the code becomes more correct and thus more stable because it doesn’t need additional changes to remove bugs, so removing bugs reduces the need for future maintenance.
If we block due to future maintenance concerns what we’re really asserting is that the requirements are unstable, and that removing today’s bugs is less valuable overall because requirement changes will remove the line of code with the bug and replace it with a new line of code with a new bug.
I suppose it depends on the code review process at at a given organization whether that’s the appropriate point at which to block code for architecture/design issues. In my experience the code review step is much too far downstream in the development process and much too narrowly focused on a subset of code to be an effective place for design changes that have significant impact on maintenance.
[1] The paper authors reviewed data in Microsoft’s internal code review tool, which is proprietary, so we can’t see what the specific bugs were.
Developers are more careful about what they write and submit when they know they’ll have someone else looking at it.
We went through a couple iterations of our code review policy on a multi-year project a while back. We never really saw code reviews catch a substantial number of bugs over time, but whenever we pulled back on code reviews we definitely saw the production error rate go up.
Working from 6 years, not much, and not in as many places like others, I have built the opinion that code reviews are like tests, they should be used as a tool when they are necessary, they shouldn't be the default for every change.
In best case scenarios is the person creating the pull requests that requests reviews or decides the place needs tests.
My opinion obviously applies to product software, for libraries, especially when publicly exposed you want as much discussions and tests as possible.
Code still gets reviewed, but you don't end up with PRs languishing for hours, days or even weeks waiting to get a review from someone.
Basically when I start the PR is approved in my head until I find something blocking. I.e. major problem that causes dataloss, big performance issue or breaks other code. Anything else is a minor comment at most. The PR is approved by default. This gives the dev responsibility and ownership. Plus it increases release cadence
Doing a follow up PR to improve or fix something is just as fast as blocking the MR, but blocking the MR can be bad for morale.
This strategy might work better in young startups where having the feature _exist_ is better than not shipping. in my experience this builds up responsibility and ownership and removes the whole policing of other peoples work vibe around code review.
Also decisions and discussion around formatting, when to test, design, features, functionality, architecture should not happen during code review, they should have happened way before coding, or collaboratively while working. Code review is the worst time for that stuff, imho it should be to sanity check implementation.
Interesting enough, this bit of code/project, that didn't have super strict code review requirements, but had a lot of tests, is the code I worked on that I would consider the most robust/high quality. It was run by > 10 million users in a fairly important application. It wasn't huge and it had good tests (and was generally quite testable).
That said, it's really hard to control review-after-commit. Maybe we need better tooling for that. In my case, for the areas of code I was involved in, it was small enough to track in my head.
The main difficulty I see with the described approach is that different changes will be interleaved in the trunk and it might be hard to extract just one of them to deploy. But that's what feature flags are for!
It is also true for the feedback on MRs. Providing compressed and succinct feedback makes it faster to address.
Almost like “if the change is difficult, refactor and make the change easy”. There are many ways to do one thing, some are better, some are not.
Companies and teams that have good review culture are successful in using the reviews as a tool.
In a huge org, with thousands of engineers that's already burdened by hours per day of interruptions and process overhead, and release runways that already involve six stamps of bureaucracy, mandatory code revies have very little downside (it's in the noise) but highly variable return (many people are just droning under the weight of process). The org loses nothing much for mandating it, but only certain teams will see a lot of value for it.
On the other extreme, a startup with five engineers will get backlogged with reviews (which then get shortchanged) because everbody either is under pressure to either stay in their high-productivity flow or put out some pressing fire. The reviews probably could catch issues and share critical knowledge very regularly, but the org pays a pronounced penalty for the overhead and interruptions.
People long for "one size fits all" rules, crafting essays and publishing research papers to justify them, but the reality of what's right is often far more idiosyncratic.
Instead, I think there are two far more interesting questions to ask:
1. Is the rate at which code review identifies defects sufficient to use code review as a detection mechanism for defects?
After nearly 20 years of writing software, I'm pretty convinced that the answer here is no. Some reviewers are better than others, and some circumstances are more favorable to finding defects than others, but we should generally try to build processes that don't assume defects will be caught at a substantial rate by code review. It's nice when it works, but it's not a reliable enough way to catch errors to be a load bearing part of the process.
2. Is mandatory review of all code justified?
This is the one I'm on the fence about. In an environment where code reviews are high priority, people are trained to review effectively, and there are minimal organizational politics at play, then I hypothesize that allowing PR authors to decide whether to get a review or not would generally improve quality and velocity because code would ship more quickly and code that would benefit from a review would still be reviewed. In that scenario, I think we'd see the benefits of getting things shipped more quickly when they don't require a review, and reviews would be higher quality because code being flagged for review would be a positive sign to pay more attention.
Unfortunately, I could be wrong, and it's not the sort of experiment anyone wants to risk their reputation pushing for, so I doubt we're likely to see an experiment at a large enough scale to know for sure. If we're going to fail one way or another, I'd prefer to fail by doing too much code review rather than not enough.
There is a trick in pharmaceutical research where you test a potential candidate drug against placebo to yield a bad study that seems to show benefit. The reason it is a trick is because in many cases the alternative isn't placebo, it is an existing treatment. Then doctors learn about a more "modern" treatment, favor it for being modern and the better treatment may not be prescribed.
The alternatives to code review aren't doing nothing. The article claims that code reviews find a defect per 10 minutes--but only in the first ten minutes. By this same argument (ignore qualifications, extrapolate the numeric result), fast automated testing can potentially find thousands of defects in a second--if they run that quickly and the defects were already tested for. Static analysers, pair programming, documentation these are all alternatives and there are many more.
If you're spending an hour a day reviewing code then you are spending 12.5% of your time doing it. Using it that way comes with an opportunity cost that may be better spent depending on your particular organization and code base. Of course, analysing everything to death also has an opportunity cost, but not analysing it generally leads to moving goal posts where the supposed rationale for doing something keeps changing. Today its purpose is uncovering defects, tomorrow it is knowledge sharing, the day after it is security. It is all of those things, but other practices may achieve these goals with more effective use of time and people's patience.
So why am I pro code review? Because choosing to interact and work together as a team, to learn about and compromise with your colleagues makes for good team building while serving other technical purposes. I do think that pair programming can achieve this to a greater level while also being more taxing on individuals. This assumes you control the process and own it though, if it has just become a rote ceremony then my feelings are you probably aren't net benefitting from it: you are simply doing it because you have no choice, not because you believe it to be a valuable use of time. If you have experienced both, a culture where people choose and find value in code reviews and a culture where people are forced to do it unquestioningly, then you may have witnessed how a dicta can destroy the prosocial value of a practice.
It's extremely difficult to adjust the time spent on reviews. The options are unattractive. Do you start blindly accepting changes when you hit the limit, or just stop and not let people merge code?
Unless you don’t trust your colleagues. If that’s the case, then code review is doomed anyway
Oh.
It normally takes me a few seconds to find bugs in code.
I always felt this was average performance for assessing software. If the average time is ten minutes per defect, I need to recalibrate my expectations for myself.
I've never had an incentive to read the Linux kernel code. I routinely find and disclosed cryptography library bugs, though usually mostly hobby projects like the "I thought it would be cool if there was a PHP implementation of GHASH" sort rather than like OpenSSL.
I see some comments about time. How long does a code review take? I can review hundreds of lines of code in a few minutes. It is much easier to review code than to write code imo, especially as you gain experience. For bigger efforts, get eyes on it throughout the process.
I’ve met a lot of developers who assume their code will just work right after they write it. They don’t test it, via code or manual qa. Then they act surprised when the stakeholder tells them it doesn’t work. Do the job right the first time. Slow is smooth and smooth is fast.
You do bring up a good point about using change defect rate though. I wish the researchers had cited that as the preferred unit of measurement. I did some research on change defect rates on popular open source projects and it's all over the map. Ranging from ~12 - ~40% [1].
The future I'd like to see is as developers we use objective measures to justify time investment for review. This is going to be increasingly important as agents start banging out small bug-fix tickets.
[1] https://www.shepherdly.io/post/benchmarking-risk-quality-kpi...
I can count on one hand the number of times someone has caught a bug in my code that should have stopped deployment. Not that I haven’t deployed serious bugs to deployment, but they’ve almost never been caught by someone reviewing my code.
Occasionally someone suggests a better way to do something, or asks a question that ends up with me coming up with a better way of doing something. But those situations are also rare. And I can’t think many times at all when the impact was worth the time spent on the process.
Pair programming and collaboration can be hugely beneficial, but the minimal effort PR approval culture we’ve developed is a very poor substitute.
Getting someone up to speed with unfamiliar code, disentangling hairy code so it becomes clearer, hunting down bugs or finding unknown unknowns such as bugs or unnecessary complexity.
However in many cases not looking at the screen when doing these kinds of things is more helpful. It's often more beneficial to build a mental model in your head and then riff off each other. Rather drawing things on a board or writing down stuff in a markdown file, explaining things in simple terms, than actually coding or reading actual code.
Not sure if that still counts as pair programming or code reviewing but this free form way of talking about code is very effective.
Even a low effort code review can identify missing unittests
Also what type of review? Is this a prototype needing a high level design review so that the actual review doesn’t turn into a design review? How often does that occur?
Who are the reviewers and what’s the process? Key stakeholders have more influence and you need to consider the reviewer’s experience, knowledge and credibility.
Finally how important is the code? Is it kernel code, or high execution daemon code needing race condition and memory leak checking? Are you using static analysis for the code? Does the code even compile and do what it is designed to do? Where are the unit test logs?
Lots to consider.
Coding standards. Don't submit code that has rando extra lines and things that will slow down the next dev from looking in to the past to learn what does what.
And most of all, make sure edge cases are covered, often via a truth table of all possible outcomes.
I often comment on a PR saying "blah blah, but not blocking" so I'll allow it but at least my entitled opinion was known in case that code issue comes up later.
My PRs take a long time, because I dig the F in.
It gives so much more context to a PR.
I think there should be a manual QA process to test the functionality of what the developer is pushing out.
The issue with code reviews is always that they take so much time for another developer and many devs are super busy so they just have a quick review of the PR and approve or feel they have to add some comments. Context switching between what the dev is already doing and having to come to the PR to review properly means they should switch to that Git branch, pull down the code, test it all and check for logical bugs that a static code review won't pick up.
For juniors, code reviews are still useful as you will be able to spot poor quality code, but for seniors, not as much for the reasons above, better off having a QA process to find and logic holes rather than expecting devs to invest so much time in context switching.
There is an approximate non linear relationship between time it takes to produce the first PR and time it takes to go through all rounds of review. This time can be pretty reliably calculated and taken into account.
A good reviewer can call out bad strategic coding decisions or misinterpretations of the requirements. QA is another layer of review entirely.
More often, code reviews become opportunities for team members to bikeshed. Worse, an opportunity for a non-team member to exert power over a project.
More generally, code review is a great opportunity for incrementally gaining or encouraging alignment across the team.
What the team chooses to align on and how strongly are left up to it, so hopefully they choose to not get bogged down in inconsequential details, but completely skipping the pretty cheap chance for reenforcing all kinds of cohesion would be a big mistake in my opinion.
Conspiracies to delay code reviews for high performers in stacked ranking organizations is common.
They can also be ruined by having the not rotating the reviewer role among eligible reviewers in the team, in that case, everything just represents the opinion of a specific group.
- Code reviews prior to acceptance of commits (Facebook does this)
- Refactoring crap that manages to get through
- Removing features
- Higher-order languages with less code
- Removal of tech debt
- More eyeballs
- Wiser engineers
- Dedicating more time to better engineering
- Coding guidelines that optimize for straightforward code while not being so strict as to hinder strategic exceptions
- Negative LoC as a KPI
