This slideshow could not be started. Try refreshing the page or viewing it in another browser.
This Code Review Talk is Excellent but…
“[P]eer code reviews are the single biggest thing you can do to improve your code.”
Code designed by Nina Geometrieva from the Noun Project
About me: Mohammad (Mo) Jangda
– Toronto, Canada
– Code Wrangler at Automattic (WordPress.com)
– Ice Cream Fan
Who, in the audience, has some code review process in their organization or workflow?
Outline
– My history with Code Review
– Why bother?
– How to implement?
– Golden Rules.
My History with Code Review
Early in my career (school, various development jobs, freelance work, open source projects), I always felt like I was missing something.
Saw the Light
First* week on the job:
“So, let’s get you started by having you review this code sent in by a client!”
— Team Lead
Thrown right in and ~50% of my job for 3 years.
* after completing Automattic’s standard support rotation
I learned three things.
Lesson #1
Reviewing someone’s code is really scary!
Lesson #2
Getting your code reviewed is really scary!
Lesson #3
Extremely rewarding!
[P]eer code reviews are the single biggest thing you can do to improve your code. If you’re not doing code reviews right now with another developer, you’re missing a lot of bugs in your code and cheating yourself out of some key professional development opportunities. As far as I’m concerned, my code isn’t done until I’ve gone over it with a fellow developer.
— Jeff Atwood
— http://codinghorror.com
Common Excuses
– We pay our developers a lot of money. They should be writing perfect code. The need to review code means it’s not perfect. Imperfect code means we’re not getting our money’s worth.
– We don’t have time.
– It’s not my feature!
“the average [defect detection] effectiveness [rate] of design and code inspections are 55 and 60 percent.”
— Steve McConnell (Book: “Code Complete”)
Compared with 25-45% for various forms of testing.
(via Jeff Atwood http://blog.codinghorror.com/code-reviews-just-do-it/)
Development projects with code review have significantly fewer bugs, reduced development costs, increased productivity, and early ship dates.
(via Jeff Atwood http://blog.codinghorror.com/code-reviews-just-do-it/)
Warning: code review can be damaging if not done carefully.
Effective Code Review is a part of the development process and the culture of the organization, not just an afterthought.
Effective Code Review needs buy-in from everyone involved.
We always get push-back from developers working on the VIP platform.
Effective Code Review sets goals
Goals: Take Your Pick
– Learning other perspectives
– Becoming a better developer
– Becoming a better communicator
Goals: Take Your Pick
– Better Code Quality!
– Have at least one other person look at the code.
– Have fun!
Have established rules, standards, and process
Even if it’s no rules, standards, and processes.
Should define: what is the scope of the review? What types of things are we looking for? How will the process works. And communicate that with the team
What does a code review entail?
Looking over code to find potential issues or areas for improvement.
Bugs | Best Practices | Design Patterns | Faulty Logic | Confusing Flows | Opportunities for code reuse | Lack of standards/style | etc.
Culture, environment and workflow will change how you adopt it.
Code Review for Individuals
It doesn’t have to be lonely.
Individuals: DIY
– Do what (some) professional writers do!
– Write/Code in the morning => lunch => Edit/Review in the afternoon
– Freewriting?
Individuals: Find a buddy
– Open Source? Find volunteers at meetups/local community/IRC/Twitter.
– Closed source? Find another freelancer and trade.
Individuals: Offer reviews
Review open pull requests on GitHub (send messages to the PR author if you don’t want to ask or discuss publicly).
(May want to be familiar with topic or code in some form before jumping in otherwise start privately.)
Individuals: Pay someone
Airpair / Helpouts / etc.
Code Review for Teams
Find a flow that works for your team.
Teams: Who?
– Gatekeeper
– Peer Review
– Committee Review
– Pair Programming
– By-request
Junior vs Senior developers?
Teams: When?
– Pre Code Review
– Iterative Review
– Pre Commit Review (for centralized source control like SVN)
– Post Commit Review
– Post Deploy Review
Teams: tools?
– Specific code review tools: Crucible, Phabricator.
– Source Control: Github, Trac Code Comments, patches.
– In-person.
– Ad hoc: Email/IM/IRC/Snapchat?
– Review as Pull Request / bugfix (https://github.com/Automattic/site-logo/pull/14)
Tools: Don’t need to get fancy.
Reviewing a simple patch/diff can go a long way.
Code Review at Automattic
Varies by team:
– no code review
– team lead or peer review
– gitflow (someone else has to review and merge)
– Code Review P2
@Automattic: Code Review P2
Blog (P2) where any developer can post a patch or commit asking for feedback.
Code Reviews are “egoless”
It’s not a competition about how many mistakes you can find. Or who is a better developer.
Nope.
I’ve been programming for n years. I don’t need someone else to tell me what’s wrong with my code.
I’ve been programming for n years. I can point out all the things wrong with someone’s code without ever looking at it.
Don’t make it personal
Reviewers: The fault doesn’t lie in the developer; it lies in the code.
Reviewees: don’t get defensive about feedback you receive about the code. Fix your mistakes and learn from them.
Remember the goals of the code review: learning and making the code better.
Caveat: If the mistakes are continually repeated in future code, then it’s a problem.
Reviewees: you can only fool your reviewer once. Accept your mistakes, but work to prevent them in the future.
Communication is everything!
How you convey your feedback and discuss the code will help ensure an egoless and effective review
Communication Tip #1
No personal pronouns in feedback.
Bad
path-to-file.php@8237#L786
Why are you not sanitizing your GET value here? You should go through your code again and properly sanitize all instances where you’re interacting with remote data.
Translation
path-to-file.php@8237#L786
You! Yes, You! I’m talking to you! You suck!
Good
path-to-file.php@8237#L786
We should sanitize the GET value here per best practices. Any reason not to? There may other instances where we should take a closer look as well.
Bonus Protip
Wherever you’re inclined to use a pronoun, replace it with “the code” or a coding concept (variable/class/etc.)
“You should be…” => “The code should be…”
Communication Tip #2
Avoid the use of “but”; prefer “and” or no conjunction instead to maintain an air of positivity.
The overall logic is great but the function could use some refactoring.
vs.
The overall logic is great. The function could use some refactoring, though.
Don’t Make Assumptions
Reviewees: optimize for code review; write your best code because you know someone will critique it (i.e. so make it good)
Reviewers: Be critical; ask questions about decisions that were made.
Safe space to ask questions
– Reviewers are okay to ask about code they do not understand. Opinions are okay as well, although, better phrased as question as a means of discussion.
– Reviewees can ask questions ahead of time about things they are unsure of. This can be provide a good starting point for the review.
Avoid Pedantics
Avoid Pedantics
Don’t get too caught up in minor details but make sure development best practices and coding standards are being followed.
Automate code style/standards issues, if possible.
Avoid too much emphasis on small patches vs big ones.
Ask a programmer to review 10 lines of code, he'll find 10 issues. Ask him to do 500 lines and he'll say it looks good.
— Giray Özil (@girayozil) February 27, 2013
Not a Silver Bullet
Code review is not going to catch everything. Bugs are inevitable.
Think Positive!
Reviewers: Remember to praise good code and incremental improvements.
Reviewees: Remember to say thank you!
Have fun!
Jokes + GIFs + Emoticons
In Closing
Takes a while to learn the rules. You will forget to follow them. You will fall out of the code review habit. You will hate yourself and your coworkers and peers. But you will become better.
And it will pay off.
Want a cool job?
Automattic is hiring code reviewers, developers (PHP/javascript/node/go/whatever), Happiness Engineers, and more!
Work from anywhere! Unlimited vacations! Cool swag!
Talk to me or http://automattic.com/jobs/
Thanks!
