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) }

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 は「下書きとして確定した