refactor: 공통 추천 대학 로직 변경 #175
Conversation
- 해당 학기에 열리는 대학들을 랜덤으로 가져오도록
- 랜덤으로 가져오는 것을 다시 셔플할 필요는 없으므로
- final 이 위로 가도록
| @Service | ||
| @RequiredArgsConstructor | ||
| public class GeneralUniversityRecommendService { | ||
|
|
There was a problem hiding this comment.
기존에 GeneralRecommendUniversities 였던 클래스 이름을
GeneralUniversityRecommendService 로 바꿨습니다.
이전 이름은 도메인의 느낌이 들어서 변경의 필요성을 느꼈고,
비지니스 로직이 담긴 클래스이니 Service 를 붙여주었습니다.
| @EventListener(ApplicationReadyEvent.class) | ||
| public void init() { | ||
| recommendUniversities = universityInfoForApplyRepository.findRandomByTerm(term, RECOMMEND_UNIVERSITY_NUM); | ||
| } |
There was a problem hiding this comment.
원래 List<UniversityInfoForApply> recommendUniversities가 final 이었고,
init() 함수에서 대학들을 리스트에 add 하는식으로 구현되었었는데, 이를 바꿨습니다.
add 를 사용하면, 외부에서 init() 함수를 호출할 때마다 list 에 12개, 18개, 24개.. 이런식으로 쌓이게 됩니다.
이는 예상치 못한 동작입니다😨
이를 예방하고자 새로운 리스트를 할당하는 식으로 변경했습니다.
| @ThunderingHerdCaching(key = "university:recommend:general", cacheManager = "customCacheManager", ttlSec = 86400) | ||
| public UniversityRecommendsResponse getGeneralRecommends() { | ||
| List<UniversityInfoForApply> generalRecommends = new ArrayList<>(generalRecommendUniversities.getRecommendUniversities()); | ||
| Collections.shuffle(generalRecommends); |
There was a problem hiding this comment.
셔플하는 코드를 없앴습니다.
랜덤한 것을 반환하기도 하고, 애초에 이 부분이 캐싱되어있어서 셔플이 무의미하다 생각합니다.
| """) | ||
| List<UniversityInfoForApply> findUniversityInfoForAppliesBySiteUsersInterestedCountryOrRegionAndTerm(@Param("siteUser") SiteUser siteUser, @Param("term") String term); | ||
|
|
||
| @Query(""" |
There was a problem hiding this comment.
RAND()는 MySQL 문법으로 알고 있는데, nativeQuery=true를 명시해주는게 어떨까요?
There was a problem hiding this comment.
이건 어떻게 구현하는 게 좋은지 잘 모르겠네요
- RAND()를 쓰면 Mysql에 종속적이고
- 서비스단에서 하기엔 전체를 다 불러와야할 거 같고
- Pageable을 쓰기엔 랜덤성이 떨어질 거 같네요
nativeQuery=true로 명시해주는 건 좋은 거 같습니다!
There was a problem hiding this comment.
이전에 코드리뷰중에 실질적으로 MySQL에서 변경할일이 없으니 다른 것을 손해보면서 까지 의존성에 크게 신경 쓸 필요 없다는 의견이 있었던 것 같네요.
There was a problem hiding this comment.
nativeQuery=true 로 바꿔주었습니다~
제가 전에는 '소스코드는 인프라 스트럭처를 몰라야한다' 강경파였는데요,
MySQL 이 아니라 다른 데이터베이스로 옮겨갈 상황은 도저히 있을 것 같지 않네요 ㅎㅎ
관련 이슈
작업 내용
해당 학기에 열린 대학들 중 랜덤하게 가져오도록 변경했습니다.
특이 사항
특이사항은 코멘트에서 더 설명할게요!