The Guide to Code Reviews
Seeing is believing
René Schwietzke, Xceptance
Introduction
The big picture about this lecture.
- Why should we do code reviews
- How to perform a review
- What to look for
- Motivation
- Motivation
To be honest, this is more of a change review training than a code review training. There are often changes that are unrelated to Java or coding details.
...but code review sounds cooler.
What is Code Review
The High-Level View
Code Review
Code review is systematic examination of computer source code. It is intended to find and fix mistakes overlooked in the initial development phase, improving both the overall quality of software and the developers' skills. Reviews are done in various forms such as pair programming, informal walkthroughs, and formal inspections.
- See the blue print
- See nuts and bolts
- See ideas and thoughts
- See the untouchable
- Not everything can be tested (easily)
- See the words, instead of just listing to the story.
No Idea of Code
No programmer? No problem!
- The code itself is one piece of the story...
- Style
- Changes
- Size of change
- Comments
- Consitency
- Touched areas
- See the blue print
- See nuts and bolts
- See ideas and thoughts
- See the untouchable
- Not everything can be tested (easily)
- See the text, instead of just listing to the story.
Review it!
Less talking, more doing!
How to review
How and where to review
- Use Stash
- Use Git
Basics
Some first things to configure
- Ignore whitespace-changes
- Use the side-by-side view
- Check the total diff only
Review Steps
How to approach the review.
- Understand the problem
- Check JIRA ticket
- No ticket, barely any commit
- Pull requests preferred
- Before squashing
- Before rebasing
- After rebasing
Pay attention to....
Just a list for now.
- Amount of files touched
- Any tests committed?
- Public API changes (dw.*)
- Database changes (*.sql, POs)
- Does touching the file make sense?
- Commit comments
- JIRA numbers
- Formatting and Style
- Comments
- Assumptions
- Synchronisation
- Logic changes
- Alarm bells: Performance, Leaks
- Range checks
- Unexpected stuff
Amount of files
- More files more risk
- Compare the area of changes
Tests commited
No tests, no fun.
- Any tests?
- Amount ok?
- Compare code and possible tests
- When public API, check for *ds based tests
- Get nervous when test are changed, not added, mostly when public API
Public API Changes
Does the customer API change and how.
- dw.* is mostly external API
- Make sure not unintentionally introduced
- Review twice and make sure it is tested
- Bugs hurt most here
Changing a file
Does make the context sense?
- Make sure not unintentionally introduced
- Review twice and make sure it is tested
- Bugs hurt most here
Style
Does the code or file look pleasant?
- We have code style style guide, no kidding.
- If the code looks strange, it is broken.
- Other file types have basic formatting guidelines too.
- It is about future maintenance, not today's blame.
Comments
This is inline documentation, because the outside documentation might get lost or out of date pretty soon.
- Well commented code looks pretty.
- The content needs meaning.
- Rather too much than too little.
- It is about future maintenance, not today's blame.
- Yes, CSS, ISML, JavaScript, and SQL too.
Database
Any SQL, any migration?
- Just check for relevant files.
Synchronisation
Our servers are multi-threaded.
- Added or removed volatile
- Added or removed synchronisation
- Parallel data structure became simple or vice versa
- Instance variables
- Any caches
- Central classes such as Managers
- Yes, it requires to understand the Java Memory Model.
- Always see if a class is subject to concurrent modification.
Assumptions
Anything that is assumed to be there or in a range.
- Any constants
- Any fixed numbers
- Comment such as "We do not have more than..."
- "This will be small"
- Loops on datasets (they assume that the set is small)
- This is one of the reasons for comments.
- Rather question stuff than silently accepting it.
Refactoring
Reworking the code to fit in something new.
- Any code change can introduce failures
- Carefully check the changes
- Nobody normally knows that old stuff was touched heavily
- Tests for the old stuff have to exist!
- Lot of one line changes with new method calls
- Parameter list changes
- Taking out code and externalizing it
JIRA
The reason for the change determines the code review depth.
- Performance!
- Memory leaks!
- Race conditions!
- Security!
- Some stuff can only be reviewed and not tested.
- A test does not reach everywhere.
Patterns
Different than the surrounding code.
Patterns
Incorrect code to deactivate it.
Patterns
Logic nightmares.
Assert an assumption
org.junit.Assert
- Verify the state.
- Proves the expectations.
- Can be used often.
- No very luxurious.
- For later: http://hamcrest.org/JavaHamcrest/.
/**
* Test adding the same data again
*/
@Test
public void addHandlingDuplicateData()
{
final String A = "foo1";
final String B = new String("foo1");
// this is the string challenge :)
Assert.assertNotSame(A, B);
Assert.assertEquals(A, B);
final ArrayList list = new ArrayList();
Assert.assertEquals(0, list.size());
}