Formal definition

Code review is the systematic examination of source code,
and is intended to find mistake overlooked in software development,
improving the overall quality of software

But...

Everyone has an opinion

Are we harder on other people's code than our own?

We're harder on code while reading

The ratio of time spent reading code versus writing is well over 10 to 1.
We are constantly reading old code as part of the effort to write new code...
[Therefore,] making it easy to read makes it easier to write.

So, what should we look for
in a code review?

Different workflows

• Gateway reviews
• Knowledge sharing
• Early design feedback

So, again:
what should we look for
in a code review?

It depends

Code review anti-patterns

• Nit picking
• Design changes when code works
• Inconsistent feedback
• The Ghost Reviewer
• Ping pong reviews

Developers hate code reviews

Code reviews are
a massive waste of time (?)

Why (do you do reviews)?

• Ensure code meets standards
• Find bugs
• Share knowledge
• Check code is understandable
• Ensure code does what it’s supposed to
• Collaborate on design
• Evolve (refactor) application code

When (do you review)?

• During implementation?
• When it's ready to merge?
• After it's been merged?

When (the review is complete)?

• When everyone agrees?
• When a gatekeeper agrees?
• When all comments are addressed?

Who?

• Who reviews the code?
• Who signs it off?

Where?

• Pairing
• Showing code to a colleague at a computer
• Mob reviewing
• Remote screen sharing
• In the IDE, checking out a commit or branch
• Using code review software

What?

Requires you know already:

1. Why
2. When
3. Who
4. Where

What to look for (1/2)

• Fit with the overall architecture
• SOLID, DDD, Design Patterns or other paradigms
• New code follows team’s current practices
• Code is in the right place
• Code reuse
• Over-engineering
• Readable code and tests
• Testing the right things

What to look for (2/2)

• Exception error messages
• Subtle bugs
• Security
• Performance
• Documentation and/or help files been updated
• Spelling, punctuation & grammar on user messages

https://blog.jetbrains.com/upsource/tag/what-to-look-for/

How?

Human reviewers should be doing what cannot be automated

Understand the constraints

• Why: Knowledge Sharing
  Purpose isn’t to reject the code
• Why: Before merging
  Focus is on how easy is to understand the code
• When: at the end
  Too late for design, but be specific

Submitting for review

• Annotate your code
• Reviews should be small
  Commits should be smaller and consistent

Reviewing

• Should be clear Who is reviewing
• Respond in a timely fashion
• Checklist of What to look for

Comments

• Bear in mind Why, When and What
• Be constructive
• Be specific

Accept or Reject

• Accept or request changes
• Next steps should be clear

Making changes

• Respond in a timely fashion
• Respond to comments (or not?)

Resolving

• The goal is to accept the review (and merge?)
• Should be clear Who signs it off
• ...and When

Have clear ojectives

Clarity comes from understanding

• Why
• When
• Who
• Where
• What
• How

The Goal Is To Ship The Code

Not to prove how clever you are

Questions?