Conversation
|
""" Walkthrough이 변경 사항은 알람 기능의 시간 관리 및 표시 방식을 전반적으로 리팩토링합니다. 주요 변경점은 AM/PM 기반 12시간제에서 24시간제 및 Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI(OrbitPicker 등)
participant ViewModel
participant Formatter as AlarmDateTimeFormatter
participant Scheduler as AlarmScheduler/AlarmTimeCalculator
participant DB as Database
UI->>ViewModel: 시간/요일/미션 등 입력(LocalTime)
ViewModel->>Formatter: 알람 메시지/남은 시간 포매팅 요청
Formatter->>Scheduler: 다음 알람 트리거 시간 계산
Scheduler->>DB: 알람 정보 조회/갱신
DB-->>Scheduler: 알람 데이터 반환
Scheduler-->>Formatter: 계산된 트리거 시간 반환
Formatter-->>ViewModel: 포매팅된 메시지 반환
ViewModel-->>UI: UI 상태 갱신(남은 시간, 알람 리스트 등)
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
core/database/src/main/java/com/yapp/database/AlarmEntity.kt (1)
41-56: 변환 함수에 누락된 필드 매핑 추가 필요
AlarmEntity에 새로 추가된missionType과missionCount필드가toDomain()과toEntity()변환 함수에서 매핑되지 않고 있습니다. 도메인 모델과 엔티티 간의 데이터 일관성을 위해 매핑을 추가해야 합니다.fun AlarmEntity.toDomain() = Alarm( id = id, hour = hour, minute = minute, second = second, repeatDays = repeatDays, isHolidayAlarmOff = isHolidayAlarmOff, isSnoozeEnabled = isSnoozeEnabled, snoozeInterval = snoozeInterval, snoozeCount = snoozeCount, isVibrationEnabled = isVibrationEnabled, isSoundEnabled = isSoundEnabled, soundUri = soundUri, soundVolume = soundVolume, isAlarmActive = isAlarmActive, + missionType = missionType, + missionCount = missionCount, ) fun Alarm.toEntity() = AlarmEntity( id = id, hour = hour, minute = minute, second = second, repeatDays = repeatDays, isHolidayAlarmOff = isHolidayAlarmOff, isSnoozeEnabled = isSnoozeEnabled, snoozeInterval = snoozeInterval, snoozeCount = snoozeCount, isVibrationEnabled = isVibrationEnabled, isSoundEnabled = isSoundEnabled, soundUri = soundUri, soundVolume = soundVolume, isAlarmActive = isAlarmActive, + missionType = missionType, + missionCount = missionCount, )Also applies to: 58-73
feature/home/src/main/java/com/yapp/alarm/addedit/AlarmAddEditContract.kt (1)
121-142: missionType이 Alarm 객체에 포함되지 않았습니다.
missionState.missionType이toAlarm()함수에서 사용되지 않고 있습니다. Alarm 도메인 모델에 missionType 필드가 추가되었다면 여기서도 설정해야 합니다.internal fun AlarmAddEditContract.State.toAlarm(id: Long = 0): Alarm { return Alarm( id = id, hour = timeState.currentTime.hour, minute = timeState.currentTime.minute, repeatDays = daySelectionState.selectedDays.toRepeatDays(), isHolidayAlarmOff = holidayState.isDisableHolidayChecked, + missionType = missionState.missionType, isSnoozeEnabled = snoozeState.isSnoozeEnabled,
🧹 Nitpick comments (11)
core/database/src/androidTest/java/com/yapp/database/MigrationTest.kt (1)
50-50: 데이터베이스 마이그레이션 테스트에서isAm설정이 불필요합니다.
isAm컬럼이 완전히 제거되는 마이그레이션이므로, 테스트 데이터에서isAm값을 설정하는 것은 의미가 없습니다. 마이그레이션 후 이 컬럼은 존재하지 않습니다.feature/onboarding/src/main/java/com/yapp/onboarding/OnboardingContract.kt (1)
10-10: 알람 시간 기본값 검토 필요새벽 1시(01:00)를 기본값으로 설정했는데, 사용자 경험 측면에서 더 적절한 시간(예: 오전 7시)을 고려해볼 필요가 있습니다.
- val selectedTime: LocalTime = LocalTime.of(1, 0), + val selectedTime: LocalTime = LocalTime.of(7, 0),core/ui/src/main/java/com/yapp/ui/component/timepicker/OrbitYearMonthPicker.kt (1)
44-53: 인라인으로 간소화할 수 있습니다.
startIndexYear와startIndexMonth는 한 번만 사용되므로 직접rememberPickerState에 전달할 수 있습니다.- val startIndexYear = yearItems.indexOf(initialYear).coerceAtLeast(0) - val startIndexMonth = monthItems.indexOf(initialMonth).coerceAtLeast(0) - val lunarState = remember { mutableStateOf(initialLunar) } val yearState = remember { mutableIntStateOf(initialYear.toInt()) } val monthState = remember { mutableIntStateOf(initialMonth.toInt()) } val dayState = remember { mutableIntStateOf(initialDay.toInt()) } - val yearPickerState = rememberPickerState(initialIndex = startIndexYear, items = yearItems) - val monthPickerState = rememberPickerState(initialIndex = startIndexMonth, items = monthItems) + val yearPickerState = rememberPickerState( + initialIndex = yearItems.indexOf(initialYear).coerceAtLeast(0), + items = yearItems + ) + val monthPickerState = rememberPickerState( + initialIndex = monthItems.indexOf(initialMonth).coerceAtLeast(0), + items = monthItems + )core/ui/src/main/java/com/yapp/ui/component/timepicker/OrbitPicker.kt (3)
31-37: 더 구체적인 enum 이름 사용 고려
TimePeriod는 너무 일반적인 이름이어서 다른 모듈과 충돌할 가능성이 있습니다.AlarmTimePeriod또는PickerTimePeriod와 같이 더 구체적인 이름을 사용하는 것이 좋겠습니다.
43-44: LocalTime 사용으로 인한 개선 확인AM/PM, 시간, 분을 분리된 파라미터에서
LocalTime으로 통합한 것은 타입 안전성과 API 일관성 측면에서 훌륭한 개선입니다.테스트 시에는
LocalTime.now()대신 고정된 시간을 사용하는 것이 좋습니다. 테스트용 파라미터를 추가로 받거나 Clock 인스턴스를 주입받는 방식을 고려해보세요.
129-140: AM/PM 전환 로직에 주석 추가 권장11시와 12시 사이 전환 시에만 AM/PM을 토글하는 로직이 정확하게 구현되었습니다. 다만 이 로직의 의도를 명확히 하기 위해 주석을 추가하면 좋겠습니다.
scope.launch { val currentHour = hourPickerState.selectedItem val currentIndex = amPmPickerState.lazyListState.firstVisibleItemIndex % amPmItems.size val nextIndex = (currentIndex + 1) % amPmItems.size + // 11시에서 12시로, 또는 12시에서 11시로 전환될 때 AM/PM 토글 if ((currentHour == 12 && previousHour == 11) || (currentHour == 11 && previousHour == 12) ) { amPmPickerState.lazyListState.animateScrollToItem(nextIndex) } previousHour = currentHour }domain/src/main/java/com/yapp/domain/formatter/AlarmDateTimeFormatter.kt (2)
55-57: 매직 넘버 상수화 및 복잡도 개선방어 코드가 잘 작성되었지만, 매직 넘버(7)와 중첩된 조건문으로 인해 가독성이 떨어집니다.
+private const val MAX_DAYS_TO_CHECK = 7 // 오늘 알람이 가능한지 확인 if (selectedDaysOfWeek.contains(currentDayOfWeek) && todayAlarmDateTime.isAfter(now)) { return todayAlarmDateTime } -for (dayOffset in 0..7) { +for (dayOffset in 1..MAX_DAYS_TO_CHECK) { val nextPotentialDate = now.toLocalDate().plusDays(dayOffset.toLong()) - val dayOfWeekPotentialDate = nextPotentialDate.dayOfWeek - val potentialAlarmDateTime = nextPotentialDate.atTime(alarmTime) - - if (selectedDaysOfWeek.contains(dayOfWeekPotentialDate)) { - if (potentialAlarmDateTime.isAfter(now)) { - return potentialAlarmDateTime - } - } + if (selectedDaysOfWeek.contains(nextPotentialDate.dayOfWeek)) { + return nextPotentialDate.atTime(alarmTime) + } }Also applies to: 66-76
167-180: 시간 차이 포매팅 로직이 견고함Duration을 사용한 시간 차이 계산이 정확하게 구현되었습니다.
duration.toNanos() <= 0체크는 이미!futureTime.isAfter(baseTime)체크와 중복됩니다. 다음과 같이 단순화할 수 있습니다:if (!futureTime.isAfter(baseTime)) { return formats.soonFormat } val duration = Duration.between(baseTime, futureTime) - -if (duration.toNanos() <= 0) { - return formats.soonFormat -} - val totalMinutes = duration.toMinutes()core/ui/src/main/java/com/yapp/ui/component/timepicker/OrbitPickerItem.kt (1)
36-46: 제네릭 타입 지원으로 재사용성 향상컴포넌트를 제네릭으로 만들어 다양한 타입을 지원하도록 한 것은 훌륭한 개선입니다.
타입 파라미터에 제약을 추가하여 안전성을 높일 수 있습니다:
-fun <T> OrbitPickerItem( +fun <T : Any> OrbitPickerItem(feature/home/src/main/java/com/yapp/alarm/addedit/AlarmAddEditScreen.kt (2)
560-562: 반응형 UI 구현이 적절함화면 너비에 따라 버튼 크기를 조정하는 것은 좋은 접근입니다.
매직 넘버를 상수로 추출하면 더 명확할 것 같습니다:
+private const val COMPACT_SCREEN_WIDTH_DP = 360 modifier = Modifier.size( - if (screenWidthDp > 360.dp) 36.dp else 34.dp, + if (screenWidthDp > COMPACT_SCREEN_WIDTH_DP.dp) 36.dp else 34.dp, ),
419-427: 미션 선택 기능 미구현미션 타입은 표시되지만 선택 기능이 구현되지 않았습니다. PR 설명에 언급된 미완성 작업인 것으로 보입니다.
TODO 주석을 추가하여 향후 구현 예정임을 명시하는 것이 좋겠습니다:
AlarmAddEditSettingItem( label = stringResource(id = R.string.alarm_add_edit_mission), description = when (state.missionState.missionType) { MissionType.TAP -> stringResource(id = R.string.alarm_add_edit_selected_mission_tap) MissionType.SHAKE -> stringResource(id = R.string.alarm_add_edit_selected_mission_shake) else -> stringResource(id = R.string.alarm_add_edit_selected_mission_none) }, - onClick = { }, + onClick = { /* TODO: 미션 선택 기능 구현 */ }, )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
build-logic/src/main/java/com/yapp/convention/TestAndroid.kt(0 hunks)core/alarm/src/main/java/com/yapp/alarm/AlarmConstants.kt(0 hunks)core/alarm/src/main/java/com/yapp/alarm/AlarmTimeCalculator.kt(1 hunks)core/alarm/src/main/java/com/yapp/alarm/AndroidAlarmScheduler.kt(2 hunks)core/alarm/src/main/java/com/yapp/alarm/di/AlarmModule.kt(3 hunks)core/alarm/src/main/java/com/yapp/alarm/receivers/AlarmReceiver.kt(1 hunks)core/alarm/src/main/java/com/yapp/alarm/services/AlarmService.kt(1 hunks)core/alarm/src/test/kotlin/com/yapp/alarm/AlarmTimeCalculatorTest.kt(1 hunks)core/database/schemas/com.yapp.database.AlarmDatabase/2.json(3 hunks)core/database/src/androidTest/java/com/yapp/database/MigrationTest.kt(2 hunks)core/database/src/main/java/com/yapp/database/AlarmDao.kt(1 hunks)core/database/src/main/java/com/yapp/database/AlarmEntity.kt(2 hunks)core/database/src/main/java/com/yapp/database/DatabaseMigrations.kt(1 hunks)core/ui/src/main/java/com/yapp/ui/component/timepicker/OrbitPicker.kt(8 hunks)core/ui/src/main/java/com/yapp/ui/component/timepicker/OrbitPickerItem.kt(6 hunks)core/ui/src/main/java/com/yapp/ui/component/timepicker/OrbitYearMonthPicker.kt(2 hunks)core/ui/src/main/java/com/yapp/ui/component/timepicker/PickerState.kt(1 hunks)data/src/main/java/com/yapp/data/local/datasource/AlarmLocalDataSource.kt(1 hunks)data/src/main/java/com/yapp/data/local/datasource/AlarmLocalDataSourceImpl.kt(1 hunks)data/src/main/java/com/yapp/data/repositoryimpl/AlarmRepositoryImpl.kt(1 hunks)domain/src/main/java/com/yapp/domain/formatter/AlarmDateTimeFormatter.kt(1 hunks)domain/src/main/java/com/yapp/domain/model/Alarm.kt(2 hunks)domain/src/main/java/com/yapp/domain/model/MissionType.kt(1 hunks)domain/src/main/java/com/yapp/domain/repository/AlarmRepository.kt(1 hunks)domain/src/main/java/com/yapp/domain/usecase/AlarmUseCase.kt(2 hunks)domain/src/test/kotlin/com/yapp/domain/formatter/AlarmDateTimeFormatterTest.kt(1 hunks)feature/home/src/main/java/com/yapp/alarm/addedit/AlarmAddEditContract.kt(6 hunks)feature/home/src/main/java/com/yapp/alarm/addedit/AlarmAddEditScreen.kt(7 hunks)feature/home/src/main/java/com/yapp/alarm/addedit/AlarmAddEditViewModel.kt(12 hunks)feature/home/src/main/java/com/yapp/alarm/component/AlarmListItem.kt(2 hunks)feature/home/src/main/java/com/yapp/home/HomeScreen.kt(0 hunks)feature/home/src/main/java/com/yapp/home/HomeViewModel.kt(7 hunks)feature/home/src/main/java/com/yapp/home/component/bottomsheet/AlarmListBottomSheet.kt(0 hunks)feature/home/src/main/res/values/strings.xml(2 hunks)feature/mission/src/main/java/com/yapp/mission/MissionScreen.kt(5 hunks)feature/onboarding/src/main/java/com/yapp/onboarding/OnboardingAlarmTimeSelectionScreen.kt(5 hunks)feature/onboarding/src/main/java/com/yapp/onboarding/OnboardingContract.kt(2 hunks)feature/onboarding/src/main/java/com/yapp/onboarding/OnboardingViewModel.kt(4 hunks)gradle/libs.versions.toml(0 hunks)
💤 Files with no reviewable changes (5)
- build-logic/src/main/java/com/yapp/convention/TestAndroid.kt
- feature/home/src/main/java/com/yapp/home/component/bottomsheet/AlarmListBottomSheet.kt
- core/alarm/src/main/java/com/yapp/alarm/AlarmConstants.kt
- feature/home/src/main/java/com/yapp/home/HomeScreen.kt
- gradle/libs.versions.toml
🧰 Additional context used
🧬 Code Graph Analysis (6)
feature/onboarding/src/main/java/com/yapp/onboarding/OnboardingViewModel.kt (1)
feature/home/src/main/java/com/yapp/alarm/addedit/AlarmAddEditViewModel.kt (1)
setAlarmTime(279-290)
feature/onboarding/src/main/java/com/yapp/onboarding/OnboardingAlarmTimeSelectionScreen.kt (1)
feature/onboarding/src/main/java/com/yapp/onboarding/OnboardingViewModel.kt (1)
setAlarmTime(127-131)
feature/home/src/main/java/com/yapp/alarm/addedit/AlarmAddEditViewModel.kt (1)
feature/onboarding/src/main/java/com/yapp/onboarding/OnboardingViewModel.kt (1)
setAlarmTime(127-131)
core/ui/src/main/java/com/yapp/ui/component/timepicker/OrbitYearMonthPicker.kt (1)
core/ui/src/main/java/com/yapp/ui/component/timepicker/PickerState.kt (1)
rememberPickerState(27-32)
core/ui/src/main/java/com/yapp/ui/component/timepicker/OrbitPickerItem.kt (1)
core/ui/src/main/java/com/yapp/ui/component/timepicker/PickerState.kt (1)
rememberPickerState(27-32)
core/ui/src/main/java/com/yapp/ui/component/timepicker/OrbitPicker.kt (1)
core/ui/src/main/java/com/yapp/ui/component/timepicker/PickerState.kt (1)
rememberPickerState(27-32)
🔇 Additional comments (59)
core/alarm/src/main/java/com/yapp/alarm/services/AlarmService.kt (1)
106-106: 메서드 이름 변경이 리팩터링 목적과 일치합니다.
scheduleWeeklyAlarm에서rescheduleUpcomingWeeklyAlarm으로 변경하여 메서드의 의도를 더 명확하게 표현했습니다. 새로운AlarmTimeCalculator클래스와 함께 알람 스케줄링 로직이 분리된 아키텍처에 잘 맞습니다.core/database/src/androidTest/java/com/yapp/database/MigrationTest.kt (2)
76-76: 도메인 모델 변경에 맞춰 올바르게 수정되었습니다.
missionType이 문자열에서 정수로 변경된 것에 맞춰 테스트 검증도 적절히 수정되었습니다.
80-80: 리소스 정리가 적절히 추가되었습니다.명시적으로 데이터베이스 연결을 닫는 것은 좋은 습관입니다.
feature/home/src/main/res/values/strings.xml (2)
35-38: 미션 타입 문자열 리소스가 적절히 추가되었습니다.새로운
missionType필드와NONE타입 추가에 맞춰 필요한 문자열 리소스들이 잘 정의되었습니다. 일관된 네이밍 컨벤션을 따르고 있습니다.
101-104: 알람 남은 시간 표시 문자열이 잘 구성되었습니다.다양한 시간 단위(일/시간, 시간/분, 분만)에 대한 포맷팅 문자열이 체계적으로 정의되었습니다. 새로운
AlarmDateTimeFormatter클래스와 함께 사용될 것으로 보입니다.core/alarm/src/main/java/com/yapp/alarm/receivers/AlarmReceiver.kt (1)
155-155:isAm제거로 로직이 간소화되었습니다.24시간 형식을 직접 사용하여 AM/PM 구분 없이 시간을 처리하는 것이 더 간단하고 명확합니다. 전체 리팩터링 방향과 일치합니다.
core/database/schemas/com.yapp.database.AlarmDatabase/2.json (3)
9-9: 데이터베이스 스키마에서isAm컬럼이 성공적으로 제거되었습니다.24시간 형식 사용으로 AM/PM 구분이 불필요해져 스키마가 간소화되었습니다. 테이블 생성 SQL에서
isAm컬럼이 완전히 제거되어 목표한 리팩터링이 잘 반영되었습니다.
99-101: 미션 관련 필드의 기본값이 적절히 설정되었습니다.
missionType의 기본값 1과missionCount의 기본값 10은 도메인 모델의 기본값과 일치하며, 새로운 미션 기능을 지원하기 위한 적절한 설정입니다.Also applies to: 106-108
5-5: 스키마 버전 업데이트와 식별자 해시가 올바르게 갱신되었습니다.데이터베이스 스키마 변경에 따라 버전이 2로 업데이트되고 식별자 해시가 적절히 갱신되었습니다.
Also applies to: 120-120
domain/src/main/java/com/yapp/domain/model/Alarm.kt (2)
37-38: 미션 관련 필드 추가가 잘 구현되었습니다.기본값 설정이 적절해 보입니다.
MissionType.TAP과missionCount = 10은 사용자 경험 측면에서 합리적인 기본값입니다.
66-69: 24시간 형식 사용으로 시간 처리가 단순화되었습니다.
isAm필드 제거와 함께 시간 포맷팅 로직이 크게 단순화되어 유지보수성이 향상되었습니다.data/src/main/java/com/yapp/data/local/datasource/AlarmLocalDataSourceImpl.kt (1)
23-27: getAlarmsByTime 호출부 모두 업데이트됨 확인
isAm매개변수 제거가 도메인(usecase/repository), DAO, 데이터 소스, ViewModel 등 모든 레이어에 정상 반영되었습니다. 추가 검토나 수정이 필요 없습니다.data/src/main/java/com/yapp/data/repositoryimpl/AlarmRepositoryImpl.kt (1)
50-51: 데이터 소스 레이어와 일관성 있게 구현되었습니다.Repository 패턴이 올바르게 유지되며, 데이터 소스로의 위임이 적절합니다.
core/alarm/src/main/java/com/yapp/alarm/di/AlarmModule.kt (1)
27-35: 의존성 주입 설정이 우수합니다.
Clock과AlarmTimeCalculator의 싱글톤 제공이 적절하며, 특히Clock주입을 통해 테스트 가능성이 크게 향상되었습니다. 알람 시간 계산 로직을 별도 클래스로 분리한 것도 관심사 분리 원칙에 부합합니다.domain/src/main/java/com/yapp/domain/repository/AlarmRepository.kt (1)
18-18: 인터페이스 단순화가 적절합니다.
isAm매개변수 제거로 인터페이스가 더 직관적이 되었고, 구현체들과의 일관성도 유지되었습니다.core/database/src/main/java/com/yapp/database/DatabaseMigrations.kt (1)
9-58: 안전한 마이그레이션 구현!테이블 재생성 방식을 사용한 마이그레이션 구현이 적절합니다.
isAm컬럼 제거와 새로운 컬럼 추가를 안전하게 처리하고 있으며, 기본값 설정도 올바르게 되어 있습니다.domain/src/main/java/com/yapp/domain/model/MissionType.kt (1)
3-22: MissionType enum 변경 사항 확인됨
NONE타입 추가와 기본값 변경이 PR 목표에 맞게 잘 구현되었습니다. 알 수 없는 값에 대해NONE을 반환하는 것이 기존의TAP을 반환하는 것보다 더 안전한 접근입니다.feature/onboarding/src/main/java/com/yapp/onboarding/OnboardingViewModel.kt (1)
24-24: LocalTime 적용이 잘 완료되었습니다알람 시간 관리를
LocalTime으로 통합한 리팩토링이 깔끔하게 구현되었습니다. AM/PM 구분을 제거하고 24시간 형식으로 통일한 것이 코드 간소화에 도움이 됩니다.Also applies to: 53-53, 127-131, 139-140
feature/onboarding/src/main/java/com/yapp/onboarding/OnboardingContract.kt (2)
4-4: LocalTime 도입으로 타입 안전성 향상시간 표현에
LocalTime을 사용하여 타입 안전성과 코드 일관성이 향상되었습니다.
50-50: LocalTime 사용으로 액션 시그니처 간소화기존의 AM/PM, hour, minute 분리 파라미터에서
LocalTime단일 객체로 변경되어 코드가 간소화되었습니다.feature/home/src/main/java/com/yapp/alarm/component/AlarmListItem.kt (2)
230-235: AM/PM 표시 로직 내부화로 캡슐화 개선24시간 형식에서 AM/PM과 12시간 형식 표시를 내부적으로 계산하도록 변경되어 컴포넌트의 캡슐화가 개선되었습니다.
267-267: 12시간 형식 표시 사용계산된
displayHour를 사용하여 사용자에게 익숙한 12시간 형식으로 시간을 표시합니다.feature/onboarding/src/main/java/com/yapp/onboarding/OnboardingAlarmTimeSelectionScreen.kt (4)
23-23: LocalTime 도입으로 타입 안전성 향상시간 처리에
LocalTime을 사용하여 타입 안전성과 코드 일관성이 향상되었습니다.
61-63: 콜백 시그니처 간소화기존의 AM/PM, hour, minute 분리 파라미터에서
LocalTime단일 객체로 변경되어 콜백 시그니처가 간소화되었습니다.Also applies to: 73-73
104-106: OrbitPicker와 LocalTime 연동OrbitPicker가
LocalTime을 반환하도록 변경되어 타입 일관성이 유지됩니다.
120-120: 프리뷰 함수 업데이트프리뷰 함수가 새로운 시그니처에 맞게 적절히 업데이트되었습니다.
core/database/src/main/java/com/yapp/database/AlarmDao.kt (2)
25-25: 정렬 로직 간소화
isAm필드 제거로 정렬 로직이 간소화되었습니다. 24시간 형식의 hour와 minute만으로 충분한 정렬이 가능합니다.
28-29: 시간 필터링 간소화
isAm파라미터 제거로 시간 필터링이 간소화되었습니다. 24시간 형식 사용으로 더 직관적인 쿼리가 가능해졌습니다.data/src/main/java/com/yapp/data/local/datasource/AlarmLocalDataSource.kt (1)
11-11: 메서드 시그니처 간소화
isAm파라미터 제거로 메서드 시그니처가 간소화되었습니다. 24시간 형식 사용으로 더 직관적인 인터페이스가 되었습니다.feature/home/src/main/java/com/yapp/alarm/addedit/AlarmAddEditViewModel.kt (5)
69-78: 시간 상태 초기화가 적절하게 구현되었습니다.
LocalTime을 사용한 통합된 시간 표현으로의 리팩토링이 잘 되었습니다.
96-101: 기존 알람 로드 시 LocalTime 변환이 올바르게 구현되었습니다.
LocalTime.of(alarm.hour, alarm.minute)를 사용한 변환이 적절합니다.
212-218: 알람 스케줄링 책임이 적절히 위임되었습니다.
AlarmScheduler직접 사용 대신AlarmUseCase를 통한 스케줄링으로 변경되어 관심사 분리가 개선되었습니다.
279-290: setAlarmTime 메서드가 LocalTime을 사용하도록 적절히 리팩토링되었습니다.별도의 AM/PM, 시, 분 파라미터 대신
LocalTime객체를 받도록 변경되어 코드가 간결해졌습니다.
502-514: 알람 메시지 포맷팅이 도메인 레이어로 적절히 위임되었습니다.
AlarmDateTimeFormatter를 사용하여 시간 차이 계산 및 포맷팅 로직을 도메인 레이어로 이동한 것이 좋은 설계입니다. 리소스 문자열을 사용한 지역화 처리도 적절합니다.domain/src/test/kotlin/com/yapp/domain/formatter/AlarmDateTimeFormatterTest.kt (2)
32-119: 알람 배송 시간 포맷팅 테스트가 포괄적으로 작성되었습니다.다양한 날짜 시나리오(오늘, 내일, 올해, 내년)와 활성/비활성 알람 케이스를 잘 커버하고 있습니다.
128-167: 시간 차이 포맷팅 테스트가 엣지 케이스를 잘 다루고 있습니다.0분, 음수, 1분 미만, 그리고 다양한 시간 범위에 대한 테스트가 적절히 작성되었습니다.
core/alarm/src/main/java/com/yapp/alarm/AndroidAlarmScheduler.kt (2)
31-51: 알람 시간 계산 로직이 적절히 분리되었습니다.
AlarmTimeCalculator로 시간 계산을 위임하여AndroidAlarmScheduler의 책임이 순수하게 AlarmManager와의 상호작용으로 제한되었습니다. 좋은 설계 개선입니다.
53-62: 주간 알람 재스케줄링 메서드가 적절히 추가되었습니다.기존 패턴과 일관되게
AlarmTimeCalculator를 사용하여 구현되었습니다.core/ui/src/main/java/com/yapp/ui/component/timepicker/PickerState.kt (2)
10-25: PickerState의 제네릭 리팩토링이 잘 구현되었습니다.타입 안전성이 향상되었고, StateFlow를 사용한 반응형 상태 관리가 적절합니다.
updateSelectedIndex의 경계 검사와selectedItem의 안전한 접근 처리가 좋습니다.
28-32: rememberPickerState 함수가 제네릭으로 적절히 업데이트되었습니다.Compose의 remember 패턴을 올바르게 사용하고 있습니다.
core/alarm/src/test/kotlin/com/yapp/alarm/AlarmTimeCalculatorTest.kt (4)
11-77: 테스트 설정이 체계적으로 구성되었습니다.고정된 시간을 사용한 결정적 테스트와 명확한 변수 명명, 헬퍼 메서드를 통한 중복 제거가 잘 되어 있습니다.
79-109: 비반복 알람 테스트가 올바르게 작성되었습니다.오늘의 미래 시간과 과거 시간에 대한 처리가 정확히 검증되고 있습니다.
112-290: 공휴일 처리를 포함한 반복 알람 테스트가 포괄적입니다.공휴일 건너뛰기 옵션의 활성/비활성 상태에 따른 다양한 시나리오가 철저히 테스트되고 있습니다. 2025년 설 연휴를 활용한 테스트 케이스 선택이 적절합니다.
293-408: 주간 재스케줄링 테스트가 다양한 케이스를 잘 다루고 있습니다.일반적인 재스케줄링, 공휴일 처리, 주를 넘어가는 스케줄링 등 중요한 시나리오들이 모두 테스트되고 있습니다.
core/ui/src/main/java/com/yapp/ui/component/timepicker/OrbitYearMonthPicker.kt (2)
40-42: 리팩토링이 잘 되었습니다!리스트들을
remember로 감싸서 불필요한 재생성을 방지한 것이 좋습니다.
56-60: 일자 동적 계산이 잘 구현되었습니다!
remember의 의존성으로 연도와 월을 설정하여 변경 시마다 일수가 올바르게 재계산됩니다.feature/home/src/main/java/com/yapp/home/HomeViewModel.kt (2)
185-188: 알람 스케줄링 로직이 잘 분리되었습니다!
alarmUseCase로 스케줄링 로직을 위임하여 ViewModel의 책임이 명확해졌습니다.
318-331: 포맷팅 로직의 도메인 레이어 이동이 적절합니다!
AlarmDateTimeFormatter를 통해 복잡한 날짜/시간 계산 로직이 도메인 레이어로 이동되어 ViewModel의 책임이 명확해졌습니다.feature/home/src/main/java/com/yapp/alarm/addedit/AlarmAddEditContract.kt (2)
30-31: LocalTime 사용으로 시간 표현이 개선되었습니다!AM/PM과 시/분을 분리하던 방식에서
LocalTime으로 통합하여 시간 처리가 단순해졌습니다.
47-49: 미션 상태가 추가되었습니다.PR 설명에 미션 타입 선택 UI가 미완성이라고 되어 있는데, 이 부분의 구현 계획을 확인해 주세요.
core/alarm/src/main/java/com/yapp/alarm/AlarmTimeCalculator.kt (1)
44-63: 반복 알람 계산 로직이 잘 구현되었습니다!요일, 시간, 공휴일을 모두 고려하여 다음 알람 시간을 정확히 계산합니다.
domain/src/main/java/com/yapp/domain/usecase/AlarmUseCase.kt (1)
28-28: isAm 파라미터 제거가 적절합니다!24시간 형식으로 통합되어 API가 단순해졌습니다.
core/ui/src/main/java/com/yapp/ui/component/timepicker/OrbitPicker.kt (1)
179-186: 시간 변환 로직 검증 완료
OrbitPicker에서 구현된 AM/PM→24시간 변환(AM 12시→0, PM 12시→12, 그 외 시각 정상 처리)은 모든 엣지 케이스를 올바르게 처리하며, 다른 코드에 중복된 변환 로직도 없어 일관성이 확인되었습니다. 추가 조치 불필요합니다.domain/src/main/java/com/yapp/domain/formatter/AlarmDateTimeFormatter.kt (1)
143-154: 스트림 연산과 예외 처리가 적절함활성 알람 필터링과 예외 처리가 잘 구현되었습니다. 다만 앞서 언급한 로깅 문제를 일관되게 해결해야 합니다.
core/ui/src/main/java/com/yapp/ui/component/timepicker/OrbitPickerItem.kt (3)
115-116: LazyColumn 최적화가 적절함안정적인 key 사용과
derivedStateOf를 통한 상태 최적화가 잘 구현되었습니다.
177-178: 빈 리스트에 대한 방어 코드 추가
require문을 사용하여 빈 리스트를 체크하는 것은 좋은 방어적 프로그래밍입니다.
85-101: Null 안전성과 상태 업데이트 최적화옵셔널 체이닝과
distinctUntilChanged()를 사용하여 null 안전성과 성능이 개선되었습니다.feature/home/src/main/java/com/yapp/alarm/addedit/AlarmAddEditScreen.kt (2)
189-194: OrbitPicker API 개선 확인
LocalTime을 직접 사용하도록 변경되어 API가 더 간결하고 타입 안전해졌습니다.
706-734: Preview 구현 개선테마와 배경색을 추가하여 실제 화면과 더 유사한 Preview를 제공하는 것은 좋은 개선입니다.
core/database/src/androidTest/java/com/yapp/database/MigrationTest.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/com/yapp/domain/formatter/AlarmDateTimeFormatter.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/com/yapp/domain/formatter/AlarmDateTimeFormatter.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
domain/src/main/java/com/yapp/domain/formatter/AlarmDateTimeFormatter.kt (1)
159-172: 중복된 검증 로직을 단순화할 수 있습니다.동일한 조건을 여러 번 검사하고 있습니다.
다음과 같이 단순화하세요:
fun formatTimeDifference( baseTime: LocalDateTime, futureTime: LocalDateTime, formats: TimeDifferenceFormats, ): String { - if (!futureTime.isAfter(baseTime)) { - return formats.soonFormat - } - val duration = Duration.between(baseTime, futureTime) - - if (duration.toNanos() <= 0) { - return formats.soonFormat - } - val totalMinutes = duration.toMinutes() + if (totalMinutes < 1) { return formats.soonFormat }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/database/src/androidTest/java/com/yapp/database/MigrationTest.kt(2 hunks)core/database/src/main/java/com/yapp/database/AlarmEntity.kt(4 hunks)core/database/src/main/java/com/yapp/database/DatabaseMigrations.kt(1 hunks)domain/src/main/java/com/yapp/domain/formatter/AlarmDateTimeFormatter.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- core/database/src/androidTest/java/com/yapp/database/MigrationTest.kt
- core/database/src/main/java/com/yapp/database/AlarmEntity.kt
🧰 Additional context used
🪛 detekt (1.23.8)
domain/src/main/java/com/yapp/domain/formatter/AlarmDateTimeFormatter.kt
[warning] 126-126: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 141-141: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
core/database/src/main/java/com/yapp/database/DatabaseMigrations.kt (2)
5-5: 상수 사용으로 코드 일관성 향상
DATABASE_NAME상수를 사용하여 하드코딩을 방지하고 코드 일관성을 높인 것이 좋습니다.
11-33: 새 테이블 스키마 기본값 검증: missionType=TAP(1), missionCount 확인 필요
- core/database/src/main/java/com/yapp/database/DatabaseMigrations.kt:29
missionType INTEGER NOT NULL DEFAULT 1→ domain 모델에서MissionType.TAP(1)과 매핑됩니다.- domain/src/main/java/com/yapp/domain/model/MissionType.kt:4–6
NONE(0), TAP(1), SHAKE(2),- core/database/src/main/java/com/yapp/database/DatabaseMigrations.kt:30
missionCount INTEGER NOT NULL DEFAULT 10→ 코드상 상수나 enum이 없어, 비즈니스 요구사항 또는 원격 설정(remote config) 상의 기본값(10)이 적절한지 확인이 필요합니다.
core/database/src/main/java/com/yapp/database/DatabaseMigrations.kt
Outdated
Show resolved
Hide resolved
core/database/src/main/java/com/yapp/database/DatabaseMigrations.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/com/yapp/domain/formatter/AlarmDateTimeFormatter.kt
Show resolved
Hide resolved
domain/src/main/java/com/yapp/domain/formatter/AlarmDateTimeFormatter.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/com/yapp/domain/formatter/AlarmDateTimeFormatter.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
domain/src/main/java/com/yapp/domain/formatter/AlarmDateTimeFormatter.kt (2)
18-21: 매직 스트링 상수화가 잘 적용되었습니다.이전 리뷰 코멘트가 잘 반영되어 "NONE"과 날짜 포맷 문자열이 상수로 정의되었습니다.
125-129: 예외 처리 개선이 필요합니다.
DateTimeParseException은 예상 가능한 경우이지만, 일반Exception을 동일하게 처리하는 것은 예상치 못한 오류를 숨길 수 있습니다.} catch (e: DateTimeParseException) { formats.noAlarm } catch (e: Exception) { - formats.noAlarm + // 예상치 못한 예외는 상위로 전파하거나 더 구체적으로 처리 + throw IllegalStateException("알람 시간 포맷팅 중 예상치 못한 오류 발생", e) }
🧹 Nitpick comments (1)
domain/src/main/java/com/yapp/domain/formatter/AlarmDateTimeFormatter.kt (1)
139-145: 예외 처리 시 디버깅 정보가 필요합니다.현재 예외를 완전히 무시하고 있어 어떤 알람에서 문제가 발생했는지 추적이 어렵습니다.
.mapNotNull { alarm -> try { calculateNextOccurrence(alarm.hour, alarm.minute, alarm.repeatDays, now) } catch (e: Exception) { - null // 예외 발생 시 null로 처리 + // TODO: 로깅 인터페이스 도입 시 알람 ID와 함께 로깅 + // 현재는 null 반환으로 해당 알람만 제외 + null } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/alarm/src/main/java/com/yapp/alarm/di/AlarmModule.kt(3 hunks)domain/src/main/java/com/yapp/domain/di/ClockModule.kt(1 hunks)domain/src/main/java/com/yapp/domain/formatter/AlarmDateTimeFormatter.kt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- domain/src/main/java/com/yapp/domain/di/ClockModule.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- core/alarm/src/main/java/com/yapp/alarm/di/AlarmModule.kt
🧰 Additional context used
🪛 detekt (1.23.8)
domain/src/main/java/com/yapp/domain/formatter/AlarmDateTimeFormatter.kt
[warning] 127-127: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 142-142: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
domain/src/main/java/com/yapp/domain/formatter/AlarmDateTimeFormatter.kt (3)
1-16: 클린 아키텍처 원칙을 잘 준수했습니다!도메인 레이어에서 Android 의존성을 제거하고 표준 Java 시간 API를 사용한 것이 좋습니다.
41-84: 알람 다음 발생 시간 계산 로직이 견고합니다.
require()를 사용한 명시적 검증이 적절합니다- 단일 알람과 반복 알람의 처리가 명확히 구분되어 있습니다
- 7일 순회 후 도달할 수 없는 코드에
error()를 사용한 것이 좋습니다
155-184: 시간 차이 포맷팅 로직이 잘 구현되었습니다.엣지 케이스 처리가 적절하고, 다양한 시간 단위에 대한 포맷팅이 명확합니다.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
feature/home/src/main/java/com/yapp/home/alarm/addedit/AlarmAddEditContract.kt (1)
47-50: 미션 타입 상태가 추가되었습니다새로운
AlarmMissionState가 추가되어 미션 타입 기능을 위한 기반이 마련되었습니다. 기본값으로MissionType.TAP이 설정된 것이 적절합니다.미션 타입 선택을 위한 UI 구현이 필요하신가요? PR 설명에 따르면 이 부분이 아직 미완성인 것으로 보입니다.
feature/home/src/main/java/com/yapp/home/alarm/addedit/AlarmAddEditScreen.kt (1)
419-427: 미션 타입 선택 기능이 아직 구현되지 않은 것으로 보입니다.현재
onClick콜백이 비어있어 사용자가 미션 타입을 변경할 수 없습니다. PR 설명에 따르면 미션 타입 선택 UI는 아직 미완성이라고 하셨는데, 사용자 혼란을 방지하기 위해 다음 중 하나를 고려해보세요:
- TODO 주석 추가
- 아이템을 비활성화 상태로 표시
- 클릭 시 "준비 중" 메시지 표시
AlarmAddEditSettingItem( label = stringResource(id = R.string.alarm_add_edit_mission), description = when (state.missionState.missionType) { MissionType.TAP -> stringResource(id = R.string.alarm_add_edit_selected_mission_tap) MissionType.SHAKE -> stringResource(id = R.string.alarm_add_edit_selected_mission_shake) else -> stringResource(id = R.string.alarm_add_edit_selected_mission_none) }, - onClick = { }, + onClick = { + // TODO: 미션 타입 선택 기능 구현 예정 + }, )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
core/alarm/build.gradle.kts(1 hunks)core/common/src/main/java/com/yapp/common/di/ClockModule.kt(1 hunks)domain/src/main/java/com/yapp/domain/usecase/AlarmUseCase.kt(2 hunks)feature/alarm-interaction/src/main/java/com/yapp/alarm/interaction/action/AlarmActionContract.kt(0 hunks)feature/home/src/main/java/com/yapp/home/HomeContract.kt(0 hunks)feature/home/src/main/java/com/yapp/home/HomeNavGraph.kt(1 hunks)feature/home/src/main/java/com/yapp/home/HomeScreen.kt(1 hunks)feature/home/src/main/java/com/yapp/home/HomeViewModel.kt(8 hunks)feature/home/src/main/java/com/yapp/home/alarm/AlarmDayLabel.kt(1 hunks)feature/home/src/main/java/com/yapp/home/alarm/addedit/AlarmAddEditContract.kt(6 hunks)feature/home/src/main/java/com/yapp/home/alarm/addedit/AlarmAddEditScreen.kt(8 hunks)feature/home/src/main/java/com/yapp/home/alarm/addedit/AlarmAddEditViewModel.kt(14 hunks)feature/home/src/main/java/com/yapp/home/alarm/component/AlarmCheckItem.kt(3 hunks)feature/home/src/main/java/com/yapp/home/alarm/component/AlarmDayButton.kt(1 hunks)feature/home/src/main/java/com/yapp/home/alarm/component/AlarmListItem.kt(3 hunks)feature/home/src/main/java/com/yapp/home/alarm/component/bottomsheet/AlarmSnoozeBottomSheet.kt(1 hunks)feature/home/src/main/java/com/yapp/home/alarm/component/bottomsheet/AlarmSoundBottomSheet.kt(1 hunks)feature/home/src/main/java/com/yapp/home/component/bottomsheet/AlarmListBottomSheet.kt(1 hunks)feature/home/src/main/java/com/yapp/home/util/AlarmDateTimeFormatter.kt(1 hunks)feature/home/src/test/kotlin/com/yapp/home/util/AlarmDateTimeFormatterTest.kt(1 hunks)
💤 Files with no reviewable changes (2)
- feature/alarm-interaction/src/main/java/com/yapp/alarm/interaction/action/AlarmActionContract.kt
- feature/home/src/main/java/com/yapp/home/HomeContract.kt
✅ Files skipped from review due to trivial changes (8)
- feature/home/src/main/java/com/yapp/home/alarm/component/AlarmDayButton.kt
- core/alarm/build.gradle.kts
- feature/home/src/main/java/com/yapp/home/alarm/AlarmDayLabel.kt
- feature/home/src/main/java/com/yapp/home/alarm/component/bottomsheet/AlarmSnoozeBottomSheet.kt
- feature/home/src/main/java/com/yapp/home/HomeNavGraph.kt
- feature/home/src/main/java/com/yapp/home/alarm/component/bottomsheet/AlarmSoundBottomSheet.kt
- feature/home/src/main/java/com/yapp/home/alarm/component/AlarmCheckItem.kt
- core/common/src/main/java/com/yapp/common/di/ClockModule.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- feature/home/src/main/java/com/yapp/home/component/bottomsheet/AlarmListBottomSheet.kt
- feature/home/src/main/java/com/yapp/home/HomeScreen.kt
- feature/home/src/main/java/com/yapp/home/HomeViewModel.kt
- domain/src/main/java/com/yapp/domain/usecase/AlarmUseCase.kt
🧰 Additional context used
🧠 Learnings (2)
feature/home/src/test/kotlin/com/yapp/home/util/AlarmDateTimeFormatterTest.kt (1)
Learnt from: DongChyeon
PR: YAPP-Github/Orbit-Android#233
File: domain/src/main/java/com/yapp/domain/usecase/AlarmUseCase.kt:38-61
Timestamp: 2025-07-16T14:32:09.264Z
Learning: In the Orbit-Android project, the team prefers to focus unit testing on core business logic rather than UseCase wrapper methods to avoid unnecessary dependency mocking costs. They consider it sufficient to test the main formatting logic (like formatTimeDifference) rather than testing thin wrapper methods in UseCases.
feature/home/src/main/java/com/yapp/home/alarm/addedit/AlarmAddEditViewModel.kt (1)
Learnt from: DongChyeon
PR: YAPP-Github/Orbit-Android#233
File: domain/src/main/java/com/yapp/domain/usecase/AlarmUseCase.kt:38-61
Timestamp: 2025-07-16T14:32:09.264Z
Learning: In the Orbit-Android project, the team prefers to focus unit testing on core business logic rather than UseCase wrapper methods to avoid unnecessary dependency mocking costs. They consider it sufficient to test the main formatting logic (like formatTimeDifference) rather than testing thin wrapper methods in UseCases.
🪛 detekt (1.23.8)
feature/home/src/main/java/com/yapp/home/util/AlarmDateTimeFormatter.kt
[warning] 127-127: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 142-142: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (14)
feature/home/src/main/java/com/yapp/home/alarm/component/AlarmListItem.kt (2)
1-1: 패키지 구조 변경이 적절합니다알람 관련 UI 컴포넌트를
feature/home모듈 하위로 이동한 것은 모듈화 관점에서 올바른 결정입니다.
230-236: AM/PM 변환 로직이 올바르게 구현되었습니다24시간 형식에서 12시간 형식으로의 변환이 정확하게 처리되고 있습니다. 자정(0시)을 12시로, 오후 시간대를 적절히 변환하는 로직이 잘 구현되었습니다.
feature/home/src/test/kotlin/com/yapp/home/util/AlarmDateTimeFormatterTest.kt (1)
14-236: 포괄적인 테스트 커버리지가 우수합니다
AlarmDateTimeFormatter에 대한 테스트가 다양한 시나리오를 잘 다루고 있습니다:
- 고정된 시계와 로케일을 사용한 재현 가능한 테스트 환경
- 오늘/내일/올해/다른 해의 알람 포맷팅
- 시간 차이 계산의 경계 케이스들
- 복수 알람 중 가장 빠른 알람 선택 로직
테스트 구조와 네이밍이 명확하여 유지보수가 용이합니다.
feature/home/src/main/java/com/yapp/home/alarm/addedit/AlarmAddEditContract.kt (1)
30-32: LocalTime 사용으로 시간 관리가 단순화되었습니다
AlarmTimeState가 AM/PM과 시/분을 분리하는 대신LocalTime을 사용하도록 리팩토링된 것이 좋습니다. 이는 타입 안전성을 높이고 시간 관련 버그를 줄입니다.feature/home/src/main/java/com/yapp/home/alarm/addedit/AlarmAddEditViewModel.kt (2)
38-39: 의존성 변경이 적절합니다
AlarmScheduler를 제거하고AlarmDateTimeFormatter를 추가한 것은 관심사 분리 원칙에 부합합니다. 스케줄링은 UseCase 계층에서, 시간 포맷팅은 전용 유틸리티에서 처리하도록 변경되었습니다.
504-523: 알람 메시지 생성 로직이 개선되었습니다
getAlarmMessage메서드가AlarmDateTimeFormatter를 활용하여 시간 계산과 포맷팅을 처리하도록 리팩토링되었습니다. 이는 ViewModel의 책임을 줄이고 테스트 가능성을 높입니다.feature/home/src/main/java/com/yapp/home/util/AlarmDateTimeFormatter.kt (2)
14-40: 알람 시간 포맷팅 유틸리티 클래스가 잘 설계되었습니다
AlarmDateTimeFormatter는 Clock 주입을 통한 테스트 용이성과 유연한 포맷 설정을 지원하는 데이터 클래스를 제공합니다. 시간 계산과 포맷팅 로직의 중앙화는 유지보수성을 크게 향상시킵니다.
62-64: require 조건의 에러 메시지가 명확합니다반복 알람에서 요일이 선택되지 않은 경우를 명확히 검증하고 있습니다. 디버깅에 유용한 정보를 포함한 에러 메시지가 좋습니다.
feature/home/src/main/java/com/yapp/home/alarm/addedit/AlarmAddEditScreen.kt (6)
1-74: 패키지 구조 변경 및 임포트 업데이트가 적절합니다!패키지 재구성과
LocalTime및MissionType임포트 추가가 PR의 리팩토링 목표와 일치합니다.
189-194:LocalTime을 사용한 시간 선택기 리팩토링이 훌륭합니다!개별 시간 구성 요소 대신
LocalTime을 사용하여 타입 안정성이 향상되고 시간 변환 오류 가능성이 줄어듭니다.
195-201: UI 구조 개선이 잘 되었습니다!요일 선택과 공휴일 설정을 별도의 섹션으로 분리하여 코드의 모듈성과 가독성이 향상되었습니다.
553-635: 요일 선택 섹션 구현이 매우 잘 되었습니다!화면 크기에 따른 반응형 버튼 크기 조정과 평일/주말 빠른 선택 기능이 사용자 경험을 향상시킵니다. UI 구조도 깔끔하고 직관적입니다.
637-669: 공휴일 설정 스위치 컴포넌트 분리가 적절합니다!단일 책임 원칙을 잘 따르고 있으며, 아이콘과 설명 텍스트로 기능이 명확하게 표현되었습니다.
671-734: 프리뷰 컴포저블 업데이트가 잘 되었습니다!
LocalTime사용과OrbitTheme래핑으로 실제 앱 환경과 더 유사한 프리뷰를 제공합니다.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
feature/home/src/main/java/com/yapp/home/util/AlarmDateTimeFormatter.kt (2)
87-133: 과거 리뷰 의견이 반영되어 예외 처리가 개선되었습니다예외 발생 시 적절한 로그를 남기도록 수정되어 디버깅과 추적이 용이해졌습니다.
135-157: 과거 리뷰 의견이 반영되어 예외 처리가 개선되었습니다예외 발생 시 알람 정보와 함께 로그를 남기도록 수정되어 문제 발생 시 원인 파악이 용이해졌습니다.
🧹 Nitpick comments (1)
feature/home/src/main/java/com/yapp/home/util/AlarmDateTimeFormatter.kt (1)
159-188: 시간 차이 포맷팅 로직이 잘 구현되었으나 중복 검사가 있습니다로직이 정확하고 다양한 엣지 케이스를 처리하고 있습니다. 다만 164번째 줄에서 이미
futureTime.isAfter(baseTime)검사를 했으므로 170-172번째 줄의 검사는 중복입니다.- val duration = Duration.between(baseTime, futureTime) - - if (duration.toNanos() <= 0) { - return formats.soonFormat - } - - val totalMinutes = duration.toMinutes() + val duration = Duration.between(baseTime, futureTime) + val totalMinutes = duration.toMinutes()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/home/src/main/java/com/yapp/home/util/AlarmDateTimeFormatter.kt(1 hunks)
🔇 Additional comments (3)
feature/home/src/main/java/com/yapp/home/util/AlarmDateTimeFormatter.kt (3)
15-22: 의존성 주입과 상수 정의가 잘 구현되었습니다
Clock의존성 주입을 통해 테스트 가능성을 높였고, 상수들이 적절히 캡슐화되어 있습니다.
24-40: 포맷 템플릿 데이터 클래스가 잘 설계되었습니다다양한 포맷팅 시나리오를 위한 템플릿을 분리하여 유연성과 재사용성을 제공합니다.
42-85: 알람 다음 발생 시간 계산 로직이 정확하게 구현되었습니다단일 알람과 반복 알람 모두를 올바르게 처리하며, 적절한 유효성 검사와 오류 처리가 포함되어 있습니다.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
feature/home/src/main/java/com/yapp/home/util/AlarmDateTimeFormatter.kt (2)
87-133: 예외 처리 개선이 반영되었습니다이전 리뷰에서 지적된 예외 로깅 문제가 해결되었습니다. DateTimeParseException과 일반 Exception을 구분하여 적절한 로그 레벨로 기록하고 있어 디버깅이 용이해졌습니다.
135-157: 예외 처리 로깅이 추가되어 개선되었습니다이전 리뷰에서 지적된 calculateNextOccurrence 예외 처리 시 로깅 부족 문제가 해결되었습니다. 이제 알람 계산 실패 시 원인을 추적할 수 있어 디버깅이 용이해졌습니다.
🧹 Nitpick comments (1)
feature/home/src/main/java/com/yapp/home/util/AlarmDateTimeFormatter.kt (1)
24-40: 포매팅 구조화가 잘 되어 있습니다데이터 클래스를 활용한 포매팅 옵션 구조화가 깔끔합니다. 다만 패턴 문자열의 기본값들이 한국어 형식으로 하드코딩되어 있는데, 다국어 지원을 고려한다면 이 부분도 주입받는 방식을 고려해볼 수 있습니다.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/home/src/main/java/com/yapp/home/util/AlarmDateTimeFormatter.kt(1 hunks)
🔇 Additional comments (3)
feature/home/src/main/java/com/yapp/home/util/AlarmDateTimeFormatter.kt (3)
1-23: 의존성 주입과 상수 정의가 잘 구현되었습니다Clock을 주입받는 설계로 테스트 가능성이 향상되었고, 상수 정의도 적절합니다.
42-85: 알람 다음 발생 시간 계산 로직이 잘 구현되었습니다단일 알람과 반복 알람을 모두 올바르게 처리하고 있으며, 예외 상황에 대한 검증도 적절합니다. 특히 오늘부터 7일까지 순차적으로 확인하는 로직이 명확합니다.
159-184: 시간 차이 포매팅 로직이 정확합니다Duration API를 활용한 시간 계산과 일/시간/분 단위별 포매팅이 잘 구현되어 있습니다. 음수 시간이나 1분 미만 케이스에 대한 처리도 적절합니다.
|
🚀 테스트 완료 및 커버리지 리포트가 생성되었습니다. |
|
🚀 테스트 완료 및 커버리지 리포트가 생성되었습니다. |
|
🚀 테스트 완료 및 커버리지 리포트가 생성되었습니다. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
core/alarm/src/test/kotlin/com/yapp/alarm/AlarmTimeCalculatorTest.kt (1)
1-468: 테스트 코드의 전반적인 품질이 우수하지만, 몇 가지 개선사항을 제안합니다.현재 테스트 코드는 매우 체계적이고 포괄적이지만, 다음과 같은 개선사항을 고려해보세요:
공휴일 데이터 하드코딩: 현재 2025년 1월 27일을 공휴일로 하드코딩하고 있습니다. 실제 공휴일 데이터가 변경될 수 있으므로 mock 객체나 테스트용 공휴일 데이터를 사용하는 것이 좋을 것 같습니다.
매직 넘버:
AlarmDay.MON.bitValue등의 상수 사용은 좋지만, 일부 날짜 계산에서 하드코딩된 숫자들이 있습니다.테스트 네이밍: 한국어 테스트 명은 가독성이 좋지만, 일부 메서드명이 매우 길어서 간결성을 고려해볼 수 있습니다.
공휴일 로직을 테스트하기 위한 mock 객체 도입을 고려해보세요:
private val mockHolidayChecker = mock<HolidayChecker> { on { isHoliday(LocalDate.of(2025, 1, 27)) } doReturn true on { isHoliday(any()) } doReturn false }gradle/dependencyGraph.gradle (2)
30-35: 중복 방지를 위해 Set 사용 권장
featureModules,coreModules,dataModules,domainModules를List로 선언하면 동일 모듈이 여러 번 add 될 때 중복 원소가 생길 수 있습니다.
contains체크에는 문제가 없지만 이후 사이즈 계산이나 디버깅 시 혼란의 여지가 있습니다.LinkedHashSet으로 선언해 순서와 유일성을 동시에 확보하는 편이 안전합니다.-// --- 모듈 유형을 저장할 리스트 추가 --- -def featureModules = [] -def coreModules = [] -def dataModules = [] -def domainModules = [] // domain 모듈 리스트 추가 +// --- 모듈 유형을 저장할 Set 추가 --- +def featureModules = new LinkedHashSet<Project>() +def coreModules = new LinkedHashSet<Project>() +def dataModules = new LinkedHashSet<Project>() +def domainModules = new LinkedHashSet<Project>() // domain 모듈 Set
98-107: 색상·분류 코드가 장황해졌습니다 – 매핑 테이블로 단순화 가능유형별 색상을 if-else 체인으로 관리하면 향후 모듈 타입 추가 시 동일한 블록을 반복해야 합니다.
Map<Set<Project>, String>형태의 매핑 테이블을 만들어 반복문 하나로 처리하면 가독성과 유지보수가 개선됩니다.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
project.dot.pngis excluded by!**/*.png
📒 Files selected for processing (3)
core/alarm/src/test/kotlin/com/yapp/alarm/AlarmTimeCalculatorTest.kt(1 hunks)feature/home/src/test/kotlin/com/yapp/home/util/AlarmDateTimeFormatterTest.kt(1 hunks)gradle/dependencyGraph.gradle(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/home/src/test/kotlin/com/yapp/home/util/AlarmDateTimeFormatterTest.kt
🔇 Additional comments (7)
core/alarm/src/test/kotlin/com/yapp/alarm/AlarmTimeCalculatorTest.kt (6)
1-10: 임포트와 기본 설정이 적절합니다.필요한 모든 클래스들이 올바르게 임포트되었고, 시간 관련 클래스들의 사용이 적절합니다.
13-56: 고정 Clock 설정과 테스트 시나리오가 체계적으로 구성되어 있습니다.다양한 시간대와 공휴일 상황을 고려한 테스트 케이스들이 잘 준비되어 있습니다. 특히 공휴일 로직 테스트를 위한 2025년 1월 27일(설 연휴) 설정이 현실적입니다.
58-77: 헬퍼 메서드들이 테스트 코드의 가독성을 높입니다.
createTestAlarm과getExpectedMillis메서드는 테스트 코드의 중복을 줄이고 가독성을 향상시킵니다.
79-117: 비반복 알람 테스트가 핵심 시나리오를 잘 다룹니다.미래 시간과 과거 시간에 대한 테스트가 올바르게 구현되어 있습니다. 특히 과거 시간일 때 다음 날로 계산되는 로직이 정확합니다.
120-330: 반복 알람 테스트가 복잡한 시나리오들을 포괄적으로 다룹니다.공휴일 건너뛰기 옵션의 활성/비활성 상태에 따른 다양한 케이스들이 체계적으로 테스트되고 있습니다. 특히 공휴일 로직과 시간 계산의 정확성을 확인하는 테스트들이 잘 구성되어 있습니다.
334-467: 주간 재예약 알람 테스트가 엣지 케이스들을 잘 커버합니다.다양한 요일과 시간대에서의 재예약 로직이 올바르게 테스트되고 있습니다. 공휴일 건너뛰기 옵션에 따른 분기 처리도 정확합니다.
gradle/dependencyGraph.gradle (1)
41-53: 분류 로직 검증 완료현재 레포지토리 내 모든 모듈 경로에 대해 누락되는 분류도, 다중 매칭으로 인한 오분류도 발견되지 않았습니다.
따라서 현 시점에서startsWith/contains기반 로직은 문제가 없습니다.– 참고: 향후 신규 모듈 추가나 네이밍 변경 시 잠재적 리스크를 줄이기 위해 정규식 기반 매핑 테이블 도입을 고려해주세요.
- Codecov에서 XML 읽어야 업로드 가능~
|
🚀 테스트 완료 및 커버리지 리포트가 생성되었습니다. |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
- 프로젝트 커버리지 목표 60%, 임계값 2%로 설정 - gcov 파서 분기 감지 설정 - di, BuildConfig, generated 디렉토리 무시하도록 설정
MoonsuKang
left a comment
MoonsuKang
left a comment
There was a problem hiding this comment.
그의 능력은 어디까지인가…
리뷰는 참고할 만한 부분만 고려해보시면 좋을 것 같습니다! 수고 많으셨습니다~!!
| return todayAlarmDateTime | ||
| } | ||
|
|
||
| for (dayOffset in 0..7) { |
There was a problem hiding this comment.
P4
월~일 이면 0..6 or 1..7도 되지 않을까염?
There was a problem hiding this comment.
넵 요것도 이전 커밋이라 현재 코드는
for (dayOffset in 1..7) {
val nextPotentialDate = now.toLocalDate().plusDays(dayOffset.toLong())
val dayOfWeekPotentialDate = nextPotentialDate.dayOfWeek
val potentialAlarmDateTime = nextPotentialDate.atTime(alarmTime)
if (selectedDaysOfWeek.contains(dayOfWeekPotentialDate)) {
return potentialAlarmDateTime
}
}반영되었습니당
|
|
||
| class AlarmDateTimeFormatter @Inject constructor() { | ||
|
|
||
| data class DeliveryTimeFormats( |
There was a problem hiding this comment.
P5
알람시간을 formatting해서 보여주는 패턴을 구현하고 있는데
AlarmDisplayFormats 요거나 AlarmTimeTextFormats 이것도 좋을 것 같아유.
그냥 넘어가셔도 됩니다잉
There was a problem hiding this comment.
홈화면에서 다음 알람 배달이 언제 되는지 알려주는 문자열이기 때문에 우선 DeliveryFormats로 했습니다!
| val formattedTimeOrDateTime: String | ||
|
|
||
| when { | ||
| // 1. 년도가 현재 년도와 다르면 'otherYear' 포맷 적용 | ||
| alarmOccurrenceDateTime.year != now.year -> { | ||
| formattedTimeOrDateTime = alarmOccurrenceDateTime.format(DateTimeFormatter.ofPattern(formats.otherYearDatePattern)) | ||
| return String.format(formats.otherYear, formattedTimeOrDateTime) | ||
| } | ||
| // 2. (년도가 같고) 날짜가 오늘이면 'today' 포맷 적용 |
There was a problem hiding this comment.
P4
formattedTimeOrDateTime 변수를 별도로 선언하지 않고 각 분기에서 바로 return하면 더 간결할 것 같아요잉
| private val timeFormats = AlarmDateTimeFormatter.TimeDifferenceFormats( | ||
| daysHoursMinutesFormat = "%1\$d일 %2\$d시간 %3\$d분 후에 울려요", | ||
| hoursMinutesFormat = "%1\$d시간 %2\$d분 후에 울려요", | ||
| minutesFormat = "%1\$d분 후에 울려요", | ||
| soonFormat = "곧 울려요" | ||
| ) |
There was a problem hiding this comment.
P4
요거랑 deliveryFormats companion object로 분리하는거 어떻게 생가하심까??
불변이라라 테스트 간 공유 문제 없고, 내부 객체가 JVM에 한 번만 올라가서 메모리 측면에서도 효율적이라고 생각해서염
There was a problem hiding this comment.
어어어어.... 이거 못보고 병합했네요
다음 PR 때 수정해서 같이 올릴게요
There was a problem hiding this comment.
매우 깔끔스 ㄷㄷㄷㄷㄷㄷ
엣지 케이스로 나중에 자정 경계값 측정이나 반복 알람 추가하면 좋을듯!
| val now = createInitialAlarmDateTime(alarm, LocalDateTime.now(clock)) | ||
| var alarmDateTime = now | ||
| .withHour(alarm.hour) | ||
| .withMinute(alarm.minute) | ||
| .withSecond(alarm.second) | ||
| .withNano(0) | ||
| val targetDayOfWeek = day.toDayOfWeek() |
There was a problem hiding this comment.
P3
now에서 시/분/초/나노초를 설정한 후 createInitialAlarmDateTime()을 호출하고 있는데, 이후에 다시 동일한 설정을 하고 있는 것 같아요잉!
There was a problem hiding this comment.
이전 커밋 내용이네요
fun calculateNextRepeatingTimeMillis(
alarm: Alarm,
alarmDay: AlarmDay,
zoneId: ZoneId = clock.zone,
): Long {
val now = LocalDateTime.now(clock)
val targetDayOfWeek = alarmDay.toDayOfWeek()
val alarmDateTimeToday = getAlarmDateTimeOnDate(alarm, now)
var nextAlarmDateTimeCandidate = alarmDateTimeToday
while (nextAlarmDateTimeCandidate.dayOfWeek != targetDayOfWeek || nextAlarmDateTimeCandidate.isBefore(now)) {
nextAlarmDateTimeCandidate = nextAlarmDateTimeCandidate.plusDays(1)
}
nextAlarmDateTimeCandidate = skipHolidaysIfEnabled(nextAlarmDateTimeCandidate, alarm)
return nextAlarmDateTimeCandidate.atZone(zoneId).toInstant().toEpochMilli()
}현재 코드에는 해당 문제 없습니당!
| val db = | ||
| helper.runMigrationsAndValidate(testDbName, 2, true, DatabaseMigrations.MIGRATION_1_2) |
There was a problem hiding this comment.
P5
반환된 db 객체도 테스트 종료 될때 자동으로 닫힐라나?(잘모름)
명시적 close 고려해보는 것도 좋을듯!
There was a problem hiding this comment.
오 위에서는 닫아주는데 여기서는 깜빡했네용
2 participants
Related issue 🛠
related #226
어떤 변경사항이 있었나요?
CheckPoint ✅
PR이 다음 요구 사항을 충족하는지 확인하세요.
Work Description ✏️
AlarmCalculator로 분리공휴일 알람 비활성화옵션 분기처리 추가calculateNonRepeatingTimeMillis)::calculateNextRepeatingTimeMillis):calculateNextWeeklyRescheduledTimeMillis):Uncompleted Tasks 😅
To Reviewers 📢
너무 많아서 한번 끊고 갑니다... 죄송함다
테스트 코드를 작성하기 위한 리팩토링이 주된 내용입니다.
테스트 코드를 작성하고 유지하는 것 또한 비용이기 때문에 앱의 중요한 부분에 투자해야 되는데 이를 알람 로직이라고 생각했습니다.
기존 ViewModel이나 AlarmScheduler에 있는 함수들은 테스트 시 의존성 모킹 등과 비용이 높았기 때문에
출력 기반 테스트가 용이한 순수 Kotlin 함수로 분리하여 테스트 내성과 회귀 방지력을 확보했습니다.
Summary by CodeRabbit
Summary by CodeRabbit
신규 기능
버그 수정
리팩터
UI/스타일
문서화
테스트