640 시간에서 교시 변환 시 동작하는 알고리즘 버그 수정 #641
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughAttendanceService에 기간/시간 검증 및 변환 로직이 강화되고, ApplicationService와 CreateEarlyReturnService에서 해당 검증을 호출하도록 통합했습니다. translateApplication/getMatchPeriods 반환 타입이 List→Pair로 변경되었고, 새로운 예외 InvalidPeriodException 기반의 검증 진입점들이 추가되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AppService as ApplicationService
participant AttendService as AttendanceService
participant Repo as Repository
Client->>AppService: createApplication(request)
AppService->>AttendService: checkApplicationTime(type, start, end)
alt Valid
AttendService-->>AppService: ok
AppService->>AttendService: translateApplication(start, end, type)
AttendService-->>AppService: Pair(startPeriod, endPeriod)
AppService->>Repo: save(application)
Repo-->>AppService: Application
AppService-->>Client: Application
else Invalid
AttendService-->>AppService: InvalidPeriodException
AppService-->>Client: Error
end
sequenceDiagram
participant Client
participant EarlyRetService as CreateEarlyReturnService
participant AttendService as AttendanceService
participant Repo as Repository
Client->>EarlyRetService: createEarlyReturn(request)
EarlyRetService->>AttendService: checkEarlyReturnTime(type, start)
alt Valid
AttendService-->>EarlyRetService: ok
EarlyRetService->>AttendService: translateEarlyReturn(start, type)
AttendService-->>EarlyRetService: periodLabel
EarlyRetService->>Repo: save(earlyReturn)
Repo-->>EarlyRetService: EarlyReturn
EarlyRetService-->>Client: EarlyReturn
else Invalid
AttendService-->>EarlyRetService: InvalidPeriodException
EarlyRetService-->>Client: Error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@src/main/kotlin/dsm/pick2024/domain/attendance/domain/service/AttendanceService.kt`:
- Around line 66-81: The checkEarlyReturnTime function currently only validates
lower bounds and allows out-of-range values (e.g., "11교시" or times after the
last period) which leads updateAttendanceToEarlyReturn to compute startIndex=0
and mark wrong periods; update checkEarlyReturnTime (handling
ApplicationType.PERIOD and ApplicationType.TIME) to enforce both lower and upper
bounds by validating PERIOD against the actual number of school periods (reject
if parsed startPeriod < 1 or > maxPeriod) and validating TIME against the
scheduled period boundaries (parse start as LocalTime and ensure it falls within
the start times of an actual period or not after the last period start); throw
InvalidPeriodException when out of range; reference the period schedule/constant
used elsewhere (e.g., the period list or maxPeriod constant used by
updateAttendanceToEarlyReturn) to determine valid upper bounds.
- Around line 170-181: getMatchPeriods currently finds startIndex/endIndex via
indexOfFirst with endTime < period.endAt, so when endTime equals the last
period's end (e.g., 20:30) endIndex becomes -1 and throws
InvalidPeriodException; update getMatchPeriods to handle that boundary by
treating an endTime equal to the final period's end as matching the last period:
compute endIndex as before, but if endIndex == -1 and endTime ==
periods.last().second then set endIndex = periods.lastIndex (use the existing
periods and periodNames variables), then keep the existing
InvalidPeriodException throw only if startIndex == -1 || endIndex == -1.
- Around line 44-63: In checkApplicationTime, guard against parse failures by
wrapping LocalTime.parse(...) and the toInt() conversions in try/catch blocks
(or a single try surrounding each ApplicationType branch) and convert
DateTimeParseException and NumberFormatException into the unified
InvalidPeriodException; specifically update the ApplicationType.TIME branch to
catch/paraphrase exceptions from LocalTime.parse and the ApplicationType.PERIOD
branch to catch exceptions from start.replace(...).toInt() /
end.replace(...).toInt(), rethrowing InvalidPeriodException when parsing fails
or when the existing logical checks fail.
Comment on lines
44
to
63
| fun checkApplicationTime(applicationType: ApplicationType, start: String, end: String) { | ||
| when (applicationType) { | ||
| ApplicationType.TIME -> { | ||
| val startTime = LocalTime.parse(start) | ||
| val endTime = LocalTime.parse(end) | ||
| if (startTime > endTime || | ||
| endTime > LocalTime.of(20, 30) || | ||
| startTime < LocalTime.of(8, 30) | ||
| ) { | ||
| throw InvalidPeriodException | ||
| } | ||
| } | ||
| ApplicationType.PERIOD -> { | ||
| val startPeriod = start.replace("교시", "").toInt() | ||
| val endPeriod = end.replace("교시", "").toInt() | ||
| if (startPeriod > endPeriod || startPeriod < 1 || endPeriod > 10) { | ||
| throw InvalidPeriodException | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
잘못된 시간/교시 포맷이 그대로 런타임 예외로 노출될 수 있습니다.
LocalTime.parse(...)와 toInt()가 실패하면 InvalidPeriodException이 아닌 DateTimeParseException/NumberFormatException으로 터져 500이 될 수 있습니다. 입력 검증으로 예외를 통일하는 편이 안전합니다.
🛠️ 제안 수정
fun checkApplicationTime(applicationType: ApplicationType, start: String, end: String) {
when (applicationType) {
ApplicationType.TIME -> {
- val startTime = LocalTime.parse(start)
- val endTime = LocalTime.parse(end)
+ val startTime = runCatching { LocalTime.parse(start) }
+ .getOrElse { throw InvalidPeriodException }
+ val endTime = runCatching { LocalTime.parse(end) }
+ .getOrElse { throw InvalidPeriodException }
if (startTime > endTime ||
endTime > LocalTime.of(20, 30) ||
startTime < LocalTime.of(8, 30)
) {
throw InvalidPeriodException
}
}
ApplicationType.PERIOD -> {
- val startPeriod = start.replace("교시", "").toInt()
- val endPeriod = end.replace("교시", "").toInt()
+ val startPeriod = start.replace("교시", "").toIntOrNull()
+ ?: throw InvalidPeriodException
+ val endPeriod = end.replace("교시", "").toIntOrNull()
+ ?: throw InvalidPeriodException
if (startPeriod > endPeriod || startPeriod < 1 || endPeriod > 10) {
throw InvalidPeriodException
}
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fun checkApplicationTime(applicationType: ApplicationType, start: String, end: String) { | |
| when (applicationType) { | |
| ApplicationType.TIME -> { | |
| val startTime = LocalTime.parse(start) | |
| val endTime = LocalTime.parse(end) | |
| if (startTime > endTime || | |
| endTime > LocalTime.of(20, 30) || | |
| startTime < LocalTime.of(8, 30) | |
| ) { | |
| throw InvalidPeriodException | |
| } | |
| } | |
| ApplicationType.PERIOD -> { | |
| val startPeriod = start.replace("교시", "").toInt() | |
| val endPeriod = end.replace("교시", "").toInt() | |
| if (startPeriod > endPeriod || startPeriod < 1 || endPeriod > 10) { | |
| throw InvalidPeriodException | |
| } | |
| } | |
| } | |
| fun checkApplicationTime(applicationType: ApplicationType, start: String, end: String) { | |
| when (applicationType) { | |
| ApplicationType.TIME -> { | |
| val startTime = runCatching { LocalTime.parse(start) } | |
| .getOrElse { throw InvalidPeriodException } | |
| val endTime = runCatching { LocalTime.parse(end) } | |
| .getOrElse { throw InvalidPeriodException } | |
| if (startTime > endTime || | |
| endTime > LocalTime.of(20, 30) || | |
| startTime < LocalTime.of(8, 30) | |
| ) { | |
| throw InvalidPeriodException | |
| } | |
| } | |
| ApplicationType.PERIOD -> { | |
| val startPeriod = start.replace("교시", "").toIntOrNull() | |
| ?: throw InvalidPeriodException | |
| val endPeriod = end.replace("교시", "").toIntOrNull() | |
| ?: throw InvalidPeriodException | |
| if (startPeriod > endPeriod || startPeriod < 1 || endPeriod > 10) { | |
| throw InvalidPeriodException | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@src/main/kotlin/dsm/pick2024/domain/attendance/domain/service/AttendanceService.kt`
around lines 44 - 63, In checkApplicationTime, guard against parse failures by
wrapping LocalTime.parse(...) and the toInt() conversions in try/catch blocks
(or a single try surrounding each ApplicationType branch) and convert
DateTimeParseException and NumberFormatException into the unified
InvalidPeriodException; specifically update the ApplicationType.TIME branch to
catch/paraphrase exceptions from LocalTime.parse and the ApplicationType.PERIOD
branch to catch exceptions from start.replace(...).toInt() /
end.replace(...).toInt(), rethrowing InvalidPeriodException when parsing fails
or when the existing logical checks fail.
Member
Author
There was a problem hiding this comment.
abf2875
time 변환은 기존에도 저런 형식으로 진행하였기 때문에 괜찮습니다
There was a problem hiding this comment.
@ByunDohwi 알겠습니다! 기존 코드베이스와의 일관성을 유지하는 것도 중요하니 이해했습니다. 입력 검증은 현재 방식대로 진행하시면 되겠습니다. 👍
src/main/kotlin/dsm/pick2024/domain/attendance/domain/service/AttendanceService.kt
Show resolved
Hide resolved
src/main/kotlin/dsm/pick2024/domain/attendance/domain/service/AttendanceService.kt
Show resolved
Hide resolved
imhanseong
previously approved these changes
Jan 21, 2026
1210thatman
previously approved these changes
Jan 21, 2026
ByunDohwi
dismissed stale reviews from 1210thatman and imhanseong
via
abf2875
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/main/kotlin/dsm/pick2024/domain/attendance/domain/service/AttendanceService.kt`:
- Around line 83-95: translateEarlyReturn currently treats the period end as
exclusive (startTime < endAt) which mismatches getMatchPeriods; change the
matching to be inclusive of the end boundary so both functions behave the same.
In translateEarlyReturn (and the lambda over periods) use an inclusive check
(e.g., startTime in start..endAt or startTime >= start && startTime <= endAt)
when finding startIndex, and keep throwing InvalidPeriodException and returning
periodNames[startIndex] as before so periods, periodNames, translateEarlyReturn
and getMatchPeriods use the same boundary logic.
♻️ Duplicate comments (1)
src/main/kotlin/dsm/pick2024/domain/attendance/domain/service/AttendanceService.kt (1)
74-79: PERIOD 분기에서.toInt()대신.toIntOrNull()을 사용하세요.
checkApplicationTime에서는.toIntOrNull()을 사용하지만 여기서는.toInt()를 사용하여 일관성이 없습니다. 잘못된 입력 시NumberFormatException대신InvalidPeriodException이 발생하도록 통일하는 것이 좋습니다.🛠️ 제안 수정
ApplicationType.PERIOD -> { - val startPeriod = start.replace("교시", "").toInt() + val startPeriod = start.replace("교시", "").toIntOrNull() + ?: throw InvalidPeriodException if (startPeriod < 1 || startPeriod > 10) { throw InvalidPeriodException } }
Comment on lines
+83
to
+95
| fun translateEarlyReturn(start: String, applicationType: ApplicationType): String { | ||
| return when (applicationType) { | ||
| ApplicationType.PERIOD -> start | ||
| ApplicationType.TIME -> { | ||
| val startTime = LocalTime.parse(start) | ||
| val startIndex = periods.indexOfFirst { (start, endAt) -> | ||
| startTime >= start && startTime < endAt | ||
| } | ||
| if (startIndex == -1) throw InvalidPeriodException | ||
| periodNames[startIndex] | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
경계 시간 처리 로직이 getMatchPeriods와 불일치합니다.
translateEarlyReturn은 startTime < endAt (종료 시간 제외)을 사용하지만, getMatchPeriods는 in start..endAt (종료 시간 포함)을 사용합니다.
예를 들어 9:40 (1교시 종료 시간)의 경우:
translateEarlyReturn: 2교시로 매칭getMatchPeriods: 1교시로 매칭
두 함수의 경계 처리 로직을 통일하는 것이 좋습니다.
🛠️ 제안 수정 (inclusive로 통일)
fun translateEarlyReturn(start: String, applicationType: ApplicationType): String {
return when (applicationType) {
ApplicationType.PERIOD -> start
ApplicationType.TIME -> {
val startTime = LocalTime.parse(start)
val startIndex = periods.indexOfFirst { (start, endAt) ->
- startTime >= start && startTime < endAt
+ startTime in start..endAt
}
if (startIndex == -1) throw InvalidPeriodException
periodNames[startIndex]
}
}
}🤖 Prompt for AI Agents
In
`@src/main/kotlin/dsm/pick2024/domain/attendance/domain/service/AttendanceService.kt`
around lines 83 - 95, translateEarlyReturn currently treats the period end as
exclusive (startTime < endAt) which mismatches getMatchPeriods; change the
matching to be inclusive of the end boundary so both functions behave the same.
In translateEarlyReturn (and the lambda over periods) use an inclusive check
(e.g., startTime in start..endAt or startTime >= start && startTime <= endAt)
when finding startIndex, and keep throwing InvalidPeriodException and returning
periodNames[startIndex] as before so periods, periodNames, translateEarlyReturn
and getMatchPeriods use the same boundary logic.
imhanseong
approved these changes
Jan 21, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.