[DDING-103] 폼지 생성 및 수정시 기간 중복 검증 로직 구현 #243
Conversation
KoSeonJe
requested review from
wonjunYou,
5uhwann and
Seooooo24
as code owners
|
Caution Review failedThe pull request is closed. Walkthrough이 PR은 폼 관리 기능 개선을 위해 여러 분야에서 변경사항을 도입합니다. 예외 처리 측면에서는 새로운 상수와 Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 10
🧹 Nitpick comments (22)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/dto/DepartmentInfo.java (1)
3-8: 인터페이스 문서화 및 Null 처리 개선이 필요합니다.인터페이스의 목적과 사용 사례를 명확히 하고, null 값 처리를 개선하기 위해 다음과 같은 수정을 제안합니다.
+import org.springframework.lang.NonNull; + +/** + * 학과별 통계 정보를 제공하는 인터페이스입니다. + * JPA Projection으로 사용되어 폼 지원서의 학과별 통계를 조회하는데 활용됩니다. + */ public interface DepartmentInfo { + /** + * @return 학과명 + */ + @NonNull String getDepartment(); + /** + * @return 지원자 수 + */ + @NonNull Integer getCount(); }src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormApplicationRepository.java (1)
32-40: 성능 최적화를 위한 인덱스 추가 고려
form_id와department컬럼에 대한 인덱스 추가를 고려해보세요. GROUP BY와 ORDER BY 연산의 성능을 향상시킬 수 있습니다.src/test/java/ddingdong/ddingdongBE/domain/form/repository/FormApplicationRepositoryTest.java (2)
156-221: 테스트 코드 개선 제안
throws InterruptedException이 불필요하게 선언되어 있습니다. 제거해주세요.- 테스트 데이터 설정을 더 간결하게 만들 수 있습니다. 테스트 데이터 생성을 위한 헬퍼 메서드를 만드는 것을 고려해보세요.
- void findTopFiveDepartmentsByForm_ShouldReturnTopFiveDepartments() throws InterruptedException { + void findTopFiveDepartmentsByForm_ShouldReturnTopFiveDepartments() {
225-275: 테스트 가독성 개선 제안날짜와 같은 매직 넘버를 상수로 추출하여 테스트의 가독성을 높이는 것을 추천드립니다.
+ private static final LocalDate START_DATE = LocalDate.of(2020, 1, 1); + private static final LocalDate END_DATE = LocalDate.of(2020, 6, 30); + private static final LocalDate FORM1_START = LocalDate.of(2020, 3, 1); + private static final LocalDate FORM1_END = LocalDate.of(2020, 4, 1); + private static final LocalDate FORM2_START = LocalDate.of(2020, 1, 1); + private static final LocalDate FORM2_END = LocalDate.of(2020, 2, 1); @Test void findMaxApplicationCountByDateRange_ShouldReturnHighestCount() { // given Form form = fixture.giveMeBuilder(Form.class) .set("id", 1L) .set("club", null) - .set("startDate", LocalDate.of(2020, 3, 1)) - .set("endDate", LocalDate.of(2020, 4, 1)) + .set("startDate", FORM1_START) + .set("endDate", FORM1_END)src/main/java/ddingdong/ddingdongBE/common/utils/CalculationUtils.java (1)
5-10: 비율 계산의 정확성을 위해 반환 타입을 double로, 파라미터 타입을 long으로 변경 제안현재
calculateRate메서드는int타입의 파라미터와 반환 타입을 사용하여 계산 결과의 정밀도가 떨어집니다. 더 큰 값과 정확한 비율 계산을 위해 파라미터 타입을long으로, 반환 타입을double로 변경하는 것을 제안합니다.-public static int calculateRate(int count, int totalCount) { +public static double calculateRate(long count, long totalCount) { if (totalCount == 0) { return 0; } - return (int) ((double) count / totalCount * 100); + return ((double) count / totalCount * 100); }src/main/java/ddingdong/ddingdongBE/domain/form/service/FormStatisticService.java (1)
11-17: 각 통계 메서드에 대한 문서화가 필요합니다.각 메서드의 목적과 반환값에 대한 자세한 설명을 JavaDoc으로 추가하면 좋을 것 같습니다. 특히:
- 각 통계의 계산 방식
- 반환되는 DTO 객체의 구조
- 예외 처리 케이스
src/main/java/ddingdong/ddingdongBE/common/exception/InvalidatedMappingException.java (1)
20-25: 에러 코드를 상수로 분리하는 것이 좋겠습니다.HTTP 상태 코드를 직접 사용하는 대신, 의미 있는 이름의 상수로 분리하면 코드의 가독성과 유지보수성이 향상될 것 같습니다.
+ private static final int INVALID_FORM_PERIOD_ERROR_CODE = BAD_REQUEST.value(); public InvalidFormPeriodException() { - super(INVALID_FORM_DATE_MESSAGE, BAD_REQUEST.value()); + super(INVALID_FORM_DATE_MESSAGE, INVALID_FORM_PERIOD_ERROR_CODE); }src/main/java/ddingdong/ddingdongBE/domain/form/repository/FormRepository.java (1)
15-23: 네이티브 쿼리 대신 JPQL 사용 고려네이티브 SQL 쿼리 대신 JPQL을 사용하면 다음과 같은 이점이 있습니다:
- 데이터베이스 독립성 향상
- 타입 안정성 보장
- 리팩토링시 더 나은 유지보수성
다음과 같이 JPQL로 변경하는 것을 고려해보세요:
- @Query(value = "SELECT * FROM form f " + - "WHERE f.club_id = :clubId " + - "AND f.start_date <= :endDate " + - "AND f.end_date >= :startDate", - nativeQuery = true) + @Query("SELECT f FROM Form f " + + "WHERE f.club.id = :clubId " + + "AND f.startDate <= :endDate " + + "AND f.endDate >= :startDate")src/main/java/ddingdong/ddingdongBE/common/utils/TimeUtils.java (2)
19-29: 중복된 null 체크 개선 필요
processDate메서드에서 null과 blank 체크가 중복되어 있습니다. 이를 단순화할 수 있습니다.다음과 같이 개선해보세요:
public static LocalDateTime processDate(String dateString, LocalDateTime currentDate) { - if (dateString == null) { - return null; - } - - if (dateString.isBlank()) { + if (dateString == null || dateString.isBlank()) { return null; } return parseToLocalDateTime(dateString); }
31-36: 메서드 이름과 매개변수 명확성 개선
isDateInRange메서드의 이름과 매개변수가 기능을 명확하게 설명하지 못합니다.다음과 같이 개선해보세요:
- public static boolean isDateInRange(LocalDate nowDate, LocalDate startDate, LocalDate endDate) + public static boolean isDateBetweenInclusive(LocalDate targetDate, LocalDate periodStart, LocalDate periodEnd)src/main/java/ddingdong/ddingdongBE/domain/form/service/vo/HalfYear.java (2)
20-27: 불변성을 위한 메서드 개선
minusHalves메서드가 객체의 상태를 직접 변경하고 있습니다. 불변 객체로 만드는 것이 더 안전할 것 같습니다.다음과 같이 개선해보세요:
- public void minusHalves() { + public HalfYear minusHalves() { if (isFirstHalf) { - year = year - 1; - isFirstHalf = false; - return; + return new HalfYear(year - 1, false); } - isFirstHalf = true; + return new HalfYear(year, true); }
29-41: 매직 넘버 상수화 필요날짜 관련 매직 넘버들을 의미 있는 상수로 추출하면 좋을 것 같습니다.
다음과 같이 개선해보세요:
+ private static final int FIRST_HALF_START_MONTH = 1; + private static final int FIRST_HALF_END_MONTH = 6; + private static final int SECOND_HALF_START_MONTH = 7; + private static final int SECOND_HALF_END_MONTH = 12; + private static final int FIRST_HALF_END_DAY = 30; + private static final int SECOND_HALF_END_DAY = 31; + public LocalDate getHalfStartDate() { if(isFirstHalf) { - return LocalDate.of(year, 1, 1); + return LocalDate.of(year, FIRST_HALF_START_MONTH, 1); } - return LocalDate.of(year, 7, 1); + return LocalDate.of(year, SECOND_HALF_START_MONTH, 1); }src/main/java/ddingdong/ddingdongBE/domain/form/service/vo/ApplicationRates.java (3)
30-31: 상수 값에 대한 의미 설명 필요
DEFAULT_APPLICATION_RATE = 100의 의미와 이 값이 선택된 이유에 대한 주석 설명이 있으면 좋겠습니다.+ // 이전 학기 대비 변화가 없음을 나타내는 기본값 (100%) private static final int DEFAULT_APPLICATION_RATE = 100;
34-42: 메서드 동작에 대한 명확한 문서화 필요
add메서드의 복잡한 로직에 대한 JavaDoc 설명이 있으면 좋겠습니다. 특히 이전 학기와의 비교 로직과 새로운 항목 추가 시의 동작에 대해 설명이 필요합니다.+ /** + * 새로운 지원율을 추가합니다. + * 첫 번째 항목 추가 시: 지정된 비교율로 저장 + * 이후 항목 추가 시: 이전 항목의 비교율을 업데이트하고, 새 항목을 기본 비교율(100%)로 추가 + * + * @param label 지원율 라벨 + * @param count 지원자 수 + * @param comparedToLastSemester 이전 학기 대비 비교율 + */ public void add(String label, int count, int comparedToLastSemester) {
44-49: null 반환 대신 Optional 사용 권장
getPrevious메서드에서 null을 반환하는 대신 Optional을 사용하면 더 안전한 null 처리가 가능합니다.- public ApplicationRate getPrevious() { - if (applicationRates.isEmpty()) { - return null; - } - return applicationRates.get(0); - } + public Optional<ApplicationRate> getPrevious() { + return applicationRates.isEmpty() + ? Optional.empty() + : Optional.of(applicationRates.get(0)); + }src/test/java/ddingdong/ddingdongBE/common/support/FixtureMonkeyFactory.java (1)
28-31: 주석 처리된 코드 제거 또는 구현 필요주석 처리된 코드는 향후 구현 예정임을 나타내는 것으로 보입니다. 이를 TODO 주석으로 변경하거나, 구현을 완료하는 것이 좋겠습니다.
-// if (context.findAnnotation(MaxOfLength.class).isPresent()) { -// return stringArbitrary.ofMaxLength(10); -// } + // TODO: MaxOfLength 어노테이션 기반 문자열 길이 제한 구현src/main/java/ddingdong/ddingdongBE/domain/form/controller/dto/response/FormStatisticsResponse.java (1)
1-23: 응답 DTO 구조가 잘 설계되었습니다.통계 데이터를 위한 응답 구조가 명확하게 정의되어 있습니다. 다만, 필드 이름에 대한 작은 개선사항이 있습니다.
fields를fieldStatistics로 변경하는 것이 의미를 더 명확하게 전달할 수 있을 것 같습니다:- List<FieldStatisticsListResponse> fields + List<FieldStatisticsListResponse> fieldStatisticssrc/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java (1)
92-100: 통계 수집 성능 개선이 필요할 수 있습니다.통계 데이터 수집 로직이 잘 구현되어 있습니다. 하지만 성능 최적화를 위한 캐싱 전략을 고려해볼 수 있습니다.
통계 데이터는 자주 변경되지 않는 특성이 있으므로, Redis나 Spring Cache를 활용하여 캐싱을 구현하는 것을 추천드립니다. 이를 통해 반복적인 데이터베이스 조회를 줄일 수 있습니다.
src/test/java/ddingdong/ddingdongBE/domain/form/service/FormStatisticServiceImplTest.java (4)
48-83: getTotalApplicationCountByForm 테스트의 데이터 다양성 개선 필요현재 테스트는 기본적인 케이스만 다루고 있습니다. 다음과 같은 경계 케이스도 추가하면 좋을 것 같습니다:
- 지원자가 없는 경우
- 지원 상태가 SUBMITTED가 아닌 경우
84-155: createDepartmentRankByForm 테스트의 중복 코드 제거 필요FormApplication 객체 생성 코드가 반복되고 있습니다. 테스트 가독성과 유지보수성을 높이기 위해 테스트 헬퍼 메서드를 도입하는 것이 좋겠습니다.
+ private FormApplication createFormApplication(Form form, String department) { + return FormApplication.builder() + .name("이름1") + .studentNumber("학번1") + .department(department) + .status(FormApplicationStatus.SUBMITTED) + .form(form) + .build(); + }
156-215: createApplicationRateByForm 테스트의 날짜 처리 개선 필요현재 테스트에서 하드코딩된 날짜를 사용하고 있습니다. 테스트의 유연성을 높이기 위해 상대적인 날짜 계산을 사용하는 것이 좋겠습니다.
- .set("startDate", LocalDate.of(2020, 2, 1)) + .set("startDate", LocalDate.now().minusYears(1).withMonth(2).withDayOfMonth(1))
216-264: createFieldStatisticsListByForm 테스트의 검증 강화 필요현재 테스트는 기본적인 검증만 수행하고 있습니다. 다음 사항들도 검증하면 좋을 것 같습니다:
- 필드 타입(FieldType.TEXT)에 따른 통계 처리가 올바른지
- 옵션 목록이 올바르게 처리되는지
- 필수 필드(required=true) 처리가 올바른지
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
src/main/java/ddingdong/ddingdongBE/common/exception/InvalidatedMappingException.java(2 hunks)src/main/java/ddingdong/ddingdongBE/common/utils/CalculationUtils.java(1 hunks)src/main/java/ddingdong/ddingdongBE/common/utils/TimeUtils.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/api/CentralFormApi.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/controller/CentralFormController.java(3 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/controller/dto/request/CreateFormRequest.java(4 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/controller/dto/request/UpdateFormRequest.java(5 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/controller/dto/response/FormStatisticsResponse.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/entity/FieldType.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/repository/FormRepository.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java(4 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FormService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FormStatisticService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FormStatisticServiceImpl.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/GeneralFormService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/command/CreateFormCommand.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/command/UpdateFormCommand.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/query/FormStatisticsQuery.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/vo/ApplicationRates.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/vo/HalfYear.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormAnswerRepository.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormApplicationRepository.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/dto/DepartmentInfo.java(1 hunks)src/main/resources/db/migration/V36__form_add_column_sections.sql(1 hunks)src/test/java/ddingdong/ddingdongBE/common/support/FixtureMonkeyFactory.java(2 hunks)src/test/java/ddingdong/ddingdongBE/common/support/TestContainerSupport.java(0 hunks)src/test/java/ddingdong/ddingdongBE/domain/form/repository/FormApplicationRepositoryTest.java(2 hunks)src/test/java/ddingdong/ddingdongBE/domain/form/repository/FormRepositoryTest.java(1 hunks)src/test/java/ddingdong/ddingdongBE/domain/form/service/FormStatisticServiceImplTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/java/ddingdong/ddingdongBE/common/support/TestContainerSupport.java
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/db/migration/V36__form_add_column_sections.sql
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (16)
src/main/java/ddingdong/ddingdongBE/domain/formapplication/repository/FormApplicationRepository.java (2)
3-14: LGTM! 적절한 어노테이션과 임포트 추가필요한 엔티티와 DTO에 대한 임포트가 잘 추가되었고,
@Repository어노테이션도 적절히 사용되었습니다.
30-30: LGTM! Spring Data JPA 네이밍 컨벤션을 잘 따름메서드 이름이 Spring Data JPA 네이밍 컨벤션을 잘 따르고 있으며, 반환 타입도 적절합니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/FormService.java (1)
18-18: 메서드 시그니처가 명확하고 적절합니다!폼 기간 중복을 검증하는 메서드의 이름과 매개변수가 의도를 잘 표현하고 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/query/FormStatisticsQuery.java (2)
8-13: 레코드와 빌더 패턴의 적절한 활용통계 데이터를 위한 불변 객체로 레코드를 사용하고, 객체 생성의 유연성을 위해 빌더 패턴을 적용한 것이 좋습니다.
15-20: 중첩 레코드의 명확한 책임 분리각 통계 항목별로 중첩 레코드를 사용하여 데이터 구조를 명확하게 분리한 것이 좋습니다.
Also applies to: 23-27, 32-37
src/main/java/ddingdong/ddingdongBE/domain/form/controller/CentralFormController.java (2)
40-41: 사용자 컨텍스트가 적절히 추가되었습니다.폼 업데이트 시 사용자 정보를 명시적으로 전달하도록 개선되었습니다.
65-69: 통계 API 구현이 잘 되었습니다.통계 데이터를 가져오는 새로운 엔드포인트가 깔끔하게 구현되었습니다.
src/main/java/ddingdong/ddingdongBE/domain/form/api/CentralFormApi.java (1)
82-88: API 문서화가 잘 되어있습니다.통계 API에 대한 OpenAPI 문서화가 상세하게 작성되어 있으며, 보안 요구사항과 응답 상태 코드가 적절히 지정되어 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java (1)
41-42: 중복 날짜 검증이 적절히 추가되었습니다.폼 생성 시 날짜 중복 검증이 추가되어 데이터 무결성이 향상되었습니다.
src/test/java/ddingdong/ddingdongBE/domain/form/service/FormStatisticServiceImplTest.java (1)
1-47: 테스트 설정 구조가 잘 구성되어 있습니다.의존성 주입과 테스트 설정이 명확하게 구성되어 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/form/entity/FieldType.java (1)
13-18: findType 메서드 구현이 적절합니다.스트림 API를 사용한 구현이 명확하고 가독성이 좋습니다. 예외 메시지도 한글로 잘 작성되어 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/command/UpdateFormCommand.java (2)
13-14: 필드 추가가 적절합니다.User와 sections 필드 추가가 CreateFormCommand와 일관성 있게 구현되었습니다.
Also applies to: 20-20
54-54: sections 필드가 Form 엔티티에 올바르게 매핑되었는지 확인이 필요합니다.Form 엔티티의 sections 필드 타입과 일치하는지 검증이 필요합니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/command/CreateFormCommand.java (1)
20-20: CreateFormCommand와 UpdateFormCommand 간의 일관성이 잘 유지되었습니다.sections 필드 추가와 Form 엔티티 변환 로직이 UpdateFormCommand와 동일한 패턴으로 구현되었습니다.
Also applies to: 54-54
src/main/java/ddingdong/ddingdongBE/domain/form/controller/dto/request/CreateFormRequest.java (2)
34-36: sections 필드의 유효성 검사와 문서화가 잘 구현되었습니다.Schema 어노테이션과 NotNull 제약조건이 적절하게 적용되었습니다.
49-49: type 필드 변경과 변환 로직이 개선되었습니다.String으로 타입을 받고 FieldType.findType을 통해 변환하는 방식이 더 유연한 구현입니다.
하지만 잘못된 type 값이 입력될 경우의 예외 처리가 컨트롤러에서 적절하게 처리되는지 확인이 필요합니다.
Also applies to: 70-70
| @Query(value = """ | ||
| SELECT COUNT(fa.id) AS count | ||
| FROM form_application fa | ||
| JOIN form f ON fa.form_id = f.id | ||
| WHERE f.end_date BETWEEN :startDate AND :endDate | ||
| GROUP BY f.id | ||
| ORDER BY count DESC | ||
| LIMIT 1 | ||
| """, nativeQuery = true) | ||
| Integer findMaxApplicationCountByDateRange( | ||
| @Param("startDate") LocalDate startDate, | ||
| @Param("endDate") LocalDate endDate | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Null 처리 및 성능 최적화 필요
- 결과가 없을 경우 null이 반환될 수 있으므로, 메서드 반환 타입을
Optional<Integer>로 변경하는 것을 고려해보세요. - 날짜 범위 검색 성능 향상을 위해
end_date컬럼에 인덱스 추가를 고려해보세요.
| public int getTotalApplicationCountByForm(Form form) { | ||
| return formApplicationRepository.countByForm(form).intValue(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
intValue() 사용으로 인한 오버플로우 위험
formApplicationRepository.countByForm(form)은 Long 타입을 반환하며, .intValue()를 사용하면 값이 Integer의 범위를 초과할 경우 오버플로우가 발생할 수 있습니다. 반환 타입을 long으로 변경하여 오버플로우를 방지하는 것이 좋습니다.
반환 타입 변경에 따른 영향 범위를 확인하기 위해 다음 스크립트를 실행하여 getTotalApplicationCountByForm 메서드의 사용처를 확인하십시오.
🏁 Script executed:
#!/bin/bash
# Description: getTotalApplicationCountByForm 메서드의 사용 위치 확인
# 메서드 사용 위치 검색
rg 'getTotalApplicationCountByForm'Length of output: 944
오버플로우 위험 관련 타입 변경 필요
getTotalApplicationCountByForm 메서드가 인터페이스와 구현체, 그리고 여러 사용처(예를 들어, FacadeCentralFormServiceImpl 및 테스트)에서 int 타입으로 사용되고 있습니다.
이로 인해, formApplicationRepository.countByForm(form)에서 반환된 Long 값을 int로 변환할 때 값이 Integer 범위를 초과하면 오버플로우가 발생할 가능성이 있습니다.
해결을 위해서는 아래 사항들을 함께 수정해야 합니다:
- 인터페이스 수정:
FormStatisticService.java에 정의된 반환 타입을int에서long으로 변경 - 구현체 수정:
FormStatisticServiceImpl.java에서intValue()대신long타입을 직접 반환하도록 수정 - 호출 및 테스트 수정: 이 메서드를 호출하는 다른 클래스(
FacadeCentralFormServiceImpl.java등)와 관련 테스트들도 변수 타입을long으로 변경
전반적인 영향 범위를 고려하여 반환 타입 변경과 이에 따른 연쇄 수정이 필요합니다.
| import ddingdong.ddingdongBE.domain.formapplication.entity.FormAnswer; | ||
| import org.springframework.data.jpa.repository.JpaRepository; | ||
|
|
||
| public interface FormAnswerRepository extends JpaRepository<FormAnswer, Long> { | ||
|
|
||
| int countByFormField(FormField formField); |
There was a problem hiding this comment.
카운트 메서드의 반환 타입을 long으로 변경 제안
countByFormField 메서드는 반환 값이 커질 수 있으므로, int 대신 long 타입을 사용하는 것이 안전합니다.
-int countByFormField(FormField formField);
+long countByFormField(FormField formField);📝 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.
| int countByFormField(FormField formField); | |
| long countByFormField(FormField formField); |
| @@ -18,4 +19,6 @@ public interface FacadeCentralFormService { | |||
| List<FormListQuery> getAllMyForm(User user); | |||
|
|
|||
| FormQuery getForm(Long formId); | |||
|
|
|||
| FormStatisticsQuery getStatisticsByForm(Long formId); | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
formId 파라미터 검증이 필요합니다.
통계 조회 시 formId의 유효성을 검증하고, null이나 음수값에 대한 처리가 구현부에서 이루어져야 합니다.
🏁 Script executed:
#!/bin/bash
# formId 파라미터 검증 로직 확인
rg -A 5 "getStatisticsByForm.*formId" Length of output: 1862
formId 파라미터에 대한 검증 로직이 누락되었습니다.
현재 FacadeCentralFormServiceImpl.java의 getStatisticsByForm 구현에서는 formId를 바로 사용하여 formService.getById(formId)를 호출하고 있으므로, null 값이나 음수와 같은 잘못된 입력에 대한 별도의 검증 로직이 보이지 않습니다.
해당 메서드에 파라미터 검증을 추가하여 올바르지 않은 값에 대해서는 적절한 예외 처리를 구현하는 것이 좋겠습니다.
- 검토 대상:
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java내getStatisticsByForm메서드 - 추가 제안:
formId의null또는 음수 여부를 검사하는 로직을 구현하여, 잘못된 입력에 대해 명확한 메시지나 예외를 발생하도록 처리
| @Override | ||
| public void validateDuplicationDate(Club club, LocalDate startDate, LocalDate endDate) { | ||
| List<Form> overlappingForms = formRepository.findOverlappingForms(club.getId(), startDate, endDate); | ||
|
|
||
| if (!overlappingForms.isEmpty()) { | ||
| throw new InvalidFormPeriodException(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
날짜 유효성 검증 로직 개선 필요
날짜 중복 검증 시 시작일이 종료일보다 이후인 경우에 대한 검증이 누락되어 있습니다. 또한, 중복된 폼이 발견되었을 때 어떤 폼과 중복되는지 상세 정보를 예외 메시지에 포함하면 좋겠습니다.
@Override
public void validateDuplicationDate(Club club, LocalDate startDate, LocalDate endDate) {
+ if (startDate.isAfter(endDate)) {
+ throw new InvalidFormPeriodException("시작일이 종료일보다 늦을 수 없습니다.");
+ }
+
List<Form> overlappingForms = formRepository.findOverlappingForms(club.getId(), startDate, endDate);
if (!overlappingForms.isEmpty()) {
- throw new InvalidFormPeriodException();
+ Form overlappingForm = overlappingForms.get(0);
+ throw new InvalidFormPeriodException(
+ String.format("기간이 중복되는 폼이 존재합니다. (중복 폼: %s, 기간: %s ~ %s)",
+ overlappingForm.getTitle(),
+ overlappingForm.getStartDate(),
+ overlappingForm.getEndDate())
+ );
}
}📝 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.
| @Override | |
| public void validateDuplicationDate(Club club, LocalDate startDate, LocalDate endDate) { | |
| List<Form> overlappingForms = formRepository.findOverlappingForms(club.getId(), startDate, endDate); | |
| if (!overlappingForms.isEmpty()) { | |
| throw new InvalidFormPeriodException(); | |
| } | |
| } | |
| @Override | |
| public void validateDuplicationDate(Club club, LocalDate startDate, LocalDate endDate) { | |
| if (startDate.isAfter(endDate)) { | |
| throw new InvalidFormPeriodException("시작일이 종료일보다 늦을 수 없습니다."); | |
| } | |
| List<Form> overlappingForms = formRepository.findOverlappingForms(club.getId(), startDate, endDate); | |
| if (!overlappingForms.isEmpty()) { | |
| Form overlappingForm = overlappingForms.get(0); | |
| throw new InvalidFormPeriodException( | |
| String.format("기간이 중복되는 폼이 존재합니다. (중복 폼: %s, 기간: %s ~ %s)", | |
| overlappingForm.getTitle(), | |
| overlappingForm.getStartDate(), | |
| overlappingForm.getEndDate()) | |
| ); | |
| } | |
| } |
| @DisplayName("동아리에 주어진 기간과 겹친 폼지를 반환한다.") | ||
| @Test | ||
| void shouldFindOverlappingForms() { | ||
| // given | ||
| Club testClub = clubRepository.save( | ||
| fixture.giveMeBuilder(Club.class) | ||
| .set("deletedAt", null) | ||
| .set("user", null) | ||
| .set("clubMembers", null) | ||
| .set("score", null) | ||
| .sample() | ||
| ); | ||
|
|
||
| Form form1 = fixture.giveMeBuilder(Form.class) | ||
| .set("title", "제목입니다") | ||
| .set("club", testClub) | ||
| .set("startDate", LocalDate.of(2025, 7, 1)) | ||
| .set("endDate", LocalDate.of(2025, 7, 10)) | ||
| .sample(); | ||
| formRepository.save(form1); | ||
|
|
||
| // when | ||
| List<Form> overlappingForms = formRepository.findOverlappingForms( | ||
| testClub.getId(), | ||
| LocalDate.of(2025, 7, 8), | ||
| LocalDate.of(2025, 7, 11) | ||
| ); | ||
|
|
||
| // then | ||
| Assertions.assertThat(overlappingForms.size()).isEqualTo(1); | ||
| Assertions.assertThat(overlappingForms.get(0).getTitle()).isEqualTo("제목입니다"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
테스트 케이스 보완 필요
현재 테스트는 기본적인 중복 케이스만 검증하고 있습니다. 다음과 같은 추가 테스트 케이스가 필요합니다:
- 기간이 겹치지 않는 경우
- 시작일만 겹치는 경우
- 종료일만 겹치는 경우
- 기간이 완전히 포함되는 경우
예시 테스트 케이스:
@Test
@DisplayName("기간이 겹치지 않는 경우 빈 리스트를 반환한다")
void shouldReturnEmptyListWhenDatesDoNotOverlap() {
// given
Club testClub = createTestClub();
createTestForm(testClub,
LocalDate.of(2025, 7, 1),
LocalDate.of(2025, 7, 10));
// when
List<Form> overlappingForms = formRepository.findOverlappingForms(
testClub.getId(),
LocalDate.of(2025, 7, 11),
LocalDate.of(2025, 7, 20)
);
// then
assertThat(overlappingForms).isEmpty();
}| FormAnswer answer = FormAnswer.builder() | ||
| .formApplication(null) | ||
| .value(null) | ||
| .formField(savedField) | ||
| .build(); | ||
| FormAnswer answer2 = FormAnswer.builder() | ||
| .formApplication(null) | ||
| .value(null) | ||
| .formField(savedField) | ||
| .build(); | ||
| FormAnswer answer3 = FormAnswer.builder() | ||
| .formApplication(null) | ||
| .value(null) | ||
| .formField(savedField) | ||
| .build(); | ||
| formAnswerRepository.saveAll(List.of(answer, answer2, answer3)); |
There was a problem hiding this comment.
FormAnswer 객체의 null 값 처리 개선 필요
테스트 데이터에서 formApplication과 value가 null로 설정되어 있습니다. 실제 시나리오를 더 잘 반영하도록 의미 있는 값을 설정하는 것이 좋겠습니다.
- .formApplication(null)
- .value(null)
+ .formApplication(formApplication)
+ .value("테스트 답변")📝 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.
| FormAnswer answer = FormAnswer.builder() | |
| .formApplication(null) | |
| .value(null) | |
| .formField(savedField) | |
| .build(); | |
| FormAnswer answer2 = FormAnswer.builder() | |
| .formApplication(null) | |
| .value(null) | |
| .formField(savedField) | |
| .build(); | |
| FormAnswer answer3 = FormAnswer.builder() | |
| .formApplication(null) | |
| .value(null) | |
| .formField(savedField) | |
| .build(); | |
| formAnswerRepository.saveAll(List.of(answer, answer2, answer3)); | |
| FormAnswer answer = FormAnswer.builder() | |
| .formApplication(formApplication) | |
| .value("테스트 답변") | |
| .formField(savedField) | |
| .build(); | |
| FormAnswer answer2 = FormAnswer.builder() | |
| .formApplication(formApplication) | |
| .value("테스트 답변") | |
| .formField(savedField) | |
| .build(); | |
| FormAnswer answer3 = FormAnswer.builder() | |
| .formApplication(formApplication) | |
| .value("테스트 답변") | |
| .formField(savedField) | |
| .build(); | |
| formAnswerRepository.saveAll(List.of(answer, answer2, answer3)); |
| @@ -40,7 +45,7 @@ record UpdateFormFieldRequest( | |||
|
|
|||
| @Schema(description = "질문 종류", example = "CHECK_BOX") | |||
| @NotNull(message = "질문 종류는 null이 될 수 없습니다.") | |||
| FieldType type, | |||
| String type, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
FieldType 변환 로직 개선 필요
type 필드를 String으로 받고 FieldType.findType()을 통해 변환하는 방식은 런타임 에러의 위험이 있습니다. 잘못된 type 문자열이 입력될 경우 예외가 발생할 수 있습니다.
다음과 같은 개선사항을 제안합니다:
type필드에 유효한 값만 입력되도록 validation 추가:
@Schema(description = "질문 종류", example = "CHECK_BOX")
@NotNull(message = "질문 종류는 null이 될 수 없습니다.")
+@Pattern(regexp = "^(CHECK_BOX|TEXT|RADIO)$", message = "유효하지 않은 질문 종류입니다")
String type,findType메소드에서 발생할 수 있는 예외를 명시적으로 처리:
- .type(FieldType.findType(type))
+ .type(Optional.ofNullable(type)
+ .map(FieldType::findType)
+ .orElseThrow(() -> new IllegalArgumentException("유효하지 않은 질문 종류입니다")))Also applies to: 69-69
| public UpdateFormCommand toCommand(User user, Long formId) { | ||
| List<UpdateFormFieldCommand> updateFormFieldCommands = formFields.stream() | ||
| .map(UpdateFormFieldRequest::toCommand) | ||
| .toList(); | ||
| return UpdateFormCommand.builder() | ||
| .user(user) | ||
| .formId(formId) | ||
| .title(title) | ||
| .description(description) | ||
| .startDate(startDate) | ||
| .endDate(endDate) | ||
| .hasInterview(hasInterview) | ||
| .sections(sections) | ||
| .formFieldCommands(updateFormFieldCommands) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
날짜 유효성 검증 로직 추가 필요
PR의 주요 목적인 "기간 중복 검증"과 관련하여, startDate와 endDate에 대한 기본적인 유효성 검증이 누락되어 있습니다.
다음과 같은 검증 로직 추가를 제안합니다:
UpdateFormRequest클래스에 날짜 검증 어노테이션 추가:
@AssertTrue(message = "종료일은 시작일보다 이후여야 합니다")
private boolean isValidDateRange() {
return startDate != null && endDate != null && !endDate.isBefore(startDate);
}- 또는 서비스 레이어에서 중복 기간 검증을 수행하도록
toCommand메소드 수정:
public UpdateFormCommand toCommand(User user, Long formId) {
+ if (endDate.isBefore(startDate)) {
+ throw new IllegalArgumentException("종료일은 시작일보다 이후여야 합니다");
+ }
List<UpdateFormFieldCommand> updateFormFieldCommands = formFields.stream()| @Schema(description = "섹션 종류", example = "['공통', '서버']") | ||
| @NotNull(message = "섹션 종류는 null이 될 수 없습니다.") | ||
| List<String> sections, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
섹션 필드의 유효성 검증 강화 필요
섹션 리스트에 대한 기본적인 @NotNull 검증은 있지만, 개별 섹션 문자열에 대한 유효성 검증이 부족합니다. 빈 문자열이나 허용되지 않는 섹션 값이 입력될 수 있습니다.
다음과 같은 개선사항을 제안합니다:
@Schema(description = "섹션 종류", example = "['공통', '서버']")
@NotNull(message = "섹션 종류는 null이 될 수 없습니다.")
+@Size(min = 1, message = "최소 하나의 섹션이 필요합니다")
List<String> sections,또한 섹션 값들의 유효성을 검증하는 커스텀 validator를 추가하는 것을 권장합니다.
📝 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.
| @Schema(description = "섹션 종류", example = "['공통', '서버']") | |
| @NotNull(message = "섹션 종류는 null이 될 수 없습니다.") | |
| List<String> sections, | |
| @Schema(description = "섹션 종류", example = "['공통', '서버']") | |
| @NotNull(message = "섹션 종류는 null이 될 수 없습니다.") | |
| @Size(min = 1, message = "최소 하나의 섹션이 필요합니다") | |
| List<String> sections, |
src/main/java/ddingdong/ddingdongBE/domain/form/service/GeneralFormService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImplTest.java (1)
121-134: 테스트 데이터 생성 방식의 일관성이 필요합니다.deleteForm, validateEqualsClub, getAllMyForm 메서드들에서 여전히 FixtureMonkey를 사용하고 있습니다. 코드베이스의 일관성을 위해 이 메서드들도 빌더 패턴을 사용하도록 변경하는 것이 좋겠습니다.
예시 리팩토링:
- User user = fixtureMonkey.giveMeBuilder(User.class) - .set("id", 1L) - .set("Role", Role.CLUB) - .set("deletedAt", null) - .sample(); + User user = User.builder() + .id(1L) + .role(Role.CLUB) + .build();Also applies to: 152-179, 195-208
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/ddingdong/ddingdongBE/domain/form/controller/CentralFormController.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java(2 hunks)src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImplTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (4)
src/main/java/ddingdong/ddingdongBE/domain/form/controller/CentralFormController.java (2)
40-41: 사용자 컨텍스트 처리가 잘 구현되었습니다!사용자 정보를 추출하고 서비스 계층에 전달하는 로직이 일관성 있게 구현되었습니다.
65-73: 폼 통계 조회 기능이 깔끔하게 구현되었습니다!
- 사용자 권한 확인
- 서비스 계층 위임
- 응답 변환
모든 관심사가 잘 분리되어 있고 다른 메서드들과 일관된 패턴을 따르고 있습니다.
src/test/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImplTest.java (2)
57-64: 테스트 데이터 생성 방식이 개선되었습니다.FixtureMonkey 대신 빌더 패턴을 사용하여 테스트 데이터를 생성하는 방식으로 변경되어 코드의 가독성과 유지보수성이 향상되었습니다.
하지만 폼 기간 중복 검증에 대한 테스트 케이스가 누락되어 있습니다.
폼 기간 중복 검증 로직에 대한 테스트 케이스를 추가하시겠습니까?
82-89: updateForm 테스트에서도 동일한 개선이 이루어졌습니다.createForm과 마찬가지로 FixtureMonkey에서 빌더 패턴으로의 전환이 일관성 있게 적용되었습니다.
다만 여기에서도 폼 기간 중복 검증에 대한 테스트가 필요합니다:
- 기간이 중복되는 경우의 실패 케이스
- 기간이 중복되지 않는 경우의 성공 케이스
이러한 테스트 케이스들을 추가하는 것이 좋겠습니다. 테스트 코드 작성을 도와드릴까요?
🚀 작업 내용
현재 dding-97 머지가 안되어서, fileChanged가 많네요. 머지 후 리뷰해도 좋을 것 같습니다.
🤔 고민했던 내용
💬 리뷰 중점사항
FormService와 FormRepository 쿼리부분만 봐주셔도 좋을 것 같아요
Summary by CodeRabbit