Jenn Creighton: Today’s topic is code reviews. I don’t know if you remember your first Pull Request, but I remember mine. I was nervous. It’s really hard to put your work up for review. And think of some of the language we use for code reviews. You’re asking for approval, you might get rejected or needs changes. Not to mention, you’re trying to communicate all of this in writing. It’s a lot. Now, joining me to talk about all of this is Faduma Mohammed. She’s a Software Engineer based in the UK. She’s witty and hilarious, and her views on code reviews are eye-opening. This conversation has it all, from nitpicking nitpicks to why GitHub is a volcano of miscommunication. So, we actually met at ReactJS Girls, what feels like many more years ago than it was.
Faduma Mohammed: Yeah, it feels like decades ago.
Jenn Creighton: Oh my God it feels like, decades. But it wasn’t that long ago. I had the pleasure of meeting you there. And unbeknownst to you, you were actually there for the beginning of a talk I decided to give that then turned into this podcast.
Faduma Mohammed: Really? I was not aware of that. That’s kind of cool.
Jenn Creighton: Yes. You were there at the inception moment. I don’t know if you recall this and it’s totally okay if you don’t. But we were talking with a gentleman at the conference and we were talking about decision making for some reason. And I remember him saying something along the lines of like, “Well, to make decisions, you really have to take all the emotion out of it.” And both of us were like -- [crosstalk]
Faduma Mohammed: Okay. I don’t remember this. Yeah. But that sounds stupid.
Jenn Creighton: Both of our reactions were like, “No, that’s not right. That’s not how this goes.” Yeah.
Faduma Mohammed: Yeah, that sounds like a trauma response. Like somebody hurt you before, like what’s going on?
Jenn Creighton: It totally is. It absolutely is. It’s like you just couldn’t handle your emotions anymore and so you’re just like, “Well, we just shut them down when we need to shut them down.” But it kicked off this whole -- [crosstalk]
Faduma Mohammed: We must be objective. Yeah.
Jenn Creighton: I’m rolling my eyes so hard at like we must be objective. Ugh, painful.
Faduma Mohammed: It is really painful. I truly hate when people bring that up. It actually makes me rage. And I really instinctively want to go into a rant about how it’s impossible to be objective. It’s just impossible.
Jenn Creighton: Oh, it’s 100% impossible. Both of our reactions to him saying this, we’re both just like, no, this is not a thing. And I think back to that moment so often, and I wrote an entire talk on your emotions are not an anti-pattern. They’re actually very useful to you. And it was because of him and also it was just so helpful to have you have the same reaction as me. Because then I didn’t feel so weird about how I feel in this field because this field tells you to stop having emotions.
Faduma Mohammed: It really does. I think it encourages people to really suppress who they are and follow a weird concept of what it means to be an engineer. Which is very heavy on being a man, being from a southern socioeconomic status, not being working-class. Because working-class people have feelings. We’re very emotional and maybe you would know about how you feel about things. But yeah, I definitely feel that at the time when I’m with another person, that’s a woman because we just have the exact same feelings, I’m really glad to be working in Conde at that time with so many women. And we sometimes leave from a meeting and look at each other and be like, “Okay. That was terrible. Did you feel that?”
Jenn Creighton: Yes. Yes. 100%. I have had that where I left a meeting and there was another woman in the room and we just kind of like look at each other too. We just like, no words are exchanged. It’s just a look of just like, “Can you believe the bullshit that went on there?”
Faduma Mohammed: It’s really like people feeling themselves, yeah.
Jenn Creighton: All of this actually, I think lends itself to our topic today, which is actually code reviews. I think a lot of this stuff actually comes up in them. I’m really curious. Do you remember your first code review?
Faduma Mohammed: I don’t exactly. But I do remember significant ones. So, the most memorable code review for me was when I was working more in a back-end service. And I had maybe 20 comments in my PR review. It was absolutely overwhelming. And I’ve never had that in my career. Yeah, so that one was most memorable. And it really impacted what I value in the code review, like what I want, what I do as a reviewer, I’m like what I expect other people to like be doing while they review my PR.
Jenn Creighton: Okay. About that particular code review, the 20 comments, was that all from multiple people, one person, like how many?
Faduma Mohammed: It was from multiple people and some people that were not even my team. So, some like, bunch of stuff engineers were just theorizing in my PR review just being like, “Oh, have you thought about this?” And like, you know, “This is a really interesting concept.” Like, “You can do this, you can do this.” And I’m like, “Well, I’ve done something.” Like, “I understand that you want to discuss options, but now I’ve actually coded an option. So, it’s not really the right time.”
Jenn Creighton: You’re like, “No, no, no, no. I made a choice, you’re reviewing that choice.”
Faduma Mohammed: Yeah, exactly. Yeah.
Jenn Creighton: That’s interesting. Oh, my God. Was this earlier in your career, or more recent?
Faduma Mohammed: It was when I was a mid-level developer. So, three to four years in. So, I was used to having code reviews, but still, I think, because I’m more of a front-end, like I do mostly front-end applications. Even though I say I'm a full second engineer, that’s really where I work mostly. So, working in purely -- The more you get away from user-centric like things and more into engineering, a world which is back-end and like infrastructure, the more higher people are with their reviews. That’s my personal experience. I’m not sure that’s what everybody experiences. But, so yeah, it was my first time. And because it was a back-end service, I was a little bit more insecure. So, I don’t think I would be like that, in my front end. Like I’d be very confident about what I was producing. So, I think with those kinds of PR reviews, it kind of spirals where you’re like, “Well, okay. Let me see if their comments are actually something valid. Let me actually think about it.
And I would not do that in front-end. I will actually know I’ve actually considered these things and this is the thing that I’ve chosen. And these are, we can discuss it offline, which is my favorite thing to do. I don’t like having long argu-- pseudo arguments on PR reviews. I think PRs are not where you make -- it’s not where you discuss options or talk about any conflict that you have with the review. PRs shouldn’t come as a surprise, if you’re working in a team. You should be talking about things that should have some alignment as to what you’re going to, like the results of the ticket. So, it shouldn’t come off the cuff, like a complete surprise as to what you’re doing. So, if there is something that’s quiet, like different, as a reviewer, I always go, “Okay. Let me pause,” and actually message them on Slack and maybe have a chat on Zoom or discuss it further. I wouldn’t -- if the comment is more than two lines, I think it’s too much. Unless it’s really nice. Unless you’re say something amazing. But if it’s super critical, just have a discussion with them.
Jenn Creighton: Yes. When I started out, I remember being -- I mean, for me, even now, when I put up a code review, I’m actually still very nervous all the time, you know. And now I work in open source, which means anyone can view my PRs, which is just, “Ohh, no.” It’s really nerve-wracking. I remember my first one being really really terrified and trying to emotionally prepare myself for criticism. And I think I also did think that those were where we went about the sort of like discussions. And they gave me so much anxiety that we were going to have that sort of back and forth in a PR and it was going to get contentious. Or, I don’t know, I was going to be totaled like how terrible my code was.
Faduma Mohammed: I was the same too. As a junior, it was really -- I would look at my PR, I would write my PR description over again and again and again trying to make it seem like write something nice. I was just super nervous. Yeah, each comment always seems like they’re being quiet like I haven’t done a good job. Like, did I do a mistake?
Jenn Creighton: Also, that like 20 comments, even if they were all legitimate issues, I do feel like the more comments on a Pull Request, the more likely I am to think that that Pull Request was a failure of mine.
Faduma Mohammed: Because like, even if you’re not doing something controversial, the mere fact that there are those comments makes it seem like it’s controversial. And I feel like those PRs never get resolved really quickly because it feels like you have to reply to everything. So, that’s some of the challenges I have as a PR reviewer. I’m like, “Is this comment even necessary? What do I expect them to do about it?” Like, that’s why I asked myself, “Is this comment going to result in something? Or is it just me saying something out of like, what I like [inaudible 00:11:38]” Because I feel like I should. So, that’s the comments I appreciate, ones that are actually about action. Like, “Oh, this should be changed to this. You haven’t done this pattern, and you haven’t done like, oh, you forgot about unit tests? Oh, look, there’s a typo that you missed.” Those are quite useful. But when it’s just chatter, I don’t think that’s useful. I don’t think it is helpful when you’re like, “Wow, this is really interesting. But have you thought about these things? Or whatever.” If it doesn’t result in you advising them to do something, I don’t think it’s a useful comment.
Jenn Creighton: I think that is a good metric to base your comments on. Like, is it actually actionable? And if you do want to have those bigger theory things, I do think it’s better just to ping the person. They usually have a lot more context than you do as the reviewer, and they probably have already thought of some things that you are thinking of. So, it’s better to have a quick chat, rather than have these overwhelming comments for them to then have to respond to. And there is nothing worse, well, actually there is but it is really terrible to have a Pull Request just sitting around, just work that you did sitting there.
Faduma Mohammed: Oh, my God. It’s the most frustrating thing because you can’t really move on because you can’t really pick up another ticket because it feels a bit weird to like, you’re doing half something else and half something, your unfinished work. But I feel like as -- I think with anything, you have to leave the trust as like a -- I always trust the person doing the work that they know more than I do. And I think that is always very helpful if you assume that they’ve done the research, that they are -- they’re your colleague so you probably know them, that they’ve actually thought about what they’re trying to achieve. I think that’s really helpful because it stops you thinking or maybe have they not thought about doing this thing. It makes it a little bit nicer when you actually believe in your people that you work with. People really sometimes, especially in engineering, they’re very distrustful of your technical skills, especially if you are a woman or especially even worse if you’re a woman of color. And I will lead as an example so always just everybody. Like, I’m just like, “Okay. Well, David is great. He does good code, he writes really well. I work with him every day. We kind of know what the problem is. We’ve discussed it in refinement, this ticket’s not something that’s unknown. So, yeah, I went on a tangent there.
Jenn Creighton: No, I was -- No one can see this, obviously, but I’m nodding my head so hard. I was like, “Oh, my head’s going to fall off.” But yes, because this thing that you’re talking about, this trust issue in engineering is, I think, one of the biggest issues that we have. It affects how we interview people. We just, naturally we’re like, you have to prove yourself to me. Code reviews, I remember them sort of being -- they’ve always been presented to me as like a code quality kind of metric. And I just don’t think that’s what they are actually.
Faduma Mohammed: I don’t think that’s what they are too.
Jenn Creighton: Yeah. No, they’re way more about like -- I don’t know, what if we instead thought about them as like quality control of like, well, people are just doing bad things, because they aren’t very good engineers or whatever? What if we thought about them as a communication tool to let people know what work has been done?
Faduma Mohammed: I see it more as like proofreading. Like, can you just like fine -- Like, sometimes I’m not -- I will miss things. Can you see why I’ve missed? Especially typos, I’m very grateful when somebody points it out. But yeah, there’s a huge issue of trust in the whole industry. So, a thing that I took from one of my tech lead was that he really wanted the team to be so in line with what we wanted as a team, and what we valued. But PRs were kind of, eventually, in an ideal world be pointless, where we would just be writing because we’re so aligned in our values that nothing will be surprising. And we could even merge to master because we were so aligned. Of course, that’s not something that we did. But I feel like you shouldn’t really see code reviews in the way that it’s sometimes presented by tech pros that are like, “Well, people are going to review all your code. I’m going to go hard on you. If you don’t write, you won’t [inaudible 00:16:38] How dare you. You have insulted your ancestry.” Like fully decimate who you are. And that’s not what they are. They are really communication tools to show everybody what you’ve done so people are aware of them, and also to help you out so that you don’t make -- so they can figure out the mistakes that you’ve made and been like, yeah, helpful tool.
Jenn Creighton: Yeah, it’s just supposed to be a helpful tool. It’s not supposed to be I can’t trust this person to do their job. I must look over every line that they did, and nitpick them to death about it. The same thing applies, I think, when you’re looking in a codebase and you see code that someone else wrote. If you change your mindset to have this idea of like, they were doing the best with what they could at the time with the information they had. Don’t just automatically assume, “Oh, they don’t know what they were doing and I can fix this.” Which I’ve seen a lot and I, early on in my career, picked up that bad, bad attitude and habit because there’s pressure to conform. And I think for a long time, my code reviews and how I worked in a codebase did, I was just trying so hard to fit in so that I could be thought of as a software engineer. I picked up some bad habits. I had to really go back over the past few years and cut some of that out.
Faduma Mohammed: And that’s how I think about this. Like, I think, especially when people are really critical or very glowing about some code, I think about what my bootcamp instructor said on the first day. And he was like, “All code is shit.” And I really think that it’s true. All code is shit. After you’ve written it, sometimes it is no longer use-- like you can critique it, like it’s no longer -- like you can always improve it. Yeah. So, I always take our motto like I don’t take -- I think Git Blame is really one of the things that kind of push this kind of narrative where people are looking at who wrote this line of code and I’m also guilty of that.
Jenn Creighton: I love that. All code is shit. It reminds me of when you buy a brand new car. As soon as you drive off the lot with it, its value decreases. And it’s the same thing with code. Once you’ve written that beautiful, shiny little code, and you’ve committed it to your codebase, its value just dropped. You’re going to look at it in a few months and be like, “Oh, wait. I could have done X or Y.” Or a year later, it’s not going to work for whatever has evolved from the codebase.
Faduma Mohammed: Or a new thing has come out because JavaScript is like that. So, it’s old now.
Jenn Creighton: I know. JavaScript is like new model in the showroom, new thing, new feature, React and all the other frameworks. New shiny things, Views coming out with stuff, like JavaScript doesn’t stay still.
Faduma Mohammed: No, it never stays still. So, you have to keep that in perspective sometimes. Like, it’s not that serious, honestly, it’s not that serious unless you are writing some code for a heart monitor thing, like where the patient will die if you don’t. Everything else is not that serious. Like, it’s basically a website. Like, please calm down. We are not going to bring the world down if I add this new code. Yeah, I do love to complain about architecture about software. But like, “Oh, did you see what they --” Why are using Angular? Like that kind of stupid conversation. But yeah, it’s not really -- it’s not serious. Like you can’t really think that it’s actually important.
Jenn Creighton: And speaking of jokes, I tend to add some brevity to code reviews by making jokes in them.
Faduma Mohammed: I do too. I love adding emoji. Like, if I’m being extra with my comments, just being like, “This is the greatest thing I’ve ever seen.” Just stupidity. But I love it. I think there needs to be more of that just like extra positivity because there’s always extra negativity. And like, just to balance it a little bit.
Jenn Creighton: I love it when people make jokes and code reviews, put in a funny GIF, or make a pun, and what you’re talking about also. Like, sometimes saying this is good work.
Faduma Mohammed: It’s really validating. Yeah.
Jenn Creighton: It’s very validating, very much so.
Faduma Mohammed: Unless you’re pairing, sometimes we don’t really get any feedback about things are we writing in a nice way. We are always very critical as like, how do we improve? What can we do better? And so I think it’s really important just to take a moment and to be like, actually, you did a really good job. You smashed it, you did the thing. You were tasked to do this, and you did it.
Jenn Creighton: Good job.
Faduma Mohammed: Good job. Yeah.
Jenn Creighton: I know, for a while, I got into a mindset that was very negative, and was really based off of the previous cultures I had been in where we take things very seriously, and your code reviews are meant to be distrustful of each other. Where I was like, “Well, why am I going to say good job just for doing your job?”
Faduma Mohammed: I see those people not just in engineering, like just in life where you’re like, “Well, it’s just, that’s the bare minimum. They’re supposed to do that.” Well, you can just say nice things. It’s not like you’re changing the bar or the expectation of their job. But you are being a nice human being by giving out a compliment. You are also taking it too seriously. Yeah, I love compliments. I like giving out compliments and I like receiving them. So, I’m used to having a nice cushy, soft review, code review enviro-- that’s my fav environment. And I was talking about the PR with the 20 comments, that was my first time and glimpse of that world where you are extra critical, where people are theorizing about architectural philosophical things about coding. And I would love to get involved in those conversations or I love having them. But sometimes I just want to do my job too. And, yeah.
Jenn Creighton: Yeah, it can get in the way of you just doing your job. And like you were saying, nothing wrong with the compliment. Compliments are free. I think I had this incorrect assumption that if I gave out compliments, that that person would get an overinflated sense of self. And it’s so ridiculous because one, like not my fucking job. Like not my fucking job to decide to hold back on compliments so that someone else doesn’t like get an inflated sense of self. But also, the likelihood that someone’s going to get an inflated sense of self is a ridiculous notion. Because just speaking for myself, I’m terrified all the time that I’m a terrible engineer. I always wonder if I’m in the right field. Compliments are things that keep me going in this field.
Faduma Mohammed: Yeah, I hate this with people with parenting as well, where they’re like, “You’re being too soft to your child. You need to harden them for the world.” And I’m like, “That’s not what you need to do. The world is already quite harsh, the environment is quite harsh. You just need to be kind and be there for that person so that they can have some sort of valid sense of self to go back to. And as engineers, especially with juniors, we have to remember that we need to build them up so that they can actually not have imposter syndrome, but feel validated in this weird-ass field that we work in.
Jenn Creighton: Yeah, you’re right. The world is harsh enough. We’re going to learn some really harsh lessons. Let’s make some more like safety for people to fall back on. Those compliments that people have given me over my career, literally kept me in this field at times when I felt like I really didn’t belong. You can do that same thing and you’re actually giving them a compliment for a good job. You’re not just doing it for funsies. Like you’re saying good job. This is actually a very difficult thing to do, to do some work, and then put it out for everyone to comment on and be critical of. And also, if you want to be critical of someone, there are ways of doing that, that are constructive and still very nice.
Faduma Mohammed: Yes. I think people need to realize that written word is a quite challenging form of communication. And you really have to try extra hard to make sure that what your intentions are actually being seen with the comment you’re making. Because I’ve seen people write really like so harsh comments on them. And I spoke to them, and they’re like, “Oh, no, I wasn’t really that serious.” I’m like, “I did not get that impression.” So, maybe in your mind, you were trying to be not serious, but I did not get that. So, you really need to work hard and how you actually communicate in writing. And we need to stop communicating in the Git comment like, “You need to finish this.” Or like three word comments where it’s just like, “Change this, please.” That’s not helpful. That’s not helpful. Like, I’m dyslexic, so a thing that I always get back to is a very formulaic way of writing because that makes it easier for me to communicate. But I work -- I know this about myself. And I take extra time in making sure my tone of voice and what I’m trying to communicate is actually [inaudible 00:26:54] through. And I’m always surprised that people don’t do that.
Jenn Creighton: This is a very difficult topic because I have struggled with -- I’m just very, like I tend to be very flat in my tone or what I perceive as flat. And other people will perceive it as being a little bit more harsh. And it has frustrated me in the past because I feel like men in our field can get away with that, but I’m expected to be softer. And at the same time, I had to have like a reckoning with myself where I was like, “Well, maybe though you do need to still adjust your tone, maybe I know it’s frustrating this gender part of it that’s coming into it that really frustrates you because you don’t want to add the emoji to seem nice. But you have to add the emoji to seem nice.” So, one of the things I started doing is I’m using Grammarly, which gives you a little bit of sentiment analysis. And I use that for like everything that I write now to try and -- I don’t want to have to add emojis to things but it does give me a little bit of like, I could say this maybe in a way that this sentiment analysis thing will like be like you’re being nice.
Faduma Mohammed: Or you’re being neutral. Yeah, I get what you mean about like the gender roles because I struggle with that because I am quite feminine. It’s just how I am. I’m a quite feminine person, and that’s completely fine. But I get what you mean about feeling like you need to not be typical, just like so that people can feel like you’re actually a software engineer, like you’re one of them. And I really hate that so much. And I actually rage against it. Like, I’m going to be my feminine self. Like, I will be my emoji-soft person. I don’t care if you see me wearing or having pink [inaudible 00:28:53] like that’s for me. And yes, I am being a typical person for my gender, but there’s nothing wrong with that. There’s nothing wrong with being feminine. And we shouldn’t really demonize anything to do with, like everything to do with femininity. Yeah, I think that’s quite sexist. I see that quite a lot.
Jenn Creighton: Earlier in my career, I really held back on a lot of things that are typically girly because I wanted to be taken seriously. And I knew, I noticed that if I came into an interview dressed very feminine, it was likely not to go well. It was weird. And if I came in dressed like jeans and a T-shirt, it tended to go better than just the perception of me like in the office and stuff. And then once I got some years of work experience under me, I started putting more pink around my desk.
Faduma Mohammed: Yeah, I have just accepted that I’m going to be a person that sticks out. I’m a black woman. I’m going to stick out. Everything about myself is going to be different. And honestly, there’s literally no point in conforming because I’m never going to get to the point where I’m fully accepted. So, why not just be myself?
Jenn Creighton: I’m so glad that you got to that place because it’s been -- you got to it earlier than I did.
Faduma Mohammed: No, it was really difficult. Like I’ve had a quite challenging experience in this industry. It’s been difficult. But I wanted to be a lawyer and that would have been difficult too. So, I really have to, like ground myself in the reality that the world is harsh anyway. But I’ve definitely had very challenging experiences, especially as a woman and a woman of color, and being the first all the time is very hard.
Jenn Creighton: Right. Because I’m sure you’re not just the one woman on the team, which is what I’ve often experienced. You’re probably the first person of color on your team, the first black person on your team.
Faduma Mohammed: Or in the whole company which I’ve had it before.
Jenn Creighton: Oh, shit. Yes.
Faduma Mohammed: Yeah. Like the only black person in the whole department, which I’ve had too. So, yeah, this comes with like major challenge. And one of them is definitely code reviews because people won’t take you seriously because they think you are like a token hire. Like people are only being nice to you. You are the person that is like here to make the company look nice and to increase the diversity scores. But I feel like I’m a really good engineer and the moment I start working, it kind of dissolves that, I think. Not entirely, but in my mind, I’m great.
Jenn Creighton: I’m sure also, what happens a lot is that anyone who is marginalized or underrepresented in tech, when someone who is overrepresented in tech looks at them, their automatic thought is that we have way less experience than we do. We’re automatically assumed to be a junior. Which nothing wrong with being a junior engineer. It’s just that if you keep being perceived that way throughout your entire career it makes it really difficult for you to get to your next level.
Faduma Mohammed: 100%. Like everybody assumes that I’m not the level that I am and I don’t know as much as I do. Because I’m a very studious person and I have so much anxiety about what I know. Still, I do study up too much and I definitely did that right in the beginning of my career where I was just coding a lot. Like, I was doing extra projects. Like I was just like, I knew about the latest everything. And now I just realized, like do you know how [inaudible 00:32:39] mediocre? Why do you sound really mediocre? And why am I trying this hard? Like, it’s not necessary.
Jenn Creighton: I did the same thing. I studied so hard, so hard, because I -- Oh, God. I just want it to be seen at the level that I was. And I couldn’t do that without studying way harder than my white male peers. And also, it did come out in code reviews. I definitely had people make comments in code reviews to me that were very dismissive. And I could tell why they were -- like, I knew why they were doing it. I had a particular code review when I was, I would say, like more mid-level easing up on senior by -- I had another partner on this team. I was supposed to be leading the project. He actually had less experience than me. I gave him some code reviews. Some of them were nitpicky, but some of them were valid and he ignored every single one. And told me no, he actually told me straight out no to all of my comments. And then if a senior dude would comment with the same thing, he would do it. And also no one believed me on why it was happening. I was like, he doesn’t think I have as much experience as I do. He thinks I am lower than him.
Faduma Mohammed: I’ve had that before. I’ve had, like me being a mid-developer and then on like, how many years and then I realized that my senior, the person that’s a senior developer has less experience than I do. And I’m just like, “What the hell? What the hell? Why do you have that title when I had to fight so hard just to be considered mid? I literally had to really go, like fight really hard to be promoted. And you just got that title because well, you did a computer science degree. Like what’s going on? Like, am I not doing the same as you did?” But yeah, I think there’s lots of misogynistic, alpha dudes that whenever you leave out like a weird comment on their reviews where you’re like, “Oh, can you change this a little bit?” They feel like it’s like a green light for them to be really hard on yours. It’s like going tit for tat. Just like you wrote a comment on mine, I’m going to do that for you and it’s not very helpful.
Jenn Creighton: No, it’s not. It does become like a little competitive thing that just serves no one in the end. Because again, code reviews are not about that. Right? They’re about communication. Speaking of that, tell me a little bit more about the code review kind of processes that you’ve worked with at companies. One of the ones that I worked with was very fond of labeling nitpicks as nitpicks. I don’t know if you’ve experienced it.
Faduma Mohammed: I’ve had. This is my pet peeve. This is truly my pet peeve. Whenever someone writes minor or nitpick, I’m like, “What am I supposed to do with this? Like, what are you telling me here? You’re just basically complaining? You’re basically complaining. I can’t do anything about your nitpick.” Yeah, I hate it so much. Just keep it to yourself. If it’s minor, then it’s minor, and it’s not worth commenting.
Jenn Creighton: That’s a very straightforward recommendation. There was a company, I want to say it was Netlify. But I don’t remember that they released their PR review guidelines. Which one, I do think it’s nice to have PR processes written down. A lot of times you go into companies and you just look at their PRs and kind of figure it out, or you’re told, but there’s no written thing. But they had a whole guideline that was like, “Oh, we put in like, how serious the comment is based off of like, it’s a pebble, it’s a stone, it’s a rock, it’s a boulder.”
Faduma Mohammed: Oh, God. Just add cute names to annoying things. Yeah.
Jenn Creighton: It’ll be fine if we call it a pebble. You won’t get so annoyed if we just call it a pebble and you have 20 pebbles on your code review.
Faduma Mohammed: That is basically a beach, pebble beach. Yeah. So, that tells me that they have bad code review culture. And when I see those, I definitely agree with you. Having a guide is really important, just so that newer people who are more engaged to your codebase or your team or your company can actually figure out how to contribute and [inaudible 00:37:21]. When I see those kinds of guidelines where they have to say that, it means that they have really struggled with their code reviews where people are adding way too much comments. And maybe some of them are not that serious. But yeah, I think it goes back to again, what are you trying to achieve, right? You want the code reviews about making sure that thing that’s been added is not going to create catastrophe. And you’re proofreading it to make sure there’s nothing extra wrong with it, making sure that they have the same value. So, if you’re testing is important, so making sure that there’s testing in it. But everything else I think is kind of not important. Like code styling, linting normally takes care of that. I’m sure -- just have Prettier, like that will help make sure everything is aligned.
Jenn Creighton: Thank God for linting. Before linting, our code reviews were just like, you need to put a semicolon here and this needs to be indented.
Faduma Mohammed: I love Prettier. Prettier, I think is one of the best things that came out in JavaScript. It literally just fixes everything, just makes everything just like perfect and I love it. Everything else, I think is not really -- Like if they are following [inaudible 00:38:37] pattern, then I think you should have a discussion with them and say, “Hey, let’s have a discussion offline. Like maybe there’s a reason why you thought of a reason why that you’re not doing it that way.” Yeah, I really like just mostly just having actual conversations with people if you have any worries about a PR. But having to leave a pebble or boulder, I don’t think it is an actual improvement or a good code review culture.
Jenn Creighton: These guidelines like that are I think indicative of people not talking to each other very much. We are really hesitant to talk to each other.
Faduma Mohammed: We really are. We don’t like to talk.
Jenn Creighton: We don’t like to have conversations. Is it because we don’t know how? I know with COVID, at least, I have forgotten how to socialize to a certain degree. But the best environments that I ever worked in were like we talked a lot about code reviews before they even went up. Or one of the things I really love to do with my code reviews is if I look through and I sense that there might be questions about something, I just go ahead and document it.
Faduma Mohammed: Me too. I add a comment straight that’d be like this is why I remove that. Or I think it’s confusing. Because sometimes you only see a snapshot of the files that you’ve changed so you don’t really have -- Like, unless -- [crosstalk] Yeah, you don’t have the full context. So, I like to add comments to mine, where I’ve like done something slightly different. Or if I have a question being like, “Hey, I don’t feel really good about this. I’ve done it this way. If you have better suggestions, let me know.”
Jenn Creighton: I think that is really, really helpful. One, just making sure your description explains things. Also, a great place to put in jokes, just a great place to put in jokes is the description.
Faduma Mohammed: Or screenshots, would be nice. Yes.
Jenn Creighton: Put in screenshots [inaudible 00:40:32]
Faduma Mohammed: Or GIFs, I love that.
Jenn Creighton: Oh, my God, I love. That changed the game for me when I was doing code reviews at previous companies; changed the game.
Faduma Mohammed: Just take a video of the thing that you’ve changed and then it makes my reviewing so much easier.
Jenn Creighton: It’s so hard if I can’t visually see what’s going on. Screenshots, GIFs, do them. There are really great little helpers that you can download to do these things really quickly on your computer, like just do it. It’s so helpful.
Faduma Mohammed: Because then the person reviewing has to like actually pull it down, run it, and see what you’ve done. [crosstalk]
Jenn Creighton: Oh, what a slog, you know.
Faduma Mohammed: And like it’s just -- Yeah. We are lazy engineers. We don’t want that.
Jenn Creighton: Even better as like a preview link. But still, then I think sometimes you just don’t know what section they’re talking about and you get kind of confused. So, screenshots, GIFs.
Faduma Mohammed: Screenshots, GIFs. Definitely, I think we have a problem with not speaking. And at the PRs, GitHub is where our failures come to light. Because it’s just like a little volcano of people misunderstanding and miscommunicating. And just -- [crosstalk]
Jenn Creighton: It’s a volcano of miscommunication. I love that. And I really want that as like a sticker or a T-shirt, volcano of miscommunication. I mean, that’s what happens when you push communication on to a certain platform. You know, we need these platforms to discuss our work and things like that. But you really do need to figure out when that discussion needs to happen offline.
Faduma Mohammed: Definitely. Like examples that I think I do all the time is that if I’m unsure, or I think there’s like a weird tech-death thing that I’ve encountered, or I’m having challenges, like the way that I have to do something is like ugly, I don't feel great about it. I’m like okay, I just add a comment in Slack saying, “Hey, these are things I’m struggling with. I want to have chats with. This is a meet-up link. Come have chats.” And I rather do that than have it as a surprise in my PR.
Jenn Creighton: Like you said in the beginning, a code review shouldn’t be surprising to you. You should have some communication beforehand about the ticket that you’re working on, or the piece of code that you’re looking at.
Faduma Mohammed: And also, if you’re not on the team, maybe code reviews -- don’t review somebody else’s stuff, like be careful about how you’re reviewing it. Don’t add weird comments like, “Oh, this is interesting.” Like, I understand. I understand that’s interesting. But I don’t know what to do with your -- you don’t have context. You have no context. But you know, I don’t want to say don’t review at all, but just be careful as to like what you don’t know. You don’t know what you don’t know. So, how can you review something when you don’t really know the full context?
Jenn Creighton: You also don’t know that person if you’re not on the team with them, and you haven’t developed any sort of rapport with them. When you have a rapport with someone, you take their written conversation way differently than if you don’t know that person. So, you really need to be careful. And if you’re joining a team for the first time, your first few code reviews that either you’re presenting your work, or you’re reviewing someone else’s, you need to be really careful about how you’re talking to these people that you haven’t built up a rapport with yet. Maybe even look back at old PRs from those same teammates and sort of try and figure out how they like to communicate before moving forward.
Faduma Mohammed: So, a thing that I love to do is PR pairing, just like PR review pairing. So, I would offer that to people just to like if you’re struggling reviewing my PR, I can walk you through what I’ve done and we can talk about why I’ve done it. I don’t mind doing that. So, just like ping me and we can do it. Or if somebody has a PR and they want me to review it but they’re not sure about it, we can review it together. I loved that as a junior when I was struggling giving reviews because I did not feel qualified to give reviews for a long time. Like I felt like I didn’t know enough about anything to be critical about like any code. And I loved reviewing things together with other engineers.
Jenn Creighton: That’s a great idea. I don’t think I’ve actually ever done a pair review before. Honestly, when I got into software engineering, I thought there would be more pair programming than there has been. I love this concept too of applying it to reviews. It’s also just a great way to build a rapport with someone. It also is a very nice way of getting you comfortable talking about your work, or getting comfortable asking people about their work.
Faduma Mohammed: And figure out how they problem solve because that’s really interesting too. So, you’re like, “Oh, I did not think about that. That’s really cool.”
Jenn Creighton: Yeah, what a great strategy. That’s really great. We have to wind it down now. I’ve actually kept you a little bit over time. So, thank you.
Faduma Mohammed: Oh, no, it’s been great.
Jenn Creighton: I love this conversation. I knew this was going to be good when I brought you on. We had such a good time at ReactJS girls -- [crosstalk]
Faduma Mohammed: That was fun. I really missed that.
Jenn Creighton: Oh, that was such a good conference. I had such a good time there. And meeting you, I was so happy to meet you. We got off really well. I just enjoyed talking with you so much.
Faduma Mohammed: Me too.
Jenn Creighton: And so when this came about, I was like, who do I want on this? Yes, yes.
Faduma Mohammed: I feel very honored that you invited me.
Jenn Creighton: It’s been great to have you and you’ve given us so much information. You know, this thought of maybe don’t say something if it’s not actionable on a code review, you know, that nitpicks maybe just don’t even bother with them. I think those are kind of standard in our industry now. And like, maybe we don’t, maybe we don’t. And I love this idea of pair review. I’ve never done that anywhere before. Fantastic.
Faduma Mohammed: Mostly, engineers need to chill and be nice to each other. I think that’s really the headline.
Jenn Creighton: That is the perfect headline. Thank you so much. So, happy to have had you here. Thank you so much.
Faduma Mohammed: Thank you.
Jenn Creighton: Y’all, how good was Faduma? How good was that? I am so happy she stopped by the podcast. She is definitely worth a follow on Twitter. I hope you had a good time. I hope you got something out of this. I will see you next week.
Top comments (0)