-
Notifications
You must be signed in to change notification settings - Fork 5
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
[refactor] 채팅 중복 체크 로직 추가 및 채팅방 멤버 리스트 관리 로직 추가 #287
Conversation
인원 제한 로직을 깜박해서 추가하고 다시 push 하겠습니다.. |
인원 제한 로직 추가 했습니다. 리뷰 해주십셔 |
프론트 로직이 완성되면 맞춰보고 merge 해야할 것 같습니다. |
@RequiredArgsConstructor | ||
public class ChatRoomMemberManager { | ||
private final MemberRepository memberRepository; | ||
private final ConcurrentHashMap<String, List<ChatRoomMember>> chatRoomMap = new ConcurrentHashMap<>(); |
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 보다 Collections.synchronizedList 를 고려해봐도 좋을 것 같습니다.
물론 지금은 2명 입장이라 괜찮을 것 같지만 그룹으로 확대된다고 했을 때를 생각해보면 한번 고민해봐도 좋을 것 같네요.
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.
멤버 목록은 ConcurrentHashMap 안에 있기 때문에 List여도 상관없다고 생각했는데, 다른 분들의 생각은 어떠신가요?
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 대신 CopyOnWriteArraySet를 사용했습니다.
ChatRoomMember member = chatRoomMemberList.stream() | ||
.filter(chatRoomMember -> chatRoomMember.memberId().equals(memberId)) | ||
.findFirst() | ||
.get(); |
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.
예기치 못하게 멤버목록이 리스트에 들어가지도 못했는데 삭제하기 위해 찾을 경우
NoSuchElementException 에러가 떨어질 수도 있지 않을까요? 예외처리 로직은 안전하게 있어야하지 않을까 싶습니다.
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.
member null 체크를 해야할 듯 합니당
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.
해당 코드가 삭제되었습니다.
String destination = accessor.getDestination(); | ||
String[] destinationSplit = destination.split("/"); | ||
|
||
return destinationSplit[destinationSplit.length - 1]; |
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.
코드 가독성을 위해
return destinationSplit[destinationSplit.length - 1];
return lastDestination;
같이 적어주는 게 좋지 않을까 합니다.
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.
null 체크도 추가하여 반영했습니다.
|
||
String sender, | ||
|
||
String senderImageUrl, | ||
|
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.
쓱삭 쓱삭
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.
리뷰 확인 부탁드려요~
추가적으로 stomp 소켓 테스트 삭제하겠습니다. |
잘 가요,, 이젠 보내줍시다... |
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.
__아직은 안돼요
이 PR은 왜 계속 남아있나요? |
- 동시성을 고려하여, ArrayList 대신CopyOnWriteArraySet을 사용
- subscribe와 disconnect할 때의 memberList를 직접 넣어서 sub에게 메시지 전달하는 로직 추가 - chatRoomMember의 memberId를 이용하여 객체 비교하도록 수정
이거 무슨 프론트랑 안 맞춰보면 머지 못하는 pr이었던것 같아요.. |
이 친구.. 언제쯤 머지되나요..? @mooncw |
로직 변경이 필요해져서 close하고 변경이 되면 다시 reopen 하겠습니다. |
PR을 보내기 전에 확인해주세요!!
관련 이슈
close: #272
개요
상세 내용