Open
Conversation
📝 WalkthroughWalkthrough이미지 태그를 중복 제거하여 카운트하도록 쿼리를 수정하고, RecapServiceImpl의 시간 범위 처리를 새로운 TimeRange 레코드로 리팩토링하며, 해당 TimeRange 값 객체를 새로 추가합니다. Changes
Possibly related PRs
🚥 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
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/umc/nuvibe/domain/archive/repository/BoardImageRepository.java (1)
173-183:⚠️ Potential issue | 🟡 Minor
distinct추가로 태그 종류 수 집계가 올바르게 수정되었습니다.
count(distinct bi.image.imageTag)로 변경하여 업로드 수가 아닌 태그 종류 수를 반환하도록 수정한 것은 PR 목적에 부합합니다. SQL의COUNT(DISTINCT ...)특성상 NULL 값은 자동으로 제외되므로imageTag가 null인 레코드도 안전하게 처리됩니다.다만, 이 쿼리의 상단 경계 연산자(
<= :end)와 다른 쿼리들의 경계 연산자 간 불일치가 있습니다:
쿼리 상단 경계 countTotalTagByPeriod(여기)<= :endcountTotalImageByPeriod<= :endfindTopTagsByPeriod< :endfindTopBoardByPeriod< :endfindTopHourByPeriod< :endWEEK 기간의 경우
end가 다음 주 월요일 00:00:00으로 설정되므로,<= :end를 사용하는 쿼리는 정확히 월요일 자정에 생성된 데이터를 포함할 수 있습니다. 모든 기간 쿼리에서 경계 조건을 통일하는 것을 권장합니다 (예: 모두< :end로 통일).🔧 경계 연산자 통일 제안
- `@Query` ("select count(distinct bi.image.imageTag) from BoardImage bi " + + `@Query`("select count(distinct bi.image.imageTag) from BoardImage bi " + "where bi.board.user.id = :userId " + "and bi.image.createdAt >= :start " + - "and bi.image.createdAt <= :end" + "and bi.image.createdAt < :end" )동일하게
countTotalImageByPeriod(Line 116),findMaxDailyDropCount(Line 189),findTopDayOfWeekByPeriod(Line 206),countActiveBoardsByPeriod(Line 164-165)에서도<= :end/between대신< :end로 통일하는 것을 검토해 주세요.
🤖 Fix all issues with AI agents
In `@src/main/java/com/umc/nuvibe/domain/archive/service/RecapServiceImpl.java`:
- Around line 41-58: Rename the method calculateStartTime in RecapServiceImpl to
calculateTimeRange (returning TimeRange) and update all references/usages
(including interfaces, callers, and tests) to the new name; keep the internal
logic that uses RecapPeriod, TimeRange, userRepository and User::getCreatedAt
unchanged, and ensure method signatures (visibility and parameters: RecapPeriod
period, Long userId) remain identical so compilation still succeeds.
- Around line 91-94: Extract the duplicated endDate calculation into TimeRange
(or a small helper) by adding a method like getDisplayEndDate()/endDate() on the
TimeRange record that returns (period == RecapPeriod.WEEK) ?
end.toLocalDate().minusDays(1) : end.toLocalDate(); then update RecapServiceImpl
to call TimeRange.endDate() (replace the three duplicated blocks in
getRecapTags, getRecapBoard, and getRecapActive) so all callers use the single
canonical implementation.
In `@src/main/java/com/umc/nuvibe/domain/archive/vo/TimeRange.java`:
- Around line 1-6: TimeRange as a record currently allows nulls and start after
end; add a compact constructor in the TimeRange record to perform defensive
validation: use Objects.requireNonNull on start and end (or throw
NullPointerException) and check that !start.isAfter(end) (or
start.compareTo(end) <= 0), throwing IllegalArgumentException with a clear
message if the invariant fails; implement this inside the TimeRange compact
constructor to guarantee the VO invariant at creation.
Comment on lines
+41
to
58
| private TimeRange calculateStartTime(RecapPeriod period, Long userId){ | ||
|
|
||
| LocalDateTime start; | ||
| if (period == RecapPeriod.WEEK){ | ||
| LocalDate startWeek = LocalDate.now().with(java.time.DayOfWeek.MONDAY); | ||
| start = startWeek.atStartOfDay(); | ||
| LocalDateTime start = startWeek.atStartOfDay(); | ||
| LocalDateTime end = startWeek.plusWeeks(1).atStartOfDay(); | ||
| return new TimeRange(start, end); | ||
|
|
||
| } else { | ||
| start = userRepository.findById(userId) | ||
| LocalDateTime start = userRepository.findById(userId) | ||
| .map(User::getCreatedAt) | ||
| .orElseThrow(()->new BusinessException(UserErrorCode.USER_NOT_FOUND)); | ||
| LocalDateTime end = LocalDateTime.now(); | ||
| return new TimeRange(start, end); | ||
| } | ||
| return start; | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
메서드 이름이 반환 타입과 불일치합니다.
calculateStartTime은 이제 TimeRange(시작 + 종료)를 반환하지만, 메서드 이름은 여전히 "StartTime"만 계산하는 것처럼 보입니다. calculateTimeRange 또는 calculatePeriodRange 등으로 변경하면 가독성이 향상됩니다.
♻️ 메서드명 변경 제안
- private TimeRange calculateStartTime(RecapPeriod period, Long userId){
+ private TimeRange calculateTimeRange(RecapPeriod period, Long userId){📝 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
| private TimeRange calculateStartTime(RecapPeriod period, Long userId){ | |
| LocalDateTime start; | |
| if (period == RecapPeriod.WEEK){ | |
| LocalDate startWeek = LocalDate.now().with(java.time.DayOfWeek.MONDAY); | |
| start = startWeek.atStartOfDay(); | |
| LocalDateTime start = startWeek.atStartOfDay(); | |
| LocalDateTime end = startWeek.plusWeeks(1).atStartOfDay(); | |
| return new TimeRange(start, end); | |
| } else { | |
| start = userRepository.findById(userId) | |
| LocalDateTime start = userRepository.findById(userId) | |
| .map(User::getCreatedAt) | |
| .orElseThrow(()->new BusinessException(UserErrorCode.USER_NOT_FOUND)); | |
| LocalDateTime end = LocalDateTime.now(); | |
| return new TimeRange(start, end); | |
| } | |
| return start; | |
| } | |
| private TimeRange calculateTimeRange(RecapPeriod period, Long userId){ | |
| if (period == RecapPeriod.WEEK){ | |
| LocalDate startWeek = LocalDate.now().with(java.time.DayOfWeek.MONDAY); | |
| LocalDateTime start = startWeek.atStartOfDay(); | |
| LocalDateTime end = startWeek.plusWeeks(1).atStartOfDay(); | |
| return new TimeRange(start, end); | |
| } else { | |
| LocalDateTime start = userRepository.findById(userId) | |
| .map(User::getCreatedAt) | |
| .orElseThrow(()->new BusinessException(UserErrorCode.USER_NOT_FOUND)); | |
| LocalDateTime end = LocalDateTime.now(); | |
| return new TimeRange(start, end); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/com/umc/nuvibe/domain/archive/service/RecapServiceImpl.java`
around lines 41 - 58, Rename the method calculateStartTime in RecapServiceImpl
to calculateTimeRange (returning TimeRange) and update all references/usages
(including interfaces, callers, and tests) to the new name; keep the internal
logic that uses RecapPeriod, TimeRange, userRepository and User::getCreatedAt
unchanged, and ensure method signatures (visibility and parameters: RecapPeriod
period, Long userId) remain identical so compilation still succeeds.
Comment on lines
+91
to
+94
| LocalDate endDate = (period == RecapPeriod.WEEK) | ||
| ? end.toLocalDate().minusDays(1) | ||
| : end.toLocalDate(); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
endDate 계산 로직이 3곳에서 중복됩니다.
동일한 endDate 계산 패턴이 getRecapTags (Line 91-93), getRecapBoard (Line 130-132), getRecapActive (Line 181-183)에서 반복됩니다. 이를 TimeRange에 메서드로 추출하거나, 별도 헬퍼로 분리하면 유지보수성이 향상됩니다.
♻️ TimeRange에 endDate 메서드 추가 제안
TimeRange 레코드에 display용 endDate를 반환하는 메서드를 추가:
public record TimeRange(LocalDateTime start, LocalDateTime end) {
+ public LocalDate startDate() {
+ return start.toLocalDate();
+ }
+
+ public LocalDate endDate(RecapPeriod period) {
+ return (period == RecapPeriod.WEEK)
+ ? end.toLocalDate().minusDays(1)
+ : end.toLocalDate();
+ }
}그러면 각 메서드에서 다음과 같이 간결하게 사용할 수 있습니다:
- LocalDate endDate = (period == RecapPeriod.WEEK)
- ? end.toLocalDate().minusDays(1)
- : end.toLocalDate();
-
return new RecapTagResponse(
period,
- start.toLocalDate(),
- endDate,
+ range.startDate(),
+ range.endDate(period),
totalDrops,
ranks
);🤖 Prompt for AI Agents
In `@src/main/java/com/umc/nuvibe/domain/archive/service/RecapServiceImpl.java`
around lines 91 - 94, Extract the duplicated endDate calculation into TimeRange
(or a small helper) by adding a method like getDisplayEndDate()/endDate() on the
TimeRange record that returns (period == RecapPeriod.WEEK) ?
end.toLocalDate().minusDays(1) : end.toLocalDate(); then update RecapServiceImpl
to call TimeRange.endDate() (replace the three duplicated blocks in
getRecapTags, getRecapBoard, and getRecapActive) so all callers use the single
canonical implementation.
Comment on lines
+1
to
+6
| package com.umc.nuvibe.domain.archive.vo; | ||
|
|
||
| import java.time.LocalDateTime; | ||
|
|
||
| public record TimeRange(LocalDateTime start, LocalDateTime end) { | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
방어적 유효성 검증 추가를 고려해 주세요.
현재 TimeRange는 start가 end보다 나중이거나, 필드가 null인 경우에도 생성이 가능합니다. 현재는 RecapServiceImpl 내부에서만 사용되어 즉각적인 문제는 없지만, VO로서 불변식(invariant)을 보장하면 향후 재사용 시 안전합니다.
🛡️ compact constructor를 통한 유효성 검증 추가
public record TimeRange(LocalDateTime start, LocalDateTime end) {
+ public TimeRange {
+ if (start == null || end == null) {
+ throw new IllegalArgumentException("start and end must not be null");
+ }
+ if (start.isAfter(end)) {
+ throw new IllegalArgumentException("start must not be after end");
+ }
+ }
}📝 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
| package com.umc.nuvibe.domain.archive.vo; | |
| import java.time.LocalDateTime; | |
| public record TimeRange(LocalDateTime start, LocalDateTime end) { | |
| } | |
| package com.umc.nuvibe.domain.archive.vo; | |
| import java.time.LocalDateTime; | |
| public record TimeRange(LocalDateTime start, LocalDateTime end) { | |
| public TimeRange { | |
| if (start == null || end == null) { | |
| throw new IllegalArgumentException("start and end must not be null"); | |
| } | |
| if (start.isAfter(end)) { | |
| throw new IllegalArgumentException("start must not be after end"); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/com/umc/nuvibe/domain/archive/vo/TimeRange.java` around lines 1
- 6, TimeRange as a record currently allows nulls and start after end; add a
compact constructor in the TimeRange record to perform defensive validation: use
Objects.requireNonNull on start and end (or throw NullPointerException) and
check that !start.isAfter(end) (or start.compareTo(end) <= 0), throwing
IllegalArgumentException with a clear message if the invariant fails; implement
this inside the TimeRange compact constructor to guarantee the VO invariant at
creation.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
🎋 이슈 및 작업중인 브랜치
🔑 주요 내용
Check List
Summary by CodeRabbit
릴리스 노트
Bug Fixes
Refactor