feature/#110 testfixtures 기반 professor testcode 구현 #116
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Walkthrough이 풀 리퀘스트는 Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
CodeRabbit Configuration File (
|
![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 and nitpick comments (12)
aics-domain/src/main/java/kgu/developers/domain/professor/domain/ProfessorRepository.java (1)
13-13: ID 기반 삭제 메서드로의 변경이 적절해 보입니다.엔티티 객체 대신 ID를 사용하는 방식으로 변경한 것이 다음과 같은 이점이 있습니다:
- 삭제 전 엔티티를 조회할 필요가 없어 성능이 개선됩니다
- 서비스 계층의 코드가 더 단순해집니다
- JPA 레포지토리 패턴의 일반적인 관행과 일치합니다
aics-api/src/main/java/kgu/developers/api/professor/presentation/request/ProfessorRequest.java (2)
Line range hint
24-25: 이메일 오류 메시지를 더 구체적으로 개선하면 좋겠습니다.현재 메시지인 "학교 이메일 형식으로 입력해주세요."보다 허용되는 도메인을 명시적으로 표시하면 사용자가 더 쉽게 이해할 수 있습니다.
다음과 같이 메시지를 수정하는 것을 제안드립니다:
-@Pattern(regexp = "^[A-Za-z0-9._%+-]+@(kyonggi|kgu)\\.ac\\.kr$", message = "학교 이메일 형식으로 입력해주세요.") +@Pattern(regexp = "^[A-Za-z0-9._%+-]+@(kyonggi|kgu)\\.ac\\.kr$", message = "이메일은 @kyonggi.ac.kr 또는 @kgu.ac.kr 도메인만 사용 가능합니다.")
Line range hint
19-21: 연락처 유효성 검사 패턴을 더 구체화하면 좋겠습니다.현재 패턴은 너무 광범위한 전화번호 형식을 허용하고 있습니다. 교수님 연락처는 주로 교내 전화번호이므로, 더 구체적인 패턴으로 제한하는 것이 좋겠습니다.
다음과 같이 패턴을 수정하는 것을 제안드립니다:
-@Pattern(regexp = "^\\d{2,4}-\\d{3,4}-\\d{4}$", message = "유효한 전화번호 형식이 아닙니다.") +@Pattern(regexp = "^031-\\d{3}-\\d{4}$", message = "경기대학교 수원캠퍼스 전화번호 형식(031-XXX-XXXX)으로 입력해주세요.")aics-domain/src/testFixtures/java/mock/FakeProfessorRepository.java (1)
44-49: 페이지네이션 도입을 고려해보세요.현재 구현은 모든 데이터를 메모리에 로드합니다. 실제 환경과 유사한 테스트를 위해 페이지네이션 지원을 추가하는 것이 좋을 것 같습니다.
+ public List<Professor> findAllOrderByRoleAndName(int page, int size) { + return data.stream() + .sorted(Comparator.comparing(Professor::getRole) + .thenComparing(Professor::getName)) + .skip((long) (page - 1) * size) + .limit(size) + .toList(); + }aics-api/src/main/java/kgu/developers/api/professor/application/ProfessorService.java (2)
13-13: @builder 어노테이션 사용 재고 필요Spring Service 클래스에 @builder 어노테이션을 추가하셨는데, Service 계층은 일반적으로 Spring의 DI를 통해 생성되므로 Builder 패턴이 불필요할 수 있습니다. @requiredargsconstructor로 충분합니다.
-import lombok.Builder; import lombok.RequiredArgsConstructor; -@Builder @Service @RequiredArgsConstructorAlso applies to: 16-16
Line range hint
32-36: 메서드 이름이 더 명확해졌습니다.getProfessorById로 메서드 이름을 변경하여 의도가 더 명확해졌습니다. 다만, 업데이트 로직에서 각각의 필드를 개별적으로 업데이트하는 것보다 단일 update 메서드로 리팩토링하는 것을 고려해보세요.
@Transactional public void updateProfessor(Long id, ProfessorRequest request) { Professor professor = getProfessorById(id); - professor.updateName(request.name()); - professor.updateEmail(request.email()); - professor.updateContact(request.contact()); - professor.updateRole(request.role()); + professor.update(request.name(), request.email(), request.contact(), request.role()); }aics-domain/src/testFixtures/java/user/domain/UserDomainTest.java (2)
Line range hint
16-42: 테스트 케이스 보완이 필요합니다.현재 성공 케이스에서는 "@kgu.ac.kr" 도메인만 테스트하고 있습니다. 요구사항에 따르면 "@kyonggi.ac.kr"과 "@kgu.ac.kr" 두 도메인 모두 허용되어야 합니다. 두 도메인에 대한 테스트 케이스를 추가하는 것이 좋겠습니다.
다음과 같이 테스트 케이스를 추가하는 것을 제안드립니다:
@Test @DisplayName("USER 객체를 생성할 수 있다") public void createUser_Success() { // ... existing test with @kgu.ac.kr } +@Test +@DisplayName("kyonggi.ac.kr 도메인으로 USER 객체를 생성할 수 있다") +public void createUser_WithKyonggiDomain_Success() { + // given + String id = "202411345"; + String password = "password"; + String name = "홍길동"; + String email = "valid@kyonggi.ac.kr"; + String phone = "010-1234-5678"; + Major major = Major.CSE; + + // when + User user = User.create(id, password, name, email, phone, major); + + // then + assertNotNull(user); + assertEquals(email, user.getEmail()); + // ... other assertions +}
Line range hint
64-81: 이메일 도메인 사용의 일관성 개선이 필요합니다.
createUser_InvalidDeptCode_ThrowsException테스트에서는 "@kyonggi.ac.kr" 도메인을 사용하고 있는데, 성공 케이스와 일관성을 유지하기 위해 "@kgu.ac.kr"로 통일하는 것이 좋겠습니다.다음과 같이 수정하는 것을 제안드립니다:
- String email = "valid@kyonggi.ac.kr"; + String email = "valid@kgu.ac.kr";aics-domain/src/main/java/kgu/developers/domain/user/domain/User.java (1)
116-119: 도메인 검증을 위해 Set 컬렉션 사용을 고려해보세요.현재 List를 사용하여 도메인을 검증하고 있는데, 검색 성능 향상을 위해 Set으로 변경하는 것이 좋을 것 같습니다.
다음과 같이 변경을 제안드립니다:
-private static final List<String> ACCESSIBLE_EMAIL_DOMAINS = List.of( +private static final Set<String> ACCESSIBLE_EMAIL_DOMAINS = Set.of( "@kyonggi.ac.kr", "@kgu.ac.kr" );aics-api/src/testFixtures/java/professor/application/ProfessorServiceTest.java (3)
1-20: 정적 임포트 구문의 가독성 개선이 필요합니다.정적 임포트 구문을 다음과 같이 그룹화하여 가독성을 향상시킬 수 있습니다:
- Assertions 관련 정적 임포트
- Domain 관련 정적 임포트
다음과 같이 수정을 제안합니다:
-import static kgu.developers.domain.professor.domain.Role.ASSISTANT; -import static kgu.developers.domain.professor.domain.Role.PROFESSOR; -import static org.assertj.core.api.Assertions.assertThatThrownBy; -import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.jupiter.api.Assertions.assertEquals; + +import static kgu.developers.domain.professor.domain.Role.ASSISTANT; +import static kgu.developers.domain.professor.domain.Role.PROFESSOR;
21-52: 테스트 데이터의 구조화가 필요합니다.테스트 데이터의 가독성과 유지보수성을 높이기 위한 개선사항을 제안드립니다:
- 테스트 데이터를 상수로 추출하여 재사용성을 높이고 중복을 제거할 수 있습니다.
- 테스트 데이터 생성을 별도의 팩토리 메서드로 분리하여 코드의 의도를 명확히 할 수 있습니다.
+ private static final String DEFAULT_CONTACT = "010-1234-5678"; + private static final String KGU_DOMAIN = "kgu.ac.kr"; + private static final String KYONGGI_DOMAIN = "kyonggi.ac.kr"; + + private Professor createTestProfessor(String email, String name, Role role) { + return Professor.builder() + .email(email) + .name(name) + .role(role) + .contact(DEFAULT_CONTACT) + .build(); + }
1-131: 테스트 구현이 전반적으로 잘 되어있으나, 몇 가지 개선이 필요합니다.
- 긍정적인 부분:
- TestFixtures 기반으로 잘 구현되어 있습니다.
- 각 테스트 메서드가 명확한 목적을 가지고 있습니다.
- AAA 패턴을 잘 따르고 있습니다.
- 개선이 필요한 부분:
- 테스트 데이터의 구조화
- 이메일 도메인 검증 테스트 추가
- 엣지 케이스 테스트 보완
테스트 코드의 유지보수성과 확장성을 위해 다음과 같은 구조적 개선을 제안드립니다:
- 테스트 데이터를 위한 별도의 TestFixture 클래스 생성
- 공통 검증 로직을 위한 커스텀 Assertion 클래스 구현
- 테스트 케이스 문서화를 위한 주석 템플릿 도입
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
aics-api/src/main/java/kgu/developers/api/professor/application/ProfessorService.java(3 hunks)aics-api/src/main/java/kgu/developers/api/professor/presentation/ProfessorController.java(1 hunks)aics-api/src/main/java/kgu/developers/api/professor/presentation/request/ProfessorRequest.java(1 hunks)aics-api/src/testFixtures/java/professor/application/ProfessorServiceTest.java(1 hunks)aics-api/src/testFixtures/java/professor/presentation/ProfessorControllerTest.java(1 hunks)aics-domain/src/main/java/kgu/developers/domain/professor/domain/ProfessorRepository.java(1 hunks)aics-domain/src/main/java/kgu/developers/domain/professor/infrastructure/ProfessorRepositoryImpl.java(1 hunks)aics-domain/src/main/java/kgu/developers/domain/user/domain/User.java(2 hunks)aics-domain/src/testFixtures/java/mock/FakeProfessorRepository.java(1 hunks)aics-domain/src/testFixtures/java/professor/domain/ProfessorDomainTest.java(1 hunks)aics-domain/src/testFixtures/java/user/domain/UserDomainTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- aics-api/src/testFixtures/java/professor/presentation/ProfessorControllerTest.java
🔇 Additional comments (15)
aics-domain/src/main/java/kgu/developers/domain/professor/infrastructure/ProfessorRepositoryImpl.java (1)
34-35: ID 기반 삭제 구현이 적절합니다!
객체 기반 삭제에서 ID 기반 삭제로의 변경이 잘 이루어졌습니다. 이는 다음과 같은 이점이 있습니다:
- 더 명확한 삭제 로직
- JPA 표준 방식과의 일관성
- 불필요한 객체 조회 없이 직접 삭제 가능
aics-api/src/main/java/kgu/developers/api/professor/presentation/request/ProfessorRequest.java (1)
9-9: Builder 패턴 적용이 적절합니다!
Record 클래스에 Builder 패턴을 적용하여 객체 생성의 유연성과 가독성이 향상되었습니다.
Also applies to: 11-11
aics-domain/src/testFixtures/java/professor/domain/ProfessorDomainTest.java (3)
1-12: LGTM! 필요한 import 문들이 잘 정리되어 있습니다.
필요한 static import와 일반 import가 깔끔하게 구성되어 있습니다.
15-17: 테스트 메서드 이름이 명확하고 @DisplayName이 잘 사용되었습니다.
테스트의 의도가 한글 DisplayName으로 명확하게 표현되어 있습니다.
24-26: Professor.create() 메서드 호출이 적절합니다.
정적 팩토리 메서드를 사용하여 객체를 생성하는 것이 좋은 방식입니다.
aics-domain/src/testFixtures/java/mock/FakeProfessorRepository.java (3)
14-15: 동시성 처리가 잘 구현되어 있습니다!
Collections.synchronizedList를 사용하여 스레드 안전성을 보장하고, 시퀀스 생성기를 통해 ID 생성을 관리하는 방식이 적절합니다.
37-41: findById 구현이 깔끔합니다!
Stream API를 효과적으로 활용하고 Optional을 적절히 사용하여 구현되었습니다.
19-29:
Builder 패턴 사용 시 누락된 필드가 있습니다.
Professor 엔티티의 모든 필드가 Builder에 포함되어 있는지 확인이 필요합니다. 현재 구현에서는 일부 필드가 누락되었을 경우 null이 될 수 있습니다.
aics-api/src/main/java/kgu/developers/api/professor/application/ProfessorService.java (3)
25-25: ID 반환 처리가 개선되었습니다.
저장된 교수 엔티티의 ID를 명시적으로 가져와서 응답을 생성하는 방식으로 개선되어 좋습니다.
Also applies to: 27-27
41-41: 삭제 로직이 개선되었습니다.
객체를 먼저 조회하지 않고 ID로 직접 삭제하도록 변경되어 성능이 개선되었습니다.
49-53: getProfessorById 메서드 접근 제어자 검토 필요
getProfessorById 메서드를 public으로 변경하셨는데, 이는 내부 구현을 외부에 노출시킬 수 있습니다. 정말 필요한 경우가 아니라면 package-private 또는 protected로 유지하는 것이 좋습니다.
다음 스크립트로 이 메서드의 외부 사용 여부를 확인해보세요:
aics-domain/src/testFixtures/java/user/domain/UserDomainTest.java (1)
25-25: 이메일 도메인 변경이 확인되었습니다.
이메일 도메인을 "@kgu.ac.kr"로 변경한 것이 요구사항과 일치합니다.
aics-domain/src/main/java/kgu/developers/domain/user/domain/User.java (1)
9-13: 컬렉션 관련 import 구문이 잘 정리되었습니다.
필요한 컬렉션 클래스들이 깔끔하게 정리되어 있습니다.
aics-api/src/main/java/kgu/developers/api/professor/presentation/ProfessorController.java (2)
Line range hint 41-95: API 구현이 잘 되어있습니다!
RESTful API 설계 원칙을 잘 따르고 있으며, Swagger 문서화도 잘 되어있습니다. HTTP 상태 코드의 사용도 적절합니다.
Line range hint 41-47: 이메일 도메인 검증 로직 추가 필요
PR 목표에 명시된 "@kyonggi.ac.kr"과 "@kgu.ac.kr" 도메인 검증이 컨트롤러에서 이루어지지 않고 있습니다. 서비스 계층에 도달하기 전에 요청 데이터의 유효성을 검증하는 것이 좋습니다.
aics-domain/src/testFixtures/java/professor/domain/ProfessorDomainTest.java
Show resolved
Hide resolved
aics-domain/src/testFixtures/java/professor/domain/ProfessorDomainTest.java
Show resolved
Hide resolved
aics-domain/src/testFixtures/java/mock/FakeProfessorRepository.java
Outdated
Show resolved
Hide resolved
aics-api/src/main/java/kgu/developers/api/professor/presentation/ProfessorController.java
Show resolved
Hide resolved
aics-api/src/testFixtures/java/professor/application/ProfessorServiceTest.java
Outdated
Show resolved
Hide resolved
aics-api/src/testFixtures/java/professor/application/ProfessorServiceTest.java
Show resolved
Hide resolved
aics-api/src/testFixtures/java/professor/application/ProfessorServiceTest.java
Outdated
Show resolved
Hide resolved
minjo-on
deleted the
feature/#110-testfixtures-based-professor-testcode
branch
Summary
testfixtures 기반 professor testcode 구현
Tasks