Confessions of a programmer: I hate code review (2010)

1. Both reviewer and reviewee are focusing on getting the best outcome possible, in good faith and with generosity.

2. The reviewer concedes that there can be equally valid approaches to a given problem or taste wrt. aesthetics. They do not try to gratuitously force their style upon the reviewee. The reciprocal must hold as well.

3.The reviewer makes practical suggestions and ideally accompany their comments with snippets of code. They involve themselves into being part of the solution.

4. The reviewer focuses on the substance of the pull request. That means putting in the time to understand the real logic, putting themselves in the shoes of the reviewee (with their help), and trying to challenge it so that the limitations of the approach are known and documented.

5. The reviewee makes an effort at slicing their request into readable and compact sub-PRs if needed. Similarly, the reviewer makes a best-effort attempt at starting their review within a reasonable time.

Those are central aspects of a productive engineering culture anyway. You only need to find someone else that shares those "values" (i.e focusing on the best outcome possible as a sort of devotion to Engineering - so to speak) to grow this attitude within your company.

Shipping bugs.

The author of this piece admits that they have to change the code they write in response to comments. This means the code review is flagging areas of improvement. Surely the resulting code is better than the original code (otherwise I'd expect the author to push back on the comments instead of implementing bad suggestion).

Similarly, when acting as the code reviewer, the author is giving feedback to others (rather than just rubber-stamping the PR), and presumably that feedback is implemented.

So code review seems to be doing its job here. It's producing better code. Ultimately I think the problem is revealed at the end, where the author says they feel like code review is optional. If you think it's optional, then the delays inherent in review are going to feel like artificial slowdowns.

The better way to think about it is that code review simply isn't optional. Yes it can be a pain, and there's a lot of friction in the process, and our tooling kinda sucks for it. But it's a necessary step. I can't even begin to count how many bugs my team has avoided shipping due to code review. And I always get really annoyed when I run across a bug that should have been caught in code review but wasn't for whatever reason (perhaps a more junior coder did the review and therefore missed stuff that a more senior coder would have caught). Code review stops bugs before they happen. And the best bug is the one that was never shipped to customers.

* "I don't have time to review people's code, I just want to code"

* "I don't have time to have my code reviewed, because if it's bad I'd have to fix it"

This really mirrors people's complaints about unit testing:

* "I don't have time to write tests, I just want to code"

* "I don't have time to run tests. If they're failing I'd have to fix it"

It also mirrors the 19th century complaints from surgeons who didn't have time to wash their hands, because they had a lot of surgeries to perform...

I suggest code reviews' primary purpose should not be to catch bugs or trivial style issues; automated tests and linters exist for those purposes.

Instead, code reviews should be primarily thought of as "human comprehensibility tests".

When we write code, we've may have spent a significant amount of time thinking about the problem. This puts us in a bad position to gauge how readily someone in the future can reason about our code in order to fix or modify it safely and easily. Code reviews are a mechanism for showing empathy to our future selves, which accumulates over time into a much more pleasant coding environment.

Relatedly, comments on a PR are ephemeral. A rule of thumb I use: if a reviewer asks a reasonable question -- something someone in the future might wonder about when I'm no longer around, I answer it by first improving the code.

1) Anything cosmetic gets a "nit" added to the comment.

2) When a review looks like mostly assets or boiler plate, I look through quickly for anything glaring and approve while noting my assumptions that the author has run the build and verified the additions.

3) If I'm in a hurry and the author is available I grab them and do an over the shoulder review. You might be surprised how much time this saves.

4) Don't struggle with difficult sections of other people's code unless you have a vested interest in remembering this specific section of code. Ask about unclear areas.

5) Trust your CI process. Don't try to consider every edge case for your reviewee.

6) Develop a culture of small work units. Encourage people to integrate every day or two. This means reviews are small and easier to grok.

Code reviews are not a guarantee of correctness. They are another tool. We should find a good trade off between effectiveness and cost to get the most value out of them not strive for perfection.

I've seen pretty horrible code constructs that actually seemed to work without bugs. What should I say? That the author of that piece of code should completely rewrite it to my taste? It would not only frustrate him/her but also slow down the entire team. So I often accept what is rubbish IMAO. And not me alone, have you ever seen two code-buddies in a team always doing each others code review and(quickly) accepting and merging everything they produce before anyone can make a comment?

I've had numerous pointless discussions about why I wrote a specific piece of code where (a junior dev) the assessor just wanted me to do things by the book as he learned it, not really understanding why I did it that way. But there was no way to convince him, it ended up in an unresolved conflict that I only could solve by messing up my code to his taste.

All these pointless discussions that particularly affect the relationship you have with your co-developers in the team you work in, are not always fruitful and can be quite destructive if you're not careful enough.

I do believe code reviews can be very important and valuable, but at the same time I've seen huge amounts of wasted time and frustration for nothing. It's not always and only a good thing is my conclusion, it depends on the variables.

This is where you need to change your mindset from writing code to delivering business value. At my first job we realized that tickets were actually spending longer in code review than in development - and therefore our primary bottleneck on delivering functionality was code review. So as a developer code review is actually your main job, and writing code is something you do in your downtime when you don't have any reviews available.

Once everyone in the team is making reviewing code their top priority (or at least, a higher priority than writing their own code), waiting for review becomes much less of a problem.

I worked on projects with systematic code reviews, and while the reviews hardly caught any issue, they did add significant delays and endless discussions with some very unreasonable people that would systematically question every single line of code for no good reason.

I had to submit reviews and wait a day for them to be approved for things such as 3 lines of CSS changed.

Code reviews should be left for occasions when the code actually needs some review: for example, the developer changed a part of the code that he/she is not familiar with and wants the code to be checked by someone who is experienced in that part of the system.

Code reviews yes, but only when it really makes sense, and not by default.

Some of the things I've seen regarding code reviews are just way too absurd and it just adds to the overall demotivation of the team to have to do something absurd every day just because its the rules.

Most of the time, these procedures are put in place by non-technical managers who have usually not coded themselves a single line of code in their lives.

That said, I think that there are some prerequisites for productive code review. Changes are preferably small, reviews need to happen in a timely fashion, the surface area for style nitpicking needs to be minimized with linting and formatting tools and the team needs to be pretty accepting when it comes to different minor design decisions that the different team members might make.

If your change is 2000 lines for what could have been several atomic changes and you get 50 comments about indentation, "you should do it this [slightly different way that the reviewer would have preferred that doesn't significantly improve legibility or design]" after prodding the other team members for a week, address the comments and wait yet another week (and also manually rebase the patch because of 10 conflicts) it's no fun.

My biggest problems with code review are:

1. The reviewer usually lacks the context to truly review the solution and all its implications, and instead gives only a cursory review focused on style and trivialities. Sometimes they're familiar enough with the code to give an insightful review, and there's benefit to commenting on even the small things, but I've seen countless major architectural problems pass review because the reviewer lacked the time and familiarity to notice them. This isn't helped by reviews typically being done on GitHub code diffs; you're seeing a couple out of context lines and it's hard to understand how it all fits together.

2. By the time code is ready for review, it's too late. Sure, you can stop the presses if you notice an egregious problem (not that anyone usually does; see #1). My team tries to keep its reviews small and frequent, but they still often contain one or more full days of effort. If the solution has major problems, catching them after the fact means a lot of wasted effort on rewrites. Often there's no time to fix it right, so someone hacks around the faults and ships a "good enough" product. The tech debt piles up quickly.

As a result of both of these, code reviews are often little more than a rubber stamp.

This isn't to say that I don't think code reviews have any merit. I think that code reviews on every single change are a waste of time. Periodic detailed reviews of architecture are valuable, and periodic team reviews of code can help spread good patterns.

Many of these problems are obviated by pair programming. It's real-time code review before it's too late. It's the single biggest benefit to pairing, although there are plenty of others (like sharing knowledge and building team bonds). Pairs are also subject to the same pitfalls, but at a far lower rate than solo developers.

^^ From the Menu of Restaurant Antoine, New Orleans, quoted in the beginning of The Mythical Man Month.

Good engineering takes time and deliberate effort. Invariably, it will sometimes be tedious, but it is for the benefit of the product. IMO that includes code reviews, and/or pair programming.

In my experience, the biggest source of friction is in comments over style and reviewers/reviewees that have an obstinate and opinionated stance on the matter. In my current project, we resolved this by simply requiring everything passes a linter (gofmt and ESLint in our case). Follow the linter, learn to like it, end of discussion. There's still structural / design style to fight over in reviews, but those are healthy fights IMO.

I can't really imagine any work that can be more important than code review for other people. This is the way Linus can have a 100x leverage on the Linux code base. This is the most important and hardest work in the Bitcoin code base as well, probably apart from cryptography research. I think the bigger problem is that code reviewing may be not well compensated, not that it's not important.

Last year I was on a big project in a senior role and doing and receiving lots of code reviews. There's a whole etiquette around these things that you have to appreciate. First off, I got lots of feedback that was valuable.

My view with code reviews is that they should be timely and appropriate in scope. Reviewers should have some time to review but not forever. I tend to do them early in the morning or before I leave but I'm not likely to interrupt my workflow for them; unless I'm asked to do so. Real time is not a reasonable expectation. But days delay is also not reasonable. The smaller the pull request, the less time is acceptable. The bigger the change, the longer you allow for reviews.

Also, the bigger the PR, the higher the workload for reviewers. Keep your PRs small and create lots of them instead of dumping lots of big changes in one PR.

It also helps to classify review comments as blocking or not blocking. A valid outcome can be to file a follow up ticket for another change. Yes, it would be nice to refactor this bit of code but given that it was a two line code change that got the job done, maybe right now that is good enough. My view is that if CI tests pass and the code is better than before it was changed in some meaningful way, it should always be OK to merge.

Finally, sometimes there's an asymmetric relation with the reviewer and reviewee in terms of seniority. You can use this as a didactic tool to educate juniors. But it is also an opportunity to point out they did well; which I'd argue is just as important. Likewise, you have to place comments from juniors in context as well. They might feel a little intimidated commenting on your code and if they mean well, it's important to give them feedback on their comments. IMHO it is important that people speak up and feel safe to do so. A simple "You are right, I will fix this. Thanks for pointing that out." makes them feel good.

Additionally, proper test-driving and testing practices are incredibly effective ways to enforce high quality code. Again, this comes down to culture and practices. Teaching and encouraging good testing will pay off far more than enforcing "two pairs of eyes" type rules.

My experience is that code review is great at linting, nit-picking, and causing arguments about idiom. The times a bug was okayed with 8 different :thumbs-up: or "lgtm" on the PR in github are uncountable. It's not that code review doesn't work, because I'm sure it often catches plenty of bugs, it's just that I have no reason to believe it's more reliable than pairing or letting authors ask for help explicitly when they need it.

Disclaimer: I spent my formative years at Pivotal Labs. Yadah Yadah Yadah it's a cult of pair programmers so something about a grain of salt.

I ran some numbers on our code review process on a project last year. Of patches returned for modification, 5% contained a bug. 95% were for entirely stylistic changes. I find it hard to square that with being a good use of time.

As an industry, we're making the mistake Deming points out in Out Of The Crisis: we're attempting to inspect defects out of existence rather than improve our production process so that they never get created in the first place. We concentrate our efforts on being better at 100% inspection, and don't allow ourselves to think that a process without that inspection could be a hell of a lot better, if we invested the effort in figuring out what that might look like.

Pairing might be one answer here, although I don't think I've heard it talked about in those terms before.

I’m a bit of a code review evangelist, so I’m surprised at the amount of negativity toward code reviews. Some of it sounds a bit self centered, upset that it slows down “thier” productivity or ability to self express through code rather than the real purpose of the reviews- to improve the product and organization as a whole.

1- anything done in a pair doesn't need a code review.

2- any story that is more than 3 story points should be worked on in pair.

3- if you are doing a code review and you find something that needs to change you don't leave a comment, you update the branch and ask the original branch owner to review your new commit.

The issue I end up taking with code review is cases where reviewers end up forcing their own stylistic choices to the detriment of my time. Insisting that all equal signs be lined up, insisting that variables must be spaced in a very certain way etc. I'm diligent at following the style guide for whatever language I'm working in, but the most annoying code reviewer I had to deal with would specifically call out things that were up to the individual developers.

The issue is procedural. If you hate that code review of an API implementation might require changing the interface, then you should do a review of the API design before you implement it. That should be the case with every phase. Any time you have to perform multiple steps and worry that review of a later step might cause an earlier step to be invalidated, that simply means you're reviewing too late, at too coarse a level.

Or not. Adding more reviews keeps the big-O factor down, but increases the constant factor. As an organization, you can pick if you want individual contributors to never have to backtrack, or if you want them to be able to use speculative execution. One way is more predictable, and the other way can be faster if you're good at guessing right.

I had seen people misuse code review to force standards that they just made up based on blog dude read yesterday - only to change opinion three weeks later.

I had seen senior developer letting junior work alone for two weeks, only to iteratively force the junior to rewrite all of it to seniors liking. Ridiculous waste of time and cruel.

So, with wrong politics and people it is hell.

Phabricator’s stacked diffs [0] works wonderfully well to support this exact workflow (it isn’t clear from the post which versioning system the author uses and what workflows it supports - maybe I missed it?)

[0] https://jg.gg/2018/09/29/stacked-diffs-versus-pull-requests/

The current practice is a coder submitting the entire code for review, and getting comments on every aspect that the reviewer feels like. This is not an efficient process as it takes up a lot of time, but reviewer also put in a position as to 'How would he solve the problem the current code has already solved'

I feel a good code review process should have two components: 1/ Self-review. A team/company should agree on baselines/conventions that any code shipped by them should have, and ensure that any code not corresponding to that will be rejected. This review should be done by developer himself, rather than a reviewer.

2/ A code review by an experienced dev who needs to check for libraries/custom functions that are used might not break something somewhere else, or wont work because some other change is going somewhere else, that they know of. This way, potentially buggy code can be avoided, and process could be faster as well. Ofcourse, if they want to provide feedback on a better algo, they can, but should be optional

Instead I thought it would help to make code reviews optional. It is dangerous and I wouldnt recommend it for everyone and it only works if you have a CI pipeline in place of good quality. What you do is that you allow people to push directly to master, then you have your CI system continously testing all the latest commits and only if something breaks will it report it back and then the person who wrote that commit needs to fix it. I have never tried this in a real working environment but I think it would decrease friction a lot and just be slightly worse in quality. Also instead of pushing to master you could have a staging area that automatically pushes to master if it passed the tests. That might even be better actually.

Other than my only advice, these points are also helpful: - Zero Code Style, Formatting Comments should be allowed on reviews. If it passes the linter, it's good. - Re-naming classes/variables comments should be kept to minimum. Unless someone is using obviously bad names like `a`, it's probably a fine name.

For me the residual benefit of code reviews is to invite people to examine how the code works so that it can be improved or simplified. By improve I mean objective concerns like execution performance, reduction of instruction size, security concerns, and so forth. I expect a very critical analysis. Don't be afraid to hurt my feelings because this code is about to go to production.

Unfortunately, often times the proper automation controls are either not in place or the developers are fragile timid souls, particularly in group settings. Code reviews can be wonderfully constructive, but many times aren't.

The "have multiple projects, or alternatively, 'scuttwork' model, likely makes most sense. A list of noncritical-but-evergreen tasks that don't require heavy cognitive load is useful. Shoring up docs, drilling on procedures, interviewing, studying new skills or tools, etc. (Retail has a similar concept for sales associates durring slack time.)

Another is to tighten or parallelise, and prioritise and reward the review process. Both familiarity and unfamiliarity with the code base can be useful -- quick assimilation, and fresh eyes / expanded institutional awarenesss and knowledge (plus increased Bus Number).

to be more specific though, i don't like "remote" or "detached" code reviews much; "over the shoulder" code reviews are a completely different thing. as you can actively communicate with the author or the reviewer, it tends to reduce the bike shedding a lot and the value of positive feedback increases.

i guess, ultimately, it depends on the team; if you're operating well together, classic code reviews might work as well. if the team is new or the members have varying styles or levels of exerience, over-the-shoulder reviews tend to pay off significantly.

I've never come up with a bullet-proof way to deal with this. The situation usually ends up with me providing a rubber stamp of approval and making some caveat commentary about "uncertainties" due to my inability to devote so much time to be rigorous. I feel bad every time I've got to do this.

The problem might be exacerbated at another level too. For features, stories might not be broken down enough.

Some of those things can be optimized or automated away. But some of those things can't: turns out, although they're completely irrelevant 98% of the time, they're completely critical in the remaning 2%, and nobody knows what those 2% are, so we're stuck with these practices.

People often discuss the topic with smalltalk and such in mind: https://heeris.id.au/trinkets/ProgrammerInterrupted.png

The code reviews go more to the core of the problem. Interruptions are often necessary but, for the person being interrupted, they always suck.

How much should we try to learn to multitask? Where's the boundary where it stops being useful and starts being toxic?

- What kind of product are you building?

- How large is the project?

- How many developers are working on it?

- How experienced are the developers?

- How is the project being managed?

- Are there any hard deadlines?

- What is the budget? Burn-rate? Runway?

- Is the product established or are you just building the MVP and trying to validate market demand?

A two-person development team building an MVP with ever-changing requirements won't have the same workflow as an established company with a large development team and a bunch of existing users.

So whats a better approach?

Fix the workflow. Reviews are the best quality control measure out there, perhaps even better than unit tests.

I think it makes sense to set premise or template for a good peer reviewing exercise. New team members should be educated about these guidelines.

* While you are waiting for code review, review someone else's code

* Aim for reviewing two CR:s per CR you submit.

This has helped us both keep on top of the CR queue and to have something that is not too distracting to do while waiting for review.

1) Automatic code formatters/linters before the code gets to review. When everybody knows there's a formatter, reviewers can spend less time looking for formatting issues. Generally, people are more inclined to take a quick look at the code then too, because they know their focusing on "just" the logic, which helps with getting a review sooner.

2) Smaller sets of changes up for review. Yes, this might mean more code reviews overall and more process, but it's way easier to catch a bug in a < 100 line change than in a 1000+ line change. This also means keeping the contents of the review as focused as possible. If you want to refactor some major chunk of code because it makes the actual thing you're trying to do easier, go for it, but do it in a first code review and separate the logical changes. This goes to both reviews being more straightforward for the reviewer to get through and them wanting to do it in the first place. I'm way more likely to avoid a massive 30 changed files code review than a quick 5 line change, because I need way less focus to review the latter.

3) (this one might be controversial) Early code reviews where appropriate. If you're making a change that's more likely to be totally rejected, get it into code review as soon as the basic structure and logic is in place and ask somebody to review it as quickly as possible. This is before tests are ready and every todo is completed or whatever else. The review should be quick and dirty, but at least that way you know if you should be proceeding with your current path at all or change course. Again, this means more time spent reviewing, but it's a lot better than getting a review after a week saying what you just wrote has already been done, or it totally misses goal, or it has some critical issue, or whatever. This helps a lot with what to do while you're waiting for feedback too. If you've got this type of code review up then you can keep working on the details while you're waiting on feedback. If your code review doesn't fall into this category, you're probably relatively safe to start working on the next thing, because at most you'll need to make some small changes to what you're working on, not throw it out altogether.

4) Making sure code review is considered a priority on the team. That includes the team lead prioritizing code reviews and encouraging others to prioritize code reviews. If the code doesn't get reviewed, it doesn't get shipped, so it's ultimately more important than writing new code.

Not black and white, but less code review is needed if a task was paired on, if at all.

Was nominal lead on a smallish project a couple months back, with 2 other folks. Worked with one before, one I hadn't worked with before.

We're all remote, and the 'new' guy started doing 'code review' on code before he had a running environment. We'd taken over another codebase, and he was pretty up-front about wanting to adopt/enforce certain stylistic standards, which... to me, I don't particularly care about as much (on the PHP side of things, PSR-2, in this case).

We're all remote, so we had a call - a couple actually - and I indicated I didn't care all that much, but he was free to make changes as he saw fit, as long as 1) he got a working environment up and running, 2) he was also spending time writing some tests around the code as he explored it, to have a firmer understanding of what was going on, and 3) he didn't break any existing tests (or new ones that were developed).

I swear that I don't think he'd actually had a working copy of the project, but was still committing code. Not a lot, but ... it broke tests. It broke stuff that was not 'tested' in an automated way yet, but was testable by running the app, and I think he'd just not bothered to test it at all.

But... hey - look at the formatting! Look - spaces, not tabs! - how useful when someone else is having to go back and debug your crap. I received a couple small lectures on variable naming and the importance of descriptive variable names.

function getListOfUsersForCompany() { $res = db::query('whatever query'); return $res; }

Apparently, that $res is confusing and 'bad practice' because it's "hard to reason" about what's going on there. Literally 2 line methods with an internal placeholder variable (which also had some working functional tests around them) - those were being criticized by someone who had committed test-breaking code multiple times.

In all this, I kept having to figure out how much of my reaction was because my code was being criticized, vs what the criticisms were. I got accused of taking things 'personally', but I kept coming back to 'working tests broke with these changes', and in my mind, that this code was considered somehow 'better' because it more closely followed a stylistic 'standard' was bothersome. Working tests should a standard we strive for too, right?

I have worked with some folks who had a real eye for analysis - remote or colo pairing on problems has helped reason out issues that I know I've missed. And when I'm pairing or doing a requested review, cosmetic style tends to be the last thing I'll bring up, because it generally has the least impact on something. Much prefer to have someone spend time writing tests and/or docs and/or insightful comments vs reformatting. When I've refactored with someone to make something more testable, better naming/comments/docs tends to flow out of that refactoring anyway.

While I understand formatting is often not that big of burden, especially with modern IDEs, it's also not terribly high value on many projects. I'm primarily speaking on projects I've come in to where there's already extant code - in this case, we had taken on a very weird hybrid of 2 PHP frameworks and 2 JS frameworks (some screens used raw vue, some compiled vue, and some compiled angular).

"Did you know you can replace that entire function with a regex?"