Code readability: Session 8 (ver. 2, En)

Session 8: Review

List of sessions:

Session 1: Introduction and Principles
- https://speakerdeck.com/munetoshi/code-readability-session-1-ver-2-en

Session 2: Naming
- https://speakerdeck.com/munetoshi/code-readability-session-2-ver-2-en

Session 3: Comments
- https://speakerdeck.com/munetoshi/code-readability-session-3-ver-2-en

Session 4: State
- https://speakerdeck.com/munetoshi/code-readability-session-4-ver-2-en

Session 5: Function
- https://speakerdeck.com/munetoshi/code-readability-session-5-ver-2-en

Session 6: Dependency I
- https://speakerdeck.com/munetoshi/code-readability-session-6-ver-2-en

Session 7: Dependency II
- https://speakerdeck.com/munetoshi/code-readability-session-7-ver-2-en

Session 8: Review
- https://speakerdeck.com/munetoshi/code-readability-session-8-ver-2-en

Previous version: https://speakerdeck.com/munetoshi/code-readability

For the Japanese version, refer to https://gist.github.com/munetoshi/65a1b563fb2c271f328c121a4ac63571

© 2019-2023 Munetoshi Ishikawa, supported by LINE corporation

Transcript

1. Code-readability / session-8 Review #8
2. Brushup: Dependency Coupling: Mitigate content/common/external/control coupling Direction: Make dependency acyclic or encapsulate the cycle Redundancy: Avoid duplication of dependency Explicitness: Make dependency visible on the definition/diagram
3. Contents of this lecture - Introduction and Principles - Natural language: Naming, Comments - Inner type structure: State, Function - Inter type structure: Dependency I, Dependency II - Follow-up: Review Review > Introduction
4. Why do we need code review? Readability - Check from the perspective of other engineers Logic correctness - Edge cases, errors and the handlings, race conditions, etc. - Basically the author's responsibility Review > Introduction
5. Efficiency of review Efficiency of the review itself is also important - Readability is for productivity - Inefficient review decreases productivity Review > Introduction
6. Topics Reviewee (author / who requests a review): - How to create a pull-request easy to review - How to address review comments Reviewer (reader / who conducts a review): - Principles - Comment contents Review > Introduction
7. Topics Reviewee (author / who requests a review): - How to create a pull-request easy to review - How to address review comments Reviewer (reader / who conducts a review): - Principles - Comment contents Review > Pull-request easy to review
8. How to create a pull-request easy to review - Clarify the scope of responsibility - Keep the pull-request size small Review > Pull-request easy to review
9. How to create a pull-request easy to review - Clarify the scope of responsibility - Keep the pull-request size small Review > Pull-request easy to review > Responsibility of pull-request
10. Responsibility of pull-request Write a description of a pull-request - Main purpose, what to achieve Clarify boundaries of responsibility - Out-of-scope - Future plan Review > Pull-request easy to review > Responsibility of pull-request
11. Why description is important 1/2 Responsibility of reviewers: - Request/suggest changes if there is rooms to improve Unlimited change requests may be made Review > Pull-request easy to review > Responsibility of pull-request
12. Why description is important 2/2 Make it easy to reach a consensus - Which pull request to apply improvement - Whether this change moves toward the final design In addition, use documents such as "design docs" Review > Pull-request easy to review > Responsibility of pull-request
13. How to create a pull-request easy to review - Clarify the scope of responsibility - Keep the pull-request size small Review > Pull-request easy to review > Small pull-request
14. Pull-request size and review efficiency Large pull-request review is inefficient - Takes long time to understand the pull-request - Requires many iterations to review Split daily work into several pull-requests Never create "a pull-request of the week" Review > Pull-request easy to review > Small pull-request
15. How to split pull-request - For a large feature implementation - For an additional task Review > Pull-request easy to review > Small pull-request
16. How to split pull-request - For a large feature implementation - For an additional task Review > Pull-request easy to review > Small pull-request > Large feature implementation
17. Ways to split a large feature implementation Development of a feature may take weekly or months - Cannot be consolidated in one pull-request - Need to merge unfinished feature implementations Two ways to split: Top-down, Bottom-up Review > Pull-request easy to review > Small pull-request > Large feature implementation
18. Top-down approach Create only the structure first, fill the implementation detail later 1. Create skelton classes to explain the structure 2. Describe the future plan with TODO/pull-request comments class UserProfilePresenter( val userProfileUseCase: UseCase val profileRootView: View ) { fun showProfileImage() = TODO("#12345: ...") Review > Pull-request easy to review > Small pull-request > Large feature implementation
19. Bottom-up approach Create small parts first 1. Implements simple classes/functions without any client code 2. Explain the client code plan with TODO/pull-request comments data class UserProfileData(val ..., val ...) // TODO(#12346): ... object UserNameStringUtils { @Suppress("unused") // TODO(#12347): ... fun normalizeEmoji(userName: String): String = ... Review > Pull-request easy to review > Small pull-request > Large feature implementation
20. How to split pull-request - For a large feature implementation - For an additional task Review > Pull-request easy to review > Small pull-request > Additional task
21. Example of additional task 1/3 Assumption 1: Implements a clock display feature class CurrentClockIndicator(...) { fun showCurrentTime() { val clockText = toClockText(...) // ... } companion object { private fun toClockText(...: Long): String = ... } } Review > Pull-request easy to review > Small pull-request > Additional task
22. Example of additional task 2/3 Commit list of implementation Commit 1: Create a skeleton class, `CurrentTimeIndicator` Commit 2: Implement a private utility function, `toClockText` Commit 3: Implement `CurrentTimeIndicator.showCurrentTime` Review > Pull-request easy to review > Small pull-request > Additional task
23. Example of additional task 2/3 Assumption 2: A copy of toClockText is found in a existing class Need to extract it to DateTimeTextFormatter object DateTimeTextFormatter { fun toClockText(...: Long): String = ... } class CurrentClockIndicator(...) { fun showCurrentTime() { val clockText = DateTimeTextFormatter.toClockText(...) // ... Review > Pull-request easy to review > Small pull-request > Additional task
24. Pull-request for additional task Question: How can you make pull-requests? - Anti-pattern 1: Consolidate into a pull-request - Anti-pattern 2: Split pull-requests in chronological order - Solution 1: Finish the additional task first - Solution 2: Restructure the commits Review > Pull-request easy to review > Small pull-request > Additional task
25. Pull-request for additional task Question: How can you make pull-requests? - Anti-pattern 1: Consolidate into a pull-request - Anti-pattern 2: Split pull-requests in chronological order - Solution 1: Finish the additional task first - Solution 2: Restructure the commits Review > Pull-request easy to review > Small pull-request > Additional task
26. Anti-pattern 1: Consolidate into a pull-request Commits of a pull-request Commit 1: Create a skeleton class, `CurrentTimeIndicator` Commit 2: Implement a private utility function, `toClockText` Commit 3: Implement `CurrentTimeIndicator.showCurrentTime` Commit 4: Extract `toClockText` to `DateTimeTextFormatter` Commit 4 is totally different && the change becomes extensive Requires check for missing extractions, etc. Review > Pull-request easy to review > Small pull-request > Additional task
27. Pull-request for additional task Question: How can you make pull-requests? - Anti-pattern 1: Consolidate into a pull-request - Anti-pattern 2: Split pull-requests in chronological order - Solution 1: Finish the additional task first - Solution 2: Restructure the commits Review > Pull-request easy to review > Small pull-request > Additional task
28. Anti-pattern 2: Split in chronological order 1/2 Commits of pull-request A Commit 1: Create a skeleton class, `CurrentTimeIndicator` Commit 2: Implement a private utility function, `toClockText` Commit 3: Implement `CurrentTimeIndicator.showCurrentTime` Commits of pull-request B Commit 4: Extract `toClockText` to `DateTimeTextFormatter` Review > Pull-request easy to review > Small pull-request > Additional task
29. Anti-pattern 2: Split in chronological order 1/2 Marge pull-request A and then pull-request B Causes technical debts - Code copy is temporarily made - Pull-request B may be left unmerged due to task interruption Review > Pull-request easy to review > Small pull-request > Additional task
30. Pull-request for additional task Question: How can you make pull-requests? - Anti-pattern 1: Consolidate into a pull-request - Anti-pattern 2: Split pull-requests in chronological order - Solution 1: Finish the additional task first - Solution 2: Restructure the commits Review > Pull-request easy to review > Small pull-request > Additional task
31. Solution 1: Finish the additional task first 1/2 Create a pull request for DateTimeTextFormatter concurrently Condition: Notices code duplication before adding toClockText Commits of pull-request A Commit 1: Create a skeleton class, `CurrentTimeIndicator` Commits of pull-request B Commit 4: Extract `toClockText` to `DateTimeTextFormatter` Review > Pull-request easy to review > Small pull-request > Additional task
32. Solution 1: Finish the additional task first 2/2 Rebase pull-request A after merging pull-request B Pull-request A can be created using toClockText after extraction Commits of pull-request A Commit 1: Create a skeleton class, `CurrentTimeIndicator` Commit 3: Implement `CurrentTimeIndicator.showCurrentTime` Review > Pull-request easy to review > Small pull-request > Additional task
33. Pull-request for additional task Question: How can you make pull-requests? - Anti-pattern 1: Consolidate into a pull-request - Anti-pattern 2: Split pull-requests in chronological order - Solution 1: Finish the additional task first - Solution 2: Restructure the commits Review > Pull-request easy to review > Small pull-request > Additional task
34. Solution 2: Restructure the commits Split, reorder, squash commits Review > Pull-request easy to review > Small pull-request > Additional task
35. Solution 2: Restructure the commits 1/4 Step 1: Split Commit 4 Commit 1: Create a skeleton class, `CurrentTimeIndicator` Commit 2: Implement a private utility function, `toClockText` Commit 3: Implement `CurrentTimeIndicator.showCurrentTime` Commit 4_EXISTING: Extract `toClockText` from the existing classes Commit 4_NEW: Extract `toClockText` from `CurrentTimeIndicator` Review > Pull-request easy to review > Small pull-request > Additional task
36. Solution 2: Restructure the commits 2/4 Step 2: Reorder commits Commit 4_EXISTING: Extract `toClockText` from the existing classes Commit 1: Create a skeleton class, `CurrentTimeIndicator` Commit 2: Implement a private utility function, `toClockText` Commit 4_NEW: Extract `toClockText` from `CurrentTimeIndicator` Commit 3: Implement `CurrentTimeIndicator.showCurrentTime` Review > Pull-request easy to review > Small pull-request > Additional task
37. Solution 2: Restructure the commits 3/4 Step 3: Squash the middle three commits Commit 4_EXISTING: Extract `toClockText` from the existing classes Commit 1: Create a skeleton class, `CurrentTimeIndicator` --squashed-- --squashed-- Commit 3: Implement `CurrentTimeIndicator.showCurrentTime` Review > Pull-request easy to review > Small pull-request > Additional task
38. Solution 2: Restructure the commits 4/4 Step 4: Create two pull-requests Pull-request B Commit 4_EXISTING: Extract `toClockText` from the existing classes Pull-request A Commit 1: Create a skeleton class, `CurrentTimeIndicator` Commit 3: Implement `CurrentTimeIndicator.showCurrentTime` Review > Pull-request easy to review > Small pull-request > Additional task
39. Note to keep pull-requests small Reach consensus before implementing - Which task is to be done, when, and in which pull-request - Whether we allow temporary debts and how to resolve them Discuss changes as soon as possible when they are found Review > Pull-request easy to review > Small pull-request > Additional task
40. Create a pull-request easy to review: Summary Clarify the responsibility of a pull-request - Main purpose, what to achieve - Out-of-scope, future plan Keep a pull-request small - For a large feature: top-down and bottom-up approaches - For an additional task: task ordering, commit restructuring Review > Pull-request easy to review > Summary
41. Aside: Application to a commit Apply the same ideas to a commit - Responsibility of a commit - How to split a commit Review > Pull-request easy to review > Summary
42. Topics Reviewee (author / who requests a review): - How to create a pull-request easy to review - How to address review comments Reviewer (reader / who conducts a review): - Principles - Comment contents Review > Addressing review comments
43. Important point in addressing comments Do not merely address comments Review > Addressing review comments
44. Do not merely address comments - Understand the comment's intent - Think why the reviewer asked/misunderstood - Apply to other places as well Review > Addressing review comments
45. Do not merely address comments - Understand the comment's intent - Think why the reviewer asked/misunderstood - Apply to other places as well Review > Addressing review comments
46. Understand the comment's intent Do not merely paste suggested code in a review comment - Understand what the key idea is - Attempt further improvement - Validate: edge cases, exceptions, race conditions, etc. Review > Addressing review comments
47. Do not merely address comments - Understand the intent of the comment - Think why reviewer asked/misunderstood - Apply to other places as well Review > Addressing review comments
48. Think why reviewer asked/misunderstood Question/misunderstanding can be caused by unreadable code Example of comment: "Why is the null case checked here?" - Remove the null check if unnecessary - Write in-line comment to explain why - Extract as a local value to explain (e.g., isUserExpired) Review > Addressing review comments
49. Do not merely address comments - Understand the comment's intent - Think why the reviewer asked/misunderstood - Apply to other places as well Review > Addressing review comments
50. Apply to other places as well Find similar code and address the comment Example of comment "Add @Nullable to this parameter": - Apply to other parameters or return value - Apply to other changed functions - Apply to future pull-requests Review > Addressing review comments
51. Addressing review comments: Summary Understand what the intention of a review comment is - Find the comment's key idea and reason - Attempt future improvement - Try to apply it to other parts Review > Addressing review comments > Summary
52. Topics Reviewee (author / who requests a review): - How to create a pull-request easy to review - How to address review comments Reviewer (reader / who conducts a review): - Principles - Comment contents Review > Reviewer principles
53. Principles for reviewers Respect reviewees - Never verbally abuse nor deny personality - Subject of review: code/specification/process (!= reviewee) Keep review process efficient - Do not use resources indefinitely - Do not cut the required time Review > Reviewer principles
54. Principles for respect and efficiency - Do not neglect review requests - Reject problematic pull-requests - Do not be too deadline-conscious - Consider other options of request/suggest Review > Reviewer principles
55. Principles for respect and efficiency - Do not neglect review requests - Reject problematic pull-requests - Do not be too deadline-conscious - Consider other options of request/suggest Review > Reviewer principles
56. Do not neglect review requests Define a reply time limit as a rule - e.g., 24 hours of working days Question: What is the "worst" behavior regarding review deadlines? Review > Reviewer principles > Do not neglect
57. Reactions to review requests GOOD: - Reviewing soon or on time - Redirecting to other reviewer - Replying as "I cannot review now" or "I'll be late" NOT SO GOOD: - Ignoring a review WORST: - Replying "I'll review" without review Review > Reviewer principles > Do not neglect
58. When you cannot review If you are busy, reply explaining so The reviewee can: - Wait for the review if it does not block other tasks - Find other reviewers if it is an urgent pull-request Review > Reviewer principles > Do not neglect
59. Principles for respect and efficiency - Do not neglect review requests - Reject problematic pull-requests - Do not be too deadline-conscious - Consider other options of request/suggest Review > Reviewer principles > Reject problematic pull-requests
60. Reject problematic pull-requests 1/2 Do not spend too much time reviewing a problematic pull-request Review > Reviewer principles > Reject problematic pull-requests
61. Reject problematic pull-requests 2/2 Ask the reviewee to re-create without reading the details Too large: - Ask to split Has critical issues on purpose, assumption, or structure: - Ask to create skeleton classes or have casual chat Review > Reviewer principles > Reject problematic pull-requests
62. Principles for respect and efficiency - Do not neglect review requests - Reject problematic pull-requests - Do not be too deadline-conscious - Consider other options of request/suggest Review > Reviewer principles > Reject problematic pull-requests
63. Do not be too deadline-conscious Do not lower the approval threshold just because of the deadline - Let the reviewee explain if they want to merge soon - Look for alternatives (simplify specifications, postpone) Exc.: critical feature to business strategy, high impact bug-fix - Requires tests and agreements/task-tickets on future plan - Reviewers should always be calmer than reviewees Review > Reviewer principles > Do not be too deadline-conscious
64. Principles for respect and efficiency - Do not neglect review requests - Reject problematic pull-requests - Do not be too deadline-conscious - Consider other options of request/suggest Review > Reviewer principles > Other options of request/suggest
65. Consider other options of request/suggest The answer is not only showing the answer To balance the reviewer's load & encourage the reviewee's growth: - Ask questions rather than spending time on unreadable code - Only point out the problem and ask to look into the cause - Write comments only to share knowledge Review > Reviewer principles > Other options of request/suggest
66. Principles: Summary Respect, and review efficiently - Only reply if you cannot review - Do not spend too much time on problematic pull-requests - Consider what an efficient and effective comment is Review > Reviewer principles > Summary
67. Topics Reviewee (author / who requests a review): - How to create a pull-request easy to review - How to address review comments Reviewer (reader / who conducts a review): - Principles - Comment contents Review > Comment contents
68. What should be commented 1/3 Anything you noticeʢUse tools if possibleʣ - Coding style, coding conventions - Language- and platform-specific idioms - Test code, what are checked on operation test - Pull-request and commit size/structure Review > Comment contents
69. What should be commented 2/3 Anything you notice - Objective and scope of a project/task - Balance between value and complexity - Bugs and security issues - Changes in performance and footprint Review > Comment contents
70. What should be commented 3/3 Everything talked in this presentation series principles, naming, comments, state, function, dependency Review > Comment contents
71. Code review case study Example to add a parameter on an existing function fun showPhotoView( photoData: PhotoData ) { if (!photoData.isValid) { return } ... Review > Comment contents > Case study
72. Code review case study Example to add a parameter on an existing function fun showPhotoView( photoData: PhotoData, isInEditScreen: Boolean ) { if (!photoData.isValid) { if (isInEditScreen) { showDialog() } return } ... Review > Comment contents > Case study
73. Code review case study Naming: Explain "what it does" rather than "where it is called" isInEditScreen → showsDialogOnError Review > Comment contents > Case study
74. Code review case study Naming: Explain "what it does" rather than "where it is called" fun showPhotoView( photoData: PhotoData, isInEditScreen: Boolean ) { if (!photoData.isValid) { if (isInEditScreen) { showDialog() } return } ... Review > Comment contents > Case study
75. Code review case study Naming: Explain "what it does" rather than "where it is called" fun showPhotoView( photoData: PhotoData, showsDialogOnError: Boolean ) { if (!photoData.isValid) { if (showsDialogOnError) { showDialog() } return } ... Review > Comment contents > Case study
76. Code review case study Dependency: Relax control coupling caused by the parameter - Extract function call of showDialog - Return success or failure as a boolean value Review > Comment contents > Case study
77. Code review case study Dependency: Relax control coupling caused by the parameter fun showPhotoView( photoData: PhotoData, showsDialogOnError: Boolean ) { if (!photoData.isValid) { if (showsDialogOnError) { showDialog() } return } ... Review > Comment contents > Case study
78. Code review case study Dependency: Relax control coupling caused by the parameter fun showPhotoView( photoData: PhotoData ): Boolean { if (!photoData.isValid) { return false } ... Review > Comment contents > Case study
79. Code review case study Dependency: Relax control coupling caused by the parameter fun showPhotoView(photoData: PhotoData): Boolean { if (!photoData.isValid) { return false } ... return true } Review > Comment contents > Case study
80. Code review case study Dependency: Relax control coupling caused by the parameter On edit screen: val isViewShown = showPhotoView(...) if (!isViewShown) { showErrorDialog(...) } On other screen: showPhotoView(...) Review > Comment contents > Case study
81. Code review case study Comments: Comment if a function has both side-effect and return value /** * Shows a photo at the center if the given photo data is ..., * and returns true. * Otherwise, this returns false without showing the view. */ fun showPhotoView(photoData: PhotoData): Boolean { if (!photoData.isValid) { return false } Review > Comment contents > Case study
82. Comment contents: Summary Comment whatever you notices - From coding style to the project's objective - Sometimes needs multiple review iterations Please "review" this code readability presentation series Review > Comment contents > Case study
83. Summary Review is important for code readability Reviewee: - Make pull-requests small and structured - Understand the intents of review comments Reviewer: - Respect, and review efficiently Review > Summary