Refactor/#45 매칭 도메인 리펙토링 및 로직 고도화 #49
Closed
Conversation
This file contains 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
suhyun0918
added
feat
coke98
approved these changes
Feb 12, 2024
|
|
||
| public Integer getMaxNormalPartners() { | ||
| return Integer.parseInt(maxNormalPartners); | ||
| return maxNormalPartners; |
| } | ||
| tryMatchBetween(participant, candidates); | ||
| } | ||
| matchingTypeGroup.getMatchingTypes().forEach(matchingType -> matchingType.doMatch(participantGroup, languageQueue)); |
There was a problem hiding this comment.
아 이거... 코드 줄어든 양에서부터 심상치 않군요...
일급 컬렉션과 매칭 타입 인터페이스 활용으로 각 매칭 단계를 관리하고, forEach로 많았던 if문들 대체...
세부적인 매칭 구현은 각 타입 구현체에서 관리
덕분에 비즈니스 클래스 내에서는 초기화와 매칭진행이라는 추상적 단위만 남았네요! 👍
특히 각 타입 클래스마다 오버라이드하여 매칭 동작 메서드들 나눈점 너무 읽기 편했습니다!
아니.. 이런 생각을 하셨군요. 덕분에 많이 배워갑니다. LGTM
There was a problem hiding this comment.
말씀하신 부분이 정확히 맞습니다 🤗
제가 리펙토링 한 모든 내용을 한번에 정리해주셨군요 ... 정확히 이해해주셔서 제가 더 드릴 말씀이 없네요..
알아봐주셔서 대단히 감사합니다 ! 😊
| import java.util.Queue; | ||
|
|
||
| public interface MatchingType { | ||
| void doMatch(ParticipantGroup participantGroup, LanguageQueue languageQueue); |
| @@ -28,26 +32,28 @@ public MatchingController(final MatchingApplicationService matchingApplicationSe | |||
|
|
|||
| @PostMapping("/applications") | |||
| public ResponseEntity<?> applyMatch(final @Login LoginMember loginMember, | |||
| final @RequestBody MatchingApplicationInput matchingApplicationInput) { | |||
| matchingApplicationService.saveParticipant(matchingApplicationInput.toRequest(loginMember.memberId())); | |||
| final @RequestBody @LanguageCheck MatchingApplicationRequest matchingApplicationRequest) { | |||
| public static MatchingApplication from(final LoginMember loginMember, | ||
| final MatchingApplicationRequest matchingApplicationRequest, | ||
| final MatchingRound matchingRound) { | ||
| return MatchingApplication.of(matchingRound, loginMember.memberId(), |
There was a problem hiding this comment.
from내에서 of메서드를 호출하도록 구현하셨군용
There was a problem hiding this comment.
of 만 new 를 통해서 생성하게 해서, 여기저기서 new 키워드를 사용하지 않도록 하고싶었습니다 😝
| import java.util.Queue; | ||
|
|
||
| public class CandidateGroup { | ||
| private final Queue<Participant> candidateQueue; |
| VALID_TIME(LocalDateTime.of(2024, 1, 29, 10, 0)), | ||
|
|
||
| MONDAY(LocalDateTime.of(2024, 1, 29, 0, 0)), | ||
| THURSDAY(LocalDateTime.of(2024, 2, 1, 0, 0)), |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🤨 Motivation
🔑 Key Changes
🙏 To Reviewers
전역변수 to 지역변수에 관하여
매칭 참가자를 지역변수화 하는 안건에 대하여 피드백을 받고, 이를 바탕으로 구현하던 도중
한가지 의문이 들었습니다.
저는 '매칭 참가자'를 일급 컬렉션으로 관리했고 이를 계속 매개변수를 통해 전달하도록 구현했는데,
매칭 비즈니스를 생명 주기 관점으로 보면 거의 매 메서드 마다 '매칭 참가자'가 필요합니다.
'매칭 참가자'를 매개변수로 관리하게 될 경우, 메서드 마다 '매칭 참가자'가 매개변수로 들어가게 되고
변경 관리 및 추적이 힘들어 오히려 가독성이 저하되는 문제점을 발견하였습니다.
저희가 리포지토리를 매 메서드 마다 매개변수로 넣지 않듯,
'매칭 참가자'라는 일급 컬렉션 데이터를 전역변수로 관리하여 그 상태가 계속 유지되도록 하며
나아가 코드가 더욱 간결해지는데에서 얻는 이점이 더 크다고 판단했습니다.
위 사유로, 일급 컬렉션 전역변수로 관리하도록 구현했습니다.