refactor: application 도메인 리팩터링 #330
Conversation
- UniversityInfoForApply FK매핑 삭제 및 Long타입으로 변경, 네이밍 변경 - 사용하지 않는 updateUniversityChoice 메서드 제거
- AS-IS: 각 지망별로 Repository를 호출하여 N+1 문제 발생 단일키 임시 캐싱 - TO-BE: 매핑 제거 후 직접 데이터를 분류하여 N+1문제 제거 및 성능 향상 캐싱 제거
- AS-IS: 기존 Repository에서 University타입으로 반환 - TO-BE: Entity변경, 비즈니스 로직 변환에 맞춰 반환타입 Long으로 수정 findByIdsWithUniversityAndLocation 메서드 추가 사용하지 않게 된 getUniversityInfoForApplyByIdAndTerm 메서드 삭제
- 서비스 로직 변경을 위해 3개로 분리되어 있는 기존 메서드를 한 번의 쿼리로 조회하도록 변경
- Entity변경에 따른 수정. 기존 로직은 변화 없음
- Entity변경에 따른 수정 - 전원 통과 - ApplicationQueryService 마지막 메서드 테스트 의도에 맞게 수정
lsy1307
requested review from
Gyuhyeok99,
nayonsoso,
whqtker and
wibaek
as code owners
Walkthrough
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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
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: 1
🧹 Nitpick comments (5)
src/main/java/com/example/solidconnection/university/repository/UniversityInfoForApplyRepository.java (1)
61-68: 1. 성능 최적화를 위한 bulk fetching 메서드 추가새로운
findByIdsWithUniversityAndLocation메서드가 훌륭하게 구현되었습니다:• join fetch를 통한 연관 엔터티 즉시 로딩으로 N+1 쿼리 문제 해결 • 여러 ID를 한 번에 처리하여 데이터베이스 호출 횟수 최소화 • university, country, region을 모두 fetch join으로 필요한 데이터를 한 번에 로드이 변경으로 기존의 개별 조회 방식보다 훨씬 효율적인 데이터 접근이 가능해졌네요.
더 나은 성능을 위해 추가로 고려해볼 수 있는 개선사항:
@Query(""" select distinct uia from UniversityInfoForApply uia join fetch uia.university u join fetch u.country c join fetch u.region r where uia.id in :ids + order by uia.id """)ID 순서로 정렬을 추가하면 결과의 일관성을 보장할 수 있습니다.
src/main/java/com/example/solidconnection/application/domain/Application.java (2)
56-63: 대학 선택 ID 필드의 null 가능성 명시화 필요현재 대학 선택 필드들이 nullable임에도
@Column어노테이션에 명시되어 있지 않습니다. 명확성을 위해 nullable 속성을 추가하는 것을 권장합니다:-@Column +@Column(nullable = true) private Long firstChoiceUniversityApplyInfoId; -@Column +@Column(nullable = true) private Long secondChoiceUniversityApplyInfoId; -@Column +@Column(nullable = true) private Long thirdChoiceUniversityApplyInfoId;
81-101: 생성자 매개변수 순서 불일치두 번째 생성자에서
updateCount가 대학 선택 ID들보다 먼저 나오는데, 이는 다른 생성자들과 일관성이 없습니다.도메인 로직 순서를 고려하면 대학 선택이 먼저 오고, 그 다음 업데이트 횟수가 오는 것이 자연스럽습니다. 매개변수 순서를 통일하는 것을 고려해보세요.
src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java (2)
56-62: Stream 처리 시 null 체크 로직 개선 가능현재 null 필터링이 잘 되어 있지만, 더 명확한 코드를 위해 다음과 같이 개선할 수 있습니다:
List<Long> universityIds = Stream.of( userLatestApplication.getFirstChoiceUniversityApplyInfoId(), userLatestApplication.getSecondChoiceUniversityApplyInfoId(), userLatestApplication.getThirdChoiceUniversityApplyInfoId() ) .filter(Objects::nonNull) .distinct() // 중복 제거 추가 .collect(Collectors.toList());동일한 대학에 중복 지원하는 경우를 고려하여
distinct()를 추가하면 불필요한 쿼리를 더 줄일 수 있습니다.
74-119: 지원서 분류 로직의 효율적인 구현 ✨Map을 활용한 지원서 분류와 스트림을 이용한 DTO 변환이 효율적으로 구현되었습니다.
다만 가독성 향상을 위해 각 선택 순위별 처리를 별도 메서드로 분리하는 것을 고려해보세요:
private List<UniversityApplicantsResponse> createChoiceApplicants( List<UniversityInfoForApply> universityInfos, Map<Long, List<Application>> choiceMap, SiteUser siteUser) { return universityInfos.stream() .map(uia -> UniversityApplicantsResponse.of( uia, choiceMap.getOrDefault(uia.getId(), List.of()).stream() .map(ap -> ApplicantResponse.of(ap, Objects.equals(siteUser.getId(), ap.getSiteUser().getId()))) .toList())) .toList(); }이렇게 하면 중복 코드를 줄이고 유지보수성을 높일 수 있습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/com/example/solidconnection/application/domain/Application.java(3 hunks)src/main/java/com/example/solidconnection/application/repository/ApplicationRepository.java(1 hunks)src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java(2 hunks)src/main/java/com/example/solidconnection/application/service/ApplicationSubmissionService.java(2 hunks)src/main/java/com/example/solidconnection/university/repository/UniversityInfoForApplyRepository.java(1 hunks)src/main/java/com/example/solidconnection/university/repository/custom/UniversityFilterRepository.java(1 hunks)src/main/java/com/example/solidconnection/university/repository/custom/UniversityFilterRepositoryImpl.java(1 hunks)src/test/java/com/example/solidconnection/application/fixture/ApplicationFixture.java(1 hunks)src/test/java/com/example/solidconnection/application/fixture/ApplicationFixtureBuilder.java(3 hunks)src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java(17 hunks)src/test/java/com/example/solidconnection/application/service/ApplicationSubmissionServiceTest.java(1 hunks)src/test/java/com/example/solidconnection/support/integration/BaseIntegrationTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (11)
src/test/java/com/example/solidconnection/application/service/ApplicationSubmissionServiceTest.java (1)
89-101: 테스트 어설션이 새로운 엔터티 구조에 맞게 올바르게 업데이트되었습니다.리팩토링 변경사항과 일치하는 테스트 수정사항들:
- 직접 ID 필드 접근으로 변경:
getFirstChoiceUniversityApplyInfoId(),getSecondChoiceUniversityApplyInfoId(),getThirdChoiceUniversityApplyInfoId()메서드를 사용하여 중첩된 엔터티 참조 대신 직접 ID 값을 확인- 가독성 향상: 어설션 포맷팅이 개선되어 각 검증 항목이 명확하게 구분됨
- 논리적 일관성 유지: 지원서 상태, 업데이트 횟수, 삭제 플래그 등 기존 검증 로직은 그대로 유지
src/main/java/com/example/solidconnection/university/repository/custom/UniversityFilterRepository.java (1)
11-11:Details
✅ Verification successful
성능 최적화를 위한 반환 타입 변경이 적절합니다.
메서드 시그니처 변경사항:
- 반환 타입 최적화:
List<University>에서List<Long>으로 변경하여 메모리 사용량과 데이터 전송량 감소- 쿼리 성능 향상: 전체 엔터티 대신 ID만 반환하여 불필요한 데이터 로딩 방지
- 리팩토링 목표 달성: JPA 프록시 오버헤드 제거 및 N+1 쿼리 문제 해결에 기여
이 메서드를 호출하는 모든 곳에서
LongID를 올바르게 처리하는지 확인해보겠습니다:
🏁 Script executed:
#!/bin/bash # Description: 메서드 호출자들이 새로운 반환 타입을 올바르게 처리하는지 확인 # Test: findByRegionCodeAndKeywords 메서드 사용처 검색 rg -A 10 "findByRegionCodeAndKeywords" --type javaLength of output: 8346
메서드 호출 검증 완료
변경된
findByRegionCodeAndKeywords메서드 호출 처리 상태를 점검했습니다:
- ApplicationQueryService
List<Long> universityIds = universityFilterRepository.findByRegionCodeAndKeywords(...)호출부에서 반환된 Long ID를 정상 처리합니다.- ApplicationQueryService 하위 로직
- 조회된 ID로
applicationRepository.findApplicationsForChoices및universityInfoForApplyRepository.findByIdsWithUniversityAndLocation호출이 문제없이 동작합니다.- UniversityQueryServiceTest
- 해당 테스트는
findByRegionCodeAndKeywordsAndLanguageTestTypeAndTestScoreAndTerm메서드를 검증하며 변경 대상 메서드에는 영향이 없습니다.- 기타 호출부 없음
- 리포지토리 내
findByRegionCodeAndKeywords는 위 외 추가 호출 지점이 발견되지 않았습니다.모든 호출자가 Long ID를 올바르게 처리하고 있어 변경 사항을 승인합니다.
src/test/java/com/example/solidconnection/support/integration/BaseIntegrationTest.java (1)
427-445: 통합 테스트 설정이 새로운 Application 엔터티 구조에 맞게 일관되게 업데이트되었습니다.테스트 데이터 설정 변경사항들:
- 생성자 파라미터 변경:
UniversityInfoForApply객체 대신LongID 값들을 전달하도록 수정- 일관된 업데이트: 모든 Application 인스턴스가 동일한 패턴으로 업데이트됨
테스트유저_2_괌대학_B_괌대학_A_린츠_카톨릭대학_지원서: 2L, 1L, 15L테스트유저_3_괌대학_A_괌대학_B_그라츠공과대학_지원서: 1L, 2L, 14L테스트유저_4_메이지대학_그라츠대학_서던덴마크대학_지원서: 18L, 13L, 9L- 기타 테스트 케이스들도 동일한 패턴으로 적용
- 예측 가능한 테스트 데이터: 하드코딩된 ID 값들로 안정적인 통합 테스트 환경 구성
src/main/java/com/example/solidconnection/university/repository/custom/UniversityFilterRepositoryImpl.java (1)
31-46: 인터페이스 변경에 맞는 구현체 수정이 올바르게 완료되었습니다.쿼리 최적화 변경사항들:
- 선택 필드 최적화:
.select(university.id)로 변경하여 필요한 ID만 조회- 쿼리 구조 개선:
.selectFrom(university)에서.from(university)로 변경하여 명시적인 선택 필드 지정- 중복 제거 추가:
.distinct()호출로 조인으로 인한 중복 ID 방지- 필터링 로직 유지: 지역 코드와 키워드 기반 필터링 로직은 그대로 유지하여 기능적 일관성 보장
- 가독성 향상: where 절 포맷팅 개선으로 조건문 구조가 더 명확해짐
이러한 변경사항들은 전체 엔터티를 로딩하지 않고도 필요한 대학 ID만 효율적으로 조회할 수 있게 해줍니다.
src/main/java/com/example/solidconnection/application/service/ApplicationSubmissionService.java (2)
53-56: 1. 대학 선택 정보 처리 방식 개선 - ID 기반 접근으로 전환UniversityInfoForApply 엔터티를 직접 조회하는 대신 ID만을 사용하도록 변경된 점이 좋습니다. 이 변경으로 다음과 같은 이점을 얻을 수 있습니다:
• JPA 프록시 객체 생성 오버헤드 제거 • 불필요한 연관 엔터티 조회 방지 • 메모리 사용량 최적화
66-76:Details
✅ Verification successful
2. Application 생성자 호출 방식 업데이트
새로운 Application 생성자가 university ID들을 직접 받도록 변경되어 리팩토링 목표와 일치합니다. 코드가 더 간결해지고 엔터티 의존성이 줄어들었네요.
Application 엔터티의 생성자가 올바르게 변경되었는지 확인해보겠습니다:
🏁 Script executed:
#!/bin/bash # Description: Application 엔터티의 생성자 시그니처 확인 # Application 엔터티의 생성자를 확인하여 Long 타입 파라미터를 받는지 검증 ast-grep --pattern $'class Application { $$$ Application($$$, Long $_, Long $_, Long $_, $$$) { $$$ } $$$ }'Length of output: 121
아래 스크립트를 실행해서 Application 엔티티의 생성자 시그니처를 직접 확인해 주세요:
🏁 Script executed:
#!/bin/bash # 1) Application.java 파일 경로 찾기 APP_FILE=$(find src/main/java -name Application.java) echo "Application 엔터티 파일: $APP_FILE" # 2) 생성자 정의 추출 grep -En 'public Application\(' "$APP_FILE"Length of output: 532
아래 스크립트를 실행해서
Application클래스의 각 생성자 시그니처를 한눈에 확인해 주세요:
🏁 Script executed:
#!/bin/bash # Application.java 경로 찾기 APP_FILE=$(find src/main/java -name Application.java) echo "📄 Application 엔티티 파일: $APP_FILE" # 60~110라인 출력하여 모든 생성자 시그니처 확인 echo "🔍 생성자 정의 전체 보기 (라인 60~110):" sed -n '60,110p' "$APP_FILE"Length of output: 2204
✅ Application 생성자 시그니처 정상 확인
교차 검증 결과, 새로운 생성자가 기대한 파라미터를 정확히 수용함을 확인했습니다.
- 생성자 파라미터 구성 검증
src/main/java/com/example/solidconnection/application/domain/Application.java81~89라인에서
SiteUser, Gpa, LanguageTest, String term, Integer updateCount, Long firstChoiceUniversityApplyInfoId, Long secondChoiceUniversityApplyInfoId, Long thirdChoiceUniversityApplyInfoId, String nicknameForApply순으로 정의된 생성자를 발견했습니다.- 리팩토링 목표 일치
- 세 개의 Long 타입 대학 선택 ID를 직접 받도록 변경되어 호출부가 깔끔해졌고, 엔터티 의존성이 줄었습니다.
src/test/java/com/example/solidconnection/application/fixture/ApplicationFixture.java (1)
17-37: 1. 테스트 픽스처의 일관된 리팩토링 적용테스트 픽스처가 프로덕션 코드의 변경사항과 완벽하게 일치하도록 업데이트되었습니다:
• 파라미터 타입을 UniversityInfoForApply → Long ID로 변경 • builder 메서드 호출도 ID 기반으로 수정 • 테스트와 실제 코드 간의 일관성 유지이런 식으로 테스트 코드를 함께 업데이트해주신 것이 매우 좋습니다. 리팩토링 시 테스트 코드도 함께 관리하는 것이 중요하죠.
src/main/java/com/example/solidconnection/application/repository/ApplicationRepository.java (1)
23-37:Details
✅ Verification successful
1. 효율적인 통합 쿼리 메서드로 성능 대폭 개선
기존의 3개 개별 쿼리를 하나로 통합한
findApplicationsForChoices메서드가 매우 잘 설계되었습니다:• 첫 번째, 두 번째, 세 번째 선택을 OR 조건으로 한 번에 처리 • join fetch를 통한 siteUser 연관 엔터티 미리 로딩 • List<Long> 파라미터로 여러 대학을 효율적으로 처리 • 데이터베이스 호출 횟수를 3분의 1로 감소이 변경으로 N+1 쿼리 문제가 해결되고 전반적인 성능이 크게 향상될 것으로 예상됩니다.
새로운 쿼리 메서드가 기존 서비스 로직에서 올바르게 사용되고 있는지 확인해보겠습니다:
🏁 Script executed:
#!/bin/bash # Description: 새로운 findApplicationsForChoices 메서드 사용처 확인 # ApplicationQueryService에서 이 메서드를 사용하는지 확인 rg -A 10 "findApplicationsForChoices" --type java # 기존의 개별 쿼리 메서드 호출이 남아있지 않은지 확인 rg "findApplicationsByFirstChoice|findApplicationsBySecondChoice|findApplicationsByThirdChoice" --type javaLength of output: 4647
새 통합 쿼리 메서드 사용 및 잔여 메서드 제거 검증 완료
- 효율적인 쿼리 메서드 사용 위치 확인
ApplicationQueryService.java에서findApplicationsForChoices가 정상 호출됩니다.- 기존 개별 쿼리 메서드 제거 확인
findApplicationsByFirstChoice,findApplicationsBySecondChoice,findApplicationsByThirdChoice호출 흔적이 없습니다.- 최종 승인
- N+1 문제 해결 및 성능이 대폭 개선되었습니다.
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java (1)
412-446: 개선된 경쟁자 목록 검증 로직 👍이전의 단순한
containsExactly검증에서 더 세밀한 검증으로 개선되었습니다:
- 각 지원 순위별로 분리하여 검증
- 빈 지원자 목록 필터링
isMine플래그 정확성 확인이러한 상세한 검증은 테스트의 신뢰성을 높여줍니다.
src/test/java/com/example/solidconnection/application/fixture/ApplicationFixtureBuilder.java (1)
21-23: 테스트 픽스처 빌더 리팩토링 완료 ✅도메인 모델의 변경사항에 따라 테스트 픽스처가 적절히 수정되었습니다:
- 필드 타입 변경: UniversityInfoForApply → Long ID
- 메서드 시그니처 업데이트: setter 메서드들이 ID를 받도록 변경
- 생성자 호출 수정: Application 생성 시 ID 사용
변경사항이 일관성 있게 적용되었습니다.
Also applies to: 42-55, 78-80
src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java (1)
38-50: 쿼리 최적화 및 벌크 조회 도입 👏기존의 여러 번의 쿼리 실행을 3번의 효율적인 쿼리로 줄인 훌륭한 리팩토링입니다:
- 대학 ID 필터링: 지역과 키워드로 한 번에 필터링
- 지원서 벌크 조회: 모든 관련 지원서를 한 번에 조회
- 대학 정보 벌크 조회: 필요한 대학 정보를 한 번에 조회
N+1 문제를 효과적으로 해결했습니다.
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
Outdated
Show resolved
Hide resolved
- UniversityInfoForApplyId 하드코딩 되어있는 부분 수정 - 테스트 통과 확인
There was a problem hiding this comment.
수연님 우선, 1차 리뷰 드립니다~
리뷰하다보니 너무 길어져서요😅 끊어서 리뷰해보도록 할게요!
그리고, 저런 코드들을 리팩터링하게 해서 정말 죄송하다는 말씀을 드립니다..
저 코드를 보아하니 그때 마침 제가 함수형 인터페이스를 새로 배워서 몹시 신이 난 상태였던 것 같습니다.
그래도 그렇지 저런 코드를 만들다니… 정말 죄송합니다 🫠
그럼에도 짧은 시간안에 도메인을 이해하시고, 우리 팀의 PR 형식에 맞게 수정해주셔서 감사합니다🥹
[컨벤션] 표시한 것은, 우리 팀에 원래 있던 컨벤션입니다.
저는 팀은 하나의 컨벤션을 따라야한다는 주의입니다.
제가 좋아하는 책의 구절을 인용합니다!
팀이나 조직 전체가 같은 코딩 스타일을 따르면, 그것은 마치 모두가 같은 언어를 유창하게 말하는 것과 비슷하다. 서로 오해할 위험이 크게 줄어들고, 이로 인해 버그가 줄어들고, 혼란스러운 코드를 이해하는 데 낭비하는 시간이 줄어든다.
‘왜 이런 컨벤션을 따라야하는지 이해가 되지 않는다’하는 부분이 있으시면 말씀해주세요~
src/main/java/com/example/solidconnection/application/domain/Application.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/application/domain/Application.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
Outdated
Show resolved
Hide resolved
| for (Application a : applications) { | ||
| if (a.getFirstChoiceUniversityApplyInfoId() != null) { | ||
| firstChoiceMap.computeIfAbsent(a.getFirstChoiceUniversityApplyInfoId(), k -> new ArrayList<>()).add(a); | ||
| } | ||
| if (a.getSecondChoiceUniversityApplyInfoId() != null) { | ||
| secondChoiceMap.computeIfAbsent(a.getSecondChoiceUniversityApplyInfoId(), k -> new ArrayList<>()).add(a); | ||
| } | ||
| if (a.getThirdChoiceUniversityApplyInfoId() != null) { | ||
| thirdChoiceMap.computeIfAbsent(a.getThirdChoiceUniversityApplyInfoId(), k -> new ArrayList<>()).add(a); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
a 말고 application으로 그냥 명확히 하는 건 어떤가요?
if (a.getFirstChoiceUniversityApplyInfoId() != null) {
firstChoiceMap.computeIfAbsent(a.getFirstChoiceUniversityApplyInfoId(), k -> new ArrayList<>()).add(a);
}
똑같은 것을 first, second, third냐에 따라만 다르게 호출하니 함수로 빼도 좋을 거 같네요!
| List<UniversityApplicantsResponse> firstChoiceApplicants = universityInfos.stream() | ||
| .map(uia -> UniversityApplicantsResponse.of( | ||
| uia, | ||
| firstChoiceMap.getOrDefault(uia.getId(), List.of()).stream() | ||
| .map(ap -> ApplicantResponse.of(ap, Objects.equals(siteUser.getId(), ap.getSiteUser().getId()))) | ||
| .toList())) | ||
| .toList(); | ||
|
|
There was a problem hiding this comment.
여기도 함수로 빼도 좋을 거 같네요!
지망별로 application을 fetch join으로 가져오면 5번의 쿼리로 분류 필요 없이 응답을 할 수 있는데 지금은 3번의 쿼리로 메모리에서 분류를 하고 있으니 어느정도의 트레이드 오프가 있을지 궁금하네요 ㅎㅎ 성능은 지금 방식이 당연히 더 좋을 거 같습니다!
There was a problem hiding this comment.
이 부분도 반영했습니다!
추가로 성능에 대한 부분은 비교를 한 번 해봐야 할 것 같긴 하네요!
| where uia.id in :ids | ||
| """) | ||
| List<UniversityInfoForApply> findByIdsWithUniversityAndLocation(@Param("ids") List<Long> ids); | ||
|
|
| join fetch u.country c | ||
| join fetch u.region r | ||
| where uia.id in :ids | ||
| """) |
There was a problem hiding this comment.
[컨벤션] """)를 혹시 일부로 왼쪽에 붙여두신건가요? 쿼리 블록들이 조금 어색하게 느껴진 거 같아요
@nayonsoso 이것도 뭔가 정해야할까요?
There was a problem hiding this comment.
일부러는 아니고 실수 입니다! 비슷한 부분 컨벤션 통일하여 반영하였습니다.
- 코드 컨벤션 통일 - 변수명 이해하기 쉽도록 수정 - Application Repository universityId기반과 uiaId기반으로 분기
- 마지막 메서드 테스트 의도에 맞게 수정 - 컨벤션에 맞게 수정 - BaseIntegrationTest에서 firstUIAId에 null값을 입력하는 부분 수정
- 조회 성능 향상을 위해 Application Entity Index추가 - Application Repository 불필요한 공백 삭제
- Application FK제약조건 제거 - column명 변경 - Index 추가
src/main/java/com/example/solidconnection/application/domain/Application.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/application/domain/Application.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/application/domain/Application.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/application/repository/ApplicationRepository.java
Outdated
Show resolved
Hide resolved
...java/com/example/solidconnection/university/repository/UniversityInfoForApplyRepository.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/application/service/ApplicationSubmissionService.java
Outdated
Show resolved
Hide resolved
...java/com/example/solidconnection/university/repository/UniversityInfoForApplyRepository.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
Outdated
Show resolved
Hide resolved
| ALTER TABLE application DROP FOREIGN KEY FKeajojvwgn069mfxhbq5ja1sws; | ||
| ALTER TABLE application DROP FOREIGN KEY FKepp2by7frnkt1o1w3v4t4lgtu; | ||
| ALTER TABLE application DROP FOREIGN KEY FKi822ljuirbu9o0lnd9jt7l7qg; |
There was a problem hiding this comment.
이 FK는 왜 없애시는건가요? 논의된게 있었나요? (제가 기억하지 못하는걸수도 있습니다 😅)
There was a problem hiding this comment.
first~third univApplyInfo Id fk매핑 제거에 따른 제약조건 삭제인데요 직접 삭제할 필요가 없을까요?? 제가 잘못생각하고 있는 걸까요??
There was a problem hiding this comment.
저는 매핑은 끊더라도 FK는 유지하는게 좋다 생각해요!
- 가벼운 수정 사항(공백 삭제 및 함수 명 변경) - Application Column명 수정 - 쿼리 간결화 - DTO에 매핑책임 이관
- 테스트 코드 데이터 형식 수정 - Flyway Column명 수정
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/example/solidconnection/application/dto/UniversityApplicantsResponse.java (1)
26-28: helper method가 명확하고 효율적입니다.
isUsers메서드가 다음과 같이 잘 설계되었습니다:
1. 메서드명이 역할을 명확히 표현
2. ID 비교를 통한 효율적인 동등성 검사
3. private static으로 적절한 접근 제어한 가지 제안사항이 있습니다 - null safety를 위한 방어 코드를 추가하는 것을 고려해보세요.
다음과 같이 null safety를 강화할 수 있습니다:
private static boolean isUsers(Application application, SiteUser siteUser) { - return application.getSiteUser().getId().equals(siteUser.getId()); + return application.getSiteUser() != null && + siteUser != null && + application.getSiteUser().getId().equals(siteUser.getId()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/java/com/example/solidconnection/application/domain/Application.java(5 hunks)src/main/java/com/example/solidconnection/application/dto/UniversityApplicantsResponse.java(2 hunks)src/main/java/com/example/solidconnection/application/repository/ApplicationRepository.java(1 hunks)src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java(2 hunks)src/main/java/com/example/solidconnection/application/service/ApplicationSubmissionService.java(2 hunks)src/main/java/com/example/solidconnection/university/repository/UniversityInfoForApplyRepository.java(3 hunks)src/main/java/com/example/solidconnection/university/repository/custom/UniversityFilterRepository.java(1 hunks)src/main/java/com/example/solidconnection/university/repository/custom/UniversityFilterRepositoryImpl.java(2 hunks)src/main/resources/db/migration/V13_add_application_index_and_delete_manny_to_one_mapping.sql(1 hunks)src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java(21 hunks)src/test/java/com/example/solidconnection/application/service/ApplicationSubmissionServiceTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/test/java/com/example/solidconnection/application/service/ApplicationSubmissionServiceTest.java
- src/main/java/com/example/solidconnection/application/service/ApplicationSubmissionService.java
- src/main/resources/db/migration/V13_add_application_index_and_delete_manny_to_one_mapping.sql
- src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
- src/main/java/com/example/solidconnection/university/repository/custom/UniversityFilterRepositoryImpl.java
- src/main/java/com/example/solidconnection/application/repository/ApplicationRepository.java
- src/main/java/com/example/solidconnection/university/repository/UniversityInfoForApplyRepository.java
- src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java
- src/main/java/com/example/solidconnection/application/domain/Application.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
src/main/java/com/example/solidconnection/application/dto/UniversityApplicantsResponse.java (3)
3-4: 새로운 import 추가가 적절합니다.리팩토링에 따라 필요한 도메인 클래스들을 정확히 import 했습니다.
1.Application- 엔티티 리스트를 직접 처리하기 위해 필요
2.SiteUser- 사용자별 컨텍스트 정보 제공을 위해 필요
15-15: 메서드 시그니처 변경이 리팩토링 목적과 일치합니다.기존의
List<ApplicantResponse> applications에서List<Application> applications, SiteUser siteUser로 변경된 것은 다음과 같은 이점을 제공합니다:
1. 서비스 레이어에서 DTO 변환 책임을 분리
2. 사용자별 컨텍스트 정보(isUsers) 제공 가능
3. Application 엔티티의 ID 기반 구조 변경에 대응
21-24: 스트림 변환 로직이 깔끔하게 구현되었습니다.Application 리스트를 ApplicantResponse로 변환하는 로직이 잘 구현되었습니다:
1. 함수형 프로그래밍 스타일 적용으로 가독성 향상
2.isUsershelper method를 통한 사용자별 컨텍스트 제공
3.toList()사용으로 불변 리스트 생성src/main/java/com/example/solidconnection/university/repository/custom/UniversityFilterRepository.java (2)
11-11: 1. 메서드 반환 타입 변경이 리팩토링 목표와 잘 일치합니다
findByRegionCodeAndKeywords메서드의 반환 타입을List<University>에서List<UniversityInfoForApply>로 변경한 것은 전체 도메인 리팩토링 방향과 일치합니다. JPA 관계 최적화를 통해 N+1 쿼리 문제를 해결하려는 목표에 부합하는 변경사항입니다.
11-11: 2. 구현체 클래스와 호출자들의 일관성 확인이 필요합니다인터페이스의 반환 타입이 변경되었으므로, 다음 항목들의 일관성을 확인해 주세요:
1. `UniversityFilterRepositoryImpl.java`의 구현체가 새로운 반환 타입에 맞게 업데이트되었는지 2. 이 메서드를 호출하는 서비스 레이어가 새로운 반환 타입을 올바르게 처리하는지다음 스크립트를 실행하여 관련 파일들의 일관성을 확인해 보겠습니다:
#!/bin/bash # 설명: UniversityFilterRepository 인터페이스 변경과 관련된 구현체 및 호출자들의 일관성을 확인합니다. echo "=== 1. 구현체 클래스 확인 ===" ast-grep --pattern 'class $_ implements UniversityFilterRepository { $$$ findByRegionCodeAndKeywords($_, $_) { $$$ } $$$ }' echo -e "\n=== 2. 메서드 호출자 확인 ===" rg -A 3 -B 1 "findByRegionCodeAndKeywords" echo -e "\n=== 3. University 타입 반환 관련 코드 잔존 여부 확인 ===" rg -A 2 -B 2 "List<University>.*findByRegionCodeAndKeywords"
There was a problem hiding this comment.
flyway 스크립트 네이밍 컨벤션을 따르려면, 파일 이름이
V13__add_application_index_and_delete_manny_to_one_mapping.sql로
V13 뒤에 언더버가 2개 와야합니다!
lsy1307
changed the title
3 participants
관련 이슈
작업 내용
📌기존 코드의 문제점
기존 Entity는 UniversityInfoForApply를 @ ManyToOne 연관관계로 직접 참조하고 있었습니다.
1. 불필요한 연관관계 관리
2. N+1 위험
📌개선 방향
1. Entity수정
2. 비즈니스 로직 수정
📌기타 변경사항
특이 사항
DB스키마가 변경되었습니다. 확인해주시기 바랍니다.
리뷰 요구사항 (선택)
불필요한 변경이나 삭제가 없었는지 확인해주시면 감사하겠습니다.
또한 성능 개선의 여지가 더 있는지, 문제가 발생할 부분이 있는지 그리고 테스트 코드가 의도된 바와 같은지 확인해주시면 감사하겠습니다.