[Feature] 카테고리 로깅 추가#1534
Conversation
Walkthrough카테고리 분석용 상수가 추가되었으며, Changes카테고리 분석 이벤트
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| description = stringResource(R.string.timetable_description), | ||
| onClick = { onMenuClick(CategoryMenuId.TIMETABLE) } | ||
| onClick = { | ||
| EventLogger.logCampusClickEvent(AnalyticsConstant.Label.Category.CATEGORY_TIMETABLE, "시간표") |
There was a problem hiding this comment.
[Major] Clean Architecture 위반 — Composable에서 직접 애널리틱스 호출
현재 EventLogger.logCampusClickEvent()가 Composable 내부 onClick 람다에서 직접 호출되고 있습니다. 이 패턴은 세 가지 문제를 일으킵니다:
- Clean Architecture 위반: UI 레이어가 애널리틱스 부수 효과(side effect)를 직접 처리합니다. 이미
onMenuClick콜백이 ViewModel로 이벤트를 전달하는 구조인데, 애널리틱스만 Composable에 남아 책임이 분산됩니다. - 테스트 불가:
EventLogger는 싱글턴 객체이므로 단위 테스트에서 모킹이 불가능합니다. ViewModel에서 처리하면EventLogger를 인터페이스로 교체해 테스트할 수 있습니다. - 패턴 불일치: 이 프로젝트는 Orbit MVI를 사용하며, 모든 부수 효과는 ViewModel의
intent { }블록에서 처리해야 합니다.
개선안 — CategoryViewModel.onMenuClick으로 로깅 이동:
// CategoryViewModel.kt
fun onMenuClick(id: CategoryMenuId) = intent {
logCategoryAnalytics(id)
postSideEffect(CategorySideEffect.NavigateToMenu(id))
}
private fun logCategoryAnalytics(id: CategoryMenuId) {
val (label, value) = when (id) {
CategoryMenuId.TIMETABLE -> AnalyticsConstant.Label.Category.CATEGORY_TIMETABLE to "시간표"
CategoryMenuId.LOST_AND_FOUND -> AnalyticsConstant.Label.Category.CATEGORY_LOST_PROPERTY to "분실물"
CategoryMenuId.OPERATING_INFO -> AnalyticsConstant.Label.Category.CATEGORY_CAMPUS to "교내 시설물 정보"
CategoryMenuId.DINING -> AnalyticsConstant.Label.Category.CATEGORY_CAMPUS to "식단"
CategoryMenuId.STORE -> AnalyticsConstant.Label.Category.CATEGORY_CAMPUS to "주변상점"
CategoryMenuId.BUS_TIMETABLE -> AnalyticsConstant.Label.Category.CATEGORY_TRANSPORTATION to "버스 시간표"
CategoryMenuId.TRANSPORT_SEARCH -> AnalyticsConstant.Label.Category.CATEGORY_TRANSPORTATION to "교통편 조회하기"
CategoryMenuId.CALLVAN -> AnalyticsConstant.Label.Category.CATEGORY_TRANSPORTATION to "콜벤팟 모집"
CategoryMenuId.HOUSING -> AnalyticsConstant.Label.Category.CATEGORY_ETC to "복덕방"
CategoryMenuId.KOIN_BUSINESS -> AnalyticsConstant.Label.Category.CATEGORY_ETC to "코인 for Business"
}
EventLogger.logCampusClickEvent(label, value)
}이렇게 하면:
- Composable의
onClick은onMenuClick(CategoryMenuId.TIMETABLE)만 호출하면 됩니다. CategoryMenuId에서 label과 value를 모두 유도할 수 있어 UI가 어느 섹션에 있는지 신경 쓸 필요가 없습니다.when식이 exhaustive해져 새 enum 값 추가 시 컴파일 오류로 즉시 알 수 있습니다.
| CategoryMenuId.CALLVAN -> "콜벤팟 모집" | ||
| CategoryMenuId.HOUSING -> "복덕방" | ||
| CategoryMenuId.KOIN_BUSINESS -> "코인 for Business" | ||
| else -> null |
There was a problem hiding this comment.
[Major] enum when에 else -> null 사용 — 컴파일 타임 안전성 소실
CategoryMenuId는 enum class이므로, when 식에서 else를 사용하면 새 enum 값이 추가되거나 주석 처리된 DEPARTMENT_INFO가 해제될 때 컴파일러가 경고를 발생시키지 않습니다. 새 메뉴 ID가 조용히 null을 반환해 애널리틱스 이벤트가 무음으로 누락됩니다.
현재 TIMETABLE, LOST_AND_FOUND가 이 함수에 없어 else로 처리되고 있는데, 이를 명시적으로 나열하면 컴파일 타임 안전성을 확보할 수 있습니다.
| else -> null | |
| CategoryMenuId.TIMETABLE, CategoryMenuId.LOST_AND_FOUND -> null |
CategoryMenu.kt의 주석 처리된DEPARTMENT_INFO가 활성화될 경우에도, 명시적 exhaustivewhen이라면 컴파일 에러가 즉시 발생해 누락된 로깅을 사전에 방지할 수 있습니다.
| items = CAMPUS_MENUS, | ||
| onItemClick = { menu -> onMenuClick(menu.id) } | ||
| onItemClick = { menu -> | ||
| categoryMenuLogValue(menu.id)?.let { EventLogger.logCampusClickEvent(AnalyticsConstant.Label.Category.CATEGORY_CAMPUS, it) } |
There was a problem hiding this comment.
[Minor] ?.let으로 인한 묵음 누락 — 로깅 실패가 드러나지 않음
categoryMenuLogValue가 null을 반환하면 ?.let 블록 전체가 조용히 건너뛰어져 애널리틱스 이벤트가 발송되지 않습니다. 새 CategoryMenuId가 CAMPUS_MENUS에 추가되고 categoryMenuLogValue에 매핑되지 않았을 경우, 로그도 없이 지나쳐 버립니다.
가장 근본적인 해결은 상위 Major 코멘트처럼 ViewModel로 이동해 exhaustive when으로 처리하는 것입니다. 현재 구조를 유지해야 한다면, 누락 감지를 위해 최소한 다음처럼 경고를 남기는 것을 권장합니다:
onItemClick = { menu ->
val logValue = categoryMenuLogValue(menu.id)
if (logValue != null) {
EventLogger.logCampusClickEvent(AnalyticsConstant.Label.Category.CATEGORY_CAMPUS, logValue)
} else {
Timber.w("categoryMenuLogValue: unhandled CategoryMenuId = ${menu.id}")
}
onMenuClick(menu.id)
}
| description = stringResource(R.string.lost_and_found_description), | ||
| onClick = { onMenuClick(CategoryMenuId.LOST_AND_FOUND) } | ||
| onClick = { | ||
| EventLogger.logCampusClickEvent(AnalyticsConstant.Label.Category.CATEGORY_LOST_PROPERTY, "분실물") |
There was a problem hiding this comment.
[Minor] TIMETABLE / LOST_AND_FOUND 로깅 패턴 불일치
TIMETABLE(line 124)과 LOST_AND_FOUND(이 줄)은 하드코딩된 문자열 "분실물"을 onClick 람다 안에서 직접 사용하는 반면, 나머지 메뉴 항목들은 categoryMenuLogValue()를 통해 매핑됩니다. 두 가지 방식이 혼용되어 향후 유지보수 시 수정 누락이 발생할 수 있습니다.
상위 Major 코멘트의 개선안(ViewModel로 통합)을 따르면 이 불일치도 자연스럽게 해소됩니다.
| object Category { | ||
| const val CATEGORY_TIMETABLE = "category_timetable" | ||
| const val CATEGORY_LOST_PROPERTY = "category_lost_property" | ||
| const val CATEGORY_CAMPUS = "category_campus" | ||
| const val CATEGORY_TRANSPORTATION = "category_transportation" | ||
| const val CATEGORY_ETC = "category_etc" | ||
| } |
There was a problem hiding this comment.
[Info] 잘 작성된 코드 — Category 상수 구조
Label.Category nested object를 별도로 분리한 구조가 깔끔합니다. 기존 Label의 flat 상수들과 섞이지 않아 가독성이 좋고, 향후 카테고리 관련 이벤트가 늘어날 때 확장하기 용이한 구조입니다. 👍
|
KYM-P
left a comment
KYM-P
left a comment
There was a problem hiding this comment.
GOOD 추가적으로 해당 부분에 로깅 event 추가해주세요
2 participants
PR 개요
PR 체크리스트
작업사항
작업사항의 상세한 설명
논의 사항
스크린샷
추가내용
Summary by CodeRabbit