Practical
Code Reviews
Ilko Kacharov
PHP UG
Edition
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
to gather the data
zendframework/zend-expressive
guzzlehttp/guzzle
tracy/tracy
Github REST / GraphQL
GraphQL
or just a REST
REST
- Good documentation
- Easy to use
- Api requests in a loop
- Too much unused data
- Data aggregation in app
GraphQL
- Query debugger tool
- Get what you need, even aggregated
- Cursor pagination behavior (before/after)
- Variables sent with query
- Still in development (bugs)
Links
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
Practical Code Reviews [PHPUG]
By Ilko Kacharov
