Upgrade to Pro — share decks privately, control downloads, hide ads and more …

オブジェクト指向コードレビューの新しいアプローチ

akkiee76
March 20, 2024

 オブジェクト指向コードレビューの新しいアプローチ

Object-Oriented Conference 2024 の「オブジェクト指向コードレビューの 新しいアプローチ」セッションの資料です。

アジェンダ
・オブジェクト指向コードレビューの手法
・コードレビューを強化する AI の応用

【Object-Oriented Conference 2024】
https://ooc.dev/2024/

【プロポーザル】
https://fortee.jp/oocon-2024/proposal/cfa1a92c-5659-4941-b9bc-c408bf3bd886

akkiee76

March 20, 2024
Tweet

More Decks by akkiee76

Other Decks in Programming

Transcript

  1. #ooc_2024 • Akihiko Sato • 𝕏 : @akkiee76 • Speaker

    Deck : @akkie76 自己紹介 懇親会を楽しみにしています 🍻 2
  2. #ooc_2024 コードレビューの7つの観点 観点 概要 Design 設計、アーキテクチャ Simplicity 理解容易性、可読性 Naming クラス、メソッド、変数などの命名

    Style コードスタイル Functionality 機能要件の充足 Test テストコードの妥当性 Document コメント、ドキュメントの妥当性 11
  3. #ooc_2024 単一責任原則違反例 fun add(product: Product): Boolean { if (product.id <

    0) { throw IllegalArgumentException() } if (product.name.isEmpty()) { throw IllegalArgumentException() } val tempPrice: Int = if (product.canDiscount) { manager.totalPrice + manager.getDiscountPrice(product.price) } else { manager.totalPrice + product.price } return if (tempPrice < 3000) { manager.totalPrice = manager.discountProducts.add(product) true } else { false } } fun add(product: Product): Boolean { if (product.id < 0) { // バリデーション 1 throw IllegalArgumentException() } if (product.name.isEmpty()) { // バリデーション 2 throw IllegalArgumentException() } val tempPrice: Int = if (product.canDiscount) { // 条件分岐 1 manager.totalPrice + manager.getDiscountPrice(product.price) } else { manager.totalPrice + product.price } return if (tempPrice < 3000) { // 条件分岐 2 manager.totalPrice = manager.discountProducts.add(product) true } else { false } } 4 つの責務を持つ関数 15
  4. #ooc_2024 fun add(product: Product) { validateProduct(product) if (canAdd(product)) { manager.totalPrice

    = manager.discountProducts.add(product) } } private fun validateProduct(product: Product) { // バリデーション if (product.id < 0 || product.name.isEmpty()) { throw IllegalArgumentException() } } private fun canAdd(product: Product): Boolean { // 条件分岐 val tempPrice = getTempPrice(product) return tempPrice >= 3000 } private fun getTempPrice(product: Product): Int { // 条件分岐 return if (product.canDiscount) { manager.totalPrice + manager.getDiscountPrice(product.price) } else { manager.totalPrice + product.price } } Design 指摘で責務分離を推奨 16
  5. #ooc_2024 密結合例 class Calculator { private val taxCalculator = TaxCalculator()

    fun calculateTotalPrice(price: Double): Double { val tax = taxCalculator.calculateTax(price) return price + tax } } class TaxCalculator { fun calculateTax(price: Double): Double { // 価格の10%を税金として計算する return price * 0.1 } } val calculator = Calculator() val totalPrice = calculator.calculateTotalPrice(100.0) TaxCalculatorクラスに依 存している 17
  6. #ooc_2024 class Calculator { private val taxCalculator = TaxCalculator() fun

    calculateTotalPrice(price: Double): Double { val tax = taxCalculator.calculateTax(price) return price + tax } } class TaxCalculator { fun calculateTax(price: Double): Double { // 価格の20%を税金として計算する return price * 0.1 return price * 0.2 } } val calculator = Calculator() val totalPrice = calculator.calculateTotalPrice(100.0) 密結合例 ロジックが変更されると 予期せぬ影響が・・・ Design 指摘で疎結合を推奨 18
  7. #ooc_2024 class Calculator(private val taxCalculator: TaxCalculator) { fun calculateTotalPrice(price: Double):

    Double { val tax = taxCalculator.calculateTax(price) return price + tax } } class TaxCalculator { fun calculateTax(price: Double): Double { // 価格の10%を税金として計算する return price * 0.1 } } val taxCalculator = TaxCalculator() val calculator = Calculator(taxCalculator) val totalPrice = calculator.calculateTotalPrice(100.0) 密結合例 依存関係を注入し 疎結合へ設計変更を推奨 19
  8. #ooc_2024 2. Simplicity Simplicity 指摘に該当するケース • 分岐が多い / ネストが深い if

    文 • 複雑な三項演算子文 • stream, filter, map を多用した Object 整形文 • 冗長な SQL など 21
  9. #ooc_2024 分岐が多い if 文例 val powerRate: Float = member.powerRate /

    menber.maxPowerRate var currentCondition: Condition = Condition.DEFAULT if (powerRate == 0) { currentCondition = Condition.DEAD } else if (powerRate < 0.3) { currentCondition = Condition.DANGER } else if (powerRate < 0.5) { currentCondition = Condition.WARNING } else { currentCondition = Condition.GOOD } return currentCondition val powerRate: Float = member.powerRate / menber.maxPowerRate var currentCondition: Condition = Condition.DEFAULT if (powerRate == 0) { // 分岐 1 currentCondition = Condition.DEAD } else if (powerRate < 0.3) { // 分岐 2 currentCondition = Condition.DANGER } else if (powerRate < 0.5) { // 分岐 3 currentCondition = Condition.WARNING } else { // 分岐 4 currentCondition = Condition.GOOD } return currentCondition 早期 return でネストの解 消を推奨 22
  10. #ooc_2024 分岐が多い if 文例 val powerRate: Float = member.powerRate /

    menber.maxPowerRate if (powerRate == 0) { return Condition.DEAD } if (powerRate < 0.3) { return Condition.DANGER } if (powerRate < 0.5) { return Condition.WARNING } return Condition.GOOD 早期 return でネストを解消し可 読性アップ! 23
  11. #ooc_2024 複雑な三項演算子例 int compare(int a, int b, int c) {

    return (a > b) ? ((a > c) ? a : c) : ((b > c) ? b : c); } int compare(int a, int b, int c) { if (a > b) { if (a > c) { return a; } } else { if (b > c) { return b; } } return c; } int compare(int a, int b, int c) { return maxOf(a, maxOf(b, c)); } int maxOf(int value1, int value2) { return Math.max(value1, value2); } 理解容易性が向上 Math.max で 理解容易性が向上 int compare(int a, int b, int c) { return (a > b) ? ((a > c) ? a : c) : ((b > c) ? b : c); } 直感的に分かりにくい 三項演算子 24 int compare(int a, int b, int c) { if (a > b) { if (a > c) { return a; } } else { if (b > c) { return b; } } return c; }
  12. #ooc_2024 stream, filter, map を多用した Object 整形文例 val people =

    listOf( Person("Alice", 25, listOf(Skill("Kotlin", 3), Skill("Java", 4))), Person("Bob", 30, listOf(Skill("Kotlin", 4), Skill("Python", 3))), Person("Charlie", 20, listOf(Skill("Java", 2), Skill("JavaScript", 3))) // 以降も続く・・・ ) val targetLanguage = "Kotlin" val minSkillLevel = 4 val result = people.stream() .filter { person -> person.skills.any { it.language == targetLanguage && it.level >= minSkillLevel } } .map { it.name } .toList() 期待値が分かりにくいので 理解容易性を推奨 25
  13. #ooc_2024 stream, filter, map を多用した Object 整形文例 val people =

    listOf( Person("Alice", 25, listOf(Skill("Kotlin", 3), Skill("Java", 4))), Person("Bob", 30, listOf(Skill("Kotlin", 4), Skill("Python", 3))), Person("Charlie", 20, listOf(Skill("Java", 2), Skill("JavaScript", 3))) // 以降も続く・・・ ) val targetLanguage = "Kotlin" val minSkillLevel = 4 val proficientInTargetLanguage = { person: Person -> person.skills.any { it.language == targetLanguage && it.level >= minSkillLevel } } val result = people .filter(proficientInTargetLanguage) .map { it.name } .toList() 理解容易性が向上 26
  14. #ooc_2024 パフォーマンス比較例 // 金額をカンマ区切りに変換する fun formatAmount1(amount: Int): String { val

    amountStr = amount.toString() var formattedAmount = "" var count = 0 for (i in amountStr.length - 1 downTo 0) { formattedAmount = amountStr[i] + formattedAmount count++ if (count % 3 == 0 && i != 0) { formattedAmount = ",$formattedAmount" } } return formattedAmount } fun formatAmount2(amount: Int): String { return amount.toString().replace(Regex("(\\d)(?=(\\d{3})+(?!\\d))"), "$1,") } 1 回あたり 0.007ms 1 回あたり 0.063ms 35
  15. #ooc_2024 public class LoginController { private static final int MAX_PASSWORD_LENGTH

    = 10; } public class LoginControllerTest { private static final int MAX_PASSWORD_LENGTH = 10; @Test void test_validate_password() { // バリデーションの処理 assertEquals(10, MAX_PASSWORD_LENGTH); } } 定数がテストクラスに再定義されている例 private を維持するため 重複定義されてる public class LoginController { private static final int MAX_PASSWORD_LENGTH = 10; private static final int MAX_PASSWORD_LENGTH = 12; } public class LoginControllerTest { private static final int MAX_PASSWORD_LENGTH = 10; @Test void test_validate_password() { // バリデーションの処理 assertEquals(10, MAX_PASSWORD_LENGTH); } } 閾値の変更があった場合もテストが成功してしまう 😱 38
  16. #ooc_2024 定数がテストクラスに再定義されている例 public class LoginController { private static final int

    MAX_PASSWORD_LENGTH = 10; static final int MAX_PASSWORD_LENGTH = 10; } public class LoginControllerTest { private static final int MAX_PASSWORD_LENGTH = 10; @Test void test_validate_password() { // バリデーションの処理 assertEquals(10, MAX_PASSWORD_LENGTH); } } package private で 同じ定数を 参照するように推奨 39
  17. #ooc_2024 PR-Agent とは? Pull Request を AI でレビューを行えるツール • PRの概要作成(

    /describe ) • コードレビュー( /review ) • 改善提案( /improve ) Pull Request 作成時にこれらの実行が可能 🤖 48
  18. #ooc_2024 PR-Agent を利用するための準備 PR-Agentを利用するためには以下を準備します。 • OpenAI API key(従量課金設定) • GitHub

    personal access token • pr-agent-review.yaml ◦ https://github.com/Codium-ai/pr-agent/blob/main/.github/workflows/pr-agent-review.yaml GitHub Action から OpenAI API で gpt-4による レビューを行うため利用規約を必ずご確認ください https://github.com/Codium-ai/pr-agent?tab=readme-ov-file#data-privacy 49
  19. #ooc_2024 name: PR-Agent on: pull_request: types: [opened, reopened, synchronize] #

    issue_comment: workflow_dispatch: permissions: # issues: write pull-requests: write jobs: pr_agent_job: runs-on: ubuntu-latest name: Run pr agent on every pull request steps: - name: PR Agent action step id: pragent uses: Codium-ai/pr-agent@main env: GITHUB_ACTION_CONFIG.AUTO_DESCRIBE: true GITHUB_ACTION_CONFIG.AUTO_REVIEW: true GITHUB_ACTION_CONFIG.AUTO_IMPROVE: true 実行タイミングを追加 PRへの書込み権限を付与 describe, review, improve を有効にする 50
  20. #ooc_2024 jobs: pr_agent_job: env: INSTRUCTIONS: >- - 日本語で回答してください - 以下の観点で指摘を分類してください

    - Design - 単一責任の原則違反、密結合、低凝集などの設計的な指摘 - Simplicity - 理解容易性、可読性観点の指摘 - Naming - クラス、メソッド、変数など命名が内容に相応しいかどうかの指摘 - Style - コードスタイルに問題がないかどうかの指摘 - Functionality - 実装されたコードが期待した挙動になるか、コードに不具合がないか - Test - 閾値のテストができているか、コードの分岐がテストコードで担保されているか - Document - コードに対する説明が適切かどうか - 上記の指摘観点をCategory欄に記載すること - 上記の指摘観点をprefixに付けること。Design観点の場合はコメントにDesign:とprefixを付けること steps: env: PR_REVIEWER.EXTRA_INSTRUCTIONS: ${{ env.INSTRUCTIONS }} PR_CODE_SUGGESTIONS.EXTRA_INSTRUCTIONS: ${{ env.INSTRUCTIONS }} レビュー観点の プロンプトを追加 具体的に例示する 51
  21. #ooc_2024 val x = 10 val y = 20 val

    z = 30 val result = if (x > y) { if (x > z) { if (y > z) { "x is the largest: $x" } else { "y is the largest: $y" } } else { "z is the largest: $z" } } else { if (y > z) { if (x > z) { "x is the largest: $x" } else { "z is the largest: $z" } } else { "y is the largest: $y" } } println(result) 意味のない定数名 ネストの深い 三項演算子 サンプルコードをコミットして PRを作成 🛠 53
  22. #ooc_2024 Simplicity -val result = if (x > y) {

    - if (x > z) { - if (y > z) { - "x is the largest: $x" - } else { - "y is the largest: $y" - } - } else { - "z is the largest: $z" - } -} else { - if (y > z) { - if (x > z) { - "x is the largest: $x" - } else { - "z is the largest: $z" - } - } else { - "y is the largest: $y" - } -} +val largest = maxOf(x, y, z) +val result = "The largest: $largest" 複数の入れ子になった if 文を使用す る代わりに、maxOf 関数を使用して コードを簡素化することを提案しま す。 これにより、コードの可読性が向上 し、保守性が高まります。 57
  23. #ooc_2024 Naming -val x = 10 -val y = 20

    -val z = 30 +val firstNumber = 10 +val secondNumber = 20 +val thirdNumber = 30 x、y、zの命名が一般的すぎます。 これらの変数が何を表しているのか をより明確にするために、より具体的 な名前を使用することを検討してくだ さい。 58
  24. #ooc_2024 Design fun main() { val x = 10 val

    y = 20 val z = 30 - ... + val result = calculateLargest(x, y, z) println(result) } +fun calculateLargest(a: Int, b: Int, c: Int): String { + val largest = maxOf(a, b, c) + return "The largest: $largest" +} + この関数は単一のタスク(最大値の 計算と表示)を実行していますが、将 来的に機能が拡張される可能性を考 慮して、最大値を計算するロジックを 別の関数に分割することを検討してく ださい。 これにより、単一責任の原則に従い、 コードの再利用性とテスト容易性が 向上します。 59