I love code-reviews. With the right people, they can be useful both to achieve high-standards of code quality and also improve your own engineering process. The requirements are quite high, I think you need some combination of the following:
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.
> 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.
If my code is being reviewed, and the reviewer has opinions about how they would do it, I like it when they share their preference but also
1. consider if the code as submitted has the same result
2. share their personal preference for how they would have written it
3. explain why they prefer it over the code I wrote
4. not block the review on me rewriting the code to match their preference
I think it’s fine (and valuable) to comment “I hate this code, shipit” or (better) “ship it, if you change foo to bar then <explaination of why they prefer it>”
The worst code review comments are “this isn’t performant/idiomatic/maintainable, fix it” or other statements of opinion presented as facts. This kind of faux-objective feedback is terrible, since a fact can be argued against (or could be wrong). A statement of opinion like “I don’t like it” or (best) “I had to think really hard before I understood it” is great, since that’s what someone diving into the code for a maintenance ticket/incident will be thinking but won’t have the original author on hand to walk them through the code.
Linking to articles or references that explain why you prefer a particular implementation also gives the reviewer a learning opportunity & lets them save face when they post a follow up CR “thanks, didn’t know that”.
I think in such a situation, receiving a review is great, even though the downsides mentioned in the article ("what am I going to do while I wait for the review to come in?") are still presents.
However, it doesn't do away with how unsatisfying performing a review is. If I spend a day doing code reviews, that is not an enjoyable day to me. This is regardless of how useful I feel it is; it's just not a job I enjoy doing. It'd be nice if there was some change we could make to make it a more pleasant aspect of the job.
...and if any of those four are not in place, it can be a nightmare. At a rough guess, less than a 1-in-16 (2^4) chance of a non-aggravating code-review process at any given company.
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.
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).
Only if the author cares enough about the code. I've implemented things I know are bad in the past because the politics of pushing back aren't worthwhile. Especially if you work in an environment where your review is driven by the opinion of your colleagues (eg stack ranking).
"Code reviews are painful" are often a sign of bad team dynamics or a culture of politically motivated decisions.
code review mandatory at last place I worked, the place had a policy of any code you push in that does not match the guidelines must be changed. Guidelines was that no em allowed, must use rem. Old part of codebase assigned to me, I found some things in CSS I improved (reuse of code, a small overflow bug)
Got comment - you need to change em to be rem.
I can't do that because there is em all over the place and this can have side effects. The diff shows it as my code but if you look you can see it is actually the old code just moved to right place.
Those are the rules fix them.
since this code was in a big release of stuff and none of our stuff was allowed through unless I changed one em to rem even though I could not be sure there wouldn't be problems I went ahead and rolled back my changes.
fixed overflow bug other way. no code reuse ever in that part of codebase.
> where the author says they feel like code review is optional. If you think it's optional
I think these are two different things. The author says they don't enjoy code review, and that that probably is because it feels optional. From the post, I gathered that that's not what the author thinks - the first few paragraphs emphasises that they think it is an important part of the work. But apparently, that knowledge isn't enough to change the experience of reviewing.
> The better way to think about it is that code review simply isn't optional.
Of course it's optional. Tests are optional too. There are many projects and situations where trading code quality for development speed makes sense. Especially when you're building MVP's, experimenting, or trying to validate market demand.
It's also often not worth it on small to medium sized projects where only one person is working on the codebase (think 3-person project: designer, front-end dev, back-end dev).
Larger projects with multiple people working on the same codebase are a completely different story. In those cases code reviews are valuable and highly recommended but still optional. The only thing that isn't optional is delivering a working product.
To me, code review is a combination of two things I would think we would try to avoid:
1. Testing implementation over behavior
2. Using a human being to test in a non-automated way
Proper test coverage and human QA to me is the most effective approach to not shipping bugs, because it really doesn't matter to the customer what the code does or doesn't look like if it runs. And I just don't believe that code review is an efficient means to catch that if user is in a state of X and tries to do Y to Z that the app blows up. That becomes most apparent when that code is running in the full context of the application, not when viewed as a git diff. A bug is so often a perfect storm of seemingly innocuous but conflicting/insufficient behavior than it is some block of code that is clearly broken at a glance.
There is something to be said about stylistic and architectural issues and code reviews do give an opportunity for asking questions and driving "better" code. But this is actually just another reason why I don't like code review. "Better" is subjective and there's always another level of "better" to be reached. If I held them to my own personal standards sometimes I'd be asking for a rewrite. So what do I do, suggest that they improve their code, but not to the full extent that I feel it could be improved?
> 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).
There's a potential problem with that idea: The assumption that, if several programmers involved offer different solutions, the best solution will prevail.
I think in many cases the accepted solution ends up being the one pushed by the person that holds more power, the one who can communicate better, or the person that 'negotiates' more aggressively.
I'm not saying that to argue against code reviews - It's just a friendly reminder that although as technical people we hope to achieve some objective level of quality, we can't escape office politics, human bias and general subjectivity.
I'm fortunate enough at my company to not have to submit to the code review bureaucracy - and I can justify it by having metrics which indicate that my code ships with fewer bugs than the code of other teams which do go through code reviews.
I think everyone should have some level of freedom to work in a manner which works best for them, rather than forcing everyone into the same process. Success should be measured by achievement of business goals and revenue.
For every anecdote you have about code reviews finding bugs, I have my own anecdote about people writing code going through a thorough code review process and the code crashes the first time an end user tries to use the functionality.
So yes, code review IS optional if you can justify it.
> 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).
Sometimes the suggestions add very little / nothing. Push-backing (i.e. adding a back and forth) would be a waste of time so you just do whatever the code reviewer says.
> So code review seems to be doing its job here. It's producing better code.
As with a lot of things, balance is needed. Better code comes with a cost (time), and that time may have been better spent somewhere else.
Wouldn’t bugs be caught during dev testing, or QA testing or automated testing or while running on a staging server? It seems nuts to rely on a developer looking at a code diff to catch bugs in an app, it happens sometimes but I generally think review is more about quality.
Code review doesn’t catch bugs. Sure, sometimes an experienced dev can spot a bounds error or race condition, but CRS are for knowledge sharing and style adherence.
I think the author and many folks here have an unhelpful understanding of code reviews.
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.
These are all good tips, but I want to stress the importance of (6). Much of what the original post is talking about can be improved by adopting the habit of making small, incremental changes. As the author, you no longer have to wait "in the order of weeks" for the feedback and a potential rebase should pose little risk. As the reviewer, you don't have as much mental burden and most reviews can be done "in-between" bigger tasks, e.g. in the 15 minutes before a meeting or before lunch, etc.
As for 1) I highly recommend to automate that away as much as possible. A strict syntax guideline and a toolchain that enforces it will go a long way.
I think these points are very salient. I'd suggest adding another one: create a checklist for code review that is relevant to the issues your team faces. The checklist makes it much easier for you as a reviewer to perform evenly.
> 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.
One concern here is you may ask questions whose answers are then given to you, and you alone. If it's likely someone else in the future will have the same questions, the "answers" should be in the form of improved code.
> 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.
I'm not clear on the distinction between "difficult sections" and "unclear areas"? Either way, one of the outcomes of the review should be to make life easier for the folks that follow us.
I've worked in teams bad enough to make code reviews a real pain.
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.
> 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.
I've left the place I worked at for over 3.5 years over stuff like this adding up. Starting at my new job in a few weeks, getting paid more than I did at my previous job and will be working with people more senior than in my previous team. All in exchange of the rapport I've built over 3.5 years.
> If I spend a day doing nothing but reading code reviews, I'll end up feeling unsatisfied and unproductive. Because code review feels fundamentally optional -- even though I believe it's beneficial, it's something we've chosen to do, not something that we absolutely have to do in order for the project or business to keep operating -- it's more frustrating to find myself spending a large amount of time on.
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'll add that code reviews are vulnerable to garbage in, garbage out. Code reviews the are magically 100% efficient at catching will still be expensive in terms of time if all incoming code is garbage. By reducing upstream garbage, code reviews become less expensive, making more time available for anything else, including coding.
Systematic code reviews is really a complete subversion of the agile spirit. Agile was about doing what makes the most sense for the project at any given time, and code reviews don't make sense all of the time, far from it.
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.
While I agree with the spirit, I disagree in whole. "One line config change? I don't need a code review." At our org, even that needs a CR. It takes a few minutes generally to request it in our chat room. Usually, the other person sees no issue and you get their stamp that guards the build process. However! Multiple times (a small percentage), the person asks for a clarification on it and it turns out that a conflicting config needs updating or a comment is warranted or they remind you that this change will (maybe minorly) affect another team and asks if you reached out to let them know.
Can it get in the way? Yes, it can. So as a team, learn how to streamline. For us, usually someone is available for a "quick cr." Larger ones take longer. Maybe some changes are too small, but for our team (no CSS), we've not found a consistent candidate.
I love reviewing code and I love having my code reviewed. In my current team we make significant improvements to the code during the review phase, both shaking off bugs and making architectural improvements.
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.
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.
1. You need to put context into comments. Code is written once but read multiple times, so you should optimize for later case.
2. "Too late" is when bug hits the client and affects income. If not, then it's not "too late", it's just "a bit late". 2-3% in effort increase to decrease number of bugs (increase customer satisfaction) by 15-20% is huge win. If you will not do that, your competitors will do that instead of you.
"Good cooking takes time. If you are made to wait, it is to
serve you better, and to please you."
^^ 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.
,,Reviewing code is one of those things that I would enjoy if I had infinite time, but that I find a nuisance when I don't.''
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.
The compensation, in Linux anyways, is that others will review your code if you review theirs. So the motivation to review stuff on the mailing list is purely that you need someone to review your code to get it in.
It sounds like quid pro quo but I found the level of review (e.g. for the DRM subsystem) to be excellent, even if it's one AMD eng reviewing some other AMD engineers code in the AMD specific code.
I get the sentiment. I'm currently in a small startup and not doing any code reviews as such. That's probably not ideal because I know I make mistakes.
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.
I've found that favoring full time pair programming and not reviewing work that was soloed upon unless the author explicitly asked for it because of self-identified uncertainty or caution has resulted in a better workflow with little to no actual hit to quality.
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.
Pair-programming 100% of the time sounds like a nightmare to me. I need uninterrupted focus - something I can't do with someone else there constantly, even if we're working on the same thing
This sort of thing is why I'm all in on strictly, program-enforced code-style. It closes off the single most common, most useless line of argument in code reviews that there is.
What I do wonder about is what else can be done to bring tooling into the process in a productive way. I'm imagining something like the Go and Rust playgrounds where reviewers actually contribute test-cases or something to the real code base.
The problem with code review is that it works. That's why we can't shake the industry's fascination with it. It works to catch bugs, but it's horrendously inefficient in doing so.
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 used to hate code-reviews but now I'm a convert after:
(1) we brought in better people. It's fun to sit with competent people and trash-talk some code, and (2) after I pushed the team to use better tooling, which in our case is GitLab + Merge Requests. You can imagine the dark ages before that.
We're happy to hear that GitLab helped you improve your code-review experience! We documented some of the advice and best practices that might be useful when performing code review or having your code reviewed [0].
Code review aren’t meant to find defects- they are meant to prevent them. Every study I’ve seen on code or peer review effectiveness (and there are many) demonstrates that when code reviews are added into an orgs development process, product defects are reduced, and reduced by far more than what is accounted for in bugs directly filled in code reviews. Knowing that code is going to be reviewed and/or having to explain that code changes the way developers write code to begin with, for the better.
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.
In one of the teams I worked in we had a policy to mitigate some of the things we found inefficient in our code reviews:
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.
For me, I've never had an issue context switching from various branches and PRs. After a moment or two of looking through my notes and my code I can quickly get back up to speed with what I was doing.
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.
Auto code formatters for the win. If that is not an option, maybe a line in the style doc that says "shall" vs "may" (ie, things that are and are not kick backable).
Only time i've come to hate code review was when my counterpart (reviewer or the author) has been overly pedantic with no hint of any empathy in hir writing. I do not know was it because of inherent insecurity or no previous experience writing in more friendly manner but boy oh boy i was boiling inside arguing about useless things. The socially awkward seem to be in high numbers amongst programmers which makes these kind of ordeals a bit of agony at times.
I don't get this. A code review is a purely technical process. If he says your code is bad / wrong etc then that is a purely technical discussion that has to be had if you feel they are wrong.
[+] [-] erwan|7 years ago|reply
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.
[+] [-] falsedan|7 years ago|reply
> 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.
If my code is being reviewed, and the reviewer has opinions about how they would do it, I like it when they share their preference but also
1. consider if the code as submitted has the same result
2. share their personal preference for how they would have written it
3. explain why they prefer it over the code I wrote
4. not block the review on me rewriting the code to match their preference
I think it’s fine (and valuable) to comment “I hate this code, shipit” or (better) “ship it, if you change foo to bar then <explaination of why they prefer it>”
The worst code review comments are “this isn’t performant/idiomatic/maintainable, fix it” or other statements of opinion presented as facts. This kind of faux-objective feedback is terrible, since a fact can be argued against (or could be wrong). A statement of opinion like “I don’t like it” or (best) “I had to think really hard before I understood it” is great, since that’s what someone diving into the code for a maintenance ticket/incident will be thinking but won’t have the original author on hand to walk them through the code.
Linking to articles or references that explain why you prefer a particular implementation also gives the reviewer a learning opportunity & lets them save face when they post a follow up CR “thanks, didn’t know that”.
[+] [-] Vinnl|7 years ago|reply
However, it doesn't do away with how unsatisfying performing a review is. If I spend a day doing code reviews, that is not an enjoyable day to me. This is regardless of how useful I feel it is; it's just not a job I enjoy doing. It'd be nice if there was some change we could make to make it a more pleasant aspect of the job.
[+] [-] Sharlin|7 years ago|reply
[+] [-] JoeAltmaier|7 years ago|reply
[+] [-] eridius|7 years ago|reply
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.
[+] [-] onion2k|7 years ago|reply
Only if the author cares enough about the code. I've implemented things I know are bad in the past because the politics of pushing back aren't worthwhile. Especially if you work in an environment where your review is driven by the opinion of your colleagues (eg stack ranking).
"Code reviews are painful" are often a sign of bad team dynamics or a culture of politically motivated decisions.
[+] [-] bryanrasmussen|7 years ago|reply
code review mandatory at last place I worked, the place had a policy of any code you push in that does not match the guidelines must be changed. Guidelines was that no em allowed, must use rem. Old part of codebase assigned to me, I found some things in CSS I improved (reuse of code, a small overflow bug)
Got comment - you need to change em to be rem.
I can't do that because there is em all over the place and this can have side effects. The diff shows it as my code but if you look you can see it is actually the old code just moved to right place.
Those are the rules fix them.
since this code was in a big release of stuff and none of our stuff was allowed through unless I changed one em to rem even though I could not be sure there wouldn't be problems I went ahead and rolled back my changes.
fixed overflow bug other way. no code reuse ever in that part of codebase.
[+] [-] Vinnl|7 years ago|reply
I think these are two different things. The author says they don't enjoy code review, and that that probably is because it feels optional. From the post, I gathered that that's not what the author thinks - the first few paragraphs emphasises that they think it is an important part of the work. But apparently, that knowledge isn't enough to change the experience of reviewing.
[+] [-] oldboyFX|7 years ago|reply
Of course it's optional. Tests are optional too. There are many projects and situations where trading code quality for development speed makes sense. Especially when you're building MVP's, experimenting, or trying to validate market demand.
It's also often not worth it on small to medium sized projects where only one person is working on the codebase (think 3-person project: designer, front-end dev, back-end dev).
Larger projects with multiple people working on the same codebase are a completely different story. In those cases code reviews are valuable and highly recommended but still optional. The only thing that isn't optional is delivering a working product.
Best practices == great, dogma == not great.
[+] [-] Eleopteryx|7 years ago|reply
Proper test coverage and human QA to me is the most effective approach to not shipping bugs, because it really doesn't matter to the customer what the code does or doesn't look like if it runs. And I just don't believe that code review is an efficient means to catch that if user is in a state of X and tries to do Y to Z that the app blows up. That becomes most apparent when that code is running in the full context of the application, not when viewed as a git diff. A bug is so often a perfect storm of seemingly innocuous but conflicting/insufficient behavior than it is some block of code that is clearly broken at a glance.
There is something to be said about stylistic and architectural issues and code reviews do give an opportunity for asking questions and driving "better" code. But this is actually just another reason why I don't like code review. "Better" is subjective and there's always another level of "better" to be reached. If I held them to my own personal standards sometimes I'd be asking for a rewrite. So what do I do, suggest that they improve their code, but not to the full extent that I feel it could be improved?
I hate code review.
[+] [-] lowercased|7 years ago|reply
[+] [-] kace91|7 years ago|reply
There's a potential problem with that idea: The assumption that, if several programmers involved offer different solutions, the best solution will prevail.
I think in many cases the accepted solution ends up being the one pushed by the person that holds more power, the one who can communicate better, or the person that 'negotiates' more aggressively.
I'm not saying that to argue against code reviews - It's just a friendly reminder that although as technical people we hope to achieve some objective level of quality, we can't escape office politics, human bias and general subjectivity.
[+] [-] lostcoder|7 years ago|reply
I think everyone should have some level of freedom to work in a manner which works best for them, rather than forcing everyone into the same process. Success should be measured by achievement of business goals and revenue.
For every anecdote you have about code reviews finding bugs, I have my own anecdote about people writing code going through a thorough code review process and the code crashes the first time an end user tries to use the functionality.
So yes, code review IS optional if you can justify it.
[+] [-] Jyaif|7 years ago|reply
Sometimes the suggestions add very little / nothing. Push-backing (i.e. adding a back and forth) would be a waste of time so you just do whatever the code reviewer says.
> So code review seems to be doing its job here. It's producing better code.
As with a lot of things, balance is needed. Better code comes with a cost (time), and that time may have been better spent somewhere else.
[+] [-] ericmcer|7 years ago|reply
[+] [-] falsedan|7 years ago|reply
[+] [-] koala_man|7 years ago|reply
* "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...
[+] [-] vfc1|7 years ago|reply
The tests break and we have to fix them not because a bug was found, but because something else changed that affected the test.
Most of the time spent with tests is not to catch bugs. When was the last time that you caught a significant bug with a test?
[+] [-] walterstucco|7 years ago|reply
[deleted]
[+] [-] rasteau|7 years ago|reply
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.
[+] [-] DubiousPusher|7 years ago|reply
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.
[+] [-] neathack|7 years ago|reply
As for 1) I highly recommend to automate that away as much as possible. A strict syntax guideline and a toolchain that enforces it will go a long way.
[+] [-] kqr|7 years ago|reply
[+] [-] rasteau|7 years ago|reply
One concern here is you may ask questions whose answers are then given to you, and you alone. If it's likely someone else in the future will have the same questions, the "answers" should be in the form of improved code.
> 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.
I'm not clear on the distinction between "difficult sections" and "unclear areas"? Either way, one of the outcomes of the review should be to make life easier for the folks that follow us.
[+] [-] messit|7 years ago|reply
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.
[+] [-] therein|7 years ago|reply
I've left the place I worked at for over 3.5 years over stuff like this adding up. Starting at my new job in a few weeks, getting paid more than I did at my previous job and will be working with people more senior than in my previous team. All in exchange of the rapport I've built over 3.5 years.
[+] [-] lmm|7 years ago|reply
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.
[+] [-] PopeDotNinja|7 years ago|reply
[+] [-] vfc1|7 years ago|reply
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.
[+] [-] sethammons|7 years ago|reply
Can it get in the way? Yes, it can. So as a team, learn how to streamline. For us, usually someone is available for a "quick cr." Larger ones take longer. Maybe some changes are too small, but for our team (no CSS), we've not found a consistent candidate.
[+] [-] boomlinde|7 years ago|reply
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.
[+] [-] caymanjim|7 years ago|reply
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.
[+] [-] v_lisivka|7 years ago|reply
2. "Too late" is when bug hits the client and affects income. If not, then it's not "too late", it's just "a bit late". 2-3% in effort increase to decrease number of bugs (increase customer satisfaction) by 15-20% is huge win. If you will not do that, your competitors will do that instead of you.
[+] [-] ereyes01|7 years ago|reply
^^ 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.
[+] [-] xiphias2|7 years ago|reply
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.
[+] [-] stefan_|7 years ago|reply
It sounds like quid pro quo but I found the level of review (e.g. for the DRM subsystem) to be excellent, even if it's one AMD eng reviewing some other AMD engineers code in the AMD specific code.
[+] [-] jillesvangurp|7 years ago|reply
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.
[+] [-] zaksoup|7 years ago|reply
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.
[+] [-] barbs|7 years ago|reply
[+] [-] XorNot|7 years ago|reply
What I do wonder about is what else can be done to bring tooling into the process in a productive way. I'm imagining something like the Go and Rust playgrounds where reviewers actually contribute test-cases or something to the real code base.
[+] [-] regularfry|7 years ago|reply
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.
[+] [-] dotdi|7 years ago|reply
[+] [-] btasovac|7 years ago|reply
[0] https://docs.gitlab.com/ee/development/code_review.html#best...
[+] [-] topkai22|7 years ago|reply
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.
[+] [-] _shadi|7 years ago|reply
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.
[+] [-] fzeroracer|7 years ago|reply
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.
[+] [-] sethammons|7 years ago|reply
[+] [-] tekkk|7 years ago|reply
[+] [-] LandR|7 years ago|reply
Why are you looking for empathy in code reviews?
Why are you getting feelings involved at all?