The Good,
The Bad,
And the Code Review

Manu Castrillón
Universidad de Antioquia
Medellín, Colombia
@manucastrillonm

A
Code
Review?

Benefits

Learn and share
All the peers can explicitly know what functionalities are being added, updated or removed
@manucastrillonm
Raise team standards
Committers will tend to produce a better quality code, because they know someone else will be examining their work.
@manucastrillonm
Set the mindset for an open company culture
By introducing the process to give and to receive feedback in our natural daily workflow.
@manucastrillonm
Reduce the cost of fixing bugs
The cost of fixing a bug increases exponentially over the time in a development workflow
@manucastrillonm

What
To
Look For

- Design: Do the interactions of various pieces of code in the PR make sense?
- Functionality: Does this CL do what the developer intended?
- Complexity: Is the CL more complex than it should be?
- Tests: Ask for unit, integration, or end-to-end tests as appropriate for the change
@manucastrillonm
- Naming: Did the developer pick good names for everything?
- Comments: Did the developer write clear comments?
- Documentation: If a PR changes how users build, test, interact with, or release code, check to see that it also updates associated documentation.
@manucastrillonm
Good
Practices

For all

Don't take things personally
Is not a "dev vs. peers"
@manucastrillonm
Use Checklists
"...we know, there are known knowns; there are things we know we know. We also know there are known unknowns; that is to say we know there are some things we do not know. But there are also unknown unknowns -- the ones we don't know we don't know."
- Donald Rumsfeld
@manucastrillonm
For committers

Provide context
-
Ensure your commit message explain what are you trying to achieve and how.
-
Link any related tickets or issues.
-
Comment your code.
@manucastrillonm
Keep the code short
Beyond 200 LOC the review effectiveness drop significantly, more than 400 it's almost pointless.

@manucastrillonm
Push commits based on earlier rounds of feedback as isolated commits to the branch
@manucastrillonm
For reviewers

You must understand the change you are reviewing
@manucastrillonm
Don't waste your time
There are plenty of tools for checking style and formatting issues.
@manucastrillonm
Everyone should review!
@manucastrillonm
Code review for short sessions
No longer than an hour

@manucastrillonm
Reviews
Communication

For all

-
Be humble
-
Keep it real
@manucastrillonm
For committers

-
Try to respond every comment
-
Be grateful for the reviewer's suggestions
@manucastrillonm
For reviewers

-
Judge the code, not the developer
-
The code is not 'his/her code' is 'our code'
-
Ask good questions. Don't make demands.
@manucastrillonm
Thank You

@manucastrillonm
The good, the bad and the code review
By Manu Castrillón
The good, the bad and the code review
- 939