Is that why we do code reviews, because we don't trust each other? I find this attitude problematic. We do code reviews because humans make mistakes. Requirements can be misinterpreted. Different work in progress can be in conflict with each other. Reviews are a good way to learn from each other and keep abreast of what work is occurring outside your own bubble. Not doing code reviews because of "trust" kind of misses the point.
I find even just cursory and brief code reviews are beneficial and often pick up on issues. It also creates a more collaborative team and gives me a reason to communicate with devs I might not otherwise communicate with, even if that is just a brief comment on a PR.
If something hit production and caused a major fuck-up because there was no peer review process, then in all the places I've worked at the first action item in the post-mortem would be "we should introduce peer review." Otherwise someone would ask how we could ensure it wouldn't happen again, and no one would be able to say "because we trust them," and leave it at that. It would sound more like "I trust you will never make that mistake again," which sounds more like a threat. You also can't go to your clients and say "sorry, we let our team deploy stuff without reviewing because we trust them... so please continue to deal with our avoidable problems."
Thing is, I've lead my fair share of teams and I trust my team-mates and colleagues implicitly. It makes for a strong team. But we still do code review (peer review) because we want to build software that works well and support each other. We're not in the job to simply deploy code to prod as quickly as possible.
I'd also add that I think any software engineer should try and have the experience of working in a highly regulated field, like healthcare. It's hard to get an appreciation for why these things exist until you realise you're held to a much higher level of accountability because your oopsie moments can have much larger consequences. For me, it's been hard to go back to my old, cavalier attitude after that.
Exactly. I want my code peer reviewed partly because it makes it clear and formal that while we succeed as a team we also fail as a team. It’s much easier to talk about failures when a failure doesn’t have a single person attached to it.
I don't believe that to be true. I believe the expected outcome is that people will still want code reviews on anything vaguely complex as insurance against fuckups. "Shit, WE missed something" is a nicer place to be than "I was sure I was perfect and I'm actually an idiot".
It does however mean you don't sit around waiting for someone to approve a three line delete PR that you know is safe. The "Rubber Stamp" review is a thing and it does waste time.
Since I am currently working in an environment that resembles GP's premise, I have to say that I find this take to be way too optimistic.
I'd be exactly in the boat you describe (and actually: I've yet to see a situation where someone reviewing my code did not lead to improvements). But: If I want my code reviewed, I have to actually fight for it. Since everybody's workload is too high, even the willing often simply don't have the time.
From higher up, at our place, there's no one that actively opposes code review as a practice, but they don't seem to get that it requires investment (i.e. time), either. That means that if management refuses to change their ways of planning or explicitly enshrine reviews as priority tasks, it simply won't happen except for maybe the gnarliest cases.
And that's from a perspective of the change's author being willing and proactive. I work with individuals that produce very questionable code at times. And I'm not talking style here: Code in 2021 that still is prone to SQL injection at every turn. Methods that are hundreds of lines long with a gigantic cyclomatic complexity that no one (including the author) will ever really understand. Method names like "process" and "process2". And so on.
Needless to say, those programmers will never push for their changes to be reviewed. The rest of the team simply discovers them after they blow up (which they do regularly) and we're debugging.
Now, these people are luckily a tiny minority (which at least makes this somewhat bearable), but I over and over again would wish for someone with more authority to show that they're interested in the matter. Which a policy on code review would do -- if only as a signal. Yes, we want to check each other's code. Yes, we want collaboration. Yes, that's more important than some arbitrary deadline for minor feature X. Yes, we expect our programmers to be professionals.
Another symptom of such an environment also is the fact that everyone is the king of their particular hill. No one every familiarizes themselves with another person's code unless they really have to. By now, I've jumped to insisting that every change on a critical component I'll ever make will be done through pair programming, and so far, at least that seemed to stick. But again -- that only happens if both parties involved actively fight for it instead of everybody understanding that this is just an expected part of work.
This is a fantastic comment, thank you for writing it. I especially appreciate the explanation of how management decisions produce engineering practices.
I appreciate the context and the different point of view! I live in a bubble of how I perceive my coworkers (and indeed myself), alongside the culture of my employer where "getting it right is more important than getting it out"
Was interesting to hear your experiences and maybe update my expectations of workplaces somewhat
> [...] alongside the culture of my employer [...]
I think you've hit the nail on the head here. As many others have said in different HN posts, culture is hard to change. And I would also understand the argument that one can also enforce culture changes with policies; my hope up to now simply has always been that engineers could "overrule" some artificial time limits in favor of more collaboration by referencing such a policy.
I guess if you're starting from a context where code reviewing practices "just work", introducing explicit policies also can't improve much, anyway. On that note: If you happen to know a good heuristic to detect such cultures during interviews or the like, I would be extremely interested :)
This is where properly done scrum shines, provided the nature of the work lends itself to greenfield work (cycle time of one iteration). Good scrum is a dysfunction surfacing machine, so if you have a good system of risk management in place, the team can address in a constructive way. For all the scrum haters out there, come up with some system that codifies certainty, stability, and quality before your leadership starts pointing fingers.
> If code reviews aren't an expected part of the development process and there's pressure on delivering, it won't happen.
This hits at the heart of why I strongly agree with this post and think PRs and code reviews are ultimately destructive.
Reviews are going to get dropped _regardless_ of whether they are in the process or not. Pressure makes code reviews and quality worse, not better.
More than that, the need to "appear" to review is going to slow things down and can cause an exponential back up. I saw this happen about a mouth ago: 3 day release cycle got bogged down and then took _3 weeks_ to get a single release out.
Process goes out the window when pressure hits. You are then operating in foreign environment at the most critical point and your safety net of useless.
Your safety net when things need to go fast should be highly automated fast build and deploy, coupled with great test coverage, from unit to end to end.
The only exception might be if you work in highly regulated field where you legal required to have a very low Mean Time To Failure. In that case things move slow unless you are very well resources.
Otherwise (ie the majority of products) focus that effort on having a very low Mean Time to Resolve. If you absolutely need some form of quality assurance, Pair. Do continuous PRs and knowledge sharing rather than out of context after the fact reviews.
This sounds similar to the peer review process in publishing. Typically, getting other people to actually agree to voluntarily review your work is indeed a significant hurdle if the requested paper/book/chapter/white paper/code review is lengthy.
There is an easy way to resolve this:
Request peer review for SMALLER chunks of code.
Git makes this exceptionally easy -- it's certainly far easier than reviewing a large document with Word track changes!
The problem is that small changes are barely worth reviewing. Also a series of small changes can obfuscate the larger context of all those changes.
Each small change on its own might make sense and be ok to a reviewer, but if you step back and look at the series of changes together there might be some issues that could have been noticed.
Sure. But if you read my previous message this relies on people being comfortable asking. I am but not of my colleagues are. The formal process would let them use the benefit that I can use when I think it’s necessary.
I agree with you there, but I'm also sure that one reason for this is that many people still see reviews as the task that only happens after everything else.
I'm convinced there's a lot of useful middle ground to be had if reviews aren't just "here's 500 LOC, please review", but maybe more… staged.
Say: Rummage around in the code a bit for yourself (ask questions if you have to), come up with a strategy of solving your problem, and submit that for review. Not in code, but maybe using markdown, preudocode, small diagrams or the like. In person (i.e. via screenshare nowadays). And if the other party doesn't find obvious holes in your strategy, then go implementing it.
I remember an interview with some Linux kernel maintainer that mentioned the same effect -- people submitting a gigantic patch for something that will simply never be merged. And all that would have been necessary to avoid this wasted work was an e-mail asking if the maintainers are at all interested in the change.
This post sounds like wisdom, but after looking at it hard, it seems that the point is no more than "peer review processes are popular". Not to say that code review is bad in any way (I do it at my current role), but, this argument for them is not very good.
One downside of code review is reduced velocity. There are teams out there who use code review so effectively that they never ship big fuck-ups to production. They never have to tell clients that they have bad practices. Instead, they struggle to get clients because they pay much more per line of code shipped and therefore don't deliver as good of a value as their competitors.
This where the argument needs to be: whether and how to have code reviews be a net positive.
Ultimately I think it's down to where you want to pay for that decision (for want of a better term), or where to put the bottleneck. I think that putting the bottleneck in production (which would block all work unrelated to a fix if a catastrophic fuckup took place) might as well be a hedge against the possibility of things fucking up so badly in a given timeframe. You could also make code review a bottleneck, but I haven't seen that work well in practice compared to putting it at QA or as part of a release process (unless you combine them in a continuous delivery workflow).
I can see how that equation balances in favour of no code review when it's in the context of a startup that is probably still figuring out how to make money, especially with a subscription based app launcher. As far as prototyping and early stage iterations go, you're probably just seeing what works and reviewing the code isn't so important at that point. There would still be one or two things you'd ask someone to check, e.g. security related stuff.
I think you make a good point though: what makes an effective code review process? I don't think there's a single answer to that because it depends on the circumstances.
I would at least say that it goes better if it's treated as a priority and issues are raised and dealt with promptly, rather than leaving it as a chore you eventually get around to. And you would have to see value in it beyond it being a sanity check, as others in this thread have described (knowledge transfer, for example).
I'd argue that social proof often makes for a good argument. Perhaps not the best argument, but still a good one. "Best practices" still carry weight. Of course you can make an argument that the thing everyone does is wrong, and win. That's happened millions of times in this industry. But if the argument is nearly strictly between "this is what everyone does" vs "this is what I do", the former is going to win.
Sure, I mostly meant automated processes. They won't always save you from yourself but doing blue-green, having an extensive test suite, etc, are all things that will help reduce the risk of a bad deployment.
Yeah exactly, code review isn't a good place to catch actual bugs. Humans are terrible at catching bugs consistently... Humans probably introduced those bugs in the first place. It's also an exceedingly poor use of human time, which costs a heck of a lot more than machine time.
We really should be relying on automated systems to catch bugs for the most part, and leave code review for what it's good for: making sure a different human can understand and maintain the code.
In fact, if we manage to catch a bug in a code review that didn't also trigger our automated systems to alert us, that should probably prompt us to look into how we can expand the coverage of that automated system, similar to what we'd do in a postmortem if the bug wasn't caught in code review and reached production, because we can't rely on that happening the next time.
> code review isn't a good place to catch actual bugs.
Disagree, but it's also a good place to be ensuring that the system is evolving in a way that is reasonable and consistent — or that it is unreasonable and inconsistent for good reasons.
How do you systematically (and proactively) ensure your testing is thorough? With testing you often run into unknown unknown cases.
I’ve always code reviewed/gotten code reviewed on both the implementation and the tests, with completeness as the main quality you’re reviewing test code for. If that’s how we do it, we’re right back at code reviews.
That's definitely an unsolved (unsolvable?) problem. The best we can really do is start with a common sense set of initial tests based on the user observable behaviors we want from the system and then add more tests organically as we learn more about the system's edge cases through incidents, bugs and regressions.
I'm not saying code review can't find any bugs (in practice bugs are found in code review all the time), I'm just saying that if we do find a bug through code review, it means we got lucky, and we shouldn't count on getting lucky again, so better add a test/linter rule for the next time when we might not get so lucky. That's just one more way to grow your automated systems organically.
If there's one thing I've learned over many years writing lots and lots of automated tests for everything is that no matter how far you go with your tests, barring formal proofs or equivalent, the tests cannot save you from, at some point, breaking production badly. One small, tiny thing that slipped through your tests and boom - huge fuck up (even small things can easily cause your whole new feature to fail miserably on the big launch).
What big launch? :) Is useful to feature flag new functionality so that you can release gradually and spot the bugs - often via error monitoring and user feedback - before they affect everyone. Yes, realize it can happen but there are techniques to minimize the chance of it happening.
If you want to learn more about this, read up on 'continuous deployment'. I remember when I was first exposed to it it seemed dangerous/scary but with the right culture changes it can work really well; lots of companies do it.
Sure, but then you are just begging the question: if you have a better process for release verification you might not be need any per-commit code review at all.
It's the same in the financial sector, you can't just push code without a review.
However, the review process is far from perfect and can create a false sense of security.
To review a piece of code, that code should ideally be small in scope. For larger pieces it's rather common that the reviewer don't have enough time to do a good job.
If there were better incentives for doing a review, then it would greatly improve the situation.
I've had a lot of trouble getting people to do proper reviews. A minimum to me is that you actually compile and execute the code in some way to check its sane. Better would be the reviewer actually adds to the test suite for the code to prove their expectations of how it works.
In almost all cases it's very hard to get people to look outside the web browser for the diffs. Diffs show you something, but never the whole context.
The other problem I've found is stamping down on style arguments: they have no place in code reviews, and a lot of the time you get a round of "please change this variable to be named something else" - which might be valid, but raises the question of why the reviewer didn't do it, and more often leaves a weird authority gap - I have no power to just reject such a request because I think it's invalid, nor does the business have anyway to resolve the issue. In fact if there's a dispute at all, very rarely is there any process for bringing in a third party to mediate or break the stalemate (which is hilarious in some ways since everyone by now should realize 2-party systems can't achieve consensus).
Code reviews are very, very cargo-culty and I've not seen them ever include a thorough design: people give up on the very near edge cases, and so the whole system falls apart because it's just a hurdle to get through and not a collaborative or constructive process.
Why should it be on the reviewer to compile and execute the code, rather than the author?
Or are you saying that one can actually find more bugs when two people compile and execute the same code rather than just one?
When I review code, I want the author to tell me how they have verified that the code has the desired effect -- but if they do that, I'm going to trust that they did the actual verification and I'm not going to simply retrace their steps. That seems like a waste of time to me.
Not OP here. Code reviews, to me, are opportunities to
1) Catch edge use case / business requirements misunderstandings
2) Mentor new hires/junior engineers on the ways things should be done (e.g. internal culture about some architecture/design).
BUT, I absolutely despise code reviews that nitpick on non documented or 'because I prefer that way' stuff:
1) Task tickets that only have a vague description, have their own 'play detective' about the requirements, but a lot of new ones surface during code review.
2) Requirements are met, but there are internal/personal technical directives that are written nowhere, are not enforced with automated checks and may dramatically change a feature/patch is implemented (I can only think about that guy that worked in a company in which only LEFT JOIN sql statements were allowed).
Compiling and running the code is handled by a build server and automated tests. That's not the point of code reviews.
The main purpose of code reviews is checking the general structure of the code and tests. Is the code well designed? Are error cases considered and tested?
Another important purpose of code reviews is knowledge sharing.
If you don't catch major errors until the QA team has time to do a full comprehensive test of the product then those errors are probably sitting in the codebase for days or weeks, and additional changes are being piled on top of them which will make it harder to identify which commit was the problem. Do a code review and you'll catch a bunch of stuff before it even hits QA.
Correct. Any time you need a completely separate team to handle any aspect of the lifecycle of the product you're introducing delays and miscommunication. Besides, a separate team to do a part of the job scales badly: it needs to grow at least linearly with the number of developers, sometimes superlinearly.
You should absolutely have a small QA team, but their job shouldn't be doing the QA -- it should be helping others do their own QA.
Major errors are easy to spot and will likely take QA minutes to find. They're also probably not going to do a full test of the entire system with every change unless you've got a very small product.
One thing I've seen that I like a lot is individual changes creating individual front ends. You test the one thing in that change, and it's done. This tends to cause a lot of problems with CORS, but then what doesn't?
Agreed with you though that good code reviews (whatever that means) obviates most of this.
The actual job is always fast. What takes time is for the job to sit in the queue outside the QA department. The figurative papers moving between desks is always what kills your efficiency, not people doing work slowly.
I don't see that as mutually exclusive. A QA team can be more effective if you're taking steps beforehand to limit the scope of their work, which means your engineers need to start getting more hands on with that kind of work (code reviews, automated testing, whatever...)
Your quality problems might well come from poor or non-existent architectural decisions and tech-debt, and QA won't pick up on that stuff unless it manifests as a faulty requirement.
The earlier you catch a problem, the easier it is to fix.
Our tests have caught 10s of thousands of bugs. We couldn't move forward without them. Maybe they just fit our use case more than yours.
In our case we have a public API and several backends. The tests run against the public API. To know that a new backend is working it has to pass all the tests. Getting any new backend working correctly without the tests would be nearly impossible.
> For me, it's been hard to go back to my old, cavalier attitude after that.
I’ve heard from my workplace’s software team that this makes hiring from certain sectors particularly difficult because they don’t get this. Too used to move fast and break things and MVP.
We (at least should) also do code reviews in order to make sure everyone understands the code.
On my team, the majority of code review comments are not discussing potential bugs, they're making sure everybody knows how the new thing works, why it was designed that way, implementation tradeoffs, etc. All that discussion is extremely valuable over the long run. For example, it means that nobody has to pay attention to email while they're on vacation in case something goes pear-shaped with a module they implemented.
That said, I 100% agree with the article's concluding paragraph about YMMV. Raycast is operating in an entirely different business domain from the one I'm in. It sounds like the reasons why Raycast likes their way of doing things don't really apply to us, and I don't think the reasons why we like our way of doing things don't really apply to Raycast.
> do code reviews in order to make sure everyone understands the code.
I like a hybrid "the person who wrote it can't deploy it" model. You get an informal code review and two sets of eyes/brains to the extent that the deploying engineer feels like they understand it enough to shepherd it into prod weighted against how important the component and the scope of the change is.
It self scales from "copy change on the website yolo" to "this involves two new racks of servers and a database migration, I'm going to need to spend a week understanding it to the point I can make a plan."
> We (at least should) also do code reviews in order to make sure everyone understands the code.
This is the most common reason I see cited for code reviews, but I'm not entirely sure if I buy it, at least not for a lot of teams.
In most cases you are going to have one, maybe two other people looking at a given PR. The positive way of looking at this is that you've just doubled (in some cases tripled) the number of people who are familiar with the code. In reality though, this is still going to be a very small fraction of most engineering organizations. If you have a small team who wholly owns a small codebase then it could be meaningful, but a lot of the time you're still talking about a rounding error from zero people knowing the code- most people in the organization will still be touching the code in the future without any prior knowledge of it, and will have to start understanding it from zero.
Even if you are dealing with a small enough code base, in a small enough team, that the reviewer is adding a substantial margin of familiarity with the codebase to the team, reviewing the code is only going to give an extremely shallow level of understanding. The reviewer isn't going to have all of the context that the original developer had. They won't know all of the things that weren't done or why, the tradeoffs that were made, requirements, business context, etc. Best case scenario, someone wrote that down in a well-known place (comments in the code near the feature, something relevant in a docs directory, hopefully not as an archived jira issue that will never be looked at again). The reviewer probably has their own work to do anyway, and once they approve the PR they'll flush most of what they had out of their short-term memory anyway.
In a lot of ways, this is the same set of reasons that people aren't very likely to find bugs in a code review either. Someone without a lot of context on the problem isn't likely to find many business logic errors, or missed requirements. Since you were busy working on your feature, you probably don't know who else on what other teams might have been doing something relevant to your work, so you wouldn't know to get a review from people who might have something useful to contribute (and they wouldn't know to proactively review your code). One of the reason senior engineering roles often devolve into telling people to talk to one another is to try to address this kind of situation, but that only scales up to a small fraction of the overall code churn at organizations of any size.
For what it's worth, I ask for PRs, and I review my co-workers code. I make comment when I see something that might be useful, and I graciously accept feedback and try to make sensible decisions about when to make changes and when to let the feedback go. I actively search for PRs that might touch sections of the code I've been working in or am very familiar with to try to optimize the quality of the feedback I can give. I'm not really convinced any of this actually helps deliver better code in the moment, but maybe it does some good as a way to help teach my co-workers about things (or to learn about things they can teach me), and anyway it's such an ingrained part of engineering culture that it doesn't seem worth the political capital you'd have to spend to try to change it, and even if you succeeded it would be a prime target for scapegoating if anything went wrong later. It does slow down the development process I think, but people need down time anyway. If waiting on a code review gets people an hour or two of downtime to check their email or read HN between tasks then maybe it's actually worth it for that. The burnout caused by superfluous process and the occasionally large pile-ons and people demanding poorly thought out changes, gatekeeping your ability to deliver business critical work is essentially a cost of doing business that we seem to have collectively accepted in software.
I should also note that I also work on some open source projects, and in that setting PRs work much better. Not only does such code often need more review, but you also typically have a smaller group of people who know the whole codebase, and are much better central repositories of knowledge. The review process is also a much more effective way of doing collaborative decision making in an open source project IMHO.
Yeah, IMO this is like a publisher saying "we don't have editors: we trust our authors". The intended ethos of editors and code review is that mistakes and imperfections are the norm, and you need a second pair of eyes to iron them out.
That said, I do sometimes encounter individuals or cultures that seem to view code review more as a mechanism for catching abnormal/unexpected mistakes than as a normal part of the process of writing code.
Something that I've discovered is that as the volume of text that I write goes up, the errors increase. More experience hasn't resulted in fewer typos. On the contrary, I make far more small mistakes now than I used to.
It's precisely because I've learned to read and write so fast that I "skip over" the simple parts of sentences, making me oddly blind to certain categories of spelling errors!
Especially when I've gone over the same text over and over, I'll have memorised it to the point that I can skip entire sentences.
This is why you need someone else to step in and read the text for the "first time" and not skip anything.
> this is like a publisher saying "we don't have editors: we trust our authors"
No it's not.
If an author produces gibberish and nobody checks, the published result is gibberish. And with print books uneditable in a run starting in the thousands.
If a programmer produces gibberish, it won't compile.
It also won't pass the unit test or acceptance tests, never mind QA or the alpha/beta testers.
True that compilers, tests, and QAing are nice protections that we get with code but not books (well sorta... books can still be alpha/beta-tested in the sense of having some people read them and share their thoughts). So yeah, not literally identical.
But I think it's still the right analogy. You code review to make sure the code is comprehensible to other humans. You code review to make sure the tests (which are great automated protections) are protecting against the right things. And (most relevant to the analogy) getting those things wrong are not abnormalities that mean you're a bad programmer and we don't trust you, they're things that competent devs can get wrong all the time.
The equivalent in book writing would be that you are not allowed to write more than one page at a time, and someone must approve it before you start working on the next page.
Another difference between books and code is that books are read and interpreted by humans (who are very tolerant of grammar bad, in the grand scheme of things) and code is read and interpreted by computers (who will swallow any ambiguity whole and just steam on with their interpretation.)
If books were read by machines rather than humans, and were published incrementally, then it would make sense to ask someone to edit each iteration.
> Books are published once, not through iterations.
Incorrect. Bugs (“errata”) are regularly fixed in bugfix versions (“printings”) of books, whereas feature changes are regularly fixed in more major versions (“editions”). And that’s after the equivalent of a “1.0” release.
Books are also regularly iteratively released either to a closed group of reviewers or even the public (e.g., Manning MEAPs, PragProg Beta Books, etc.) prior to the equivalent of a “1.0” release.
> But those fixes don't fix the already printed books.
For actual physical books (as opposed to ebooks, where making the complete new version available to purchasers for errata, but usually not new editions, is common), this is usually true for logistical reasons, though issuing errata sheets (usually online now, though the practice is older but notification and distribution was harder in the past) to keep existing users up-to-date is not uncommon.
That final, 1 word sentence is one of the major problems of PRs. An engineer might spend several hours really thinking through a problem, talking with colleagues, whiteboarding options and coming up with a workable solution that addresses all the obvious issues and a bunch of non-obvious ones.
Only for a drive-by take-down by someone with none of the context. It's a totally asymmetric investment of time. Even helpful review comments might only take a minute to write but a day to incorporate.
> Only for a drive-by take-down by someone with none of the context
If that's a regular issue that's a culture problem. Starting with "if you've been talking with colleagues about your problem, why is someone with no context reviewing the result?", people not investing time in reviews, people doing "take-downs" instead of asking questions if they don't understand things, hold up merge for non-urgent concerns ...
> Even helpful review comments might only take a minute to write but a day to incorporate.
Either the feedback is worth the investment of the day of time, then no problem, otherwise it's not that important and doesn't need to be done (or depending on what it is can be done later or ...)
> If that's a regular issue that's a culture problem.
Couldn’t one also argue that if code reviews are routinely catching problems mentioned earlier in the thread (misinterpreted requirements, conflicts with other WIP, etc.) then that’s a cultural problem? It just seems odd to assume that the person assigned the task couldn’t possibly be expected to routinely avoid those problems, but that adding rigorous code review could routinely avoid them.
Yup, definitely. Code review discovers both issues stemming from cultural problems and silly bugs and understandability issues. Peer review in general is a very powerful tool for things that should be designed to satisfy customer needs.
> otherwise it's not that important and doesn't need to be done
Except that this decision is often not up to the code author, because the tooling rejects for example committing changes until all discussions are resolved (see Gitlab for example) and some @holes make their quest for any reason not to resolve the discussions that they started until the code author writes the exact code with the exact words in the exact indentation with the exact architecture, etc that those @holes like, which makes the whole code review like a torture and the REAL productivity killer. Not to mention that this does not provide any improvement to the code base either in most cases.
> drive-by take-down by someone with none of the context.
If this is an issue in your workday, you should bring it up with management or the individual rejecter. If someone has the authority to kill PRs, they should also be required to put in the effort to understand it and/or be available for discussion at an earlier stage of the development process. PRs shouldn't ever be rejected without mutual agreement - neither internally in an organization, or in an open source project.
Is this a common practice? I've never worked under a code review process where the accept / reject decision was being made by "drive-by" reviews from "someone with none of the context". It has always been team members with a lot of context reviewing each others' code.
I've never even worked with a code review process where outright rejecting PRs is a normal part of the process for anything other than external contributions. The normal process is more of that if you think a PR is a bad idea it's up to you to persuade the author of that. You'll certainly sometimes have the author withdraw the PR without being truly convinced, but something like "I don't like this. Rejected." would just be disregarded if you can get other people to approve it.
I think the issue is not that people make drive-by issues, the problem is that people argue about things endlessly which does not bring any value neither to the customers nor the company, that is only for the vanity of the reviewer, and many reviewer get offended when someone does not take blindly their "wisdom" and for retaliation they block code commits endlessly which is enforced by automated tools like Gitlab.
Running through `git blame` and looking at the commit message, the PR, and the PR comments is a very practical way to learn about a codebase! Beyond some high level code organization stuff that exists in a readme, learning a codebase is really about getting a history lesson of how the product evolved, the company pivoted, and the team re-org'd.
It'll be 6-months if you're lucky. Try figuring out why there's a particular "if" clause, one or two or four years later. Software maintenance, especially when the original author has moved onto another team/organization/company/career, is a frustrating art of almost remembered stories and hoping that you can figure out Chesterton's fence, lest it become Chesterton's barbed wire. The worst is when you fix a small bug with code and introduce an even bigger big in the process.
Not necessarily looking through PRs, but presumably they need to work with actual code in your code base that was merged at some point, unless your product is growing so fast that new people just write new code all the time? (Same applies for not-new coworkers that now need to touch that code area anyways)
Sure. If they are good and have a decent grasp of using version control then will look at the history of a certain piece of code if they don't understand why it is the way it is. I do it often, even when I'm no longer a new hire.
That's what YOU think. But in practice they produce sub-par solutions and worse quality in my experience. Code reviews are basically a virtual piss-contest where people argue endlessly, everyone trying to show-off their intellectual superiority. And reviewers (not code authors) often can't handle rejection for some reason.
Take this with a grain of salt, but a good code review should be an exploratory process. Most of the non-trivial comments should be questions[0], and the reviewer should be operating under the assumption they'll have to jump on a call or even fix it themselves.
[0] Not just because the reviewer is trying to be nice, but because they don't know what constraints and problems the author discovered in the process of writing the PR. If the reviewer writes "use a map here, instead of scanning an array" without knowing how many items are handled, then they may be making the PR worse by replacing a cheap linear scan over a maximum of 50 items with a bunch of costly yet constant time hashing. Often the solution is an explanatory comment rather than the "obvious" fix.
Fair, and I've seen this, but usually it's because the reviewers are treating "I would personally prefer to do this another way" as blocking feedback, when it isn't.
Good reviews can take a light touch, in part by differentiating non-blocking ideas and suggestions from "hey I think this is a bug."
Some of my favorite reviews of all time actually don't change a single line of code, but the questions reviews ask zero in on what's going to confuse the next developer so it can be documented appropriately.
Usually I answer unspecific 1-sentence criticisms with a question what alternative solution the reviewer would suggest. Sometimes they look into it more deeply and really come up with a better solution. If they don't, at least the discussion is over.
The best code I have seen was on projects without code review. And projects with it were more mess - they were surface consistent but overall hard to comprehend.
The deciding factor was ownership and accountability tho - you maintained own code and if you done it crappily, you knew.
The code review is related to assumption that everyone can change everything - meaning all in all inconsistent mess. It is also related to unfortunate assumption that code review is how people learn system. They don't, unless they are original author. The rest of then knows pieces of it only.
I prefer a top down where project leads are responsible for code, I.e. they review all code and they are simultaneously responsible for getting things done. It forces balance. I will let juniors make a mess in certain areas and not others. Sometimes you even let bugs in if you know a specific test will fail so people are more careful and don't depend on reviews too much. If you are a bad lead and nitpick every little thing, then you get to explain why your project isn't ready. Honestly getting devs to git add -p and look over their own pr catches much of it.
I agree that code review is not a great medium to disseminate information about the codebase, but assigning individual ownership and accountability doesn't make the need to share information go away.
Ownership and accountability are workable as long as the same person sticks around long enough to fix all their mistakes. If that person leaves, the code becomes orphaned and it's up to one of their teammates to find out where the skeletons are buried.
No it does not. But it also leads to one-two clear person you know to ask. It also leads to same person explaining same thing multiple times. So as long as he cares just a little, the explanation will improve.
And the person doing explanation actually understand the whole part. That is big one too - you are not explained bits of it by someone who knows only small parts of it. And the system was not changed by third party without the person who explains having at least vague idea it was changed.
> Ownership and accountability are workable as long as the same person sticks around long enough to fix all their mistakes. If that person leaves, the code becomes orphaned and it's up to one of their teammates to find out where the skeletons are buried.
That is actually exactly the same without clear areas. Except everyone is new all the time.
If you have high turnover, this aspect will be bad matter what.
And it is not that bad with responsibility either. Old person does explains things to new person. Taking over, when you are working consistently in same area is not that hard, actually. Some parts you conclude to be mess, but usually not whole of it.
---------
And imo, quality and knowledge heavily depends on other parts of team. Analysis and testing. Both, if work reasonably well are massive resource for knowing how the hell the system is supposed to work.
And once you have reliable source for requirements, that is not developers, then deciphering the code is much easier.
In my mind at least, the issue with relying on on ownership, especially in a smaller team with limited resources, is that the owner isn't just the person who authoritatively understands that subsystem. The owner ends up being assigned _all_ of the work on that subsystem because it's "their responsibility".
As a result, any work that's assigned to someone else will only interact with at the interfaces / boundaries. And of course, that's sort of the intention with modularity / loose coupling - it's not necessary to learn the gory details of the XYZ subsystem's implementation, only its API. Knowing the details is certainly valuable, but there's no baseline impetus to get the owner to explain the details.
Taking over from someone who's ceding responsibility, but staying at the company, I agree that's manageable. But sometimes people get laid off suddenly (or perhaps get hit by a bus, etc) and there's no chance to get the one person who has it all in their head to explain it to you.
You mentioned high turnover, but if anything, a place with high turnover has less information locked up in a single person's head. If Alice who's worked at the company for 25 years and has always owned the code leaves, you're going to have a much harder time getting all of the tribal knowledge out of her head on two weeks notice compared to Bob who only made it 6 months.
To me, the value of code review is that even at its worst, someone other than the author is forced to look at the code. No process can force a reviewer to take an active interest in a particular subsystem, but at least we can make them look at pieces of it and see if it gets them curious enough to ask further questions of the author and better understand the system.
For myself, knowing that someone else will have to understand my code, even through the limited lens of a code review, makes me more diligent about making sure that the code is clean and the pull request has enough context that someone playing code archeologist will be able to make sense of it, even if I'm not walking them through it in real time. I will admit that this is not a universal thing - I've worked with coworkers where no matter what process is in place, they won't do the basic courtesy of reading their own changelists to see that they accidentally committed random junk files from their local machine.
I agree that good documentation around requirements is valuable. I find pull requests / code reviews are a great opportunity to highlight how the code relates to specific requirements, since the focus is narrower.
The more problems you have the more policies you add to prevent future problems. I really doubt that a low quality codebase would become higher quality with the addition of code reviews and vice versa.
A formal review step where you accept or reject the code, yes, is always there because of lack of trust.
Informal reviews can have a lot of different shapes, and provide a lot of different benefits. But anything that is a stop-the-process activity is there because you expect quality problems from the earlier steps.
(That said, no good developer trusts oneself. So if you don't have anything on your process that makes you trustworthy, lack of trust is the correct approach.)
I tend to scan most reviews for the team even if the code review itself is not directed to me (we use a round-robin/domain expert routing method). It helps me understand what my teammates are working on and give me a better understanding of our system. I can't imagine anyone interpreting that as a lack of trust. Is looking at our source code inherently distrustful?
Agreed, so many times reviewers have saved me from myself as well as passed on solutions I didn't know, pointed out functions in our giant codebase I wasn't aware of, taught me new language concepts I wasn't familiar/comfortable with (ie, C++11->14->17->20) and generally helped limit our technical debt.
I'm a fan of code review
For me, code review is collaboration. We're helping each other.
The one thing I'd suggest is, as much as possible, try to phrase reviews like a question: "Do you mean to do X here?", "Would using fooTheFob here work?", "Is this a race condition?". At least it feels better to me.
I think it is trust in the trust and verify sense. Meaning, we trust each other to do our best but let us verify that we are all on the same page and aren't breaking things or doing something harmful.
What is the "trust and verify sense"? I've been told this by managers before but I've honestly never understood it. "Trust" to me means "be willing to take action without further examination of the facts at hand". If I trust your code I wouldn't review it. But I don't trust your code, and I don't trust you to have taken all of the appropriate considerations when you were writing it. What's the difference between "trust but verify" and "don't trust, verify"?
Is the phrase "trust but verify" an attempt to avoid damaging self-esteem? There seems to be some emotional/ego-based attachment to the concept of trust.
I think it's differentiating total lack of trust ("all your PRs suck while mine are perfect, so I have to read yours") from universal fallibility ("your PRs are great, but all code has bugs, so we all review each other's code to catch some of them").
Absolutely. I take issue with all 3 of his main points. You covered the first point well, so, I'll take the last 2.
> Pull requests don't prevent bugs. Reviewing code is hard! You can catch obvious mistakes, but you won't have consistent in-depth scrutiny. Many issues reveal themselves only after extensive use, which doesn't happen during code reviews. We rely on fast iterations with user feedback.
So many things I can say about this. "Fast iterations with user feedback" are great, until you introduce a security bug that you find out about because someone exploits it. Catching "obvious" mistakes alone has significant value. Since he acknowledges that code reviews catch some bugs, this criticism seems to be that they don't catch all bugs. To that, I say: show me a practice which does catch all bugs, and I'll show you why it doesn't. Even the most rigorous development process doesn't prevent all bugs. [0] This just doesn't sound like a fair criticism to me.
> Pull requests slow down. Code reviews aren't high priority for engineers. They have to make time to clean up their review queue. In reality, PRs are an afterthought and not what you want to spend your time on as an engineer.
This is largely a culture problem. Code review should be a high priority for engineers. Maybe it's not the highest priority, but, it's fair to expect one could get a decent review on a PR that isn't too large in, say, 1-2 days or less. If you have PRs sitting in review for weeks at a time on a regular basis[1], one of two things is happening: either engineers aren't properly prioritizing code review, or your PRs are too large and/or have too many dependencies.
Which brings me to the second thing you can do, which is make PRs that are as small and simple as possible, but no more. Minimize dependencies to help make your PRs understandable. And, it should be fairly obvious, but the fewer lines of code (including config changes) you have in your PR, the easier it will be to review, so, the faster it will get done.
Code review is also a great opportunity to learn and teach. Every senior+ engineer should be reviewing code frequently, IMO.
Finally, yes, code reviews will slow you down if your basis for comparison is just "engineers merge code when they think it's ready." That's intentional, and good. The cost is more than paid for by better code quality, leading to fewer instances of "here's a PR to fix the issues with my previous PR."
[1]: I've only had this happen once or twice in my career so far. In each case, it was because the code surrounding the code I was working on was not stable and kept changing. These were clearly exceptional instances.
Is that why we do code reviews, because we don't trust each other? I find this attitude problematic. We do code reviews because humans make mistakes. Requirements can be misinterpreted. Different work in progress can be in conflict with each other. Reviews are a good way to learn from each other and keep abreast of what work is occurring outside your own bubble. Not doing code reviews because of "trust" kind of misses the point.
I find even just cursory and brief code reviews are beneficial and often pick up on issues. It also creates a more collaborative team and gives me a reason to communicate with devs I might not otherwise communicate with, even if that is just a brief comment on a PR.
Rejected.