Ping 통신을 위한 데이터 타입 변경 #774
Open
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
JJ503
requested changes
Apr 16, 2024
Comment on lines
-25
to
26
| public Set<WebSocketSession> getSessionsByChatRoomId(final Long chatRoomId) { | ||
| final WebSocketSessions webSocketSessions = chatRoomSessions.get(chatRoomId); | ||
|
|
||
| return webSocketSessions.getSessions(); | ||
| public WebSocketSessions findSessionsByChatRoomId(final Long chatRoomId) { | ||
| return chatRoomSessions.get(chatRoomId); | ||
| } |
There was a problem hiding this comment.
질문
인메모리기에 db와 동일하게 get대신 find로 변경해 주신 게 맞을까요??
Comment on lines
+5
to
+8
| public record PingDataDto(long chatRoomId, long lastMessageId) { | ||
|
|
||
| private static final String CHAT_ROOM_ID_KEY = "chatRoomId"; | ||
| private static final String LAST_MESSAGE_ID = "lastMessageId"; |
There was a problem hiding this comment.
질문
변수명이 json의 키 명과 동일하다면 Object.convertValue()를 통해 자동으로 역직렬화가 수행되지 않나요?
해당 방식을 사용하게 된 이유가 궁금합니다!
| return sendMessageDtos; | ||
| } | ||
|
|
||
| private void updateReadMessageLog( |
There was a problem hiding this comment.
필수
해당 메서드가 createTextMessage 아래로 내려가야겠네요!
| return objectMapper.convertValue(attributes, SessionAttributeDto.class); | ||
| } | ||
|
|
||
| private CreateMessageDto createMessageDto(final ChatMessageDataDto messageData, final Long userId) { |
Comment on lines
+75
to
+78
| return readMessageDtos.stream() | ||
| .map(readMessageDto -> MessageDto.of(readMessageDto, isMyMessage(sessionAttributeDto, | ||
| readMessageDto.writerId() | ||
| ))).toList(); |
There was a problem hiding this comment.
선택
아래처럼 수정하면 가독성이 더 좋아질 것 같습니다!
Suggested change
| return readMessageDtos.stream() | |
| .map(readMessageDto -> MessageDto.of(readMessageDto, isMyMessage(sessionAttributeDto, | |
| readMessageDto.writerId() | |
| ))).toList(); | |
| return readMessageDtos.stream() | |
| .map(readMessageDto -> MessageDto.of( | |
| readMessageDto, | |
| isMyMessage(sessionAttributeDto, readMessageDto.writerId()) | |
| )).toList(); |
|
지난번에 말씀드린 대로 type의 대소문자를 무관하게 하도록 하는 로직 추가해 두었습니다. |
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.
📄 작업 내용 요약
Ping 통신을 위한 데이터 타입을 변경했습니다.
data에 해당하는 값에 ping, message를 구분하는 type을 추가했습니다.
브랜치를 변경해 새로운 PR을 생성했습니다.
이번 리팩터링은 변경 사항이 조금 있는 편이었습니다.
먼저 ping과 message를 핸들링하는 클래스를 분리했습니다.
리뷰 주셨던 것처럼
WebSocketHandler에서 너무 많은 역할을 하고 있다고 생각해서 각각을 핸들링하는 클래스를 분리하고 Composite 클래스에서 타입에 맞춰 provider를 반환하도록 구현했습니다.MessageTypeHandler와PingTypeHandler에서 사용하는 Map타입의 data를 dto로 변환해 사용하도록 변경했습니다.Message와 Ping 타입에 따라 data 구조가 달라서 타입에 따라 request에 어떤 값이 넘어오는지 확인해야 하는 불편함이 있었습니다.
따라서 DTO로 변환해 사용하도록 변경했습니다.
논의하고 싶은 내용
현재 웹소켓 관련해 전반적인 클래스와 dto명을 이해하기 쉽게 변경하는 작업을 하면 좋을 것 같습니다.
작업하던 중 클래스명만으로 어떤 데이터가 넘어오는지 추측하기가 어렵더라구요ㅠㅠ
혹시 관련해 제이미도 불편함이 있었는지 궁금합니다.
이건 회의에서 이야기해보면 좋을 것 같아요!
지난 리뷰 코멘트
지난 리뷰에 남긴 코멘트도 한번 확인해주세요!
🙋🏻 리뷰 시 주의 깊게 확인해야 하는 코드
📎 Issue 번호