Code Review, Forwards and Back

by Sumana Harihareswara and Jason Owen

The video titled "Code Review, Forwards and Back" features a playful and insightful examination of the code review process at RubyConf 2018, presented by Sumana Harihareswara and Jason Owen. The narrative unfolds in a conference room setting where a developer receives feedback from a team lead on their Ruby code for a feature addressing bug 1104. This performance illustrates various scenarios of code reviews, highlighting how code quality and team dynamics are influenced by feedback practices. Key points discussed throughout the video include:

Importance of Commit Messages: The team lead emphasizes the need for clear, concise commit messages, suggesting that active verbs at the beginning and adherence to character limits enhance readability.
Code Consistency: The discussion points to the significance of maintaining consistent styling in codebases, arguing that uniform code makes reviewing diffs easier.
Developer Experience: The need to prioritize the developer experience in each commit was highlighted, suggesting that a thoughtful approach makes it easier for future contributors to understand and navigate the codebase.
Reviewing Approaches: Several approaches to code review are modeled, with various outcomes discussed to show how critical and constructive feedback can lead to improved software quality and team morale.
Learning and Growth: The importance of viewing code reviews as learning opportunities for both parties is articulated, promoting a culture of collaboration and skill enhancement.
Batching Code Reviews and Feedback: Recommendations on how to handle large code deployments and ensure feedback is actionable are shared, illustrating how to improve the review process continuously.
Handling Bugs and Outages: A segment discusses resolving issues relating to bugs and monitoring, showcasing the need for effective communication in addressing software failures.

Throughout the performance, the presenters utilize humor and relatable scenarios to engage the audience, allowing them to reflect on their own code review experiences. The video concludes with a message that emphasizes the impact of code reviews on a team's culture and individual growth, advocating for a more constructive and educational approach to providing feedback.

00:00:15.680 Hey, so I just saw your message. Is your feature branch up to date? Yes, I just checked it. Okay, I’m pulling it down now. You know, most of the time we won't need to do this face-to-face, right? We'll do it online on GitHub. Remind me what this is for. Okay, so this is for bug 1104, which is to stop parsing the HTML and start using the API and stuff. Okay, yeah, I remember this. I figured while I was working on that, I could clean up some of the code a little bit, add a bug class to take care of some of these interactions, and, of course, add tests.

00:01:01.289 Okay, so just to start with, having your commit messages say that with an active verb as the first word is important. Also, this commit message goes over 50 characters on the first line, and we really don't like that. The reason this is messy is that you're not taking notes. Oh, sure, yeah, there’s a pen there. Here in `test_extract_bug_info`, you see how you don't have spaces after the commas in the parameters? That makes it harder to read. What is this? I’m not really used to caring about these kinds of things. In my assignments, if the code works, then it passes, and if it doesn't, then it doesn't.

00:02:14.130 Yeah, I mean, working is important; that is definitely the most important thing. So, by focusing on this, you’re saying that these things are also important? Yes, they are important. Look, this is small, but it's about consistency. If all of the code in the codebase is consistent and looks the same, then it’s easier for us to change it. It's easy to see what’s different in every diff. If the codebase looks the same on a stylistic level, then when we review, it’s easier to actually see what changes, what’s really important beneath the logic.

00:03:14.100 Right. Okay, so that matters. Here's another point: you've added `retriable` and `httparty` as gems to this project, but you’ve put them at the end of the gem file. If you keep a gem file alphabetical, then it’s easy for someone, five months from now, to check and see whether a particular gem is used in the project. Okay, I can fix that. Couldn't they just use search? Your colleagues are capable of using grep, but with every commit you make, think about developer experience. How can you make it easy for someone else to read this code?

00:03:39.480 If it’s 2:00 in the morning and they rolled out of bed because they got paged, then they're hitting scroll to see what it does. By thinking about developer experience in every commit, you’re making it easier for possibly yourself in the future to know what your code does. So, you know what to fix? Yes, um, spacing. Right? Alphabetize the gem file. Your first line of a commit message is too long, and it gets truncated. Right? I’ve noticed that. I don’t GitHub too often, and when someone’s running `git log`, that’s annoying. To back up a little bit, the point of this is: if we’re integrating a CMS and a bug tracker, the feeds from it are important.

00:04:56.450 Oh, someone’s pinging me. Okay, so I'll fix those things, check the style guide, look at some other code, and make sure it matches up with what's in the existing codebase. Then it’s probably good to go ahead and merge it into master. Okay? Oh boy, it’s been two days.

00:05:29.200 Oh boy, this is going to be hard to explain. Oh no! No, no, no! There are only three kinds of corporate blog posts: there's 'Please get your friends to try our new beta,' then there's one we've gotten very good at, which is 'We apologize for the recent outage.' And then there's one we may be doing soon, which is 'Thank you for our incredible journey.' Oh yeah. There’s going to be a meeting tomorrow in a big room where I explain to a number of people with interesting titles why we had this outage and why we didn’t prevent it.

00:06:35.060 Okay, so on the code level, we figured this out. We were streaming the data into the disk cache, and that worked when we had the VM get replaced every deploy, right? And then we were deploying over a couple of days, but Darren went out of town for a week or two, so we didn’t have any deploys during that time.

00:07:28.080 That is the most robust thing I’ve ever heard! So the data just filled up and the application crashed. They are going to ask the extremely reasonable question of why we were not monitoring this extremely important process. He thought we were! We wrote code specifically to handle this situation, but it turns out that code just never ran. How did we not notice that? I mean, there's another one that's new, another method named `somemethod`, and we used the wrong one. It doesn't do the same thing, and no one saw! Who would have seen?

00:08:54.330 I am going to have to explain why we switched to self-review. You think we would have caught this in code review? Yes! We never caught stuff like that. We were always focused on like the silly Robocop-level stuff. Our code is super pretty; it just does useless things. You know, if I could do a few things over again. Oh hey! Yeah, I just saw your IRC message.

00:10:04.110 Yes, yes, yes! I just updated the feature branch based on comments! Okay, remind me what this does. Yes, this is for bug 1104, which is to stop parsing HTML and start using the API. I figured while we're in there, I could clean up some of the code, add a bug class to handle some of these interactions, and of course that suggests. Okay, so why did you put the bug class in this file? I mean, it made sense to me. Oh, that should go in our utilities library.

00:11:42.300 One of the comments Randall had on something else was that classes should be close to their uses. Randall is very often right about this kind of thing. Across the organization, we are trying to make the developer experience more robust. We are trying to standardize interfaces to external resources, and that’s why that should go in the utilities library. Okay, so what else here? This test, the name of it, you should rename it to be consistent with the rest of the files—in snake case with underscores instead of camel case.

00:12:59.820 Yep. Okay, all right. So here you're using HTTP client as the base case. That's actually deprecated, and you'll need to use API client. Do you know where that is? Oh yeah, I thought that HTTP client was the new thing! Who told you that? Randall! In one of the resolution comments on the pull request. I originally wrote the API client, and I just switched it! So there’s a number of component choices, including this one, where I’m sorry that it didn’t come up yet before the architecture committee.

00:13:57.030 So there's this sort of cross-org task force. I remembered it being on the previous agenda, but now I remember it got pushed. I’m sorry; I thought we’d made a decision on this one already. But anyway, it’s for a meeting next week, so we need to actually check which one we’re going to use across the org. It's kind of a formality, but when you do a bake-off, oh really? Yeah, I mean, look, API client is clearly technically superior.

00:15:00.049 But in fact, I can use your branch as an example. Did you just switch it, though? Okay, so anyway, I cannot merge all of this pull request until that’s finished. Okay, when will that be? So, let’s see. They meet every other week, and the bake-off starts next week. Oh right, Thanksgiving! So late, like five weeks?

00:16:38.370 Okay, look, I know how this goes. We want to be robust and defensible in our component choices across the organization so that team members who need to switch into other projects are reusing the same components. In the long run, if you own the same toolchain, that's going to increase organizational velocity. So look, if you change the bug class, move that to the utilities library, clean up the test name, and so on, I mean, I’ll let you know what the architecture committee decides. Maybe we can speed it up.

00:17:16.920 You have runway, right? You have something else to do? Yeah, I can do that. Great! Oh, so I was hoping we would have your help with the rewrite, but congrats on the new gig! Thank you! I’m super excited for the opportunity; it just felt like the right time to move on, you know? Well, okay, so it’s only two weeks, right? Okay, let’s see what you have been working on for your last two weeks.

00:18:31.690 You were figuring out the Sinatra to Rails migration number two ten, right? Yes, that PR got a lot of comments, so it’s going to take at least a few rounds to address all of those. More than two weeks, definitely. Okay, um, what about you were also working on bug 1509 over here?

00:19:33.740 Right? Yes, I got that working on my machine, but Sam pointed out that it’s not going to work on Oracle, and DevOps can't give me a new instance until they get the new hardware, so I’m blocked on them. Oh, okay, what about 167, the security one? Yeah, um, that one I’m blocked on the security team. They don’t have any capacity for this branch! I thought we did plan this out, but they have other stories prioritizing!

00:20:19.810 Okay, okay, so how many bugs do you have assigned? Like twelve, I think. Okay, well, I mean, most of them we can close, I’m gonna, you know, because we can won’t fix them because the rewrite is going to obviate them. Oh sure, oh yes, that's pretty soon. So I can do that. Then on Sinatra to Rails, start addressing just as many of those. Like, mention me on as many of those comments as you need to, and assign or—no, I mean assign stuff to me.

00:21:32.490 But first see what you can do this week! Try to hand stuff off, and next week there’s a new dev starting, and I figure you can train them up and help them get up to speed on the current system. Sure, I mean, you could even, like, when you’re on your way out, you could give them your dev box. Yeah, I’ll get started on that tomorrow.

00:22:46.160 Yeah, if I could just do a few things over again. Hey! Oh hey! Did I get a text message from you? Yeah, I was wondering if you had a chance to take a look at the code review for— You mean is this the feature branch that’s up to date? Yes, I pushed all my latest changes, and it’s ready for you to look now. Mm-hmm. Oh, okay! Okay, fun! Don’t get help on under your 45.

00:23:41.140 Yeah, okay, I’m pulling it down. I wasn’t sure if I should prepare something for this meeting, like an outline, or a walkthrough, or a slide deck. What firm for? What for the code review? Oh no! I mean, oh, okay, so we’re ramping, right? So like, you’re not gonna have too many of these; once you finish on-ramping, you’re probably gonna only ask for code review on really major changes.

00:24:57.570 Okay, and so it’s only if it’s big, right? But this isn’t—oh, actually you do have kind of a big commit here, don’t you? That’s a lot of lines. Actually, do you know how to use `git add -p` when you're making a commit to make really tight little...

00:26:12.850 Oh, I saw a blog post about it. It was really great! I can text you the link unless you want too many emails or source lock it to you. Yeah, okay, oh, I don’t know how much we want to be using the API. I don’t know. Is it getting deprecated or what’s the concern? Not that I know.

00:28:02.290 I mean, I like places like St. Thomas; they like rate-limit or something, right? Hmm. So I noticed that there was another class that was calling the API at the CMS helper, which is kind of similar to what we're doing but was kind of distinct also, so I wanted to get your opinion on whether that was worth trying to merge or not!

00:29:58.080 I mean, I thought you gotta check the different behavior if you want to do that. Okay, so should I add some code to check for the rate-limiting case? Is that something you think you need to do? I mean, also, like, have we actually had that come up? We’re at like a hundred to two hundred requests a day right now.

00:31:25.220 So I mean, if it doesn't come up, that’s just like extra code and—but I’d like to keep things refactored and streamlined. Okay, yeah, all right. So that’s what I saw. If you want to take a second look at that stuff. Okay, I will take a pass at fixing those things and then ping you to take a second look and merge it!

00:32:37.920 Do you not have the merge trick? Are you able to push to the repo? Cannot master? Do you need to have me ask IT? I don’t know! I mean, I haven't tried. I thought you needed a plus one before I could merge it. No! You could have merged this! Really? Like, yeah! I mean, you asked for a code review!

00:33:38.130 So I told you a few things you could look at! And then you see what you think? Means changing what you think is good!

00:34:17.050 Okay! Like no one’s gonna micromanage you! How can I? We don’t have like a heavy pre-commit review process like that. Is that like your last place? Kind of. Okay, so I’ll just merge it? Yeah! I mean, look, you have the commitment because you got it when we hired you. We trust you.

00:35:25.340 I am just completely stuck! I have no idea what to work on next. I mean, kind of like I figured out what was going wrong! It was that Randall was so passing from issue, and Andreas was crossing from bug. And this, like, they’re similar but not the same and have different behavior!

00:36:49.170 Like it’s not even obvious to me which one though! I mean, I could but like which is correct? I can't ask PM because they’re on like a retreat right now and their entire calendar next week is booked for meetings.

00:37:42.770 And like, what if functionality? Right? Exactly! Yeah, I mean, you go to this kind of detective work though, right? Thank you! But like since Nancy left, all this work has been falling to me and I can’t do all of it! There's just so much of this kind of problem that we're facing. Our code is a maze of twisty little functions.

00:38:43.720 All like, you know, if we could do a lot of this over again! Yeah, it's not so great to do code review!

00:40:03.920 Oh, good for you! Is your feature branch up to date? I mean, so you know how Robin is working on continuous delivery? My PR was doing those open, so they did a quick review and merged it to test, but it’s ready for you to take a look at now?

00:41:02.160 Yes! So this is for bug 1104, which is to stop parsing the HTML and start using the API! I figured while we're in there, I could clean up some of the code and add a bug class to handle some of these interactions.

00:41:09.650 And of course, add some tests. Ding ding ding! Yes, that is exactly what you say in your excellent commit messages! Thank you! Yes, Sarah was real big on commit messages, and she was also able to help me break up a big commit that I did before, so I got to learn about `git reset` and `git add -p`.

00:42:38.420 Oh, that’s really awesome, and tiny commits are so much easier to review! But you’ve heard me say that! Okay, it looks like the build finished; it’s time to pull that lever and find out if it’s all green!

00:43:41.455 There are just a few little style guide violations that the linter caught; but those are super easy to fix. We have some docs about how to run the linter locally that are on the wiki. I'll make sure you have a link.

00:44:09.120 Let me just keep looking—this is good overall! This test here is pretty well named, I wanted to call that out. Good! You are handling the error case of this API call correctly, and that is one of the main things I wanted to look for!

00:45:45.320 Yes, I was poking through Brian's library as I was trying to figure out how to use it, and I’m so glad I didn’t have to find and write code for all the edge cases this API has.

00:46:38.770 I know! Isn't it great how much detective work he did on that? Yeah! And it’s not the only one! I want to show you here—huh!

00:47:18.620 See this comment? He actually filed a bug with the vendor upstream! Oh good! Yes! So maybe we won’t have to have this hacky workaround forever! That would be nice!

00:48:32.230 Yeah! And this plus some other detective work that he’s done on some other clients, with other APIs for other vendors, he’s actually thinking of collecting those together into kind of like a case study that he’s going to be submitting as a talk to RubyConf.

00:49:32.200 That’d be wonderful! Okay, yep! This is looking good! Let us head over to your desk and fix those tiny linter violations, and then we can merge this!

00:50:12.880 Oh, okay, I’m a little nervous! Yeah, I can understand that. Let me just say a little bit by saying that this is a good pull request! Like, I took a look earlier, and you did a good job! I was nervous my first time getting a code review too!

00:51:45.870 But I’m not here to critique you or to point out flaws in you as a person. You are not your code! If we find a bug and it’s super easy to fix because it’s just code, and now is the cheapest time to fix it because it’s in your head and a little bit in my head!

00:53:14.450 And it’s way easier than in, like, six or twelve months when we’ve forgotten how all this works and we have to figure it out when it’s breaking! And also, I hope this will be a learning opportunity for both of us.

00:54:42.230 I’ll get to see how you’ve approached the problem and thought about it, and maybe learn a thing or two from you! And then you will get to see what I am going to be looking for and asking questions about.

00:56:11.150 Hopefully, we’ll both get to learn from that! Yeah, I guess that seems like a good way to do it! Great! So how did you find it? How easy was it for you to make this change? Actually, it was like surprisingly easy.

00:58:27.990 Once I got the linter running locally, I mean, good! Like, I was able to use `git blame` for a lot—everyone's commit message is really clear! Yes, isn’t it great to be able to look through the history and learn from previous developers and their mistakes?

01:00:06.170 We have a few thank yous! We would like to thank Betsy Hable for co-starring with us! Thank you to the sound and light crew, particularly Audrey Ash, who is over at the AV table.

01:00:20.040 We would also like to thank our director, Jonathan Galvez, and the staff here who did the sound, as well as the event crew from the AV staff—specifically Randy and Paul—who have worked with us.

01:00:36.740 Thank you to the RubyConf organizers, especially event producer Heather Johnson for coordinating with us! And thank you all so much for coming to our play! Enjoy your break!

01:01:00.080 Yes! Have a good time!