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

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
 ՄಡੑΛߴΊΔͨΊͷίʔυϨϏϡʔςΫχοΫ
2. Objective of code review Detects points that may be overlooked by the author - Find problems: bug, security or performance issues - Improve readability
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
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
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
6. Check out a pull-request May overlook issues only by looking at the changed code private fun storeMessages(messages: List<MessageModel>) { if (messages.isEmpty) { return } ... } fun existingCaller(messages: List<MessageModel>) { if (messages.isNotEmpty) { storeMessages(messages) } } newly added code duplicated conditional branch
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
8. Example: Inconsistent name May need to rename existing properties when a new one is added class FooEntry( val !!", val timestampInMillis: Long, !!" )
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
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
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
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
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
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
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." } }
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
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." }
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
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)
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
21. Example 1/2: Method chain, good case Can understand the return value without reading the chain val logCountByUser: Map<User, Int> = userActionLog .groupBy { log -> log.user } .map { (user, logs) -> user to logs.size } val userListSortedByLogCount: List<User> = logCountByUser .sortedBy { (_, messageCount) -> messageCount } .map { (userId, _) -> userId } return userListSortedByLogCount.takeLast(10)
22. Example 1/2: Method chain, good case Can understand the return value without reading the chain val logCountByUser: Map<User, Int> = userActionLog ... ... val userListSortedByLogCount: List<User> = logCountByUser ... ... return userListSortedByLogCount.takeLast(10) Code summary: Return the 10 users with the highest log count
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
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 || ...) { ... }
25. How to make the code scannable - Renaming, writing comments - Restructuring val cachedMessageModel = messageCache[messageKey] ... if (cachedMessageModel == null || ...) { ... }
26. Scan the code: Summary Check the code is readable without understanding the details - "Got it!" is a bad signal
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
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
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...
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)
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?" ???
32. Aside: Other applications of emulation - Document writing: PRD, Design doc - Presentation - Discussion - etc.
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
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
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
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
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
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"
39. Use other communication tools: Summary Chat offline/online or using messaging tool - Leave a comment on the pull-request to explain the conclusion
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
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
42. Drawbacks of reviewing problematic pull requests - May overlook issues - Consumes too much reviewer's and author's resources
43. Actions for problematic pull requests 1. Ask to close the pull request first
 
 
 
 
 
 2. Follow up for the next step
44. Options of the next steps - Split the pull-request into smaller ones - Write a document for design (design-doc) - Make skeleton classes first
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 } }
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("!!"") }
47. Don't review problematic pull-requests Ask to close, and follow up - Save reviewer's and author's resources - Follow up is mandatory
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
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
50. Related materials
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
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