-
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
[채팅] 채팅 방 목록 조회, 생성 등 #86
Conversation
User partner = chatRoom.getSentUser().equals(user) ? chatRoom.getReceivedUser() | ||
: chatRoom.getSentUser(); |
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.
if
문이 아닌 삼항연산자를 사용하신 이유가 있을까요?
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.
if 문을 사용하면 어떤 장점이 있나요?
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.
개인적으로 삼항연산자보단 if
문을 사용하는 것이 조건에 따른 로직 흐름을 더 명확히 표현할 수 있다고 생각합니다. 그런 사유로 저는 단일 조건이더라도 삼항 연산자를 사용하지 않았습니다. 위 코드 자체가 크게 가독성이 떨어진다고 생각되지 않고 어디까지나 의견이니 한 번 고려해보고 최종 반영해주시면 될 것 같습니다.
ChatMessage lastMessage = chatMessageQuery.findLastMessageByChatRoomId(chatRoom.getId()); | ||
|
||
return new ChatRoomResponse(chatRoom.getId(), partner.getNickname(), | ||
partner.getProfilePhotos().get(0).getPhotoUrl(), |
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.
User
엔티티의 getRepresentativeProfilePhoto()
를 이용하지 않은 이유가 있을까요?
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.
있는지 몰랐습니다. 수정할게요
ChatRoom chatRoom = chatRoomQuery.findBy(sender, receiver); | ||
chatRoom.activeRoom(); | ||
|
||
String activeRoom = "채팅방이 활상화되었습니다."; |
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 activeRoom = "채팅방이 활상화되었습니다."; | |
String activeRoom = "채팅방이 활성화되었습니다."; |
오타 수정 부탁드립니다!
NotificationSendEvent notificationSendEvent = new NotificationSendEvent( | ||
NotificationType.CHATTING_REQUESTED, sender, receiver, | ||
LocalDateTime.now()); | ||
|
||
eventPublisher.publishEvent(notificationSendEvent); | ||
|
||
// 비활성화된 채팅방 생성 | ||
chatRoomCommand.createChatRoom(new ChatRoom(sender, receiver)); | ||
} |
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.
개인적으로 생각하였을 때 알림은 알림과 관련된 도메인 작업이 이상 없이 수행된 이후에 사용자에게 전달되어야 한다고 생각되는데, 채팅방 생성 이전에 알림 이벤트를 발행한 이유가 있을까요?
NotificationSendEvent notificationSendEvent = new NotificationSendEvent( | ||
NotificationType.CHATTING_REQUEST_ACCEPTED, receiver, sender, | ||
LocalDateTime.now()); | ||
|
||
eventPublisher.publishEvent(notificationSendEvent); | ||
|
||
// 채팅방 활성화 | ||
ChatRoom chatRoom = chatRoomQuery.findBy(sender, receiver); | ||
chatRoom.activeRoom(); |
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.
앞선 requestChat()
에 남긴 리뷰와 같은 이유로 채팅방 활성화 이전에 알림 이벤트를 먼저 발행한 이유가 있을까요?
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.
고생하셨습니다!
작업 대상
📄 작업 내용
🙋🏻 주의 사항
📎 관련 이슈
레퍼런스