可読性を高めるためのコードレビューテクニック

2023年3月16日に開催された「SoftBank Tech Night Fes 2023」の講演資料です。

コードレビューを行うことで、ロジックのバグやセキュリティ上の欠陥、パフォーマンスの問題といった、間違いを発見することができます。その「間違いを発見する」効果が注目されがちですが、コードレビューは「コードの可読性を高める」ためにも重要です。第三者の視点でコードを見ることで、書いた人だけが持つ前提知識がなくても読めるコードになっているかを確認できます。このセッションでは、コードレビューを通じてどのように可読性を高めるのか、そのテクニックを抜粋して紹介します。

■関連リンク
・LINE Engineering
https://engineering.linecorp.com/ja/

■作成者
石川 宗寿（いしかわ むねとし）
LINE株式会社
LINE Platform Developmentセンター2 モバイルエクスペリエンス開発室
ディベロッパーエクスペリエンス開発チーム
シニアソフトウェアエンジニア

■SoftBank Tech Nightについて
ソフトバンク株式会社のエンジニア有志が開催するテックイベントです。
各分野のエキスパートが、日頃培った技術や事例、知見について発信しています。
イベントは開催スケジュールはconnpassをご確認ください。
https://sbtechnight.connpass.com/

SoftBank Tech Night Fes 2023公式サイト
https://www.softbank.jp/biz/events/tech-night-fes-2023/

Transcript

1. Munetoshi Ishikawa, LINE Corporation
   Code review techniques for readability

   ՄಡੑΛߴΊΔͨΊͷίʔυϨϏϡʔςΫχοΫ
   

   View Slide

2. Objective of code review
   Detects points that may be overlooked by the author
   
   - Find problems: bug, security or performance issues
   
   - Improve readability
   

   View Slide

3. 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
   

   View Slide

4. 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
   

   View Slide

5. 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
   

   View Slide

6. 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
   

   View Slide

7. 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
   

   View Slide

8. Example: Inconsistent name
   May need to rename existing properties when a new one is added
   class FooEntry(
   val !!",
   val timestampInMillis: Long,
   !!"
   )
   

   View Slide

9. 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
   

   View Slide

10. 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
    

    View Slide

11. 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
    

    View Slide

12. 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
    

    View Slide

13. 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
    

    View Slide

14. 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
    

    View Slide

15. 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."
    }
    }
    

    View Slide

16. 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
    

    View Slide

17. 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."
    }
    

    View Slide

18. 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
    

    View Slide

19. 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)
    

    View Slide

20. 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
    

    View Slide

21. 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)
    

    View Slide

22. 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
    

    View Slide

23. 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
    

    View Slide

24. 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 || ...) {
    ...
    }
    

    View Slide

25. How to make the code scannable
    - Renaming, writing comments
    
    - Restructuring
    val cachedMessageModel = messageCache[messageKey]
    ...
    if (cachedMessageModel == null || ...) {
    ...
    }
    

    View Slide

26. Scan the code: Summary
    Check the code is readable without understanding the details
    
    - "Got it!" is a bad signal
    

    View Slide

27. 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
    

    View Slide

28. 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
    

    View Slide

29. 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...
    

    View Slide

30. 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)
    

    View Slide

31. 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?"
    ???
    

    View Slide

32. Aside: Other applications of emulation
    - Document writing: PRD, Design doc
    
    - Presentation
    
    - Discussion
    
    - etc.
    

    View Slide

33. 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
    

    View Slide

34. 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
    

    View Slide

35. 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
    

    View Slide

36. 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
    

    View Slide

37. 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
    

    View Slide

38. 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"
    

    View Slide

39. Use other communication tools: Summary
    Chat offline/online or using messaging tool
    
    - Leave a comment on the pull-request to explain the conclusion
    

    View Slide

40. 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
    

    View Slide

41. 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
    

    View Slide

42. Drawbacks of reviewing problematic pull requests
    - May overlook issues
    
    - Consumes too much reviewer's and author's resources
    

    View Slide

43. Actions for problematic pull requests
    1. Ask to close the pull request first

    

    

    

    

    

    2. Follow up for the next step
    

    View Slide

44. Options of the next steps
    - Split the pull-request into smaller ones
    
    - Write a document for design (design-doc)
    
    - Make skeleton classes first
    

    View Slide

45. 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
    }
    }
    

    View Slide

46. 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("!!"")
    }
    

    View Slide

47. Don't review problematic pull-requests
    Ask to close, and follow up
    
    - Save reviewer's and author's resources
    
    - Follow up is mandatory
    

    View Slide

48. 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
    

    View Slide

49. 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
    

    View Slide

50. Related materials
    

    View Slide

51. 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
    

    View Slide

52. 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
    

    View Slide