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