Mo Jangda from Automattic gave a presentation and lead a discussion on Code Review at a recent WordPress Big Media Meetup in New York City now with full transcript.
So to get a sense of what the room looks like, how many people here are developers? So the majority of people, probably 70-80 per cent. How many people are editorial? A few people, that’s good. How many people are management? You guys in the suits.
If you’re a developer, how many people have a code review process built into their teams right now? Not everyone, which is disappointing. So that’s what I’m here to talk about.
The goal should be to have at least one other person than the person who wrote the code look it over and to review it for things like quality standards, security concerns and performance and so on, things like that.
You can’t code review if you’re doing it live. So, ultimately the goal with code review is basically to improve the quality of your code.
So if you’re editorial, right, the idea is that you’re not going to push something out any sort of published articles without going through the copy editing phase, unless you care about money, in which case you don’t care.
You want to make sure the stuff you’re putting out is good quality, and that’s where code review is – that’s why it’s sort of nice to do.
The goal should be to have at least one other person than the person who wrote the code look it over and to review it for things like quality standards, security concerns and performance and so on, things like that.
So it doesn’t necessarily have to be a junior to senior. It’s a learning opportunity for both, different skill sets as well.
And the secondary goal is you sort of becomes that you’re learning from each other as developers. It doesn’t necessarily have to be a junior developer passing off their work to a senior developer and them telling them basically everything they did wrong.
It can actually go the other way where a senior developer passes off their work to a junior developer and says here’s the code I’ve written,can you look it over and see what I’m doing?
It gives the junior developer an opportunity to look at how the code is being written by the senior developer and learn from them, right?
So it doesn’t necessarily have to be a junior to senior. It’s a learning opportunity for both, different skill sets as well.
The other thing to sort of keep in mind is that code reviews shouldn’t be something else that you tack on. It’s part of the development process, right?
So it’s not something that you should consider a tedious thing that should be going on, it’s something that you should have built into your processes as part of your deployment, as part of your coding, so something you do on a regular basis.
There’s different ways you can do code review obviously. You can have a gatekeeper-type approach. Where you have one person who’s sort of like master of the code review, so every bit of code that gets written goes through that person.
So that person can be a senior person, can be a rotator role so all code goes to that person, they review it, send feedback and iterate on the code that way.
One flow, or one specific approach to code review is not going to work for everyone. So you’ve got to find something that works for you.
You can do some things like peer review, where you have teams of developers put together, so one person is buddied up with another so any time a person needs feedback, anytime a person is finished working on a patch or working on a new feature they can pass it off to their fellow developer and say, hey can you give me feedback.
You can have a committee-based approach, where you have multiple people giving feedback on patches. If you’re using something like Git or GitHub you can use pull requests. GitHub makes it really easy to comment on code, and pull requests and commenting and so on.
You can also do pair programming, which I personally dislike, but it works for a lot of people if you’re into extreme programming or agile processes and stuff like that. You can have people working on code at the same time and the cool feature of that is that you’re essentially doing code review live because you’re questioning each other as you’re writing code.
One person types up something, the other person sort of questions: Okay, why did you write that?
There’s different points in time where you can actually use code review so you can actually start doing code review before writing a single piece of code and you’re essentially talking out the concepts with each other.
You know, we planned out the specs and functionality, we’ve thought about what my classes are going to look like, what my functions are going to look like.
So it’s a good opportunity to actually talk with you, what approach you’re taking to code review, to talk with the person with whom you’re doing the code review to see you know, does this make sense?
Obviously you do things like pre-commit reviews, look at patches, so before you even commit the code send the patch over, send over and review it that way.
It’s a great way for your team to actually write better code and build a more cohesive unit.
We also do post-commits, so once the changes are done, committed, you can review the pull request or the actual committed code.
You can also do post-deploy, so if you don’t actually have time, to do a full code review before pushing it out, you can still go back and do code review on stuff that’s pushed live and make changes over time.
The most important thing obviously is you got to find a flow that sort of works for your team.
So one flow, or one specific approach to code review is not going to work for everyone. So you’ve got to find something that works for you.
Doesn’t necessarily have to be anything very specific. It can be sort of totally casual, like a conversation between two developers.
So it’s pretty important to find something that works for you. You also don’t need fancy tools, you don’t necessarily need to go out and get your own license of Phabricator, which is a code review tool that Facebook has built up.
To be honest, it can just be as simple as two developers passing around a patch to each other. Looking it over.
So it doesn’t have to be complex, but if you want it to be complex, you can.
But you can keep it simple, find something that works for you and so on. When it actually comes to doing the code review, there’s a few things to keep in mind.
Throw your ego out the door.
The most important thing, and this goes for both the person reviewing the code and the person receiving feedback is throw your ego out the door.
There’s no such thing as ego and emotion during code review. That’s the most important thing to keep in mind.
The reviewer is not smarter or dumber than the person whose code they’re reviewing. In the same sense, the person whose code is being reviewed they’re at the same level. So ego is not involved, it’s not supposed to be personal. It’s supposed to basically be about questioning the code, right?
Not questioning the person. So one thing that I usually try to keep in mind while I’m personally reviewing code is that I usually recommend to people is that when you’re phrasing your feedback never include the word “you” .
So it’s not YOUR code, it’s THE code. Right? So that takes away the personal aspect of it and it makes the reviewer feel less attacked.
Because getting your code reviewed can be a very scary thing, but it shouldn’t be.
You should get to a point, where you should actually be proud to have your code reviewed and proud of the code you’re presenting to your teammate, senior developer or boss and so on.
To show that this is the amazing thing that I’ve done, and you know what, I expect there to be flaws.
Chances are, there might not be, but you know, if there are issues with it, it’s something you know, the mistakes that you find, is something that I can learn from.
The other thing to sort of keep in mind, is that you as a reviewer want to be critical about things, right? So question the decision that the developer is making.
Why did they name that variable that way? Is that a valid variable name, right? Why is this function name so long? Can this be abstracted? Right? So design decisions like that can be a good way to root out potential problems in the code.
But obviously, you don’t want to get too caught up in the minor details, so getting caught up on spaces over tabs which we’ve had problems in the past with before, in VIP, where some developers would reject code commits for using space instead of tabs, you know that’s a minor implementation don’t get too caught up on that.
Point it out, but it’s not going to be a blocker. But it’s important that coding standards and best practices are still followed, so it’s important that your team is following those that in your reviews, you’re actually flagging those and so on.
The other thing to keep in mind is as a reviewer, don’t worry about catching everything. ‘Cause you’re not. I don’t mean to brag, but I think I’ve reviewed about 20,000 commits on VIP. Of the many commits that I’ve reviewed on VIP there’s been stuff that I’ve missed and that’s naturally going to happen.
Because manual code review is not going to be perfect. Automated code review is not going to be perfect.
There’s no such thing as ego and emotion during code review. That’s the most important thing to keep in mind.
Things will get missed, things will go live that will break your site. But that’s, an opportunity to learn from so the next time you review your code you’re going to be extra conscious about it and try to find that mistake that you made and try and prevent it from happening again.
So that’s the other thing as a reviewee, you should sort of keep in mind. When a mistake is found, and pointed out to you, you should try and avoid making that mistake again. It’s a very important thing. You should be using it as a learning opportunity, right?
If you are making the same mistake over and over again. Chances are there’s something wrong. Either with your processes or with how you’re developing. That’s something you should work to change.
Never focus on the negatives.
As a reviewer, if you find the same problems over and over again, that’s when you need to question what’s going on with this developer? Why are they not sort of picking up the problems that I’m seeing? Another thing I sort of do, and I mentioned this earlier with not making it personal, try to keep in mind try to stick to the positives, right?
Never focus on the negatives. What’s a good example? It’s important to start a phrase with: How can we do this better? Instead of this is dumb, this is stupid.
So again, avoiding the personal attacks, trying avoid getting too emotional about things and staying focused on this code could be optimized more instead of this code will break your site, things like that.
I think that just about covers it. That’s sort of one of the biggest things I wanted to go over. But also, I guess it’s ultimately important to find something that works for you.
Code review is, it’s something that’s near and dear to my heart because I’ve been doing it for the past three years as a reviewer, it’s the best way to learn best practices and learn new code.
I’ve actually learned more new WordPress functions by looking at other people’s code than I have through just following news and following WordPress codex and things like that.
So it’s a great opportunity for reviewers to sort of look at how other people code or how other people think when they’re approaching problems.
And as a reviewee, it’s a humbling experience and a great opportunity to learn. And ultimately, it’s a great way for your team to actually write better code and build a more cohesive unit to ultimately do cool stuff.
Thank you.
Q: It’s easy to think that if you don’t review everything, then you should just give up because if you’re not reviewing everything, then the problem is going to be in the spot you didn’t review.
So, I think that maybe some of the thought process behind not everyone here doing code reviews I think it’s something that I think everyone wants to do so I was wondering if you could provide tips is there any sort of lightweight code review process? Is there anything that doesn’t require every commit to be reviewed by somebody else?
A: So that’s where I would look at post-deploy commits or post-deploy reviews so reviews don’t necessarily have to block things from going out but you still take the opportunity to go back at some point.
Let’s say you have a work week, you set aside two hours in the week Friday or something where you as a team, get together and review each other’s code, and look at various commits and stuff.
So to give you an example, on WordPress.com, the platform, we have about a 120 or so developers. I may be fudging our numbers. We have a lot of developers pushing out code daily. We probably have anywhere from 60 to 100 to 200 commits going out every single day and we haven’t really actually found a good mix, or good flow for us what makes sense for a code review.
So we end up relying on post-deploy code reviews where the idea is certain developers will take some time on the side, a couple hours a day, a couple of hours a week to sort of look, skim through commits that people have done.
They’ll just look through the log to see a commit message or if there’s a particular feature they’re interested in or that they’ve worked on in the past to sort of give it a quick skim and see if there’s something that they can flag for the developer to work on.
So it doesn’t necessarily have to be a very time intensive or blocking process. Ideally, you want to get to a point where it does become integrated into your development flow but it doesn’t necessarily have to.
To be honest, spending even an hour a week is a perfect starting point.
Q: Is there a good size team for doing code review? Like if you have fewer people,are there better methods? Like for smaller teams, to do it so it’s not interfering, so it’s not just like back and forth constantly?
A: Again depends on your type of workflow and your team dynamic.
In that case, it’s like if you’re using like Git for example it’s a simple pull request is probably a great way to go.
If you’re working on a feature, send a pull request and have the guy sitting beside you say can you take a quick scan. Or if there are specific things that you are worried about, have them look it over and point out specific things.
Like I’m not really sure about this particular function, I’m not sure about how this class interacts with this feature, can you just take a look at it.
So it doesn’t necessarily have to be you’re reviewing the whole commit, just skimming through something and seeing if anything jumps out.
Participant: I have a small addition. So one thing that I find really helps out if you have a definition of the process so for example if you have code review documentation, like what kind of code structure you need to keep, templates defaults you need to use also speeds up a lot of reviews because you can automatically assume the person is following the structure, because they technically know about it.
The easy way to speed it up is to also just I mean ignore JavaScript and CSS, because if you have a lot you can just ignore CSS. If your quality assurance team passes it and it looks fine, also JavaScript if you really don’t have time.
A: I mean for CSS especially, as a code reviewer when I look at the VIP code, I basically ignore CSS, because from my perspective, I care more about the security and performance.
So you’re right, there are things you can sort of ignore, if you are a designer and like reviewing CSS, then go for it. But also, like you said, standards are really important, and processes are really important.
Especially, as you grow as a team, again doesn’t necessarily have to be super formal and super strict, but it helps to have some sort of definition in place so you can follow, your team knows what to do.
So they’re actually trying to have their code reviewed, instead of pushing it live.
Steph: Out of curiosity, how many people here have had Mo review their code?
Participant: He’s the best!
How many of you have had code rejected by me?
Participant: How many people want to beat Mo up after this meetup?
Participant:I’m in
Participant: Actually out code was reverted 2 min before the meetup.
What I usually tell people at like VIP workshops and stuff is that your ultimate goal should be to make me so happy that I would never want to revert that code ever.
We actually revert code once a day. The interesting thing is that if you have some sort of code review process, we actually notice when VIP’s adopt some sort of code review process, internally, so they have someone on their team review their code before it comes to us.
The quality, there’s a significant increase. The number of issues we have to flag, the number of reverts that actually end up happening to that code goes significantly down.
It’s a testament that code review can actually work and keep us happy and keep your code getting deployed much faster.
If you’re not on VIP, still a great opportunity to make sure your team is working together, make sure your team is writing good code. ‘Cause the last thing you want is to push out a security hole and all of a sudden has your homepage hacked the Syrian army or whatever.
See the presentations from previous Big Media & Enterprise WordPress Meetups. For Big Media & Enterprise WordPress Meetup groups in other cities, see the full list on VIP Events and join your local group.
Want more information about WordPress services for media or enterprise sites? Get in touch.