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

Slide 1

Slide 1 text

Code-readability / session-8 Review #8

Slide 2

Slide 2 text

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 deﬁnition/diagram

Slide 3

Slide 3 text

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

Slide 4

Slide 4 text

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

Slide 5

Slide 5 text

Efficiency of review Efficiency of the review itself is also important - Readability is for productivity - Inefficient review decreases productivity Review > Introduction

Slide 6

Slide 6 text

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

Slide 7

Slide 7 text

Slide 8

Slide 8 text

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

Slide 9

Slide 9 text

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

Slide 10

Slide 10 text

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

Slide 11

Slide 11 text

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

Slide 12

Slide 12 text

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 ﬁnal design In addition, use documents such as "design docs" Review > Pull-request easy to review > Responsibility of pull-request

Slide 13

Slide 13 text

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

Slide 14

Slide 14 text

Pull-request size and review efﬁciency Large pull-request review is inefﬁcient - 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

Slide 15

Slide 15 text

How to split pull-request - For a large feature implementation - For an additional task Review > Pull-request easy to review > Small pull-request

Slide 16

Slide 16 text

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

Slide 17

Slide 17 text

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 unﬁnished feature implementations Two ways to split: Top-down, Bottom-up Review > Pull-request easy to review > Small pull-request > Large feature implementation

Slide 18

Slide 18 text

Top-down approach Create only the structure ﬁrst, ﬁll 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

Slide 19

Slide 19 text

Bottom-up approach Create small parts ﬁrst 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

Slide 20

Slide 20 text

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

Slide 21

Slide 21 text

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

Slide 22

Slide 22 text

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

Slide 23

Slide 23 text

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

Slide 24

Slide 24 text

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 ﬁrst - Solution 2: Restructure the commits Review > Pull-request easy to review > Small pull-request > Additional task

Slide 25

Slide 25 text

Slide 26

Slide 26 text

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

Slide 27

Slide 27 text

Slide 28

Slide 28 text

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

Slide 29

Slide 29 text

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

Slide 30

Slide 30 text

Slide 31

Slide 31 text

Solution 1: Finish the additional task ﬁrst 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

Slide 32

Slide 32 text

Solution 1: Finish the additional task ﬁrst 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

Slide 33

Slide 33 text

Slide 34

Slide 34 text

Solution 2: Restructure the commits Split, reorder, squash commits Review > Pull-request easy to review > Small pull-request > Additional task

Slide 35

Slide 35 text

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

Slide 36

Slide 36 text

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

Slide 37

Slide 37 text

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

Slide 38

Slide 38 text

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

Slide 39

Slide 39 text

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

Slide 40

Slide 40 text

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

Slide 41

Slide 41 text

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

Slide 42

Slide 42 text

Slide 43

Slide 43 text

Important point in addressing comments Do not merely address comments Review > Addressing review comments

Slide 44

Slide 44 text

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

Slide 45

Slide 45 text

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

Slide 46

Slide 46 text

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

Slide 47

Slide 47 text

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

Slide 48

Slide 48 text

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

Slide 49

Slide 49 text

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

Slide 50

Slide 50 text

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

Slide 51

Slide 51 text

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

Slide 52

Slide 52 text

Slide 53

Slide 53 text

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

Slide 54

Slide 54 text

Principles for respect and efﬁciency - Do not neglect review requests - Reject problematic pull-requests - Do not be too deadline-conscious - Consider other options of request/suggest Review > Reviewer principles

Slide 55

Slide 55 text

Slide 56

Slide 56 text

Do not neglect review requests Deﬁne 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

Slide 57

Slide 57 text

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

Slide 58

Slide 58 text

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

Slide 59

Slide 59 text

Slide 60

Slide 60 text

Reject problematic pull-requests 1/2 Do not spend too much time reviewing a problematic pull-request Review > Reviewer principles > Reject problematic pull-requests

Slide 61

Slide 61 text

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

Slide 62

Slide 62 text

Slide 63

Slide 63 text

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 speciﬁcations, postpone) Exc.: critical feature to business strategy, high impact bug-ﬁx - 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

Slide 64

Slide 64 text

Slide 65

Slide 65 text

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

Slide 66

Slide 66 text

Principles: Summary Respect, and review efﬁciently - Only reply if you cannot review - Do not spend too much time on problematic pull-requests - Consider what an efﬁcient and effective comment is Review > Reviewer principles > Summary

Slide 67

Slide 67 text

Slide 68

Slide 68 text

What should be commented 1/3 Anything you noticeʢUse tools if possibleʣ - Coding style, coding conventions - Language- and platform-speciﬁc idioms - Test code, what are checked on operation test - Pull-request and commit size/structure Review > Comment contents

Slide 69

Slide 69 text

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

Slide 70

Slide 70 text

What should be commented 3/3 Everything talked in this presentation series principles, naming, comments, state, function, dependency Review > Comment contents

Slide 71

Slide 71 text

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

Slide 72

Slide 72 text

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

Slide 73

Slide 73 text

Code review case study Naming: Explain "what it does" rather than "where it is called" isInEditScreen → showsDialogOnError Review > Comment contents > Case study

Slide 74

Slide 74 text

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

Slide 75

Slide 75 text

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

Slide 76

Slide 76 text

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

Slide 77

Slide 77 text

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

Slide 78

Slide 78 text

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

Slide 79

Slide 79 text

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

Slide 80

Slide 80 text

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

Slide 81

Slide 81 text

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

Slide 82

Slide 82 text

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

Slide 83

Slide 83 text

Summary Review is important for code readability Reviewee: - Make pull-requests small and structured - Understand the intents of review comments Reviewer: - Respect, and review efﬁciently Review > Summary