Practical

Code Reviews

Ilko Kacharov

Experience

Lead Dev @Proxiad / @Mailjet

 

11+ years in PHP

Zend Framework Certified Architect

Engineer in communications

Why

should we bother

  • Loosing developer's time
  • No short term ROI
  • Inefficient for small teams
  • Arguing over formatting and code style
  • Introduce unstated bussiness requirements
  • Tension and problems in the team
  • Highly subjective and might get personal
  • Missing milestones if done late

Why

do we need the process

  • Reading code gets you in the mood for coding
  • Keeps you up to date with the codebase
  • Helps others in the right moment
  • Reduces back and forth with QA team
  • Provides early feedback on the code
  • Better overall performance of the team
  • Coding becomes social activity
  • Starts discussions and generates ideas

What

  • Code repetition
  • Functionality duplication
  • Introducing or reducing technical debt
  • Bugs and inconsistencies
  • Dependencies to libraries and repos
  • Security and sensitive data protection
  • Performance bottlenecks
  • Maintainability

to look for in the code

When

You know the code

  • Suggest architecture improvements
  • Add code snippets (grabs attention, legitimacy, facts)         
     

You don't know the code

  • Ask for details or explanation (you might learn something)
  • Too obvious things to ignore
     

Gut feelings & raised eyebrows

  • Check your sources first (links & references to source code)

  • Uncontroversial approach

 

to comment the code

How

to be formally polite 

Proposals

We can use … instead

We better do … before we try to …

You can check if … is available

How

to be formally polite 

Questions

We've changed this recently. Have you seen it?

Have you checked the … at … ?

Is this ... gonna be able to do ... if … ?

How

to be formally polite 

Appeals

Please do … so it includes …

Please use … when …

Can you please add … when doing …

How

to be formally polite 

Positivity

Good catch!

Clever approach!

Never knew we can use that here

How

to be formally polite 

Opinions are last

I think this already exists at …

I think we can do … instead

The one who gives only opinion takes no responsibility

To improve is to change

To perfect is to change often

 

Winston Churchill   

«

 »

First feedback

30% done feedback

  • Early feedback on the approach
  • Focus on architecture decisions
  • Skip minor details, code style

Second feedback

90% done feedback

  • Final touches
  • Code style and trivial things
  • Configurations and environment

Iterations and passes

  • Code review checklist for consistency
  • Monitor and track security patterns
  • Review individual commits on meaningful changes
  • Early feedback on general issues
  • Daily code review routine

~ 40 lines

~ 200 lines

> 500 lines

Lines changed vs Number of interactions

https://github.com/ansible/ansible

35k pull requests

graph based on 2.2k

Releases

Tools

GitHub | BitBucket | GitLab

  • Diff

  • History

  • Blame

  • Ignore whitespace

  • Ticket references #

  • Pre-commit hooks

  • Pull Requests

  • Labels (ask me later)

  • Discussions

  • Inline resolving of conflicts

  • Automatic code quality tools

  • Slack / Email notifications

to improve the process

Thank you!

@kachar136
https://github.com/kachar/practical-code-reviews/
https://slides.com/kachar/practical-code-reviews/

Questions?

@kachar136
Made with Slides.com