-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat #38 매칭 필터링 및 알고리즘 구현 #38
The head ref may contain hidden characters: "feat/#30/\uB9E4\uCE6D-\uD544\uD130\uB9C1-\uBC0F-\uC54C\uACE0\uB9AC\uC998-\uAD6C\uD604"
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨어요!!
Members user = memberRepository.findById(userId) | ||
.orElseThrow(MemberNotFoundException::new); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MemberService에 findById 메서드가 있습니다!!
해당 메서드를 사용하는게 좋아보여요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MemberService에 예외처리까지 되어있는 메서드를 사용하면,
불필요한 메서드 추가랑 예외처리를 안해줘도 될거같네요. 수정 완료했습니다 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다!
mysql에서 거리로 검색하는 기능이 있다는 것은 처음 알았네요!
현재 방식이라면 차후 사용자의 현재 위치를 입력받을 때도 확장성 있게 사용이 가능할 것 같습니다!
*/ | ||
Members user = memberService.findById(userId); | ||
|
||
if (matchingRoomRepository.existsByMemberInMatchingRoom(user)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예외를 던져주게 되면 매칭 이벤트가 종료 되나요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
매칭 이벤트가 종료되거나 기존 매칭방이 삭제되는 것은 아니라고 생각해주시면 됩니다. 기존의 매칭방은 정상적으로 유지되고,
만약에 사용자가 이미 생성된 매칭방에 대해 매칭 종료를 요청하거나 취소를 원할 경우는 매칭 취소와 관련된, 별도의 권한이라 매칭 취소의 프로세스에 맞다고 생각했습니다
해당 부분에 대해서는 고민을 많이 했던 부분인데 400을 던져주는 방식으로 설계한 이유는,
사용자가 이전에 설정한 조건과 동일한 조건으로 매칭방을 생성하려고 시도하는 상황에서는 이미 해당 조건에 맞는 매칭방이 존재하는 전제가 있는거니까 정상적인 흐름이 아니라고 간주하고, 이를 막아주는 것이 올바른 흐름이라고 판단하였습니다
그래서 동일한 조건으로 생성된 방이 이미 존재하는 상황에서는 중복된 방 생성을 방지하기 위해
서버단에서 예외를 던져주도록 설계하였습니다. 원래는 클라이언트 측에서
이미 참여 중인 매칭방 화면으로 유저를 리다이렉트하는 것이 더 적절하다고 생각했지만,
안정성을 고려했을 때 서버단에서도 중복 여부를 검증하는 것이 필요하다고 생각했습니다
.roomId(matchingRoom.getId()) | ||
.maxCapacity(matchingRoom.getCapacity()) | ||
.build()); | ||
FindRoomResult.builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
room을 전달해줘서 객체 내부에서 생성하도록 캡슐화를 시키면 통일성 측면에서 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 빌더 부분도 객체 내부에서 생성하도록 해서 반영하여 수정하였습니당
/* | ||
ACTIVE 상태인 방만 필터링 | ||
*/ | ||
matchingRooms = matchingRooms.stream() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DB에서 가져올 때 Active인 방들만 가져오는 것은 어떨까요??
Jpa 메서드로도 작성이 가능하니 조회할 때 필터링을 거쳐주는 것도 성능에 좋아보여요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 원래는 Active 상태의 방과 태그 조건까지 모두 쿼리로 조회해 오도록 설계했었는데, 주영님의 의견을 반영하여 수정했습니다
해당 조건들을 모두 쿼리에서 처리하려면 쿼리가 너무 복잡하고 길어져서 서버단 성능에 부담이 갈 수도 있고, 가독성이 떨어질 수도 있다는 피드백이 있었어서, 해당 부분 고려한 뒤에 Active 상태와 태그 필터링은 서비스 단에서 처리하는 방식으로 이전하였습니다...!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아항 알겠습니당
List<MatchingRoom> matchingRoomList = this.matchingRoomRepository.findAll(); | ||
if (!matchingRoomList.isEmpty()) { | ||
MatchingRoom matchingRoom = matchingRoomList.get(0); | ||
public Optional<FindRoomResult> findRoom(Long userId, double startLongitude, double startLatitude, double destinationLongitude, double destinationLatitude, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재는 서버에 저장된 Rount(가천대 - 정문) 노선만 제공을 하고 있으니, 서버에서 제공하는 정보를 토대로 클라이언트에서 위도 경도 값을 그대로 넣어줘야할 것 같아요!
그렇다면 입력받은 값에 대한 유효성 검증이 추가되면 좋을 것 같아요. 위도, 경도 값으로 Route가 존재하는지 조회를 해본다거나 하는
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
만약 클라이언트에서 보내주는 위도, 경도의 정보로 Route를 조회할 수 없다면,
잘못된 위도경도 입력 자체에 대한 내용은 컨트롤러 단에서 validation으로 1차적으로 걸러져서 필터링 됩니다
그렇다면 2차적으로 올바른 위도 경도가 입력 되었을때, 라우트가 조회되지않아서 방이 검색되지 않는 경우는 정상적인 플로우라고 생각했어서 해당 부분에 대한 유효성 검증은 필요없다는 생각했었습니당
추후 리팩토링시 만약 해당 부분에 대해서 Haversine이랑 Radian을 사용해서 위도 경도 자체를 세밀하게 계산을 해준다고 하면 필요한 예외처리를 추가하는 방식으로 해도 좋을거같습니다 !
|
||
public class PageNotFoundException extends BaseException { | ||
public PageNotFoundException() { | ||
super(HttpStatus.NOT_FOUND, ErrorMessage.NOT_FOUND_PAGE.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
정적 경로 설정이 빠진 것 같아요!
"WHERE " + | ||
"FUNCTION('ST_Distance_Sphere', FUNCTION('POINT', :startLongitude, :startLatitude), FUNCTION('POINT', r.route.startLongitude, r.route.startLatitude)) <= 300 " + | ||
"AND FUNCTION('ST_Distance_Sphere', FUNCTION('POINT', :destinationLongitude, :destinationLatitude), FUNCTION('POINT', r.route.endLongitude, r.route.endLatitude)) <= 300 ") | ||
List<MatchingRoom> findRoomsByStartAndDestination( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
거리도 파라미터로 빼주고 호출하는 쪽에서 상수로 관리해주면 거리 변경이 필요할 때 유용할 것 같아욤
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기존 쿼리의 <= 300 부분 :radius로 설정해줘서 아래 내용처럼 상수로 선언해서 사용하겠습니당
private static final double SEARCH_RADIUS = 300.0;
double endLongitude = Double.parseDouble(endCoordinates[0]); | ||
double endLatitude = Double.parseDouble(endCoordinates[1]); | ||
|
||
Route route = Route.builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 객체 생성의 책임도 객체 내부로 이전하면 통일성 있고 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 부분은 원래 구현되어 있던 내용을 그대로 사용했기 때문에 미처 확인하지 못했던 부분입니다
현재 구현한 설계들과 동일한 패턴들 처럼, Route 객체 내부로 이전하여 통일성을 유지하는 것이 저도 맞다고 생각되어서 반영하도록 하겠습니다 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨어용!!
📌 관련 이슈
관련 이슈 번호 #30
Close #
🚀 작업 내용
*거리 조건은 300m 이내로 설정했고, 태그가 비어있는 경우에도 처리할 수 있도록 설정하였습니다
📸 스크린샷
SSE 구독 후 포스트맨에서 테스트 진행했습니다
DB에 매칭방 생성까지 확인했습니다
추가로 동일한 유저가 똑같은 조건의 방을 생성하려고 할시, 이미 존재하는 매칭방이라는 예외를 던져주도록 설계했습니다
📢 리뷰 요구사항
주영님과 충돌을 방지하고자 매칭시 필터링 로직이랑 전체 매칭방 리스트 조회 먼저 해당 PR에서 작업해서 머지하도록 하고,
블랙리스트와 친구추가는 별도의 이슈를 생성해서 추후에 작업하도록 협의했습니다
현재 저희가 출발지(가천대학교 정문, 1번출구) 와 도착지(가천대학교 기숙사, AI 공학관)로 정해져있는 설계였기에
Haversine이랑 Radian을 통한 위도 경도 계산법은 사용하지 않았습니다
하지만 추후에 앱으로 마이그레이션 후 출발지와 도착지를 지도에서 동적으로 선택할 수 있도록 확장되는 경우에는 리팩토링이 필요해보입니다