Kotlin Fest 2025：コードレビュー問題集

こんにちは。Yahoo!オークションでAndroidアプリの開発を担当している高松です。

2025年11月1日（土）に開催されたKotlin Fest 2025にて、LINEヤフー株式会社は「ことりプラン」として協賛しました。本記事では、スポンサーブースで実施した「Code Review Challenge」で出題した各問題を解説します。会場で参加した方も、参加できなかった方も、「自分ならどうレビューするか」を考えながら読んでみてください。

The Art of Misprocessing Access Logs

Webバックエンドエンジニアの早坂です。1問目には以下のコードを出題しました。

open class AccessLog (
    open val clientIp: String,
    open val path: String,
    open val method: String,
)
  
data class LoggedInUserAccessLog (
    override val clientIp: String,
    override val path: String,
    override val method: String,
    val userId: String,
) : AccessLog(clientIp, path, method)
  
fun AccessLog.summarize(): String = "${this.method} '${this.path}'"
fun LoggedInUserAccessLog.summarize(): String = "@${userId}${this.method} '${this.path}'"
  
object LogRepository {
    // フォーマット: clientIp,userId,method,path
    fun fetchTodaysRawLogs(): List<String> = listOf(
        "192.168.100.12,user-456,POST,/api/update",
        "192.168.100.50,,GET,/login",
        "127.0.0.1,,GET,/health_check",
        "192.168.5.10,user-123,GET,/search",
        "192.168.120.30,user-123,POST,/api/data",
    )
}
  
fun parseAccessLog(line: String): AccessLog? {
    val parts = line.split(',')
    if (parts.size < 4) return null
    val clientIp = parts[0]
    val userId = parts[1]
    val method = parts[2].uppercase()
    val path = parts[3]
    // userId が含まれているものは LoggedInUserAccessLog、そうでないものは AccessLog
    return when (userId) {
        "" -> AccessLog(clientIp, path, method)
        else -> LoggedInUserAccessLog(clientIp, path, method, userId)
    }
}
  
class AccessLogProcessor {
    val output = mutableListOf<String>()
    var counter = 0
    fun process() {
        val logs: List<AccessLog> = LogRepository.fetchTodaysRawLogs()
            .map { parseAccessLog(it) }
            .filter { it != null }
            .map { it as AccessLog }
        // health check endpoint のログはスキップする
        logs.forEach { log ->
            if (log.path == "/health_check") {
                return
            }
            counter += 1
            output.add(log.summarize())
        }
    }
}
  
fun main() {
    val processor = AccessLogProcessor()
    processor.process()
    println(processor.output)
    println("Received ${processor.counter} requests.")
}

解説

この問題では、アクセスログをパースし、内容に応じて要約文字列を生成する処理を題材に、Kotlin における拡張関数、コレクション操作、制御構文の落とし穴をレビューしてもらいました。上記のコードはコンパイルは通過しますが、実際には意図しない挙動や設計上の問題をいくつか盛り込んであります。当日は多角的な視点から多くのレビューを寄せていただきましたが、ここでは出題時に想定していた主なレビューポイントを紹介します。

拡張関数と静的ディスパッチ

対象箇所

fun AccessLog.summarize(): String = ...
fun LoggedInUserAccessLog.summarize(): String = ...

問題点

Kotlin の拡張関数は仮想メソッドではなく、呼び出し時の静的な型に基づいて解決されます。今回のコードでは、logs の型が List<AccessLog> になっているため、実体が LoggedInUserAccessLog であっても、呼び出されるのは AccessLog に対する summarize 関数です。その結果、ログイン済みユーザー用に定義した summarize が使われず、userId を含まない出力になります。クラスごとに振る舞いを変えたい意図と、実際の動作が一致していません。

改善案

ポリモーフィズムを期待する場合は、open fun と override を使ってメンバー関数として定義します。これにより、実行時の実体型に応じたメソッドが呼び出されます。

open class AccessLog(...) {
    open fun summarize(): String = ...
}
 
class LoggedInUserAccessLog(...) : AccessLog(...) {
    override fun summarize(): String = ...
}

return による forEach の途中終了

対象箇所

logs.forEach { log ->
    if (log.path == "/health_check") {
        return
    }
    ...
}

問題点

forEach の中で return を使うと、ラムダからではなく、process 関数自体からの return になります。そのため、health_check のログが出現した時点で process 関数全体が終了し、それ以降のログが処理されません。コメントからは、特定のログだけをスキップしたい意図が読み取れますが、実際の挙動は異なります。

改善案

continue に相当する挙動をさせたい場合は、通常の for ループを使うか、return@forEach のようにラベル付き return を使用します。

for (log in logs) {
    if (log.path == "/health_check") continue
    ...
}

冗長な null ハンドリング

対象箇所

.map { parseAccessLog(it) }
.filter { it != null }
.map { it as AccessLog }

問題点

parseAccessLog は null を返す可能性があるため、後段で null チェックとキャストが必要になっています。この書き方でも動作はしますが、Kotlin らしさという点では改善の余地があります。

改善案

mapNotNull を使うことで、null を除外しつつ型も確定させることができます。結果としてコードが短くなり、意図も明確になります。

val logs = LogRepository.fetchTodaysRawLogs()
.mapNotNull { parseAccessLog(it) }

mutable な変数の保持

対象箇所

class AccessLogProcessor {
    val output = mutableListOf<String>()
    var counter = 0

問題点

AccessLogProcessor は、処理結果を output という mutableList として保持しています。この設計では、process の実行後に、呼び出し側が output の中身を自由に変更できてしまいます。その結果、処理結果がどの時点の状態を表しているのかが不明確になり、意図しない変更が混入するリスクがあります。また、counter も var として保持され、ループ内でインクリメントされていますが、実際には output に追加された要素数と常に対応しています。独立した可変状態として持つ必然性は高くありません。

改善案

改善案としては、process を「入力を受け取り、結果を返す」純粋な関数に近づけることが考えられます。例えば、process が List を返すようにし、件数はその戻り値に対して count を取る形にすれば、クラス内部に mutable な状態を持たずに済みます。このように、不変のデータを基本とした設計にすると、状態の追跡が容易になり、テストや保守もしやすくなります。

The Role-Ordered Directory

Androidアプリエンジニアの高島です。2問目には以下のコードを出題しました。

enum class Role { ADMIN, EDITOR, VIEWER }
  
data class User(val id: Int, val name: String, val roles: List<Role>)
  
class UserDirectory {
    private val users = mutableListOf<User>()
  
    fun add(user: User) {
        if (!users.contains(user)) {
            users.add(user)
        }
    }
  
    fun duplicate(): UserDirectory {
        val d = UserDirectory()
        users.forEach { d.add(it.copy()) }
        return d
    }
  
    fun exportByRole(): List<User> {
        val admins = users.filter { Role.ADMIN in it.roles }
        val editors = users.filter { Role.EDITOR in it.roles }
        val viewers = users.filter { Role.VIEWER in it.roles }
        return admins + editors + viewers
    }
  
    fun allUserNames(): String = users.joinToString(", ") { it.name }
      
    fun sortByPrimaryRole(): List<User> =
        users.sortedBy { it.roles.firstOrNull()?.ordinal ?: Int.MAX_VALUE }
  
    // Utility function
    fun <T: Any> extractFrom(items: List<Any>, type: Class<T>): List<T> =
        items.filter { type.isInstance(it) }
            .map { type.cast(it) }
}
  
fun main() {
    val dir = UserDirectory()
    dir.add(User(1, "alice", mutableListOf(Role.ADMIN)))
    dir.add(User(2, "bob", mutableListOf(Role.EDITOR)))
    println(dir.exportByRole())
    println(dir.allUserNames())
    println(dir.sortByPrimaryRole())
  
    val mixed: List<Any> = listOf(1, "x", User(3, "charlie", listOf(Role.VIEWER)))
    val users = dir.extractFrom(mixed, User::class.java)
    println(users)
}

解説

この問題では、ユーザーとそのロール（ADMIN / EDITOR / VIEWER）を管理する UserDirectory を題材に、コードレビューの観点を考えてもらいました。UserDirectory は、ユーザーの追加や複製、ロールごとの並び替えやエクスポートなど、ありそうな管理処理をまとめたクラスです。当日はさまざまな観点からレビューしていただきましたが、ここでは意図的に仕込んでいたレビューポイントをいくつか紹介します。

List 結合による中間リスト生成

対象箇所

fun exportByRole(): List<User> {
    val admins = users.filter { Role.ADMIN in it.roles }
    val editors = users.filter { Role.EDITOR in it.roles }
    val viewers = users.filter { Role.VIEWER in it.roles }
    return admins + editors + viewers
}

問題点

+ による結合は、そのたびに新しい List を作って要素を詰め直すため、中間の List が増えやすくなります。処理内容は分かりやすいものの、データ量が増えた場合には不要なオーバーヘッドになりがちです。

改善案

buildList で 1 つの List に順番に addAll していく形にすると、結合時のコピーを減らせる場合があります（今回の例では件数が少ないため影響は小さいですが、件数が多い場合にはレビューで気にしたいポイントです）。

fun exportByRole(): List<User> = buildList {
    addAll(users.filter { Role.ADMIN in it.roles })
    addAll(users.filter { Role.EDITOR in it.roles })
    addAll(users.filter { Role.VIEWER in it.roles })
}

extractFrom を Kotlin らしい API に

対象箇所

fun <T: Any> extractFrom(items: List<Any>, type: Class<T>): List<T> =
    items.filter { type.isInstance(it) }
        .map { type.cast(it) }
 
// usage
val users = dir.extractFrom(mixed, User::class.java)

問題点

Java の Class<t> を引数に取る API になっており、Kotlin から見るとやや冗長です。呼び出し側で User::class.java を渡す必要があり、Kotlin を使っているメリットが薄れてしまいます。</t>

改善案

inline と reified を使えば、型をそのまま指定でき、より簡潔に書けます。頻出ではありませんが、「こう書ける」という選択肢を知っておくと役立つ場面があります。

inline fun <reified T : Any> extractFrom(items: List<Any>): List<T> =
    items.filterIsInstance<T>()
 
// usage
val users = dir.extractFrom<User>(mixed)

mutableListOf と copy() の参照共有

対象箇所

fun duplicate(): UserDirectory {
    val d = UserDirectory()
    users.forEach { d.add(it.copy()) }
    return d
}
 
// fun main
dir.add(User(1, "alice", mutableListOf(Role.ADMIN)))

問題点

Userクラスのroles の型は List<role> ですが、main関数を見ると、実体として MutableList が渡されています。data class の copy() は参照のコピーなので、コピー元とコピー先で同じ MutableList を共有することになります。その結果、どちらか一方の roles を変更すると、もう一方にも影響が出るという、意図しない副作用が発生します。</role>

改善案

• Userクラスの実体生成時にroles に不変の List を渡す
• copy 時に roles の List を新しく作り直す

など、「copy したら独立したデータになる」という直感と実装を一致させる設計が安全です。

// fun main
dir.add(User(1, "alice", listOf(Role.ADMIN)))

ordinal 依存のソート順

対象箇所

enum class Role { ADMIN, EDITOR, VIEWER }
...
fun sortByPrimaryRole(): List<User> =
    users.sortedBy { it.roles.firstOrNull()?.ordinal ?: Int.MAX_VALUE }

問題点

ordinal は enum の定義順に依存するため、enum の並びを変更しただけでソート結果が変わってしまいます。また、「どの role を primary とするか」というルールが暗黙的で、コードから読み取りづらい点も気になります。

改善案

Roleの定義をする時に明示的に優先度を定義しましょう。こうしておくと、列挙子の並びを変更してもソート順が変わらず、安全です。

enum class Role(val priority: Int) {
    ADMIN(0), EDITOR(1), VIEWER(2)
}

Message Draft Saving Process

Androidアプリエンジニアの高松です。3問目には以下のコードを出題しました。

import java.util.Date
  
data class MessageDraft(
    var sender: String,
    var receiver: String,
    var content: String,
    var createdYear: Int? = null
)
  
class DraftSaver {
    private var lastDraft: MessageDraft? = null
    fun save(sender: String, receiver: String, content: String): MessageDraft {
        return MessageDraft(sender, receiver, content).run {
            val trimmed = content.trim()
            val safeContent = if (trimmed.isEmpty()) "No message" else trimmed
            val now = Date()
            val year = now.year + 1900
            val draft = this.also {
                it.content = safeContent
                it.createdYear = year
            }
            this@DraftSaver.lastDraft = draft
            draft
        }
    }
  
    fun getLastDraftSummary(): String? {
        val draft = lastDraft ?: return null
        val preview = draft.content.substring(15) + if (draft.content.length > 15) "..." else ""
        return "${draft.sender} →${draft.receiver}: $preview (${draft.createdYear})"
    }
  
    fun wasLastDraftEmpty(): Boolean {
        return lastDraft?.let {
            val text = it.content.ifEmpty { "No message" }
            text.contains("no", ignoreCase = true)
        } ?: false
    }
}
  
fun main() {
    val saver = DraftSaver()
    val draft = saver.save("Alice", "Bob", "   ")
    val summary = saver.getLastDraftSummary()
    val check = saver.wasLastDraftEmpty()
      
    println(draft)
    println(summary)
    println(check)
}

解説

この問題は、メッセージの下書きを保存する DraftSaver を題材にしたコードレビューです。save() で本文の整形や作成年の付与、直近ドラフトの保持を行い、getLastDraftSummary() でプレビュー付きのサマリを返し、wasLastDraftEmpty() で「空メッセージだったか」を判定する想定になっています。一見すると動きそうに見える一方で、保守性・可読性・安全性の観点で改善したい点がいくつかあります。以下では、レビューで指摘したいポイントと、改善の方向性を順に紹介します。

非推奨 API（Date().year）の使用

対象箇所

val now = Date()
val year = now.year + 1900

問題点

Date().year は非推奨 API であり、将来的な互換性や保守性の観点から避けたい実装です。加えて + 1900 の補正が必要になる点も直感的ではなく、補正の意図がコードから読み取りづらくなります。

改善案

年月日などの扱いは java.time（LocalDate など）を使うのが安全で読みやすいです。

val now = LocalDate.now()
val year = now.year

スコープ関数の多用による可読性低下

対象箇所

MessageDraft(sender, receiver, content).run {
    ...
    val draft = this.also { ... }
    ...
}

問題点

run の中で also を使ってインスタンスを書き換える構造は、this / it の対象が切り替わりやすく、ネストも深くなります。スコープ関数自体は便利ですが、過剰に使うと「何を操作していて、何を返したいのか」が追いづらくなりがちです。

改善案

「値の計算 → インスタンス生成 → 代入/返却」という素直な流れにすると意図が明確になります。スコープ関数は「短く、対象が一目で分かる」範囲に絞ると読みやすさが保てます。

fun save(sender: String, receiver: String, content: String): MessageDraft {
    val safeContent = content.trim().ifEmpty { DEFAULT_MESSAGE }
    val year = LocalDate.now().year
 
    val draft = MessageDraft(
        sender = sender,
        receiver = receiver,
        content = safeContent,
        createdYear = year
    )
 
    lastDraft = draft
    return draft
}

also 内でのインスタンス更新

対象箇所

val draft = this.also {
    it.content = safeContent
    it.createdYear = year
}

問題点

also はオブジェクトを返しつつブロック内で変更もできるため、今回のようにブロック内で値を更新すると「返ってくるのは同じインスタンスだけど中身が変わっている」状態になります。その結果、処理の意図や値の確定タイミングが読み取りづらくなります。

改善案

値を整形したら、その値で新しいインスタンスを生成して返す形にすると明確です。既存インスタンスをベースにするなら copy() を使って変更点を表現します。

val draft = this.copy(
    content = safeContent,
    createdYear = year
)

lastDraft がスレッドセーフではない

対象箇所

private var lastDraft: MessageDraft? = null

問題点

private var lastDraft: MessageDraft? は複数スレッドから読み書きされると不整合が起こる可能性があります。今回の問題では「単純なサンプル」として見逃しがちですが、実運用では事故につながりやすいポイントです。

改善案

ReentrantLock を用いて、lastDraft の更新と参照を同一ロックで排他制御します。これにより、読み取り中に別スレッドから書き換えられる状況を防げます。

private val lock = ReentrantLock()
 
fun save(sender: String, receiver: String, content: String): MessageDraft {
    val draft = /* 下書きの生成（省略） */
    lock.withLock { lastDraft = draft }
    return draft
}

空メッセージ判定ロジックの誤り

対象箇所

text.contains("no", ignoreCase = true)

問題点

wasLastDraftEmpty() が text.contains("no", ignoreCase = true) だと、"No problem" のように "no" を含む通常のメッセージまで「空」と判定してしまいます。空判定の意図に対して条件が広すぎるため、誤判定が起きやすくなります。

改善案

空のときに代入する文言（例：NO_MESSAGE_TEXT）を定数化し、lastDraft?.content == NO_MESSAGE_TEXT のように明確な条件で判定します。

private const val NO_MESSAGE_TEXT = "No message"
 
class DraftSaver {
    private var lastDraft: MessageDraft? = null
 
    fun wasLastDraftEmpty(): Boolean {
        return lastDraft?.content == NO_MESSAGE_TEXT
    }
}

var の多用（不変にできるものは val に）

対象箇所

data class MessageDraft(
    var sender: String,
    var receiver: String,
    var content: String,
    var createdYear: Int? = null
)

問題点

初期化時点で値が確定しているプロパティまで var にすると、後からどこでも変更できてしまい意図しない変更のリスクが上がります。また createdYear: Int? が必ず設定される前提なら nullable にする理由が薄く、呼び出し側に無駄な null ハンドリングを要求します。

改善案

MessageDraft は「下書きとして確定した状態」を表すデータなので、基本は val にして不変にしておくと安心です。createdYear は Int にし、生成時に必ず値を入れる形にするとシンプルになります。

data class MessageDraft(
    val sender: String,
    val receiver: String,
    val content: String,
    val createdYear: Int
)

マジックリテラル・マジックナンバーの使用

対象箇所

"No message"
draft.content.length > 15

問題点

"No message" や 15 のような値がコード中に直書きされていると、値の意味が読み手に伝わりにくくなります。また同じ値が複数箇所に散らばると、文言変更や表示仕様変更の際に修正漏れが起きやすく、保守性が下がります。

改善案

定数として定義し、意味を名前で表現します。結果として可読性が上がり、変更時の影響範囲も最小化できます。

const val NO_MESSAGE_TEXT = "No message"
const val PREVIEW_MAX_LENGTH = 15

プレビュー生成のバグ（substring(15)）

対象箇所

val preview = draft.content.substring(15) + if (draft.content.length > 15) "..." else ""

問題点

サマリに表示するプレビュー（本文の先頭数文字の抜粋）を作る処理で、substring(15) を使っているため、意図した「先頭15文字」ではなく「15文字目以降」が取得されてしまいます。また、本文が15文字未満の場合は StringIndexOutOfBoundsException が発生します。

改善案

「先頭N文字を取りたい」なら take(N) を使うと意図が明確で、N文字未満でも安全に動作します。省略記号（...）の付与も、元の本文がN文字を超えている場合にのみ付けるようにすると分かりやすくなります。あわせて 15 は定数化して意味を明確にします（例：PREVIEW_MAX_LENGTH）。

private const val PREVIEW_MAX_LENGTH = 15
 
val preview = draft.content
    .take(PREVIEW_MAX_LENGTH)
    .let { head ->
        if (draft.content.length > PREVIEW_MAX_LENGTH) "$head..." else head
    }

まとめ

本記事では、Kotlin Fest 2025 のスポンサーブースで実施した「Code Review Challenge」で出題した各問題の解説をまとめました。当日は多くの方に挑戦していただき、さまざまな観点のフィードバックをいただきました。ありがとうございました。今後も、より多くの方に楽しんでいただける企画を考えていく予定です。なお、準備や当日の様子など運営面については、関連記事で紹介しています。

関連記事：LINEヤフー Tech Blog『Kotlin Fest 2025にブース出展しました！』（公開日：2026年1月14日）