 Hello everyone. Hey, my name is Ashley Clojie. I'm assistant creative director at Boston University And today I want to talk to you about code review and more specifically making code review manageable I think as people who work with WordPress and people who work with the web Whether you're a designer developer producer anyone who produces content We all recognize the importance of getting feedback and getting people to weigh in on what we've done throughout our process But sometimes when it comes to code review People get a little like whoo about it. They get a little scared This talk is going to be about how we got past that at Boston University and implemented this in our interactive design team and How we made sure that code review was manageable not only from a business perspective keeping our projects moving and Keeping all of everything flowing, but also from a human perspective making a really safe environment to share in progress work So before we get into all that there's a couple of things you should probably know about our team First of all, we're sort of like an internal agency. So when we talk about the code review process it has to work in Project process of you know deadlines, you know, we're in sprint meetings We're presenting things to people on a regular basis and we're generally building themes from a Frameworks, we're not starting entirely from scratch but there is a lot of Development and iteration that goes on so our process really had to work with that Our department works mostly on themes and our theme framework So you may have heard about our friends at IS&T information services and technology a few of them are in the audience today So I'm happy to see you guys They actually work on our WordPress installation and develop most of our plugins We do a little bit of plug-in work, too But if you're going to ask me lots of big WordPress multi-network questions in our Q&A I'm not going to be able to answer them. Unfortunately. Do ask me theme questions and Finally, the last thing to know is we think about this presentation is all designers right at least some HTML and CSS and Contribute to themes. So as we talk about code review, we're not only considering our developers But also anyone who contributes to themes including designers web producers student workers things like that So the first thing that we had to answer when coming up with the new code review process Was why do we want code review for our team and there's a number of different reasons why you might want code review for your team It's different for everyone For us we started going through the benefits and we were trying to find one that really resonated with who we were So some benefits you probably know about our improved code consistency easier to maintain code catching bugs early especially from Perspective of you know the way different WordPress filters and actions are used or different libraries are used Code review and getting different perspectives from people is a great way to catch those kinds of bugs early or things that you might not have thought of Authoring thoughtful code you can be proud of that starts to get it something like who I like that That's a benefit that I think people can get behind But the biggest benefit that resonated with us and that really made sense for our team was sharing knowledge and teaching across teams Working at a university. I think you really have to buy into this idea of Learning and knowledge and teaching and sharing your wisdom with others and code review can be an amazing tool for that So as we thought about how we were going to structure our code review process, we decided that's the spirit We want to do it in so in our training I'm going to be showing some of our training slides in here so that you can see how this all worked out in the end Code review looked a little bit like this. This is how we defined it Code review is just asking someone to look at your work to help make it better It's an opportunity to give kudos and recognize the good work in your co-workers Code you can say hey look I see something that I learned from your code and that's really cool You know recognize that when you see it It's also a chance to help each other out. It's a chance to give back and say Hey, this is an area that I feel I have expertise in and I can see some places that you can improve your code Code review gives you that chance to teach It is not having perfect code all the time if you had perfect code all the time You we wouldn't be able to teach you anything code review wouldn't be doing its job for us So when we review code we expect to be reviewing in progress work And that was something really important for us to mention because we're asking a lot of people when we say hey Show something that's not polished yet show the general direction you're going in and let us give feedback for that It's not judging your worth as a contributor and this goes along with not having perfect code, right? We expect it to be rough. We expect there to be things to be improved done. That's why code review exists So when we find those things we're not saying well, why didn't you get this? We're explaining why? The improvement would be helpful and you know sort of the reasoning behind it and Finally code review is not one-sided when you're reviewing We really want you to explain the why behind your recommendations And if you're you as someone who's being reviewed wow, excuse me If you as someone who's being reviewed don't understand the why behind their reasoning We want you to ask that question. We want there to be a healthy debate because without that healthy debate and that Constant conversation that happens during code review All review would be would be sort of like a dictatorship and that's not the spirit of what we're trying to do here We want to come to the best solution together Code review lets us teach so this is a screenshot of where we do our code review the tool. We use github One of our fellow designers MJ is teaching me about flexbox Something I didn't know about it and he's doing it through line by line comments, so it's really easy for me to see And it lets us work with neat people Projects and stuff that we don't normally get to do so this is an example of a poll request that was opened by one of our fellow members in IS&T and Carlos he saw an opportunity to fix a bug in our framework that had been really kind of troubling us But we hadn't had a chance to get to yet with this code review and pull request process in place He was able to contribute to that and that meant that I got to work with someone on a different team and give feedback To someone on a different team and we shared knowledge about what the framework is like and the best way to approach that That's really cool Basically you get to help make things better and try new things which is amazing So as you can see defining the goals for code review helps that really clear expectations and the tone for contributing When we were training all of our designers and developers on this new process The second thing that we had to do was pick a good workflow This is tougher than it looks When we started looking at code review for our team all we were doing was in custom themes Everybody would commit directly to the master branch And this was kind of a byproduct of originally being on subversion That's kind of the way people had worked for years and years and years and it worked But if you're all committing directly to the master branch there's no real way to do code review in an easy way and No way to prevent things that are not good going into the master branch So we needed to pick what that workflow looked like and that means picking a branching model So how do we pick a branching model? For us, we were like get flow. We're gonna do get flow We love get flow the reason why we picked to get flow initially was because it was what we were familiar with I had been taught get flow in the process of working on our parent theme framework and our senior developers were Used to get flow from other places. They've worked as well as working on plugins at Boston University So give flow seemed like a natural thing to try for our projects as well But unfortunately what we found is it didn't quite work like we thought it would for the custom theme development and project process And this stumped us for a bit because even I got confused when we applied get flow which is Sort of branching model that's very built around this idea of features hot fixes and releases To a project and the reason why we realized that this wasn't quite working is because these projects that we had Applied get flow to our framework and our plugins. They were sort of long-standing Repositories with a lot of history. They were very stable and we are generally iterating on our changes in there Whereas with a project Where what we tend to be doing is working on a custom theme from start to finish So we're building a whole website and we're iterating very very quickly on things and things are often broken And we're fixing things all the time throughout the course of a few months This type of rapid development didn't work well with get flow because it felt like everything was a hot fix, right? you know, you've got bug here you got bug there and You know three-spits for a means layer you realize you completely blew something up people quickly abandoned merging things back into develop When creating their fixes and so what would happen is master would be ahead of develop There'd be a branch over here that was stale people just got really confused So what we did was we said, okay? This isn't stable yet. This isn't we wouldn't even consider this a release yet It's very in its beginning stages when we're doing this sort of development And we simplified our branching workflow for these types of projects only our custom themes to just having one master Branch and then having people build feature branches or you know pick a branch off when they're trying to solve the problem And then just merge that right back into master no develop branch None of the release branching process you might be familiar with from get flow Once we did that things became a lot easier and they were a lot less confusing for everyone So the moral of the story there is to pick a branching model that makes sense for how you work and that's going to differ project by project and By the different problems you're tackling we weren't tackling the same problems in our custom themes that we were in our framework in our Plugins, so we needed to pick a different workflow that worked for everyone The second thing that we needed to tackle was making sure that we were using people's time really effectively One of the major concerns about bringing code review to our process was that it would take a long time and People would be spending a lot of time reviewing and making changes from it and we wanted to get ahead of that So the first steps we took towards that we're picking Linting tool and we chose code climate because it runs on GitHub on every branch push It'll check every all of your code and for your coding standards and make sure they're all in line It basically looks at all of your standards and picks out the stuff. That's easy for a computer to see that stuff like spacing conventions naming conventions Even some things like you know code complexity It'll pick up stuff like that the things that you don't want to be doing over and over and over again on a review We let code climate do that job and then we instruct everybody as they're reviewing to do things that a computer can't do like discuss approach Find ways to make code easier and more maintainable in the future and watch for ways to share and improve code across projects There's no way that code climate is ever going to be able to figure that out So we really want people to make that connection and to look for that in their reviews Once we had a loose process to find the next step was to test and refine it This was a really tough part of the process This is where we had to sit everybody down and start explaining the basics of git And it meant sitting people down and saying okay. This is what a branch is. This is how branches work This is what emerges and this is what that looks like It meant that people tripped over each other during merge conflicts and we had to teach them how to resolve the merge conflict That you can't just take your own code That's not necessarily the source of truth You actually have to look at both the originating branch and your own branch and see and Read the code and understand what it's all doing together to figure out which parts you should keep from each branch while you merge It took a lot of time. I won't say it was easy But with time and refinement it was manageable The first project that we did this with it was a lot of desk sides And it was a lot of just sitting down and making sure people were comfortable and had the right sport going through the process That said we got a ton of good information out of it So some things we found while testing out the code review process Firstly, there was a lot of confusion about when to open and close branches This was particularly true for designers and I come from a design background So I can speak to some of this the way that we typically work at the beginning of a new site build is We work in a very global way. We're painting broad strokes So I might say oh, I'm going to set all of the colors in the typography And then I'm going to do the site navigation and then I'm going to do the footer things that make the site the whole site feel like it's really progressing but what that led to was Hundreds and hundreds of lines of CSS that were really tough to review and in some of those there needed to be some PHP Adjustments some light markup adjustments and that caused a lot of tension between our designers and developers Because our developers were more used to working in smaller more focused branches That were really Reusable and componentized so we'd have a developer going in double-check our PHP work Which we were stretching our legs a little bit in and you know Just trying out some new things and getting the feedback that we needed and they would be like whoa What is this 500 lines of CSS doing here? That's a lot to look at So that was something that we really needed to solve in order for this to work out and be sustainable in the long run Response time was critical if people didn't get to pull requests they and Their review requests weren't being looked at within a day or so they got really frustrated And even a day is a really long time to wait for a review Because reviews if you're waiting for that kind of feedback and you can't merge your branch. It's holding you up We created some Ways in our workflow to work around this Right now our recommendation is if you're waiting for something in review And you have to start on something that depends on it You're allowed to branch off that original review branch and then once The stuff in the branch that's under review it gets merged into master And then you pull it back into the branch you're working on now It all sort of resolves itself. So there are workarounds to this But ultimately the idea was to solve this issue of response time in order to get past this Large code reviews were really hard on both the reviewer and the reviewe. So like I said when I First started this process out all of the CSS code reviews were going through me and that meant that I would get a 500 line Bunch of CSS I would review every single line and there'd be a lot of repeat things that somebody would have to take care of There'd be a lot of feedback and then the poor person who wrote that CSS would get all 25 of my comments all at once and suddenly be very overwhelmed by how much they had to fix and There's nothing like getting 25 comments on your lovingly crafted code and things you have to fix to make you feel like you just are not very good at your job so we really really need to fix this and Figure out how to get people to work in a smaller fashion Interesting tidbit without guidance people go to extremes to fix linting errors the little red X next to code climate That says you're failing your checks is a really strong People don't like seeing that red X. So sometimes what would happen is we'd have something that would fail our Coding standards for example We follow WordPress coding standards and the CSS coding standards say that you should be hyphenating your classes and not using Underscores in there, but we also use gravity forms and gravity forms has underscores in its classes So what would happen is put climate would be like why you got these underscores here But go fix them and then designers would like kind of tentatively come up to me and be like Ashley, how do I fix this though? Do I add a new class to it? I'm like no no, that's completely out of your control It's okay to leave that there You don't have to have everything perfect for it to merge We just want to do the best that we can and start making our code base more clean and consistent There's gonna be times where there's things you can't fix and that's perfectly okay Finally we found that very direct feedback was easily misinterpreted as negative So this is something that you may know from sending an email when you are maybe a little more frustrated than you wish you were It's really easy to misinterpret tone and text and what would happen is sometimes people would get a review They'd be in this very vulnerable state where they're showing their in-progress work And they're not sure about what's happening and then they would read this very direct review that was perfectly fine It was it was just saying you know what needed to be improved But it the tone would easily get misinterpreted and they get really frustrated So you might have heard a couple of common themes in there one of the bigger ones being getting people to reduce their branch size and Telling them when to open and close branches So that was something we had to figure out how to explain to everyone How you explain when to open and close a branch? In our training how I did this was I used a site that I had been working on as an example To show how we open and close branches. So this is the Boston University Dining Services website It's actually live now. I was working on it at the time and The way that I explain this is I was like, okay when I started on this website You know, I really wanted to make those kind of global changes that would when I went to my first sprint meeting with the Dining Services people I could say wow look at all the great big work. I've done. So I started with the initial site navigation styling The second sort of little bit labeled on here. I said, okay I'm gonna focus really really hard on this one problem initial site navigation styling they get perfect I'm gonna you know make sure it's all nice and working well and then I'm going to close that branch And I'm going to pick another problem to work on Maybe something like styling a banner template. I Went through each of these pieces my mock-up and the thing that I really hammered home Was that you're going to open and close branches every time that you start a new problem? and that's really important to Distinguish because the immediate question I got when I talked about branching and working in smaller chunks was well Do I need a branch for every single commit I make which is you know? Probably sounds pretty silly to us, but it's a reasonable question and the honest answer is bald Yeah, you could have a one commit problem. That's maybe this pull request up at the top You know fixes incorrect icon in the site light alert That's probably one commit just to change that little icon in the CSS The big idea is that we're opening and closing branches based on problems not a number of commits not a specified Number of lines and we want to try and keep those small Problems small and easy to maintain of course this required a shift in thinking for many people including myself So what I really made sure to do is I was explaining this was empathize with people and say okay You have to change your thinking now. You have to change your workflow, but I promise it will be better in the end And the way that I said that is I was like look this is really tough to get it first But once you do it helps focus your changes for sprint meetings I know exactly now when I go into a sprint meeting and present to a client What components of the site are finished and I can really direct their attention to those things? It prevents the meeting from going on off on all different directions because I made a very sort of global styling decision and Didn't say okay I have the navigation and the footer done today, but we're not going to look at the content at all Focusing those changes keeps meetings on track It also makes debugging easier in the history of your repository when you use pull requests like this and you Makes things small and manageable and you focus them around problems When you blow something up three weeks later and something you know picks it here You're like oh, oh, I remember I worked on BU banners way back here It's really easy to jump back to that point in history and take a look at all the changes you made there That helps with debugging so much because when you're committing directly the master or when you have tons and tons of changes of all in a single branch You're not able to see just the lines of code that might have affected that problem Finally it helps with documentation So I don't know if anybody else here has looked at their code from five years ago and said oh my gosh What was I thinking I find myself in that position a lot? But when you're saying oh my gosh, what was I thinking there's an answer to that when you're opening and closing pull requests like this And you're doing code review and it's all in the discussion around that branch So now when I go back to this repository of five years I'm like oh why on earth did I do this on the dining website? I can see exactly why because I discussed it with other people and that'll keep me from having to re-research and read Read and think through my code and waste time doing that instead. I can get right to fixing whatever the root problem is To address the time issue we created a central place for all teams to request review For us that took the form of slack We created a new slack channel called code reviews and then we created user groups that people could quickly ping So that they got the feedback that they needed at the right times These user groups include the things that you would probably think of Job script feedback sass feedback and PHP feedback for all the different languages we're writing in But it's also worthwhile to have different perspectives on your code, right? So we created a few other user groups that provide a different perspective regardless of what language you work in So for example, we have a plug-in feedback group and that's a really good group for that Instruct people to ping if you want to understand how some work you're doing in a theme could later translate into a plug-in that Works across all things, you know, these people are experts in making code that works across many many different themes There's a WordPress core feedback group for people who are really interested in WordPress core and want to see how they can Future-proof their code later This works so much better than manually assigning people in code reviews, which is what we started with in github Because it allows people to grab code reviews during their downtime and when they have time to do it This sped up response time immensely I was going for one day in The response time for code reviews now It's more like an hour people get to it really really quickly with this setup finally to address some of the concerns with tone and making sure that we were Providing a really consistent great experience for everyone. We created a code review etiquette guide and that really helped us Make sure that we were providing a positive experience for everyone and that we set some expectations for how reviews should be And that looks a little bit like this. There's a longer version at the end They'll share my original training slides then But the big main points were keep your review requests All right, thank you to our sponsors All right, I already talked about why we want to keep our review requests small and focused I won't linger on that We do want to try and get to review the same day is requested at this point with more experience in this I would say within the same half day or a couple of hours We depend on code for our meetings For our user experience to address bugs that are happening. We really want to try and get to these quickly Be clear about what needs addressing When we were originally trying out this code review process What would happen sometimes is in github people would just comment on a code review And they wouldn't explicitly say approve changes or request changes And this is really troublesome because if you lock down your branches and your repositories, you know to enforce the code review process Commenting doesn't allow you as the author to do any sort of merging Or actually finish up the pull request. You have to keep pinging people and be like, okay I made the changes. I made the changes. Can I can I merge now? It's much better to explicitly approve or deny a pull request and say why and what things need to be addressed to change Status that makes it really clear for everyone and it prevents people from being bugged again again and again For the same sorts of things just to merge their code in Review with empathy. It's really hard being a new person in an organization. It's really hard showing your in progress code It's really hard being a junior developer or designer and seeing all of the people with great knowledge around you Mentor you You recognize all of these things and you recognize who you are in your level of knowledge And we want to make sure that we're thinking about that as we review So we ask that everyone reviews with empathy for project timelines for meetings that are coming up for the Feeling that you might be having in creating this action and we do this by encouraging the use of very liberal use of emojis Which sounds super super silly, but it's a great way to say hey, you know, this is direct feedback But smiley face. It's not negative You know simple addition that really helped clear that up or even just pointing out the good stuff and the great things that you're seeing in code and Finally review with common sense if the pull request is happening right before a meeting Right before QA right before something where the code base needs to be really stable That's not the time to insist on refactoring a bunch of JavaScript That's been in there for a while and is maybe pretty messy We do want to get to that and we do want to fix those problems But that's not the time to do it You want to make sure that you're acting in the best interests of the project and the humans behind the project And asking them to totally refactor something in that situation is not in the best interest of everyone It's just going to make the person stressed and the project more unstable So all in all it worked out really great, but we did have some cool surprises come out of it First of all as I said a big concern was the amount of time that this would add to our projects We found that code review actually only add about 2% extra time to our projects And this is literally just the time spent reviewing and making revisions based on reviews It doesn't count the time that you save in that consistent code in the maintainability of your code in catching bugs early So although it does add extra time to projects I think in the long run it definitely saves time and it's also improved our communication across teams Not only as interactive design adopted this but as you saw in my original example our IS&T friends adopted it too So we're constantly in that code reviews channel reviewing each other's code and talking to each other So much more than we were before Everyone including designers onboard just fine with the right support I'm really a big believer that you can teach this process to anyone as long as you are mindful of the workload that's being happening of the actual Work that you're doing as long as your process is well-defined for that anyone can learn this So don't assume just because We're not working with developers here that Other people can't learn it is basically what I'm getting at And now everybody asks questions before they get too deep into their code And that's a really cool benefit that I hadn't anticipated people are now coming to me to other developers to other Designers asking hey, can you give me a pin opinion on this direction or what's the best way to do this? That ensures they're in the right track room before review And it keeps them from going off and spinning their wheels in an odd direction It also like I said improves our communication, and that's really awesome So this is all great and fine and as far as the actual timeline for this we went from committing to master to Having this whole code review process all together and about a year It took about a year to teach everyone how to do this, but even that whole year There was still a lot of foundational work that had to happen. It didn't happen overnight Over the past four years we learned get together Someone had to sit us down when we were originally on subversion and say this is what get is this is how you're going to use it Now we're committing to get and they sat us down, and they taught us all get We moved our themes to subversion that was a baby step, but it was a big baby step just getting our Custom child themes right and to get and saying this is how we're going to work from now on that was a step that we had to take Someone had to teach me how to contribute someone had to teach me the get pull process, and they did they were very patient I kept opening bugs on our parent theme framework repository, and they said you have a lot of good ideas I think you should fix them yourself So they sat me down and they went through the process of teaching me this and if they had never done that if they Had just gone on and you know kept fixing the bugs themselves. I never would have been able to teach this to everybody else It took four years for us to get ready to change But the results are worth it today everyone uses get Everyone opens pull requests and everyone can contribute and when I say everyone I really mean everyone on our team So this is a screenshot of our responsive foundation repository It's basically a front-end framework that powers our WordPress themes But also static sites and a bunch of other web projects that we work on throughout the university This is a really lonely repository for a while It was a lot of just me committing to it and committing to it and committing to it And I really don't like coding by myself But what happened after we ran department-wide code review training and got everybody into this process is we got four Whole new contributors, and that was amazing We got two newly designers contributing we got a web producer Contribute we even got a student worker to contribute and these were like you know a little little eddy bay changes But some are really great big awesome features too So it was worth the work putting into it to get four new contributors in my mind I don't have to be all alone in this repo anymore Basically All I if I say nothing Else today. I want to say that change is a lot of work, but it's totally worth it Even if it takes years even if it takes baby steps to get there You're going to do an amazing job, and I hope that you Iterate and you try little things and that you put things into place and test them out and get feedback from your team Because if you can do that you can create change and it will eventually stick and totally be worth it And you'll learn a lot of really cool things on the way, you know Don't wait to be perfect. You're allowed to learn while you're learning new processes So thank you so much for listening My Twitter handle is at Ashley Cloj and I have I'll tweet this link out as well as the slides for the training that this is based off of Are there any questions that I can answer? There's a lovely microphone right up here that you can ask them in so I don't I can actually hear you That's an excellent question. We struggle with that too we we slap Trello and We get feedback in all kinds of places even email The way that we're still working out Kingston that process But the way that I tend to think of it is Trello is really good for on-the-fly feedback from clients and development But anything any sort of issue with code that may last into the future that you can't Take care of right away You can you should put that in GitHub you want that record next to the code so that later on down the line when you can't address it You can understand it Other ways to kind of help with that. I would say there's power-ups in Trello that can help you connect the dots That's been I think that's an unfair question for my talk. I'm just gonna say Anyway, there's ways to connect the dots we're looking into Xavier too Which is a service that connects different tools like that to help keep the connections together if we do want to Continue working in Trello and then GitHub and several different places for different needs Any other questions? I love Rachel cherry. Oh, I didn't even tell you Rachel cherry Is the director of WP campus and she has amazing swag Well, I've been in that instance before I have very opinion. I have a lot of opinions on code and You know sometimes people don't Agree with me and that's okay Other people do too, but by setting the tone I think in this training and saying look we're coming from a place of learning and that's our goal with code review We've been able to get ahead of a lot of that. So people are very Diplomatic in their discussions now. They say hey, this is the way I think it should be and they'll the explaining why that part of the learning process that really helps take away a lot of the misunderstandings that happen with budding heads and code and maybe just having Opinions come out like oh, I think it should be this way and leaving it that that's not in our spirit of code review We don't do that. So, you know when we say you should question that if you see it that helps get ahead of those issues and Diffuse them so they end up being a great discussion instead of an argument Yes. Oh, yeah So the follow-up question was if there would be any sort of resources to how we do this, right? Like our rules around this in this in this slide deck you can see this top link is actually the training This is based off of I wish I had an actual real-style guide for it But I'll tweet out the link for that as well and that has a lot of those and at this point We're out of time. So thank you very much for coming to my talk and listening to you