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

  • 803