Slide 1
Slide 1 text
Munetoshi Ishikawa, LINE Corporation
Code review techniques for readability
ՄಡੑΛߴΊΔͨΊͷίʔυϨϏϡʔςΫχοΫ
Slide 2
Slide 2 text
Objective of code review
Detects points that may be overlooked by the author
- Find problems: bug, security or performance issues
- Improve readability
Slide 3
Slide 3 text
Assumptions for this talk
- Developing a product in a team
- Using GitHub for codebase hosting
merge
request review pull request
author reviewer
“Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/
codebase
Slide 4
Slide 4 text
Contents of this talk
- Check out a pull-request
- Scan the code
- Emulate another code reviewer
- Use other communication tools in addition to review comments
- Don't review problematic pull-requests
Slide 5
Slide 5 text
Contents of this talk
- Check out a pull-request
- Scan the code
- Emulate another code reviewer
- Use other communication tools in addition to review comments
- Don't review problematic pull-requests
Slide 6
Slide 6 text
Check out a pull-request
May overlook issues only by looking at the changed code
private fun storeMessages(messages: List) {
if (messages.isEmpty) {
return
}
...
}
fun existingCaller(messages: List) {
if (messages.isNotEmpty) {
storeMessages(messages)
}
}
newly added code
duplicated conditional branch
Slide 7
Slide 7 text
What to confirm by checking out
- Inconsistent name
- Code duplication
- Replaceable code with standard library/API
- Layer to implement logic: caller or callee
- Warnings and metrics
Slide 8
Slide 8 text
Example: Inconsistent name
May need to rename existing properties when a new one is added
class FooEntry(
val !!",
val timestampInMillis: Long,
!!"
)
Slide 9
Slide 9 text
May need to rename existing properties when a new one is added
Example: Inconsistent name
class FooEntry(
val !!",
val timestampInMillis: Long,
!!"
val lastUpdateTimestampInMillis: Long
)
newly added property
Slide 10
Slide 10 text
May need to rename existing properties when a new one is added
Example: Inconsistent name
class FooEntry(
val !!",
!!"
val creationTimestampInMillis: Long,
val lastUpdateTimestampInMillis: Long
)
newly added property
need to rename & reorder
Slide 11
Slide 11 text
Tools to check out a pull-request
To check out pull-request #123456
- GitHub CLI: https://cli.github.com
gh pr checkout 123456
- `hub` command: https://github.com/github/hub
hub checkout https://github.com/.../pull/123456
Slide 12
Slide 12 text
Check out a pull-request: Summary
Don't focus only on the changed code
- Check existing code as well
- Use a tool to check out a pull request
Slide 13
Slide 13 text
Contents of this talk
- Check out a pull-request
- Scan the code
- Emulate another code reviewer
- Use other communication tools in addition to review comments
- Don't review problematic pull-requests
Slide 14
Slide 14 text
Scan the code
Code should be able to be outlined without reading the detail
Able to understand with:
- Signature (name, type, parameters, etc.)
- Documentation comment
- Happy path logic (= the main case logic)
- The left-side of a code block
Slide 15
Slide 15 text
Example 1/2: Conditional branch, bad case
Need to read if body to understand the function responsibility
fun ...(inputNumber: Int) {
if (inputNumber < 0) {
submitButton.isEnabled = false
messageTextView.text = "Invalid number!"
} else {
submitButton.isEnabled = true
messageTextView.text = "Valid number."
}
}
Slide 16
Slide 16 text
Example 1/2: Conditional branch, bad case
Need to read if body to understand the function responsibility
fun ...(inputNumber: Int) {
if (inputNumber < 0) {
...
...
} else {
...
...
}
}
Code summary:
Do something depending on whether inputNumber is valid or not
Slide 17
Slide 17 text
Example 1/2: Conditional branch, good case
Can understand what the function does without reading the details
fun someFunction(inputNumber: Int) {
val isValidNumber = inputNumber >= 0
submitButton.isEnabled = isValidNumber
messageTextView.text =
if (isValidNumber) "Invalid number!" else "Valid number."
}
Slide 18
Slide 18 text
Example 1/2: Conditional branch, good case
Can understand what the function does without reading the details
fun someFunction(inputNumber: Int) {
val isValidNumber = ...
submitButton.isEnabled = ...
messageTextView.text =
...
}
Code summary: Enable/disable the button and update the text
depending on whether inputNumber is valid or not
Slide 19
Slide 19 text
Example 2/2: Method chain, bad case
Need to read all the lambda bodies and the receivers
return userActionLog
.groupBy { it.user }
.map { it.key to it.value.size }
.sortedBy { it.second }
.map { it.first }
.takeLast(10)
Slide 20
Slide 20 text
Example 2/2: Method chain, bad case
Need to read all the lambda bodies and the receivers
return userActionLog
.groupBy ...
.map ...
.sortedBy ...
.map ...
.takeLast(10)
Code summary: Return a sub-list created from userActionLog
Slide 21
Slide 21 text
Example 1/2: Method chain, good case
Can understand the return value without reading the chain
val logCountByUser: Map = userActionLog
.groupBy { log -> log.user }
.map { (user, logs) -> user to logs.size }
val userListSortedByLogCount: List = logCountByUser
.sortedBy { (_, messageCount) -> messageCount }
.map { (userId, _) -> userId }
return userListSortedByLogCount.takeLast(10)
Slide 22
Slide 22 text
Example 1/2: Method chain, good case
Can understand the return value without reading the chain
val logCountByUser: Map = userActionLog
...
...
val userListSortedByLogCount: List = logCountByUser
...
...
return userListSortedByLogCount.takeLast(10)
Code summary: Return the 10 users with the highest log count
Slide 23
Slide 23 text
Why we scan at code review
Readable for author != readable for other members
Knowledge gap between authors and readers/reviewers
- Authors: write code with knowledge of the details
- Readers/reviewers: read the code without the knowledge
Slide 24
Slide 24 text
How to confirm if the code is scannable
"Got it!" is a bad signal
- "Why is null check needed here?"
- "Got it! It means a non-cached value."
val messageModel = messageCache[messageKey]
...
if (messageModel == null || ...) {
...
}
Slide 25
Slide 25 text
How to make the code scannable
- Renaming, writing comments
- Restructuring
val cachedMessageModel = messageCache[messageKey]
...
if (cachedMessageModel == null || ...) {
...
}
Slide 26
Slide 26 text
Scan the code: Summary
Check the code is readable without understanding the details
- "Got it!" is a bad signal
Slide 27
Slide 27 text
Contents of this talk
- Check out a pull-request
- Scan the code
- Emulate another code reviewer
- Use other communication tools in addition to review comments
- Don't review problematic pull-requests
Slide 28
Slide 28 text
Emulate another code reviewer
Check if the code is readable for all team members
- You may have feature specific knowledge
- Avoid knowledge siloing
Imagine whether another reviewer can understand or not
Slide 29
Slide 29 text
Image of emulation 1/2: Normal code reading
reviewer
module specific knowledge
computer science knowledge
platform/language knowledge
context of the project
“Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/
fun function(foo: Int) {
if (foo == 42) {
return
}
...
}
"Why 42?"
"Got it!"
guessing, deduction...
Slide 30
Slide 30 text
Assume prerequisite knowledge
Can't expect feature/module/project specific knowledge or context
Things that new team members should know:
- Basic computer science
- Basic knowledge of the programming language and platform
- Knowledge of tools used in the organization (if training is available)
Slide 31
Slide 31 text
Image of emulation 2/2: With emulation
reviewer
imaginary new member
computer science knowledge
platform/language knowledge
“Twemoji” ©Twitter, Inc and other contributors (Licensed under CC-BY 4.0) https://twemoji.twitter.com/
fun function(foo: Int) {
if (foo == 42) {
return
}
...
}
"Why 42?"
???
Slide 32
Slide 32 text
Aside: Other applications of emulation
- Document writing: PRD, Design doc
- Presentation
- Discussion
- etc.
Slide 33
Slide 33 text
Emulate another code reviewer: Summary
Check if the code is readable for all team members
- Assume prerequisite knowledge
- Imagine how a new member understand the code
Slide 34
Slide 34 text
Contents of this talk
- Check out a pull-request
- Scan the code
- Emulate another code reviewer
- Use other communication tools in addition to review comments
- Don't review problematic pull-requests
Slide 35
Slide 35 text
Use other communication tools
Communication on review comments can be slow
- 6pm, Mar 6. An author send a review request
- 1pm, Mar 7. Reviewer: "Why do you check null here?"
- 5pm, Mar 7. Author: "This is because..."
- 2pm, Mar 8. Reviewer: "If so, we can restructure..."
Consider using another communication way
Slide 36
Slide 36 text
Considerable options
Face-to-face discussion
- With a white board, bringing a computer, etc.
Audio/video conference
- With screen sharing, pair programming tools, etc.
Messaging tools
Slide 37
Slide 37 text
Follow up of communication
Leave a comment to explain the conclusion
- To help other members understand the conclusions
- To be able to trace the context later
Slide 38
Slide 38 text
Useful phrases for leaving a note
- "For the record: ..."
- "As we chatted offline", "As we discussed on another thread"
- "Refer to (chat log link) for more details"
Slide 39
Slide 39 text
Use other communication tools: Summary
Chat offline/online or using messaging tool
- Leave a comment on the pull-request to explain the conclusion
Slide 40
Slide 40 text
Contents of this talk
- Check out a pull-request
- Scan the code
- Emulate another code reviewer
- Use other communication tools in addition to review comments
- Don't review problematic pull-requests
Slide 41
Slide 41 text
Don't review problematic pull-requests
Don't spend too much time on a problematic pull-request
- Too large
- Has too many issues
- Has a fundamental issue on the objective/structure
Slide 42
Slide 42 text
Drawbacks of reviewing problematic pull requests
- May overlook issues
- Consumes too much reviewer's and author's resources
Slide 43
Slide 43 text
Actions for problematic pull requests
1. Ask to close the pull request first
2. Follow up for the next step
Slide 44
Slide 44 text
Options of the next steps
- Split the pull-request into smaller ones
- Write a document for design (design-doc)
- Make skeleton classes first
Slide 45
Slide 45 text
Make skeleton classes first
Focus only on the structure first
class UserProfilePresenter(
val useCase: UserProfileUseCase,
val profileRootView: View
) {
fun showProfileImage(): UiResult {
... // Implementation detail
}
private fun someDetailedPrivateFunction() {
... // Implementation detail
}
}
Slide 46
Slide 46 text
Make skeleton classes first
Focus only on the structure first
Review the detail code after the structure
class UserProfilePresenter(
val useCase: UserProfileUseCase,
val profileRootView: View
) {
fun showProfileImage(): UiResult = TODO("!!"")
}
Slide 47
Slide 47 text
Don't review problematic pull-requests
Ask to close, and follow up
- Save reviewer's and author's resources
- Follow up is mandatory
Slide 48
Slide 48 text
Summary
- Check out a pull-request
- Scan the code
- Emulate another code reviewer
- Use other communication tools in addition to review comments
- Don't review problematic pull-requests
Slide 49
Slide 49 text
Summary
- Read existing code as well
- Check the code is readable without understanding the details
- Confirm if the code is readable for new members
- Chat offline/online or using messaging tool
- Ask to close, and follow up
Slide 50
Slide 50 text
Related materials
Slide 51
Slide 51 text
Related materials
Watch a presentation from DroidKaigi 2021
͘ੜ͖Δίʔυϕʔεͷʮ࣭ʯʹ͖߹͏
https://droidkaigi.jp/2021/timetable/276957/?day=2
1. Process and activities for readability
2. Code readability
Slide 52
Slide 52 text
Related materials
Presentation "code readability"ɻ
https://speakerdeck.com/munetoshi/code-readability
Book "ಡΈ͍͢ίʔυͷΨΠυϥΠϯ
-࣋ଓՄೳͳιϑτΣΞ։ൃͷͨΊʹ"
https://gihyo.jp/book/2022/978-4-297-13036-7
1. Process and activities for readability
2. Code readability