29 ms·

Code reviews do find bugs

221 points by _xivi 2y ago

epolanski 2y ago
My beef with code reviews is that often they lead to tremendous amounts of wasted time, that's many thousands spent in a single week sometimes for simple pull requests. Working from 6 years, not much, and not in as many places like others, I have built the opinion that code reviews are like tests, they should be used as a tool when they are necessary, they shouldn't be the default for every change. In best case scenarios is the person creating the pull requests that requests reviews or decides the place needs tests. My opinion obviously applies to product software, for libraries, especially when publicly exposed you want as much discussions and tests as possible.
- simonw 2y ago
  This is certainly true for blocking code reviews. I'm interested in exploring the alternative, which is review-after-commit. There's an article describing those here: https://copyconstruct.medium.com/post-commit-reviews-b4cc2163ac7a https://copyconstruct.medium.com/post-commit-reviews-b4cc216... Code still gets reviewed, but you don't end up with PRs languishing for hours, days or even weeks waiting to get a review from someone.
  - interactivecode 2y ago
    I review a lot of code with the mindset of yes and… Basically when I start the PR is approved in my head until I find something blocking. I.e. major problem that causes dataloss, big performance issue or breaks other code. Anything else is a minor comment at most. The PR is approved by default. This gives the dev responsibility and ownership. Plus it increases release cadence Doing a follow up PR to improve or fix something is just as fast as blocking the MR, but blocking the MR can be bad for morale. This strategy might work better in young startups where having the feature _exist_ is better than not shipping. in my experience this builds up responsibility and ownership and removes the whole policing of other peoples work vibe around code review. Also decisions and discussion around formatting, when to test, design, features, functionality, architecture should not happen during code review, they should have happened way before coding, or collaboratively while working. Code review is the worst time for that stuff, imho it should be to sanity check implementation.
  - YZF 2y ago
    I used to do a lot of this in a small team where we didn't block on reviews (we did reviews but didn't block). I was a senior developer on the team and I'd take time to read through new sections of code that came in. That worked pretty well. Interesting enough, this bit of code/project, that didn't have super strict code review requirements, but had a lot of tests, is the code I worked on that I would consider the most robust/high quality. It was run by > 10 million users in a fairly important application. It wasn't huge and it had good tests (and was generally quite testable). That said, it's really hard to control review-after-commit. Maybe we need better tooling for that. In my case, for the areas of code I was involved in, it was small enough to track in my head.
  - kqr 2y ago
    I really like this idea! It's not like I need to check how any individual developer approaches their work (although that could become a useful mentoring session in some cases) but what matters is what it looks like before going into production. The main difficulty I see with the described approach is that different changes will be interleaved in the trunk and it might be hard to extract just one of them to deploy. But that's what feature flags are for!
  - pavel_lishin 2y ago
    I'd love to explore that alternative, but I'm not sure how to actually make sure that any errors found/changes suggested after the commit is pushed and deployed actually get implemented.
- closeparen 2y ago
  The discipline of putting up small, coherent, explained, tested, and reviewable units of change, that you have looked over and feel comfortable showing off to other people as your work product, is 80% of the value for me. Whether anyone else actually thinks about it deeply or has something useful to say about it is secondary.
  - goosejuice 2y ago
    Indeed. It's kind of like rubberducking.
- kqr 2y ago
  This is so far from my experience with code reviews that I'd like to ask some questions to follow up on your experience. Do you mind shooting an email to hn@xkqr.org?
- ycombinatornews 2y ago
  There’s probably more to this story. It’s somewhat a skill and maybe art to create MRs so that they are easy to review and get approved. It is also true for the feedback on MRs. Providing compressed and succinct feedback makes it faster to address. Almost like “if the change is difficult, refactor and make the change easy”. There are many ways to do one thing, some are better, some are not. Companies and teams that have good review culture are successful in using the reviews as a tool.
willio58 2y ago
Agreed. I mainly manage and review code at this point in my career. I find many bugs, every once in a while finding something that would have caused an outage or notable problem for users. What I find more though is code that isn't thought through. Tech debt and code smell are real, and they affect the performance of a team. Nipping that in the bud takes quality PR reviews and time to meet with submitters around issues you find. Knock on wood but working at the company I do now where I, along with my team, have made quality PR reviews normal.. our codebase is now enjoyable and fun to work on. I highly recommend it! One key aspect is being “kind, not nice”. Be helpful when leaving comments in PRs, but don’t be nice for the sake of avoiding conflict. Also if you find code reviews to be a waste of time I can reccomend one thing I do often - give warnings. I approve and give comments around things I’d like to be fixed in the future for similar PRs. I don’t hold up the merge for little things, but at the same time I won’t let the little things slide forever
- saulpw 2y ago
  How do you phrase these warnings? "Next time.."? I have a hard time being serious with my own warnings if it's fine enough for now.
  - audiodude 2y ago
    "This is okay for now, but we should think about how we want to serialize these objects. Feel free to remove the N^2 algorithm in a follow up."
    - est31 2y ago
      That works great in a setting where you are both employees of the same company, and you respect each other, but it often doesn't work in the open source world, people just disappear and you never hear from them again. It is possible that they do file follow-ups, but in my experience it's rare.
      - sfink 2y ago
        Yes, even within a company my threshold for accepting a change can vary pretty widely depending on my experience and relationship with the author. For an external contributor or someone I've never collaborated with (by reviewing code or having my code reviewed), I don't accept the code until almost everything is worked out to my satisfaction. With someone I work with regularly, it's not uncommon to accept a change with a comment like "this is all good, but you need to take X into account which will change almost everything in this patch" (I exaggerate, but only slightly). I know whether an update could be problematic and whether it is necessary to see it again. Sometimes there are a couple of obvious ways that something could be done, they picked one but weren't tied to it if I had a reason for picking a different one, I picked a different one for $REASON. Most are somewhere in between. Though in some ways it works the other way around. For an unfamiliar open source contributor, I need to be confident that the change is worthwhile. I will be lenient on stylistic things, and I'll just land their patch and then fix it up afterwards. For someone I've worked with a bunch (whether a familiar contributor or a coworker), I will trust their opinion on the underlying quality of a change, but be less tolerant of unnecessary stylistic differences since they should have already come into alignment on those and it's more likely to be an oversight if they missed something. (Plus, I don't want to be fixing up their changes after the fact, given that >90% of patches will come from regular contributors.)
godelski 2y ago
I think this is really important in that it is bigger than "code reviews." It does show how people greatly misunderstand statistics[0]. And what's even funny is at surface level the claim that code review "does nothing" __sounds__ ludicrous. But people "believe" because they are annoyed with code review, not because they "actually" believe the results. But statistics are tricky. With the example given in the article "15% of smokers get lung cancer" compared to "80% of people with lung cancer smoke." These two are not in contradiction with one another but are just different ways to view the same thing. In fact, this is often how people will mislead you (or how you may unintentionally mislead yourself!) with statistics. Another famous example is one that hits HN every once in awhile: "Despite just 5.8% sales, over 38% of bug reports come from the Linux community"[1]. In short this one is about how linux users are just more trained to make bug reports and how most bugs are not system specific. So if you just classify bugs by the architecture of those submitting them, you'll actually miss out on a lot of valuable information. And because how statistics work, if the architecture dependence rate was as low as even 50% (I'd be surprised!) then that's still a huge amount of useful bug reports. As a linux user, I've seen these types of bugs, and they aren't uncommon. But I've frequently seen them dismissed because I report from a linux system. Or worse, support sends you to their page that requests you to "upvote" a "feature" or bug issue. One you have to login to. I can't take a company like that seriously but hell, Spotify did that to me and I've sent them the line of code that was wrong. And Netflix did it to me saying "We don't block firefox" but switching user agents gave me access. Sometimes we got to just think a bit more than surface level. So I guess I wanted to say, there's a general lesson here that can be abstracted out. [0] Everyone jokes that stats are made up, but this is equally bad. [1] https://news.ycombinator.com/item?id=38392931 https://news.ycombinator.com/item?id=38392931
- jiggawatts 2y ago
  > support sends you to their page that requests you to "upvote" a "feature" or bug issue. Microsoft does this for enterprise products where customers might be paying $100K/mo or even millions. “We hear you, but your complaint is just not popular enough so go away.” “Sure it’s a catastrophic data loss bug that ate your finance transactions, but if other people can’t identify that their seemingly unrelated crash is the exact same issue then no fix for you.” “Now that you did get ten thousand votes on an issue titled ‘Consiser doing your job’, we’ve decided to improve your experience by wiping out the bug forum and starting a new one from scratch that has fewer scathing comments from upset users.”
sarchertech 2y ago
I remember a time before you needed an approval to merge a PR (I also remember a time before PRs or any widespread version control system). I can count on one hand the number of times someone has caught a bug in my code that should have stopped deployment. Not that I haven’t deployed serious bugs to deployment, but they’ve almost never been caught by someone reviewing my code. Occasionally someone suggests a better way to do something, or asks a question that ends up with me coming up with a better way of doing something. But those situations are also rare. And I can’t think many times at all when the impact was worth the time spent on the process. Pair programming and collaboration can be hugely beneficial, but the minimal effort PR approval culture we’ve developed is a very poor substitute.
- wrsh07 2y ago
  I've caught bugs in reviews, but even better I've requested tests and those tests have caught bugs Even a low effort code review can identify missing unittests
- dgb23 2y ago
  Both code reviews and pair programming can be very useful if they serve a specific purpose. Getting someone up to speed with unfamiliar code, disentangling hairy code so it becomes clearer, hunting down bugs or finding unknown unknowns such as bugs or unnecessary complexity. However in many cases not looking at the screen when doing these kinds of things is more helpful. It's often more beneficial to build a mental model in your head and then riff off each other. Rather drawing things on a board or writing down stuff in a markdown file, explaining things in simple terms, than actually coding or reading actual code. Not sure if that still counts as pair programming or code reviewing but this free form way of talking about code is very effective.
- kqr 2y ago
  It certainly sounds like you write seriously high-quality code! And judging from your profile, I'd be inclined to think you know what you are talking about. I'd like to ask a little more around your experience here. Do you mind sending an email to hn@xkqr.org so that I can ask some follow-ups?
alex_lav 2y ago
Code reviews can find bugs. More often, code reviews become opportunities for team members to bikeshed. Worse, an opportunity for a non-team member to exert power over a project.
- spankalee 2y ago
  Bikeshedding in a team can be good. If you're all painting the shed, it helps to agree on the color. More generally, code review is a great opportunity for incrementally gaining or encouraging alignment across the team. What the team chooses to align on and how strongly are left up to it, so hopefully they choose to not get bogged down in inconsequential details, but completely skipping the pretty cheap chance for reenforcing all kinds of cohesion would be a big mistake in my opinion.
  - alex_lav 2y ago
    You're making a lot of positive-upsided assertions about code review. My point is there is too much opportunity for negative behavior. It's the same as everything in tech, in life, "It can be good if everyone does their part to keep it good". And yet, most don't.
    - spankalee 2y ago
      "most don't" is a strong claim. In my experience, core review has been undoubtedly good. I would never run or join a company without it. I'm writing code solo for the moment, and code review is maybe the thing I miss the most.
      - alex_lav 2y ago
        > "most don't" is a strong claim. I'm happy to be reasonable. I guess my greater feeling is that most devs aren't great at identifying when they should identify restraint. For the same reason that most devs are abysmal interviewers, I think devs forget that code review is ultimately a human endeavor. Give your average dev the smallest amount of power and not enough guardrails and legitimate silliness ensues. > I'm writing code solo for the moment, and code review is maybe the thing I miss the most. I feel as though "code review" is taking on too many meanings in this conversation. Code review in the form of a second (or more) qualified dev reading and commenting on code for the greater good? Obvious good. Code review in the form of github PRs at a non-FAANG company? Skip it. Kangaroo court.
david2ndaccount 2y ago
In my experience, code reviews catch a lot of bugs. However, if you find yourself catching the same kind of bugs over and over again in review you should be finding ways to catch them automatically without involving a reviewer (static analysis, tests, linters, etc.)
- cjriley 2y ago
  Completely agree on utilizing static analysis as much as possible. My first instinct when finding an issue in a code review is to think, "could we have caught this with a <lint rule> of some kind?"
Mathnerd314 2y ago
My question is, do human reviewers find more bugs than ChatGPT? Because finding a cofounder costs a lot but asking ChatGPT is free. https://www.thatsoftwaredude.com/content/12848/chatgpt-can-do-code-reviews-but-are-they-any-good https://www.thatsoftwaredude.com/content/12848/chatgpt-can-d... says it is mediocre, but that was a year ago and honestly mediocre code reviews seem sufficient.
- deleted 2y ago
  [deleted]
sys_64738 2y ago
There are various levels to code reviews. Code review tools that are web based are pretty poor in my experience. Anything more than a few lines across multiple files needs a cscope type tool. Also what type of review? Is this a prototype needing a high level design review so that the actual review doesn’t turn into a design review? How often does that occur? Who are the reviewers and what’s the process? Key stakeholders have more influence and you need to consider the reviewer’s experience, knowledge and credibility. Finally how important is the code? Is it kernel code, or high execution daemon code needing race condition and memory leak checking? Are you using static analysis for the code? Does the code even compile and do what it is designed to do? Where are the unit test logs? Lots to consider.
29athrowaway 2y ago
If you have a spellchecker, code formatter and a linter, code reviews improve significantly. Much better than having to do that work by hand, or reviewing it by hand, leaving code reviews for higher level ideas.
- zelos 2y ago
  Exactly. Code reviews shouldn't be about code formatting or anything that can be automated away with linters, formatters, code coverage limits and static analysis. If the build is green for the PR, then all that is already acceptable.
bluGill 2y ago
Bugs are 'easy' to fix, I don't worry about finding them. I worry about the interfaces as they quickly become a nightmare to change just because of all the users.
- kqr 2y ago
  I'd be interested to hear more about your experience with code reviews. Could you send an email to hn@xkqr.org so that I can ask some follow-up questions, please?
poikroequ 2y ago
The value of code reviews really depends on the code and the person working on the code. For a team who have spent years working on the same repo, code reviews may not hold much value. But if you have a new guy on the team, or a junior, you'll definitely want to review their code. Code reviews can also do more than just find bugs. You can point out a better way of doing things. Maybe this SQL could be more efficient. Maybe you can refactor some bit of code to make it more robust. Maybe you should put a logging statement here. This method name is confusing, may I suggest renaming it to xyz?
- kqr 2y ago
  I got really curious and I'd like to ask you some follow-up questions on your experience in reviewing and receiving reviews. Do you mind shooting an email to hn@xkqr.org?
- gwd 2y ago
  > For a team who have spent years working on the same repo, code reviews may not hold much value. I have definitely found bugs [ETA during code review] in code written by very senior developers in code they've been familiar with for over a decade.
- phito 2y ago
  Code reviews also keep the team up to date with what is changing in the code
  - seadan83 2y ago
    Commonly stated, except I think that statement is wrong. A PR is the thing that has the effect of sending emails to the team, it is the PR that leaves behind a webpage & summary of what has changed. It is the PR that is doing the work there, not the CR. Which implies, you can open and then immediately merge PRs without CR to get that same benefit. CR does give others a chance to study code, and become familiar with it - but that is different from "keeping up to date."
- dkdbejwi383 2y ago
  > But if you have a new guy on the team, or a junior, you'll definitely want to review their code. Reviews _from_ juniors or new team members are also really valuable, as they don't have the history or tribal knowledge that others may have. They'll often spot things that have gone overlooked because "that's how it is".
some_furry 2y ago
> During the first 60 minutes of code review of the day, the reviewer finds roughly one defect per ten minutes of reviewing – as long as they review less than about 50 lines of code per ten minutes. Oh. It normally takes me a few seconds to find bugs in code. I always felt this was average performance for assessing software. If the average time is ten minutes per defect, I need to recalibrate my expectations for myself.
- lazyasciiart 2y ago
  Presumably you understand that how long it takes to find bugs in code depends on the code. If not, then I hope you've read the code for Linux and SSL, etc.
  - some_furry 2y ago
    Yes, of course it depends a lot on context. I've never had an incentive to read the Linux kernel code. I routinely find and disclosed cryptography library bugs, though usually mostly hobby projects like the "I thought it would be cool if there was a PHP implementation of GHASH" sort rather than like OpenSSL.
- jonobird1 2y ago
  It really depends on the code. To find a CSS bug, yes easy peasy. To find a logic hole in a payment integration of what someone has missed or should have implemented but didn't (eg webhooks), then this requires a lot more time and the developer basically has to sit down properly to work out exactly what should have been implemented / how they would have developed it, and then cross-check it against what has been done, otherwise you won't be able to easily find those logical holes which effectively are bugs, just not simple code bugs like a missing semicolon.
  - some_furry 2y ago
    My day job is auditing cryptography. I'd probably be slower to find the root cause of a CSS bug than most of the folks that read HN. :3
mgreene 2y ago
The paper's title is a bit provocative but I think the findings are interesting. Mainly around long-held beliefs about what developers perceive as the value vs what is actually happening. You do bring up a good point about using change defect rate though. I wish the researchers had cited that as the preferred unit of measurement. I did some research on change defect rates on popular open source projects and it's all over the map. Ranging from ~12 - ~40% [1]. The future I'd like to see is as developers we use objective measures to justify time investment for review. This is going to be increasingly important as agents start banging out small bug-fix tickets. [1] https://www.shepherdly.io/post/benchmarking-risk-quality-kpis-in-popular-open-source-projects#how-often-are-bug-fixes-happening https://www.shepherdly.io/post/benchmarking-risk-quality-kpi...
jt2190 2y ago
I’m not sure why the author ignores the “… that should block a submisson” part. The abstract of the paper: > Because of its many uses and benefits, code reviews are a standard part of the modern software engineering workflow. Since they require involvement of people, code reviewing is often the longest part of the code integration activities. Using experience gained at Microsoft and with support of data, we posit (1) that code reviews often do not find functionality issues that should block a code submission; (2) that effective code reviews should be performed by people with specific set of skills; and (3) that the social aspect of code reviews cannot be ignored. We find that we need to be more sophisticated with our guidelines for the code review workflow. We show how our findings from code reviewing practice influence our code review tools at Microsoft. Finally, we assert that, due to its costs, code reviewing practice is a topic deserving to be better understood, systematized and applied to software engineering workflow with more precision than the best practice currently prescribes. “Code Reviews Do Not Find Bugs: How the Current Code Review Best Practice Slows Us Down” https://www.microsoft.com/en-us/research/wp-content/uploads/2015/05/PID3556473.pdf https://www.microsoft.com/en-us/research/wp-content/uploads/...
- gwd 2y ago
  The "that should block submission" is always one of the trickiest parts. There's a saying: "Everyone that drives slower than you is an idiot, and everyone that drives faster than you is a maniac." But it is true that going faster increases danger, and there is a speed that appropriately balances benefit against risk; but everyone perceives it differently. The same is true of "code smell" issues: Everyone who asks you to change things is a pedant who's slowing down the project for pointless aesthetics, and everyone who pushes back against changes you've requested is a cowboy who is going to make the code harder to maintain in the future. So in the paper, how did they decide whether a non-bug change "should block submission" or not?
  - jt2190 2y ago
    If a comment points out a bug/defect [1], then it should block. If you think about it, as bugs/defects are removed, the code becomes more correct and thus more stable because it doesn’t need additional changes to remove bugs, so removing bugs reduces the need for future maintenance. If we block due to future maintenance concerns what we’re really asserting is that the requirements are unstable, and that removing today’s bugs is less valuable overall because requirement changes will remove the line of code with the bug and replace it with a new line of code with a new bug. I suppose it depends on the code review process at at a given organization whether that’s the appropriate point at which to block code for architecture/design issues. In my experience the code review step is much too far downstream in the development process and much too narrowly focused on a subset of code to be an effective place for design changes that have significant impact on maintenance. [1] The paper authors reviewed data in Microsoft’s internal code review tool, which is proprietary, so we can’t see what the specific bugs were.
nitwit005 2y ago
> Developers spend six hours per week reviewing. This is a bit too much It's extremely difficult to adjust the time spent on reviews. The options are unattractive. Do you start blindly accepting changes when you hit the limit, or just stop and not let people merge code?
- dakiol 2y ago
  Why should one block merging code? The idea of someone spending hours working on some code changes being blocked by another individual that doesn’t allocate time for reviewing is alien to me. We are all professionals, let people merge their changes and make sure you find time to review them. If you can’t review them and they still need your approval, then approve. Unless you don’t trust your colleagues. If that’s the case, then code review is doomed anyway
jonobird1 2y ago
I'm not sure code reviews hold much merit. I've been a web developer for around 12 years and I've worked in companies big and small. I think there should be a manual QA process to test the functionality of what the developer is pushing out. The issue with code reviews is always that they take so much time for another developer and many devs are super busy so they just have a quick review of the PR and approve or feel they have to add some comments. Context switching between what the dev is already doing and having to come to the PR to review properly means they should switch to that Git branch, pull down the code, test it all and check for logical bugs that a static code review won't pick up. For juniors, code reviews are still useful as you will be able to spot poor quality code, but for seniors, not as much for the reasons above, better off having a QA process to find and logic holes rather than expecting devs to invest so much time in context switching.
- skywhopper 2y ago
  Nah, automated testing cover basic functionality. For most PRs, a senior familiar with the code wouldn’t need to check it out and manually test anything, that’s not what “code review” is most of the time. If you need them to look at the code in a running state, that should be part of the CI process, not a manual task for the developer. A good reviewer can call out bad strategic coding decisions or misinterpretations of the requirements. QA is another layer of review entirely.
- hakunin 2y ago
  The problem here is not that developers are too busy, but that code reviews are considered second class citizens to churning out new code. It's like saying "many devs are super busy working on feature A so they just write quick and dirty code for feature B". If reviews are integral part of feature production pipeline, there should be no issue to sit down and spend a day reviewing code. For bigger, more complex things it could be a few rounds of reviews. There is an approximate non linear relationship between time it takes to produce the first PR and time it takes to go through all rounds of review. This time can be pretty reliably calculated and taken into account.
- kqr 2y ago
  I'm a little surprised to hear this. Would you mind sending an email to hn@xkqr.org so that I can ask some follow-up questions, please?
andirk 2y ago
Technical debt. Keep it minimal, and when needed, write a task for it to be looked in to later. Coding standards. Don't submit code that has rando extra lines and things that will slow down the next dev from looking in to the past to learn what does what. And most of all, make sure edge cases are covered, often via a truth table of all possible outcomes. I often comment on a PR saying "blah blah, but not blocking" so I'll allow it but at least my entitled opinion was known in case that code issue comes up later. My PRs take a long time, because I dig the F in.
swatcoder 2y ago
As with most processes, the dilemma with code reviews is in figuring out how they impact your team and your organization. In a huge org, with thousands of engineers that's already burdened by hours per day of interruptions and process overhead, and release runways that already involve six stamps of bureaucracy, mandatory code revies have very little downside (it's in the noise) but highly variable return (many people are just droning under the weight of process). The org loses nothing much for mandating it, but only certain teams will see a lot of value for it. On the other extreme, a startup with five engineers will get backlogged with reviews (which then get shortchanged) because everbody either is under pressure to either stay in their high-productivity flow or put out some pressing fire. The reviews probably could catch issues and share critical knowledge very regularly, but the org pays a pronounced penalty for the overhead and interruptions. People long for "one size fits all" rules, crafting essays and publishing research papers to justify them, but the reality of what's right is often far more idiosyncratic.
- kqr 2y ago
  I don't disagree with the idea that "it depends" but for me, code review has generally worked better with lower overhead in the "startup with five engineers" type organisation. Can I ask you some follow-up questions on your experience in reviewing and receiving reviews? If so, send me an email at hn@xkqr.org!
grumple 2y ago
I find things wrong with virtually every nontrivial pull request when I’m the reviewer. Sometimes these are minor issues, but I spot bugs and edge cases all the time. I see some comments about time. How long does a code review take? I can review hundreds of lines of code in a few minutes. It is much easier to review code than to write code imo, especially as you gain experience. For bigger efforts, get eyes on it throughout the process. I’ve met a lot of developers who assume their code will just work right after they write it. They don’t test it, via code or manual qa. Then they act surprised when the stakeholder tells them it doesn’t work. Do the job right the first time. Slow is smooth and smooth is fast.
- farmeroy 2y ago
  I'm always surprised how often I get a pull request which either doesn't build or has failing unit tests or both. These are pretty easy to address at least - but when I think that certain code might be difficult to maintain, be an anti-pattern, or possibly present bugs in non-obvious ways, I find it really hard to effectively address those issues and often end up doubting my own suggestions
- aitchnyu 2y ago
  When I was working with Django, those who added code ran it in their systems. With lambda, which we mostly deploy and test in the cloud, people tend to dump code and leave.
rebeccaskinner 2y ago
I think the article is taking the wrong view. The statistic cited by the article that 15% of comments were about a bug seems in line with expectations, and I think it would only really be worth discussing if the number were _much higher_ or _much lower_. Instead, I think there are two far more interesting questions to ask: 1. Is the rate at which code review identifies defects sufficient to use code review as a detection mechanism for defects? After nearly 20 years of writing software, I'm pretty convinced that the answer here is no. Some reviewers are better than others, and some circumstances are more favorable to finding defects than others, but we should generally try to build processes that don't assume defects will be caught at a substantial rate by code review. It's nice when it works, but it's not a reliable enough way to catch errors to be a load bearing part of the process. 2. Is mandatory review of all code justified? This is the one I'm on the fence about. In an environment where code reviews are high priority, people are trained to review effectively, and there are minimal organizational politics at play, then I hypothesize that allowing PR authors to decide whether to get a review or not would generally improve quality and velocity because code would ship more quickly and code that would benefit from a review would still be reviewed. In that scenario, I think we'd see the benefits of getting things shipped more quickly when they don't require a review, and reviews would be higher quality because code being flagged for review would be a positive sign to pay more attention. Unfortunately, I could be wrong, and it's not the sort of experiment anyone wants to risk their reputation pushing for, so I doubt we're likely to see an experiment at a large enough scale to know for sure. If we're going to fail one way or another, I'd prefer to fail by doing too much code review rather than not enough.
- kqr 2y ago
  [flagged]
bigcat12345678 2y ago
Hah? Code review of cuz finds bugs... It's like people do see...
robertclaus 2y ago
I find many standard processes can be described similarly - if you're mindful of what problem they solve, they should be incredibly useful. The tricks are the important but subtle details like not spending 2 hours straight reviewing an excessively long PR. Those are easy to forget once it's just part of the process.
idkdotcom 2y ago
[dead]
sriharshamur 2y ago
What are some amazing blogs/resources to read to learn how to review PRs?
topkai22 2y ago
Code reviews don’t just find bugs, they prevent them from being introduced in the first place. Developers are more careful about what they write and submit when they know they’ll have someone else looking at it. We went through a couple iterations of our code review policy on a multi-year project a while back. We never really saw code reviews catch a substantial number of bugs over time, but whenever we pulled back on code reviews we definitely saw the production error rate go up.
BlackFly 2y ago
Bear in mind I am pro code review, but... There is a trick in pharmaceutical research where you test a potential candidate drug against placebo to yield a bad study that seems to show benefit. The reason it is a trick is because in many cases the alternative isn't placebo, it is an existing treatment. Then doctors learn about a more "modern" treatment, favor it for being modern and the better treatment may not be prescribed. The alternatives to code review aren't doing nothing. The article claims that code reviews find a defect per 10 minutes--but only in the first ten minutes. By this same argument (ignore qualifications, extrapolate the numeric result), fast automated testing can potentially find thousands of defects in a second--if they run that quickly and the defects were already tested for. Static analysers, pair programming, documentation these are all alternatives and there are many more. If you're spending an hour a day reviewing code then you are spending 12.5% of your time doing it. Using it that way comes with an opportunity cost that may be better spent depending on your particular organization and code base. Of course, analysing everything to death also has an opportunity cost, but not analysing it generally leads to moving goal posts where the supposed rationale for doing something keeps changing. Today its purpose is uncovering defects, tomorrow it is knowledge sharing, the day after it is security. It is all of those things, but other practices may achieve these goals with more effective use of time and people's patience. So why am I pro code review? Because choosing to interact and work together as a team, to learn about and compromise with your colleagues makes for good team building while serving other technical purposes. I do think that pair programming can achieve this to a greater level while also being more taxing on individuals. This assumes you control the process and own it though, if it has just become a rote ceremony then my feelings are you probably aren't net benefitting from it: you are simply doing it because you have no choice, not because you believe it to be a valuable use of time. If you have experienced both, a culture where people choose and find value in code reviews and a culture where people are forced to do it unquestioningly, then you may have witnessed how a dicta can destroy the prosocial value of a practice.
banish-m4 2y ago
Eliminating bugs requires sustained, vigilant, holistic, overlapping approaches: - Code reviews prior to acceptance of commits (Facebook does this) - Refactoring crap that manages to get through - Removing features - Higher-order languages with less code - Removal of tech debt - More eyeballs - Wiser engineers - Dedicating more time to better engineering - Coding guidelines that optimize for straightforward code while not being so strict as to hinder strategic exceptions - Negative LoC as a KPI
dakiol 2y ago
The only thing I don’t like about code reviews are nitpick comments. Everyone has their own subjective way of writing code, if my code works and looks good enough, let it be.
OpenDrapery 2y ago
I like to see working software alongside a PR. GitLab has the concept of a Review Environment, and it spoiled me. Ephemeral, dynamically provisioned environments that are deployed from a feature branch, are absolutely amazing productivity boosts. It gives so much more context to a PR.
titanlon 2y ago
[flagged]
jamesyates 2y ago
[dead]

221 points by _xivi 2y ago

My beef with code reviews is that often they lead to tremendous amounts of wasted time, that's many thousands spent in a single week sometimes for simple pull requests. Working from 6 years, not much, and not in as many places like others, I have built the opinion that code reviews are like tests, they should be used as a tool when they are necessary, they shouldn't be the default for every change. In best case scenarios is the person creating the pull requests that requests reviews or decides the place needs tests. My opinion obviously applies to product software, for libraries, especially when publicly exposed you want as much discussions and tests as possible.

simonw 2y ago
This is certainly true for blocking code reviews. I'm interested in exploring the alternative, which is review-after-commit. There's an article describing those here: https://copyconstruct.medium.com/post-commit-reviews-b4cc2163ac7a https://copyconstruct.medium.com/post-commit-reviews-b4cc216... Code still gets reviewed, but you don't end up with PRs languishing for hours, days or even weeks waiting to get a review from someone.
- interactivecode 2y ago
  I review a lot of code with the mindset of yes and… Basically when I start the PR is approved in my head until I find something blocking. I.e. major problem that causes dataloss, big performance issue or breaks other code. Anything else is a minor comment at most. The PR is approved by default. This gives the dev responsibility and ownership. Plus it increases release cadence Doing a follow up PR to improve or fix something is just as fast as blocking the MR, but blocking the MR can be bad for morale. This strategy might work better in young startups where having the feature _exist_ is better than not shipping. in my experience this builds up responsibility and ownership and removes the whole policing of other peoples work vibe around code review. Also decisions and discussion around formatting, when to test, design, features, functionality, architecture should not happen during code review, they should have happened way before coding, or collaboratively while working. Code review is the worst time for that stuff, imho it should be to sanity check implementation.
- YZF 2y ago
  I used to do a lot of this in a small team where we didn't block on reviews (we did reviews but didn't block). I was a senior developer on the team and I'd take time to read through new sections of code that came in. That worked pretty well. Interesting enough, this bit of code/project, that didn't have super strict code review requirements, but had a lot of tests, is the code I worked on that I would consider the most robust/high quality. It was run by > 10 million users in a fairly important application. It wasn't huge and it had good tests (and was generally quite testable). That said, it's really hard to control review-after-commit. Maybe we need better tooling for that. In my case, for the areas of code I was involved in, it was small enough to track in my head.
- kqr 2y ago
  I really like this idea! It's not like I need to check how any individual developer approaches their work (although that could become a useful mentoring session in some cases) but what matters is what it looks like before going into production. The main difficulty I see with the described approach is that different changes will be interleaved in the trunk and it might be hard to extract just one of them to deploy. But that's what feature flags are for!
- pavel_lishin 2y ago
  I'd love to explore that alternative, but I'm not sure how to actually make sure that any errors found/changes suggested after the commit is pushed and deployed actually get implemented.
closeparen 2y ago
The discipline of putting up small, coherent, explained, tested, and reviewable units of change, that you have looked over and feel comfortable showing off to other people as your work product, is 80% of the value for me. Whether anyone else actually thinks about it deeply or has something useful to say about it is secondary.
- goosejuice 2y ago
  Indeed. It's kind of like rubberducking.
kqr 2y ago
This is so far from my experience with code reviews that I'd like to ask some questions to follow up on your experience. Do you mind shooting an email to hn@xkqr.org?
ycombinatornews 2y ago
There’s probably more to this story. It’s somewhat a skill and maybe art to create MRs so that they are easy to review and get approved. It is also true for the feedback on MRs. Providing compressed and succinct feedback makes it faster to address. Almost like “if the change is difficult, refactor and make the change easy”. There are many ways to do one thing, some are better, some are not. Companies and teams that have good review culture are successful in using the reviews as a tool.

willio58 2y ago

Agreed. I mainly manage and review code at this point in my career. I find many bugs, every once in a while finding something that would have caused an outage or notable problem for users. What I find more though is code that isn't thought through. Tech debt and code smell are real, and they affect the performance of a team. Nipping that in the bud takes quality PR reviews and time to meet with submitters around issues you find. Knock on wood but working at the company I do now where I, along with my team, have made quality PR reviews normal.. our codebase is now enjoyable and fun to work on. I highly recommend it! One key aspect is being “kind, not nice”. Be helpful when leaving comments in PRs, but don’t be nice for the sake of avoiding conflict. Also if you find code reviews to be a waste of time I can reccomend one thing I do often - give warnings. I approve and give comments around things I’d like to be fixed in the future for similar PRs. I don’t hold up the merge for little things, but at the same time I won’t let the little things slide forever

saulpw 2y ago
How do you phrase these warnings? "Next time.."? I have a hard time being serious with my own warnings if it's fine enough for now.
- audiodude 2y ago
  "This is okay for now, but we should think about how we want to serialize these objects. Feel free to remove the N^2 algorithm in a follow up."
  - est31 2y ago
    That works great in a setting where you are both employees of the same company, and you respect each other, but it often doesn't work in the open source world, people just disappear and you never hear from them again. It is possible that they do file follow-ups, but in my experience it's rare.
    - sfink 2y ago
      Yes, even within a company my threshold for accepting a change can vary pretty widely depending on my experience and relationship with the author. For an external contributor or someone I've never collaborated with (by reviewing code or having my code reviewed), I don't accept the code until almost everything is worked out to my satisfaction. With someone I work with regularly, it's not uncommon to accept a change with a comment like "this is all good, but you need to take X into account which will change almost everything in this patch" (I exaggerate, but only slightly). I know whether an update could be problematic and whether it is necessary to see it again. Sometimes there are a couple of obvious ways that something could be done, they picked one but weren't tied to it if I had a reason for picking a different one, I picked a different one for $REASON. Most are somewhere in between. Though in some ways it works the other way around. For an unfamiliar open source contributor, I need to be confident that the change is worthwhile. I will be lenient on stylistic things, and I'll just land their patch and then fix it up afterwards. For someone I've worked with a bunch (whether a familiar contributor or a coworker), I will trust their opinion on the underlying quality of a change, but be less tolerant of unnecessary stylistic differences since they should have already come into alignment on those and it's more likely to be an oversight if they missed something. (Plus, I don't want to be fixing up their changes after the fact, given that >90% of patches will come from regular contributors.)

godelski 2y ago

I think this is really important in that it is bigger than "code reviews." It does show how people greatly misunderstand statistics[0]. And what's even funny is at surface level the claim that code review "does nothing" __sounds__ ludicrous. But people "believe" because they are annoyed with code review, not because they "actually" believe the results. But statistics are tricky. With the example given in the article "15% of smokers get lung cancer" compared to "80% of people with lung cancer smoke." These two are not in contradiction with one another but are just different ways to view the same thing. In fact, this is often how people will mislead you (or how you may unintentionally mislead yourself!) with statistics. Another famous example is one that hits HN every once in awhile: "Despite just 5.8% sales, over 38% of bug reports come from the Linux community"[1]. In short this one is about how linux users are just more trained to make bug reports and how most bugs are not system specific. So if you just classify bugs by the architecture of those submitting them, you'll actually miss out on a lot of valuable information. And because how statistics work, if the architecture dependence rate was as low as even 50% (I'd be surprised!) then that's still a huge amount of useful bug reports. As a linux user, I've seen these types of bugs, and they aren't uncommon. But I've frequently seen them dismissed because I report from a linux system. Or worse, support sends you to their page that requests you to "upvote" a "feature" or bug issue. One you have to login to. I can't take a company like that seriously but hell, Spotify did that to me and I've sent them the line of code that was wrong. And Netflix did it to me saying "We don't block firefox" but switching user agents gave me access. Sometimes we got to just think a bit more than surface level. So I guess I wanted to say, there's a general lesson here that can be abstracted out. [0] Everyone jokes that stats are made up, but this is equally bad. [1] https://news.ycombinator.com/item?id=38392931 https://news.ycombinator.com/item?id=38392931

jiggawatts 2y ago
> support sends you to their page that requests you to "upvote" a "feature" or bug issue. Microsoft does this for enterprise products where customers might be paying $100K/mo or even millions. “We hear you, but your complaint is just not popular enough so go away.” “Sure it’s a catastrophic data loss bug that ate your finance transactions, but if other people can’t identify that their seemingly unrelated crash is the exact same issue then no fix for you.” “Now that you did get ten thousand votes on an issue titled ‘Consiser doing your job’, we’ve decided to improve your experience by wiping out the bug forum and starting a new one from scratch that has fewer scathing comments from upset users.”

sarchertech 2y ago

I remember a time before you needed an approval to merge a PR (I also remember a time before PRs or any widespread version control system). I can count on one hand the number of times someone has caught a bug in my code that should have stopped deployment. Not that I haven’t deployed serious bugs to deployment, but they’ve almost never been caught by someone reviewing my code. Occasionally someone suggests a better way to do something, or asks a question that ends up with me coming up with a better way of doing something. But those situations are also rare. And I can’t think many times at all when the impact was worth the time spent on the process. Pair programming and collaboration can be hugely beneficial, but the minimal effort PR approval culture we’ve developed is a very poor substitute.

wrsh07 2y ago
I've caught bugs in reviews, but even better I've requested tests and those tests have caught bugs Even a low effort code review can identify missing unittests
dgb23 2y ago
Both code reviews and pair programming can be very useful if they serve a specific purpose. Getting someone up to speed with unfamiliar code, disentangling hairy code so it becomes clearer, hunting down bugs or finding unknown unknowns such as bugs or unnecessary complexity. However in many cases not looking at the screen when doing these kinds of things is more helpful. It's often more beneficial to build a mental model in your head and then riff off each other. Rather drawing things on a board or writing down stuff in a markdown file, explaining things in simple terms, than actually coding or reading actual code. Not sure if that still counts as pair programming or code reviewing but this free form way of talking about code is very effective.
kqr 2y ago
It certainly sounds like you write seriously high-quality code! And judging from your profile, I'd be inclined to think you know what you are talking about. I'd like to ask a little more around your experience here. Do you mind sending an email to hn@xkqr.org so that I can ask some follow-ups?

alex_lav 2y ago

Code reviews can find bugs. More often, code reviews become opportunities for team members to bikeshed. Worse, an opportunity for a non-team member to exert power over a project.

spankalee 2y ago
Bikeshedding in a team can be good. If you're all painting the shed, it helps to agree on the color. More generally, code review is a great opportunity for incrementally gaining or encouraging alignment across the team. What the team chooses to align on and how strongly are left up to it, so hopefully they choose to not get bogged down in inconsequential details, but completely skipping the pretty cheap chance for reenforcing all kinds of cohesion would be a big mistake in my opinion.
- alex_lav 2y ago
  You're making a lot of positive-upsided assertions about code review. My point is there is too much opportunity for negative behavior. It's the same as everything in tech, in life, "It can be good if everyone does their part to keep it good". And yet, most don't.
  - spankalee 2y ago
    "most don't" is a strong claim. In my experience, core review has been undoubtedly good. I would never run or join a company without it. I'm writing code solo for the moment, and code review is maybe the thing I miss the most.
    - alex_lav 2y ago
      > "most don't" is a strong claim. I'm happy to be reasonable. I guess my greater feeling is that most devs aren't great at identifying when they should identify restraint. For the same reason that most devs are abysmal interviewers, I think devs forget that code review is ultimately a human endeavor. Give your average dev the smallest amount of power and not enough guardrails and legitimate silliness ensues. > I'm writing code solo for the moment, and code review is maybe the thing I miss the most. I feel as though "code review" is taking on too many meanings in this conversation. Code review in the form of a second (or more) qualified dev reading and commenting on code for the greater good? Obvious good. Code review in the form of github PRs at a non-FAANG company? Skip it. Kangaroo court.

david2ndaccount 2y ago

In my experience, code reviews catch a lot of bugs. However, if you find yourself catching the same kind of bugs over and over again in review you should be finding ways to catch them automatically without involving a reviewer (static analysis, tests, linters, etc.)

cjriley 2y ago
Completely agree on utilizing static analysis as much as possible. My first instinct when finding an issue in a code review is to think, "could we have caught this with a <lint rule> of some kind?"

Mathnerd314 2y ago

My question is, do human reviewers find more bugs than ChatGPT? Because finding a cofounder costs a lot but asking ChatGPT is free. https://www.thatsoftwaredude.com/content/12848/chatgpt-can-do-code-reviews-but-are-they-any-good https://www.thatsoftwaredude.com/content/12848/chatgpt-can-d... says it is mediocre, but that was a year ago and honestly mediocre code reviews seem sufficient.

deleted 2y ago
[deleted]

sys_64738 2y ago

There are various levels to code reviews. Code review tools that are web based are pretty poor in my experience. Anything more than a few lines across multiple files needs a cscope type tool. Also what type of review? Is this a prototype needing a high level design review so that the actual review doesn’t turn into a design review? How often does that occur? Who are the reviewers and what’s the process? Key stakeholders have more influence and you need to consider the reviewer’s experience, knowledge and credibility. Finally how important is the code? Is it kernel code, or high execution daemon code needing race condition and memory leak checking? Are you using static analysis for the code? Does the code even compile and do what it is designed to do? Where are the unit test logs? Lots to consider.

29athrowaway 2y ago

If you have a spellchecker, code formatter and a linter, code reviews improve significantly. Much better than having to do that work by hand, or reviewing it by hand, leaving code reviews for higher level ideas.

zelos 2y ago
Exactly. Code reviews shouldn't be about code formatting or anything that can be automated away with linters, formatters, code coverage limits and static analysis. If the build is green for the PR, then all that is already acceptable.

bluGill 2y ago

Bugs are 'easy' to fix, I don't worry about finding them. I worry about the interfaces as they quickly become a nightmare to change just because of all the users.

kqr 2y ago
I'd be interested to hear more about your experience with code reviews. Could you send an email to hn@xkqr.org so that I can ask some follow-up questions, please?

poikroequ 2y ago

The value of code reviews really depends on the code and the person working on the code. For a team who have spent years working on the same repo, code reviews may not hold much value. But if you have a new guy on the team, or a junior, you'll definitely want to review their code. Code reviews can also do more than just find bugs. You can point out a better way of doing things. Maybe this SQL could be more efficient. Maybe you can refactor some bit of code to make it more robust. Maybe you should put a logging statement here. This method name is confusing, may I suggest renaming it to xyz?

kqr 2y ago
I got really curious and I'd like to ask you some follow-up questions on your experience in reviewing and receiving reviews. Do you mind shooting an email to hn@xkqr.org?
gwd 2y ago
> For a team who have spent years working on the same repo, code reviews may not hold much value. I have definitely found bugs [ETA during code review] in code written by very senior developers in code they've been familiar with for over a decade.
phito 2y ago
Code reviews also keep the team up to date with what is changing in the code
- seadan83 2y ago
  Commonly stated, except I think that statement is wrong. A PR is the thing that has the effect of sending emails to the team, it is the PR that leaves behind a webpage & summary of what has changed. It is the PR that is doing the work there, not the CR. Which implies, you can open and then immediately merge PRs without CR to get that same benefit. CR does give others a chance to study code, and become familiar with it - but that is different from "keeping up to date."
dkdbejwi383 2y ago
> But if you have a new guy on the team, or a junior, you'll definitely want to review their code. Reviews _from_ juniors or new team members are also really valuable, as they don't have the history or tribal knowledge that others may have. They'll often spot things that have gone overlooked because "that's how it is".

some_furry 2y ago

> During the first 60 minutes of code review of the day, the reviewer finds roughly one defect per ten minutes of reviewing – as long as they review less than about 50 lines of code per ten minutes. Oh. It normally takes me a few seconds to find bugs in code. I always felt this was average performance for assessing software. If the average time is ten minutes per defect, I need to recalibrate my expectations for myself.

lazyasciiart 2y ago
Presumably you understand that how long it takes to find bugs in code depends on the code. If not, then I hope you've read the code for Linux and SSL, etc.
- some_furry 2y ago
  Yes, of course it depends a lot on context. I've never had an incentive to read the Linux kernel code. I routinely find and disclosed cryptography library bugs, though usually mostly hobby projects like the "I thought it would be cool if there was a PHP implementation of GHASH" sort rather than like OpenSSL.
jonobird1 2y ago
It really depends on the code. To find a CSS bug, yes easy peasy. To find a logic hole in a payment integration of what someone has missed or should have implemented but didn't (eg webhooks), then this requires a lot more time and the developer basically has to sit down properly to work out exactly what should have been implemented / how they would have developed it, and then cross-check it against what has been done, otherwise you won't be able to easily find those logical holes which effectively are bugs, just not simple code bugs like a missing semicolon.
- some_furry 2y ago
  My day job is auditing cryptography. I'd probably be slower to find the root cause of a CSS bug than most of the folks that read HN. :3

mgreene 2y ago

The paper's title is a bit provocative but I think the findings are interesting. Mainly around long-held beliefs about what developers perceive as the value vs what is actually happening. You do bring up a good point about using change defect rate though. I wish the researchers had cited that as the preferred unit of measurement. I did some research on change defect rates on popular open source projects and it's all over the map. Ranging from ~12 - ~40% [1]. The future I'd like to see is as developers we use objective measures to justify time investment for review. This is going to be increasingly important as agents start banging out small bug-fix tickets. [1] https://www.shepherdly.io/post/benchmarking-risk-quality-kpis-in-popular-open-source-projects#how-often-are-bug-fixes-happening https://www.shepherdly.io/post/benchmarking-risk-quality-kpi...

jt2190 2y ago

I’m not sure why the author ignores the “… that should block a submisson” part. The abstract of the paper: > Because of its many uses and benefits, code reviews are a standard part of the modern software engineering workflow. Since they require involvement of people, code reviewing is often the longest part of the code integration activities. Using experience gained at Microsoft and with support of data, we posit (1) that code reviews often do not find functionality issues that should block a code submission; (2) that effective code reviews should be performed by people with specific set of skills; and (3) that the social aspect of code reviews cannot be ignored. We find that we need to be more sophisticated with our guidelines for the code review workflow. We show how our findings from code reviewing practice influence our code review tools at Microsoft. Finally, we assert that, due to its costs, code reviewing practice is a topic deserving to be better understood, systematized and applied to software engineering workflow with more precision than the best practice currently prescribes. “Code Reviews Do Not Find Bugs: How the Current Code Review Best Practice Slows Us Down” https://www.microsoft.com/en-us/research/wp-content/uploads/2015/05/PID3556473.pdf https://www.microsoft.com/en-us/research/wp-content/uploads/...

gwd 2y ago
The "that should block submission" is always one of the trickiest parts. There's a saying: "Everyone that drives slower than you is an idiot, and everyone that drives faster than you is a maniac." But it is true that going faster increases danger, and there is a speed that appropriately balances benefit against risk; but everyone perceives it differently. The same is true of "code smell" issues: Everyone who asks you to change things is a pedant who's slowing down the project for pointless aesthetics, and everyone who pushes back against changes you've requested is a cowboy who is going to make the code harder to maintain in the future. So in the paper, how did they decide whether a non-bug change "should block submission" or not?
- jt2190 2y ago
  If a comment points out a bug/defect [1], then it should block. If you think about it, as bugs/defects are removed, the code becomes more correct and thus more stable because it doesn’t need additional changes to remove bugs, so removing bugs reduces the need for future maintenance. If we block due to future maintenance concerns what we’re really asserting is that the requirements are unstable, and that removing today’s bugs is less valuable overall because requirement changes will remove the line of code with the bug and replace it with a new line of code with a new bug. I suppose it depends on the code review process at at a given organization whether that’s the appropriate point at which to block code for architecture/design issues. In my experience the code review step is much too far downstream in the development process and much too narrowly focused on a subset of code to be an effective place for design changes that have significant impact on maintenance. [1] The paper authors reviewed data in Microsoft’s internal code review tool, which is proprietary, so we can’t see what the specific bugs were.

nitwit005 2y ago

> Developers spend six hours per week reviewing. This is a bit too much It's extremely difficult to adjust the time spent on reviews. The options are unattractive. Do you start blindly accepting changes when you hit the limit, or just stop and not let people merge code?

dakiol 2y ago
Why should one block merging code? The idea of someone spending hours working on some code changes being blocked by another individual that doesn’t allocate time for reviewing is alien to me. We are all professionals, let people merge their changes and make sure you find time to review them. If you can’t review them and they still need your approval, then approve. Unless you don’t trust your colleagues. If that’s the case, then code review is doomed anyway

jonobird1 2y ago

I'm not sure code reviews hold much merit. I've been a web developer for around 12 years and I've worked in companies big and small. I think there should be a manual QA process to test the functionality of what the developer is pushing out. The issue with code reviews is always that they take so much time for another developer and many devs are super busy so they just have a quick review of the PR and approve or feel they have to add some comments. Context switching between what the dev is already doing and having to come to the PR to review properly means they should switch to that Git branch, pull down the code, test it all and check for logical bugs that a static code review won't pick up. For juniors, code reviews are still useful as you will be able to spot poor quality code, but for seniors, not as much for the reasons above, better off having a QA process to find and logic holes rather than expecting devs to invest so much time in context switching.

skywhopper 2y ago
Nah, automated testing cover basic functionality. For most PRs, a senior familiar with the code wouldn’t need to check it out and manually test anything, that’s not what “code review” is most of the time. If you need them to look at the code in a running state, that should be part of the CI process, not a manual task for the developer. A good reviewer can call out bad strategic coding decisions or misinterpretations of the requirements. QA is another layer of review entirely.
hakunin 2y ago
The problem here is not that developers are too busy, but that code reviews are considered second class citizens to churning out new code. It's like saying "many devs are super busy working on feature A so they just write quick and dirty code for feature B". If reviews are integral part of feature production pipeline, there should be no issue to sit down and spend a day reviewing code. For bigger, more complex things it could be a few rounds of reviews. There is an approximate non linear relationship between time it takes to produce the first PR and time it takes to go through all rounds of review. This time can be pretty reliably calculated and taken into account.
kqr 2y ago
I'm a little surprised to hear this. Would you mind sending an email to hn@xkqr.org so that I can ask some follow-up questions, please?

andirk 2y ago

Technical debt. Keep it minimal, and when needed, write a task for it to be looked in to later. Coding standards. Don't submit code that has rando extra lines and things that will slow down the next dev from looking in to the past to learn what does what. And most of all, make sure edge cases are covered, often via a truth table of all possible outcomes. I often comment on a PR saying "blah blah, but not blocking" so I'll allow it but at least my entitled opinion was known in case that code issue comes up later. My PRs take a long time, because I dig the F in.

swatcoder 2y ago

As with most processes, the dilemma with code reviews is in figuring out how they impact your team and your organization. In a huge org, with thousands of engineers that's already burdened by hours per day of interruptions and process overhead, and release runways that already involve six stamps of bureaucracy, mandatory code revies have very little downside (it's in the noise) but highly variable return (many people are just droning under the weight of process). The org loses nothing much for mandating it, but only certain teams will see a lot of value for it. On the other extreme, a startup with five engineers will get backlogged with reviews (which then get shortchanged) because everbody either is under pressure to either stay in their high-productivity flow or put out some pressing fire. The reviews probably could catch issues and share critical knowledge very regularly, but the org pays a pronounced penalty for the overhead and interruptions. People long for "one size fits all" rules, crafting essays and publishing research papers to justify them, but the reality of what's right is often far more idiosyncratic.

kqr 2y ago
I don't disagree with the idea that "it depends" but for me, code review has generally worked better with lower overhead in the "startup with five engineers" type organisation. Can I ask you some follow-up questions on your experience in reviewing and receiving reviews? If so, send me an email at hn@xkqr.org!

grumple 2y ago

I find things wrong with virtually every nontrivial pull request when I’m the reviewer. Sometimes these are minor issues, but I spot bugs and edge cases all the time. I see some comments about time. How long does a code review take? I can review hundreds of lines of code in a few minutes. It is much easier to review code than to write code imo, especially as you gain experience. For bigger efforts, get eyes on it throughout the process. I’ve met a lot of developers who assume their code will just work right after they write it. They don’t test it, via code or manual qa. Then they act surprised when the stakeholder tells them it doesn’t work. Do the job right the first time. Slow is smooth and smooth is fast.

farmeroy 2y ago
I'm always surprised how often I get a pull request which either doesn't build or has failing unit tests or both. These are pretty easy to address at least - but when I think that certain code might be difficult to maintain, be an anti-pattern, or possibly present bugs in non-obvious ways, I find it really hard to effectively address those issues and often end up doubting my own suggestions
aitchnyu 2y ago
When I was working with Django, those who added code ran it in their systems. With lambda, which we mostly deploy and test in the cloud, people tend to dump code and leave.

rebeccaskinner 2y ago

I think the article is taking the wrong view. The statistic cited by the article that 15% of comments were about a bug seems in line with expectations, and I think it would only really be worth discussing if the number were _much higher_ or _much lower_. Instead, I think there are two far more interesting questions to ask: 1. Is the rate at which code review identifies defects sufficient to use code review as a detection mechanism for defects? After nearly 20 years of writing software, I'm pretty convinced that the answer here is no. Some reviewers are better than others, and some circumstances are more favorable to finding defects than others, but we should generally try to build processes that don't assume defects will be caught at a substantial rate by code review. It's nice when it works, but it's not a reliable enough way to catch errors to be a load bearing part of the process. 2. Is mandatory review of all code justified? This is the one I'm on the fence about. In an environment where code reviews are high priority, people are trained to review effectively, and there are minimal organizational politics at play, then I hypothesize that allowing PR authors to decide whether to get a review or not would generally improve quality and velocity because code would ship more quickly and code that would benefit from a review would still be reviewed. In that scenario, I think we'd see the benefits of getting things shipped more quickly when they don't require a review, and reviews would be higher quality because code being flagged for review would be a positive sign to pay more attention. Unfortunately, I could be wrong, and it's not the sort of experiment anyone wants to risk their reputation pushing for, so I doubt we're likely to see an experiment at a large enough scale to know for sure. If we're going to fail one way or another, I'd prefer to fail by doing too much code review rather than not enough.

kqr 2y ago
[flagged]

bigcat12345678 2y ago

Hah? Code review of cuz finds bugs... It's like people do see...

robertclaus 2y ago

I find many standard processes can be described similarly - if you're mindful of what problem they solve, they should be incredibly useful. The tricks are the important but subtle details like not spending 2 hours straight reviewing an excessively long PR. Those are easy to forget once it's just part of the process.

idkdotcom 2y ago

[dead]

sriharshamur 2y ago

What are some amazing blogs/resources to read to learn how to review PRs?

topkai22 2y ago

Code reviews don’t just find bugs, they prevent them from being introduced in the first place. Developers are more careful about what they write and submit when they know they’ll have someone else looking at it. We went through a couple iterations of our code review policy on a multi-year project a while back. We never really saw code reviews catch a substantial number of bugs over time, but whenever we pulled back on code reviews we definitely saw the production error rate go up.

BlackFly 2y ago

Bear in mind I am pro code review, but... There is a trick in pharmaceutical research where you test a potential candidate drug against placebo to yield a bad study that seems to show benefit. The reason it is a trick is because in many cases the alternative isn't placebo, it is an existing treatment. Then doctors learn about a more "modern" treatment, favor it for being modern and the better treatment may not be prescribed. The alternatives to code review aren't doing nothing. The article claims that code reviews find a defect per 10 minutes--but only in the first ten minutes. By this same argument (ignore qualifications, extrapolate the numeric result), fast automated testing can potentially find thousands of defects in a second--if they run that quickly and the defects were already tested for. Static analysers, pair programming, documentation these are all alternatives and there are many more. If you're spending an hour a day reviewing code then you are spending 12.5% of your time doing it. Using it that way comes with an opportunity cost that may be better spent depending on your particular organization and code base. Of course, analysing everything to death also has an opportunity cost, but not analysing it generally leads to moving goal posts where the supposed rationale for doing something keeps changing. Today its purpose is uncovering defects, tomorrow it is knowledge sharing, the day after it is security. It is all of those things, but other practices may achieve these goals with more effective use of time and people's patience. So why am I pro code review? Because choosing to interact and work together as a team, to learn about and compromise with your colleagues makes for good team building while serving other technical purposes. I do think that pair programming can achieve this to a greater level while also being more taxing on individuals. This assumes you control the process and own it though, if it has just become a rote ceremony then my feelings are you probably aren't net benefitting from it: you are simply doing it because you have no choice, not because you believe it to be a valuable use of time. If you have experienced both, a culture where people choose and find value in code reviews and a culture where people are forced to do it unquestioningly, then you may have witnessed how a dicta can destroy the prosocial value of a practice.

banish-m4 2y ago

Eliminating bugs requires sustained, vigilant, holistic, overlapping approaches: - Code reviews prior to acceptance of commits (Facebook does this) - Refactoring crap that manages to get through - Removing features - Higher-order languages with less code - Removal of tech debt - More eyeballs - Wiser engineers - Dedicating more time to better engineering - Coding guidelines that optimize for straightforward code while not being so strict as to hinder strategic exceptions - Negative LoC as a KPI

dakiol 2y ago

The only thing I don’t like about code reviews are nitpick comments. Everyone has their own subjective way of writing code, if my code works and looks good enough, let it be.

OpenDrapery 2y ago

I like to see working software alongside a PR. GitLab has the concept of a Review Environment, and it spoiled me. Ephemeral, dynamically provisioned environments that are deployed from a feature branch, are absolutely amazing productivity boosts. It gives so much more context to a PR.

titanlon 2y ago

[flagged]

jamesyates 2y ago

[dead]