 So, hello everyone and welcome to Code Review Best Practices. I'm XJM and I arrived from the U.S. very late last night, so I apologize if I'm a bit overcaffeinated, which is what I'm trying to use to ride through the jet lag. I'd also like to thank the conference organizers for having the flexibility to allow me to get back on the schedule. I was on the schedule originally, I had to withdraw my session for family reasons, and everyone was really cool about shuffling things around to get me back here, albeit at 11.15 in the morning right after I traveled from the U.S. So nothing's perfect, but it's fantastic to be here and I'm happy to be presenting this talk. Now, if it helps you follow along with the slides, I have posted them on my Twitter, which is XJMDrupal. If you're the sort of person who likes to read along or needs assistive tech to help with the text on the screen, those are available for you. I'm not wearing a mask at this event because it muffles my... Sorry, I am wearing a mask at this event, but I can't while I'm speaking because it muffles my speaking and makes it harder for people to lip read who need that support. So if you do have a mask in your pocket, I'd really appreciate it if you'd put it on. I meant to bring a box of masks and pass it around to everyone, but unfortunately the 36-hour turnaround time for this fit into that. So I'd appreciate it if you'd put a mask on if you do have one. Also, one other note before we get started, I ask that people not take photos or videos of me for privacy reasons. If I see a camera pointed towards me, I have this reflex where I'll sort of like duck out of the way, hide behind the podium that can be a little distracting, so please don't do it. If there is a session recording, as we've already established, it'll be audio and slides only. Also, out of respect, I'd like to make a land acknowledgement. I acknowledge the traditional Aboriginal and Torres Strait owners of the land, the traditional custodial lands of the land that I'm visiting. I'm told that the Gatagala, the Oro Nation, please tell me if I'm pronouncing that incorrectly, are the traditional custodians of this area. So, hi. My name is Jess. As I said, I'm XJM on Drupal.org, and I'm a Drupal Core Committer. Now, there are thousands of people who contribute to Drupal Core, but as a core committer, I'm one of about 15 people who can worldwide, that's 15 people everywhere in the world, who can actually accept or merge changes into the core code base so that they become a part of everyone's application. That means one of primary responsibilities is reviewing other people's code. I'm also a member of the Drupal Security team and a core release manager, so I actually create many of the releases of Drupal that you can, and I hopefully do, install on your Drupal site each month. One organization that's funding 16 hours per week of my contribution time is HeroDevs. HeroDevs is currently the only official extended long-term to partner for Drupal 7. They have basically a fork of Drupal 7 that will be maintained after its end of life. FabianX, the Drupal 7 core framework manager, helped them build the product along with a number of other Drupal 7 contributors and community members. They sought advice from the Drupal Association, the security team and so forth, so they're doing a responsible job of it and it should be pretty legit. The platform will scan for security vulnerabilities and in fact, HeroDevs will also handle in collaboration with Klausi and some other members of the community a security reporting and patching process for any new Drupal 7 vulnerabilities that you discover. It's not just for core, either top contributed modules will be included and now they're funding me to work on Drupal 10. I don't know anything, like very little about Drupal 7 these days anymore, so my role is primarily as a contribution mentor for them and to help them gain reputation in the community because I'm a core committer, so if you are interested in the Drupal 7 NeverEnding Support product talk to me afterward and I can put you in touch with some of the folks there. Alright, so the primary goal of Peer Code Review is to catch bugs in new or changed code before it goes into production. Peer Code Review also helps code health increase over time rather than declining. It's not just about code quality though. Software code is written by human beings, for human beings, and it's important that your code review process reflect that. Put simply, Peer Code Review is also a tool for building stronger team. By reviewing each other's changes as they're developed, you build a shared understanding of your code base across your team. If done properly, it should foster open constructive communication. It can also help you create a sense of pride and shared ownership in the projects you develop together. It's also a really excellent opportunity to mentor junior developers on your team. It's some of the best onboarding that there is. And finally, Peer Code Review will help reviewers and code authors alike write better code the next time because you learn from the review process too as the reviewer. Now, of course, if you want your code reviews to be a positive experience for your team, you need to have a positive Peer Review culture. I'm not big on emotional labor myself. I find it exhausting. But Peer Code Review is the one place where it really is worth it to me to put in that extra effort to make sure that everyone on my team feels safe and supported. The most important thing is that everyone involved in a review should treat each other as equals. The reviewer isn't above the developers. You all bring different knowledge and experiences to the process. Start from a place of humility. We do Peer Reviews precisely because it's difficult for us to catch the errors in our own work, and so we're relying on the other members of our team to help us with that. And that's true for the reviewer as well. I want to reiterate this. In the years that I've had Commit Access to Core, which is going on 10 now, I've noticed that many contributors just kind of do anything I say without questioning it in the Drupal issue queues. But actually I want to hear the author's thoughts on feedback that I've given them, especially if they disagree because I make mistakes too. That's the most important lesson. It's also important to have empathy for others involved in the Code Review process to consider their needs and their constraints. If something seems wrong or confusing, ask a specific question about it that will help both of you understand what's going on. If something technically works but could be improved, phrase your proposed improvements as a suggestion. And finally, when you and others disagree about something in the Code, encourage others to explain their perspective. Even if something definitely needs to be changed from your perspective as the reviewer, it's helpful to have misunderstandings and additional perspectives documented for future reference. So I structure my Code Reviews in three parts to improve the chances that they'll be well received by Code Authors. I try to start by thanking the author for something specific that they did well. This helps them read the review positively and also shows them that I'm aware of the effort that's gone into their work. If I dive straight into asking someone to change this, that, and the other thing, it might seem like I've missed the big picture or the value of the work overall. Next, in my comments about specific lines in the Code snippet, I tried to provide references backing up my recommendations. Even if you, as a reviewer, have seen like a particular problematic pattern hundreds of times, the Code Author might not have. So provide them with policy documentation, references to other Code that's similar or the like. Providing a reference helps the Code Author internalize best practices a little bit at a time and also sets a good example for other reviewers to follow so that everyone learns from the process. Finally, after I've finished my detailed review, I close with a summary and an actionable next step for the issue. If I've given a lot of feedback, I'll highlight what I think the most important questions or concerns to respond to are. It's always good to help contributors understand what the path done is and that starts with an actionable next step for the issue. Now, before I get into any more specific advice about the technical aspects of Code Review, I need to explain some background concepts from psychology. Of all the tools that we use to do peer code review, the most important one is actually our brains, right? No brain, no code review. So it's important to understand the limitations on our brains and how we can enable them to do their best work. And by the way, I am not counting AIs that do code review because no substitute. So the first concept I have to introduce is that of decision fatigue, which simply stated is that after we make many decisions, our ability to make additional decisions becomes worse. If you've ever come home after a long day and found yourself kind of staring into the refrigerator as if it helped answers or been completely paralyzed by the array of choices in a big box store, I'm not sure if y'all are familiar with U.S. supermarkets, but it's an experience. This is probably what's going on. It's decision fatigue. We make hundreds of small decisions every single day. Many of them are completely trivial. What to say or text at a given moment, what to have for breakfast. But when you make many decisions in a row, your brain gets fatigued. And symptoms of that fatigue can include feelings of exhaustion or mental fog, impulsiveness, and procrastination or avoiding making future decisions. And code review does involve a lot of decisions. For every single line we decide whether the code is right, what other side effects it might have on the application, whether there's a better way. Another important concept is that of directed attention fatigue, or DAF. When we have to focus on a single task, like we do when we do a code review, our brains have to suppress stimuli from distractions. The distractions can be external, background noise, the kids, the pets, messages from coworkers. They can also be internal, random unrelated thoughts passing through our minds, feeling hungry, worrying about the bills. Finally, the distractions can also come from the code itself. Code style errors, bugs in nearby code, and so forth. And this in particular is something we'll talk more about later. Unfortunately, the mechanism in our brains that allows us to put aside these distractions and focus on the task at hand becomes fatigued and overused. And symptoms of that fatigue can include restlessness, irritability and apathy, none of which are going to help you have a positive review culture, by the way. Directed attention fatigue can also cause you to miss social cues, which can make it harder to communicate empathetically with the developers whose code you're reviewing. And finally, it can cause misperceptions, confusion, impulsiveness, or poor decision making, all of which will lower the quality of your code review. It's also worth pointing out that distractions are an accessibility issue. Directed attention fatigue does affect neurotypical people, but it can be compounded for folks with ADHD. There's been research that shows that the same mechanism in people's brains is stressed in both directed attention fatigue and ADHD. The important difference, of course, being that directed attention fatigue is a temporary condition that you can recover from. It takes time for that distraction mechanism in our brains to recover when it's been stressed, which is probably one of the reasons that a lot of people with ADHD use a strategy of taking frequent short breaks when they work, followed by a longer break after several cycles. Overall, you should treat distractions both in the work day in general, and in a code change set itself as accessibility and inclusivity concerns. And this is something that I'm trying to get better at in my own code reviews as well. It's a process. So with that context, let's talk about some research that's been done on how different factors can increase or decrease the effectiveness of our code development process. References for the research are in the speaker notes of the slides that I've shared on my Twitter. They're the Google slides that are there. The key concept to understand for this section is that of defect detection. This means of the existing potential bugs or flaws in a code change set, how many are actually identified by a given process. Now, higher, a little bit counter-intuitively, is better, because it means you've found more bugs in the code before they can actually become bugs in the application. And early defect detection is really valuable. According to one of the sources that I looked at, even low-impact bugs are twice as costly to fix once the code is in production, and a critical bug can be as much as 100 times more costly to fix. Now, for Drupal Core, I'm a core developer, so I always try to reframe everything between the normal world and the open-source contribution world. For our purposes as core developers, that's probably, in productions, probably the equivalent of it being present in like a tagged release of Drupal that someone might have installed on their site already. And then something you might not have expected. A formal peer code review with multiple reviewers is on average twice as effective as automated testing for defect detection. This is actually amazing, right? That people simply reading the code are twice as good at finding bugs in it as computers that can actually run it. Of course, combining all of the above is even better. I have some slides to illustrate this. They compare the effectiveness of peer code review with different kinds of quality assurance. In this case, if you used each one of these things by itself before combining them, how effective is it? First off, a word about unit tests. These can catch anywhere from 15 to 50% of the bugs within their scope or 30% on average. Now, I didn't show the error bars for that on this slide because unit tests were the only tool with such unpredictable effectiveness. And the reason that varies so widely is because the usefulness of the unit test depends on the scope and encapsulation of the change set that you're reviewing. Furthermore, misusing unit tests in place of integration tests can lead to a very high rate of false positives. And false positives are distractions from your review. So, use with caution. Use them right. Don't use them if you don't need to. Static analysis is pretty great and can catch 33% of potential defects, more actually than typical unit tests or regression tests, which is worth knowing because they're also way faster. We recently started doing static analysis on Drupal Core these past couple of years, and it's awesome. It's made a huge difference on the quality of our code. Integration tests are typically the most effective automated tests, catching 35% of potential defects. So, as an aside, when in doubt, if you're like, you only have time to write one test, write an integration test over unit tests or regression tests for the code. Now, compare all of the above to a formal peer code review process with multiple reviewers, which will catch 60% of defects. Again, that's peer code review by itself before you have included automated testing in the process. Finally, for context, I've also included a statistic about the effectiveness of large beta tests with over 1,000 participants in them. Such beta tests typically identify 75% or so of defects with the code. Now, just think of all the time and cost it takes to set up a beta test of that scale for your application to say nothing of the free work that's being done by your beta test participants, right? A formal code review costs a fraction of that, but it still might find as much as 80% of the same stuff, even if for some reason you don't have automated tests, which please, I hope you all do. So, like I said, those statistics were all about using each tool by itself. But peer code review is also even more effective when combined with other strategies. If you combine design reviews, automated testing, QA, and then peer code reviews, it can increase your defect detection rates to over 90%. That means over 90% of bugs are found before they can make it to production. So you should still write integration tests and still use linting and static analysis tools. And actually, you should have all that run before your peer reviewer even looks at the code. That will reduce the number of defects present in the code when they review it, which in turn will make the reviewer more effective because there are fewer defective, sorry, fewer defects they have to focus on, fewer distractions in the code. In summary, peer code reviews are your most effective resource, so you should optimize your development process for them. So how do you do that? How do you optimize for a good code review? I'm going to spend the next several sections exploring how you and your team can work together to optimize for code's review ability. So in the graph on this slide, it compares the defect intensity found in a review, that's the y-axis, the vertical one, with the number of lines of code in the set under review. That's the x-axis here. Higher defect detection is a good thing. It means that you're catching more of the bugs that already exist in a piece of code early before the code goes into production. So this part up here, good. Lower defect density, on the other hand, is a bad thing. These huge chain sets out here with 700 or 1,000 lines of code under review, it's not that they didn't... it's not that they had fewer bugs, it's just that the reviewer was so overwhelmed by the scope of the code that they were reviewing that they couldn't find the bugs anymore. There's a pattern of exponential decay there. Reviewers are really good at spotting bugs in small chain sets, and if you've ever submitted code to a peer reviewer, you've definitely noticed this. But then the reviewer's effectiveness drops off rapidly as the size of the chain set increases. So why does this happen? The reason is that reviewing too many lines of code at once can cause both decision fatigue and directed attention fatigue. So the best way to fix it is to reduce the number of lines of code under review in a single given chain set. Split it up into smaller logical pieces. Fewer lines to review means fewer decisions and fewer distractions. Based on the data, it's best to review up to about 200 lines of code at a time. And note that that includes removed lines of code because you as a peer reviewer need to read the removed lines as well to ensure that the change is correct and to ensure that there aren't regressions. So you add up the absolute value of the added and removed lines, and then that gives you the count of the total lines under review. And I have a... you know, since that's arithmetic, I have an example to illustrate that just for your reference. So this is a random example from Triple Chorus Commit History. This change added 113 lines and deleted one. So adding 113 in one, that's a total of 114 lines under review. That's a good change set size with a hell with the review process. Now in general, to drive this point home a little more, you should really never review more than 400 lines at a time. Change sets even that large should be reserved for simple one-to-one replacements of one thing or the like. For example, replacing a deprecated function's usages across the code base with one-to-one replacement with something else. At 400 lines, that's fine, it's scannable. Here's an example of a problematic change set scoping from another issue I reviewed a while back. It was a bit much for me. I felt like physically exhausted every time I got more than halfway through this. And then sure enough, I checked and the scope of the change set was just too darn big. The issue changed 94 files 229 insertions and 201 deletions so a total of 430 lines of code under review in that set. Over our recommendation and that's part of why it was tiring for me and part of why I couldn't do my best work on it. Another thing that helps is to never spend more than 60 minutes doing code review at a time. Now note that that's sort of the recommended maximum for people on average and it might not work for everyone. You might find, for example, that shorter 30 minute code review sessions at a time work better for you. And between those 30 or 16 minute sessions you need to take breaks so that your brain can recover. Remember, that's the distraction suppression mechanism that you need to give a rest. You deal with distractions that have come up during the review, but also just let your brain rest. Some folks might find they work better even with cycles of 20 minutes of code review plus a short break followed by a long break after four sessions. That's something that the ADHD community uses. It's called the Pomodoro Method, by the way. I learned that from my little sister. So, speaking of distractions and code review, let's talk about handling nitpicks. Now, nitpicks by themselves by definition are trivial but how you handle them is actually a really important aspect of code review that does need to be discussed. I'll take a drink to talk about this. So to me, reading code style errors is like watching a speaker who has food in their teeth. Imagine a little flock of something green stuck right up here. It has no pairing whatsoever on what I'm saying. But you still might not be able to concentrate on what it is that I'm saying because of this one out of place awkward thing. And that's how I feel when I review a chain set that has code or documentation style errors. They're minor, they're unimportant and utterly distracting for me. I can't see your code if your white space is screwed up. I don't understand your inline comments if they contain grammatical errors. It's a real problem, actually. Part of it is me, but it's also a real problem. Code style errors are distractions that can increase your directed attention fatigue and make the review harder on the reviewer. Now, trivial decisions about fixing them will also add to decision fatigue for both the reviewer and the code author. Are you using spaces? Are you using tabs? Reward this, rewrap it. You can imagine. Should I even bother that? Leave it alone, etc. That whole conversation. Waste of your time. On the other hand, though, nitpicky reviews are also discouraging to code authors. They spend hours or days fixing something that's important, something in the open source community that they may have donated their time to fix, and then instead of evaluating their design or appreciating their work, you get lost in distractions of formatting issues or spelling errors or whatever. It can seem oppressive or like a lack of appreciation for the contributor's work. So how do we fix this problem, this balance? On the one hand, code style errors get in the way of effective code review, but on the other, they are the least important thing about your change set. The answer is to use automated tooling that runs immediately every time you submit a code change before your peer reviewer looks at the issue. And I'm not talking just about static analysis here. Instead of dozens of tiny nitpick decisions, if you have the right automated tooling for coding standards checking setup, you'll have one binary decision. Did it pass the automated coding standards checking? The Drupal Core community has now spent over a decade incorporating automated coding standards checking into core process and making core actually comply with its own coding standards, which was not true. Many of you from back in the day will know this. And when I talk about this, it's hard for me not to get really excited and melodramatic about it, because it has had such less impact on our community health. It has made our code review process so much healthier and so much more focused on what actually matters, which is what Drupal can do for people, right? So here are some of the automated tools to use. PHPCS, PHPStan, C-spell, ESLint and Stylint. This is assuming a PHP application. There are, if you for applications in other languages, there are other equivalent tools to use. Now, Drupal Core has a script that will automatically run a number of linting checks, including these tools on the code base, both as part of Drupal.org's automated testing and as a pre-commit hook when core committers like myself add changes to head. You can also configure jobs for these tools using GitHub Actions, GitLab CI, or your own continuous integration or QA process. I won't go into detail about how to set this up, that information is on the internet. But my slides are posted so you can find the list again if that helps. I strongly encourage everyone to use these tools for both contributed and client projects. In each case, Drupal Core already provides a default rule set you can use to make your project compliant with Drupal Core standards, if that's your goal. You also have the option of developing your own rule set if your org has its own standard. Now, automated tooling can't catch every nitpick type problem. And I have an example on the slide of a minor documentation fix, where someone should have used the ActC PHP.tag, but used a short sentence instead. Fortunately, GitLab makes it super easy to deal with this kind of thing as the reviewer with the Insert Suggestion feature. You just highlight the lines your nitpick is about, click the Insert Suggestion button, which is the left-most one in the widget. I have an illustration of this because I had no idea when I started using GitLab what this little piece of paper with arrow on it was. It's the best button in the user interface. All you have to do is highlight the lines, click the button and edit the snippet. You click start a review or add comment now and as a reviewer you've just saved an entire review cycle over this silly little thing. Once you post the comment the code author can then add your Suggestion change to the merge request with a button click. It saves you time having to write out the entire sentence of this is why we should use the PHP.tag here blah blah blah. And it saves them time having to make this let's be honest, ridiculous nitpick change that you suggested that is necessary to meet the code quality standards, whatever. Love it. Please use it. So with all this background information let's now talk about how we can optimize our change sets so that peer reviewers can do their best work. Remember, human peer reviewers are the single most efficient thing you have to catch issues early, safely and cheaply so lean into that. There's something quite important about your peer reviewers. They are your first insight into what the experience someone is going to have in maintaining this code yours down the line. If the reviewer can't follow your code just by reading it chances are that's going to happen again in the future when someone else looks at it so optimize for readability and reviewability. It's essential that someone in the future can understand your code easily. It might even be you in a few years, right? You don't want to find yourself thinking did I write that? I'm so embarrassed. Of course, it's much better to think oh, thank you past self. That was very clear. Thank you for documenting that. I understand what's going on. I've had both experiences I'm sure many of you have as well. While smaller change sets are easier to review it's definitely possible also to take that too far. So first and foremost you shouldn't ever commit something that leaves your main branch in an unshippable, incomplete or broken state. If you were involved in or even using Drupal a little more than a decade ago and wondering why Drupal 8's release date kept getting pushed back it's because of this right here. We didn't keep the code base in a shippable state so we ended up with hundreds of critical issues blocking Drupal's release. Another mistake is to make the diff stat smaller by separating out test coverage or documentation. This is an idea because the tests and docs are an important part of understanding the change and a diff that's missing them isn't optimized for reviewability. This affects the immediate experience for the reviewer, the shippability of the code base and the usefulness of your git history. A lot of times actually I read the tests first to understand what's going on in a change. Finally, scope creep. You're editing a file and you see something else that's wrong couple lines above so you think why not fix that while I'm here? The problem with this is that it adds distractions for the reviewer and forces them to think outside the context of the primary change. It also creates an ever-expending set of changes that need to be involved in the issue blurring when the issue is really done. Now reviewers can make this mistake too, by the way. Especially when they're reading moved or context lines in the diffs. Someone moves code from here to there and all of a sudden the reviewer starts posting all of these things that you should change with it and you're like it's already in the code base, man. I just put in a different file. It's very, very easy to start posting suggestions on how to fix it but it's actually not helpful feedback at that point because the issue is just changing the factoring. So instead of fixing that one little thing near changes, get in the habit of filing separate follow-up issues for unrelated problems you spot. And, again, as a reviewer, especially for moved code, just make a note of it somewhere else and then after your review is done, by the way, we should have a follow-up to read about this hard-to-read thing that we refactored and do a new place in our code base. Now sometimes you'll get a reply from a code author that a certain pattern you're back on in your review is used elsewhere in the core code base. Happens all the time or your application's code base. But core is huge, so more times in the core code base. Now even if you've documented a real technical problem with the approach that the authors use, they might still push back. And there are actually kind of two kinds of code authors here. Some code authors will take your response to your feedback too far and then they'll change all the other instances of that same bad pattern everywhere in the file or even everywhere in the application. Others, they'll push back on your pushback and want to add new code that's using what's a known bad pattern just for the sake of consistency because the pattern exists elsewhere. Now both of those things are the wrong approach to take. File a follow-up, write good new code now and fix the old bad code later. Separate parts of your brain, separate scope, separate priority. You also shouldn't even fix out of scope things on the same line as the one that you're changing. Now when I was first working on core a lot in 2011, I personally made this particular mistake a lot. I would think, oh well, we're updating this line. We might as well improve this other thing too. I can improve the wording. I can improve the formatting. It'd be cleaner with a ternary and you can use those now, whatever. Bad idea, don't do it. It's still Sculpt Creep and it will still make your peer reviews less effective. And here's an example of why. So in the dif on the slide a change is standardizing how the word writable is spelled throughout the code base. Serious issues here, people. I'm not kidding you, this was an actual Drupal Core issue. Let's say I thought of a better way to word the method doc block that says a writable store. It sounds a little awkward to me so when I read it I was like, is that the best way to say it? It could be tempting while I'm changing that to say well, I might as well improve the rest of it as well. Now the problem with that is that it completely changes the kind of review that's needed. If the original chainset is only a couple dozen instances of standardizing how writable is spelled, that's very easy for me to scan. Writtable with an E without an E, with an E without an E. It'll only take a couple minutes. And here's why. So these kinds of changes are very easily scannable if you use a word diff. If you have not used this before, please do get diff dash dash color words. It's very useful for replacing all instances of a misspelled word, fixing one coding standard, replacing a deprecated method for a new one. If you have not used it before, I promise it would change your life. It's great. So if we suddenly add a bunch of rewritten docs my word diff is no longer useful. Instead of checking the spelling the reviewer has to think about the whole sentence. Is the new version actually an improvement? Is it grammatically correct? Are there other errors being introduced? Like did they make it 83 characters instead of 80? What's more, the reviewer will have to actually read the entire method to determine whether the new documentation is actually accurate. Suddenly what was a two-minute review is instead going to take 20 or more depending on how many lines we decided we wanted to improve. The approach we should take instead is to get this simple change set in, the one that just fixes spelling, and then have follow-up tasks for other improvements if you want to make it. Note that get diff color words can also take a reg X to specify what should count as a word break for the purposes of the diff. I find that specifying a pattern of a single dot is really useful for reviewing things like replacing one function call with another. So the example on the slide is from a recent change we made to adopt the new PHP 8 stir starts with function. The original merge request for this lumped in adopting stir starts with, stir ends with, and stir contains. It changed like 300 lines which is 600 added or removed lines under review. And there were nine or ten different patterns of replacements each with their different internal logic. I spent two hours trying to review it because the contributors had already done the work were very insistent they'd already done the work. So I did my best, but after two hours I just felt like my brain was melting and I couldn't see anymore. It was a clear sign that despite the effort that it had already gone in it was not well-scoped. So what I did is I just went in myself and split the issue up into five different separate sub-issues each with one pattern using git add dash p. Great strategy to use if you've received a change set that mixed stuff together or you're the author and you've gotten pushed back there are too many things in the set. Just split it into separate merge requests based on each. So after it was split up for the first step we had to review only three things. Does the original code have a check for whether stirposs is zero? Are the comparison operator and the zero removed from the line and does the updated code have the correct logic? If originally it was checking if the position was not zero like so the function should be negated otherwise who's checking that the position was zero then it should be a direct replacement with the stirposs call. So now there were only three decisions and very simple ones at that and I was a rather than you know a dozen depending on which pattern was used. And there were only a hundred lines to deal with rather than 600. Both these things improve the reviewer's ability to give an accurate review and decrease the time large change sets like this will spend stuck in the queue fighting merge conflicts and so forth. Alright so now let's transition a bit and talk about the most important questions to ask during your peer code review. The first thing you should always ask is if we should even be making this change in the first place and for Drupal core there's a corollary to that which is that maybe this shouldn't be a core change but maybe it should be done in a contributed project instead. There can be pressure in open source to merge a change because other contributors have already done the work on it but unfortunately there really are changes that make the application worse or deviate from the product vision or less maintainable in general. And one example of this is that we get lots of requests for specific configuration options to be added to the Drupal core user interface. Sounds great, right? More features, yay! Except that more elements exposed in the user interface cause the usability and the accessibility to decline. So additional things on the page are distractions. The second question is sometimes the only one that an inexperienced reviewer might know to ask. Does the change set completely resolve the issue under question without introducing any regression? Now the without regressions part is important and it means that anything that might introduce behavior changes like a bug fix should also be manually tested. Don't just write an integration test and assume it works that happens to us all the time. You would not believe the number of people who don't manually test the changes they make and then the committer finds out that it's broken. The third question to ask is whether the code quality is better after the change than before. Generally a change that decreases the quality of the code base should only be made temporarily in a theoretical situation. For example in a core security release we'll often add a duplicated API instead of adding a new, sorry add duplicated code instead of adding a new API in order to reduce as much as possible the chances that any theoretical custom code might break due to a name collision or whatever. After the security release goes out though we file a public follow up issue to properly add a new API in a minor release that properly refactors the code base. Note also about this point that it says better and not perfect. If something seems to be getting stuck in analysis paralysis refocus and just ask whether the current proposed change is at least better than the state it was in before and if there's other ideas for how to improve it further follow up. Fourth question. Does the change set meet standards for usability accessibility testing and documentation? These four categories also correspond to what in Drupal core we call the Drupal core gates. For each of these categories core has some minimum requirements that must be met. Specific accessibility testing that needs to be done if the user interface is affected. Specific test coverage and manual testing that should be included. Specific types of documentation that must be added and so on. And finally, is it the best fix we can come up with right now? This can be a harder question to answer especially for someone who's newer to the application under review. Now having multiple reviewers is in this situation really increases your chances of finding optimizations in writing cleaner code. For example, I always value working with peer reviewers who identify unnecessary API surface. It's something that I'm not great at myself and it's fantastic to have someone give me that feedback because a smaller APA surface makes the code cleaner and easier to understand reduces the chances of regressions being introduced and reduces maintenance. Now we're almost done and I have some final tips about how to do a peer code review. These aren't from any particular source unlike the rest of the session. These are just XJM's random thoughts. First piece of advice, pay attention to the bigger picture of what's being changed. To really understand a change you need to understand the API that it affects. Read the whole method. Look up the callers. Trace the call stack in your IDE or your API documentation. Or even step through it in a debugger if it's complicated code until you understand that the bigger picture. There's something about this. If you only review code in its own context you might miss something important. Recommendation number two grep or searching your code base. After Drupal.org and get itself grep is actually the tool I use the most in my code reviews. I grep for a huge range of things and probably at least one thing in actually a majority of issues I review for core. If I don't understand something or it looks funky check for other places the same pattern is used in core. If the user is only fixing one instance of something and I think the same bug might be present in other places I grep. If I want to compare what the author is doing to other uses of the same API I grep. Be curious and think not only about the code in front of you but out of the code base as a whole. And for my final recommendation be curious about why something is wrong in the first place so few people do this. If there's dead code unused variables, incorrect documentation or strange bugs it really is worth looking into how it got to be that way in the first place. For example, if you delete dead code without understanding how it became dead code you might be removing the last evidence of an unreported bug. If you don't understand the history or context of an issue you might inadvertently make it worse. So the best tool for letting the history of a line is git log dash capital L. A lot of people know how to use git blame but more often than not git blame will not actually give you the answer you're looking for. A few months, actually about a year ago now already I had someone ask me to clarify a to-do comment in code related to an issue I had never even seen before because they saw with git blame a change to that line. In reality, all I did in the situation was commit a patch, bulk update to add like HTTPS to all the URLs on triple.org. Or no, it's the one standardizing. Standardizing with the www, that's the one it was. And I had never actually seen the actual to-do comment that they were talking about because I used a word diff and just made sure that www was going away. Now if the contributor has used git log slash l instead it would have been clearer to them. And there's the example of that on the slide. Now you can't always stop there because git isn't great at tracking when lines are moved from another file. So sometimes you might have to check out the commit right before the last one on the list. Find where the lines were moved from and then repeat the process. But eventually you will get to the relevant commit and if you look up the for triple core there's an issue on triple.org to find the answer to why the weird thing is there. So, as a final summary I have tried to reduce the most important considerations for code review to six points that will fit on one easily photographed slide. You're welcome. First and foremost, foster a supportive review culture. Manage nitpicks with tooling like code style testing, static analysis, and merge request suggestions. Code scoping and optimized for reviewability. Decide as early as possible whether a change should be made in the first place. Ensure that all changes either maintain or improve the code bases code quality, usability, accessibility, test coverage, and documentation. And finally, ask yourself why issues exist to begin with. Look at the big picture and consider better alternatives for the solution. So that's all the content I have. Thank you for listening. It looks like I think this was a 45 minute session and I used up more time than I was supposed to. Sorry about that. But here's a list of references I recommend for anyone who's interested in learning more about this topic. So you can look up my slides on Twitter to read those there. And I'm assuming since the door is open and there are people staring we don't have any time for Q&A. Thank you.