refactor : 채팅 메시지 전송 리팩토링#153
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces chat notifications by integrating NotificationService into ChatStompController and updating NotificationDto to include a message field. NotificationServiceImpl now supports batch fetching sender nicknames and generating messages for notification lists. A review comment suggests simplifying the sender nickname resolution logic in the service implementation.
| Set<Long> senderIds = combinedNotifications.stream() | ||
| .map(Notification::getSenderId) | ||
| .filter(Objects::nonNull) |
There was a problem hiding this comment.
senderNickname을 결정하는 로직을 더 간결하게 개선할 수 있습니다. nicknameMap.getOrDefault는 키가 null인 경우에도 안전하게 동작하므로 명시적인 null 체크를 생략할 수 있습니다.
String senderNickname = nicknameMap.getOrDefault(n.getSenderId(), "시스템");
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request integrates chat notifications into the messaging flow and enhances the notification DTO to include descriptive messages. Key changes include the addition of notification triggers in the chat controller and batch processing of sender nicknames in the notification service. Feedback highlights a redundant notification call in the controller that could lead to duplicate alerts and suggests optimizations for database queries and null safety when mapping user nicknames.
| // 상대방에게 채팅 알림 발송 | ||
| ChatRoom room = saved.getChatRoom(); | ||
| Long receiverId = room.getMentorId().equals(senderId) ? room.getMenteeId() : room.getMentorId(); | ||
| notificationService.createNotification(receiverId, senderId, NotificationType.CHAT_MESSAGE, roomId); |
There was a problem hiding this comment.
chatService.saveMessage() (47번 라인) 내부에서 이미 notificationService.createNotification()을 호출하여 알림 생성 및 발송 로직을 수행하고 있습니다. 현재 코드대로라면 채팅 메시지 전송 시 알림이 중복으로 생성되어 발송됩니다. 30번 라인의 notificationService 필드 주입과 본 메서드의 알림 생성 로직을 제거하는 것을 권장합니다.
| Map<Long, String> nicknameMap = userRepository.findAllById(senderIds).stream() | ||
| .collect(Collectors.toMap(User::getId, User::getNickname)); |
There was a problem hiding this comment.
senderIds가 비어있는 경우 불필요한 DB 조회를 피하기 위해 조건문을 추가하는 것이 효율적입니다. 또한, Collectors.toMap은 value가 null인 경우 NullPointerException을 발생시키므로, User::getNickname이 null을 반환할 가능성이 있다면 이에 대한 방어 코드를 추가하는 것이 안전합니다.
| Map<Long, String> nicknameMap = userRepository.findAllById(senderIds).stream() | |
| .collect(Collectors.toMap(User::getId, User::getNickname)); | |
| Map<Long, String> nicknameMap = senderIds.isEmpty() ? Map.of() : userRepository.findAllById(senderIds).stream() | |
| .collect(Collectors.toMap(User::getId, u -> u.getNickname() != null ? u.getNickname() : "알 수 없음")); |
…P-Link-it/backend into feature/notification/message
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a message field to NotificationDto and implements batch fetching of sender nicknames in NotificationServiceImpl to optimize the notification retrieval process. A review comment suggests refining the logic to distinguish between system-generated notifications and users with missing nicknames, while also recommending the use of projections for more efficient database queries.
| Map<Long, String> nicknameMap = senderIds.isEmpty() | ||
| ? Collections.emptyMap() | ||
| : userRepository.findAllById(senderIds).stream() | ||
| .collect(Collectors.toMap(User::getId, | ||
| u -> u.getNickname() != null ? u.getNickname() : "시스템")); | ||
|
|
||
| List<NotificationDto> notificationDtos = combinedNotifications.stream() | ||
| .map(NotificationDto::from) | ||
| .map(n -> { | ||
| String senderNickname = nicknameMap.getOrDefault(n.getSenderId(), "시스템"); | ||
| String message = generateNotificationMessage(n.getNotificationType(), senderNickname); | ||
| return NotificationDto.from(n, message); | ||
| }) |
There was a problem hiding this comment.
현재 로직은 시스템 알림(senderId가 null인 경우)과 닉네임이 없는 사용자를 모두 "시스템"이라는 닉네임으로 처리하고 있습니다. 시스템 알림과 일반 사용자를 구분하여 처리하는 것이 좋습니다. 다만, 탈퇴한 사용자와 같은 예외 케이스 처리는 현재 단계에서 나중에 진행해도 무방합니다.
또한, userRepository.findAllById(senderIds)는 User 엔티티 전체를 조회하므로, 알림 목록이 길어질 경우 성능 최적화를 위해 닉네임만 조회하는 프로젝션 사용을 고려해볼 수 있습니다.
| Map<Long, String> nicknameMap = senderIds.isEmpty() | |
| ? Collections.emptyMap() | |
| : userRepository.findAllById(senderIds).stream() | |
| .collect(Collectors.toMap(User::getId, | |
| u -> u.getNickname() != null ? u.getNickname() : "시스템")); | |
| List<NotificationDto> notificationDtos = combinedNotifications.stream() | |
| .map(NotificationDto::from) | |
| .map(n -> { | |
| String senderNickname = nicknameMap.getOrDefault(n.getSenderId(), "시스템"); | |
| String message = generateNotificationMessage(n.getNotificationType(), senderNickname); | |
| return NotificationDto.from(n, message); | |
| }) | |
| Map<Long, String> nicknameMap = senderIds.isEmpty() | |
| ? Collections.emptyMap() | |
| : userRepository.findAllById(senderIds).stream() | |
| .collect(Collectors.toMap(User::getId, | |
| u -> (u.getNickname() != null && !u.getNickname().isBlank()) ? u.getNickname() : "이름 없음")); | |
| List<NotificationDto> notificationDtos = combinedNotifications.stream() | |
| .map(n -> { | |
| String senderNickname = n.getSenderId() == null ? "시스템" : nicknameMap.getOrDefault(n.getSenderId(), "알 수 없는 사용자"); | |
| String message = generateNotificationMessage(n.getNotificationType(), senderNickname); | |
| return NotificationDto.from(n, message); | |
| }) |
References
- Implementation of edge cases, such as handling withdrawn users in queries, can be postponed.
|
고생하셨습니다! |
3 participants
🔥ISSUE
채팅 메시지 전송 시 실시간 알림 기능 개선 및 알림 메시지(message) 필드 추가
📒TODO