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 some sort of code review process
built into their teams right now?
Not everyone, which is disappointing.
So that's what I'm here to talk about.
(...)
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.
And the secondary goal becomes
is 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 and so on
so all code goes to that person,
they review it, send feedback
and iterate on the code that way.
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 so 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 actually commit the code
send the patch over, send the dif over
and review it that way.
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.
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 when you're doing 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
when 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
to make sure
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.
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 to
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 to
avoid making that mistake again.
That's a very important thing.
You should be using that
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.
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,
the other thing you should try to
keep in mind is that
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.
(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?)
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 if
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.
(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?)
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.
(I have a small addition.)
(So one thing that also 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)
(and the easy way to speed)
(it up is to also just)
(I mean ignore JavaScript and CSS, because)
(Terrible idea)
(It is)
(but a lot of times it helps out)
(If you have a lot you can)
(just ignore CSS)
(If your quality assurance)
(team passes it and it looks fine)
(but JavaScript also if you)
(really don't have time)
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.
(Out of curiosity, how many people)
( here have had Mo review their code?)
(He's the best!)
How many of you have had
code rejected by me?
(How many people want to beat)
(Mo up after this meetup?)
(I'm in)
(Actually out code was reverted)
(like 2 min before the meeting)
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 at least once a day.
The interesting thing is that if you
have some sort of code review process,
we've actually noticed 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 to like
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.