Skip to content

Comments

[박지은] sprint3#40

Open
clover6559 wants to merge 36 commits intocodeit-bootcamp-spring:박지은from
clover6559:박지은-sprint3

Hidden character warning

The head ref may contain hidden characters: "\ubc15\uc9c0\uc740-sprint3"
Open

[박지은] sprint3#40
clover6559 wants to merge 36 commits intocodeit-bootcamp-spring:박지은from
clover6559:박지은-sprint3

Conversation

@clover6559
Copy link
Collaborator

요구사항

기본

  • Spring 프로젝트 초기화
  • Bean 선언 및 테스트
  • Lombok 적용

추가 기능

  • 시간 타입 변경하기
  • 새로운 도메인 추가하기
  • 도메인 service 구현
  • 새로운 도메인 Repository 구현체 구현

심화

  • 어떤 구현체를 Bean으로 등록할지 application.yaml 설정 값을 통해 제어
  • 파일을 저장할 경로를 application.yaml 설정 값을 통해 제어

주요 변경사항

  • update 반환타입 변경
  • DateUtil 클래스 생성

멘토에게

  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@joonfluence
Copy link

커밋 메세지에 대한 규칙을 컨벤션으로 적어보시면 좋습니다!
https://www.conventionalcommits.org/en/v1.0.0/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ChannelFind는 메서드 느낌이 나서 명시적으로 뒤에 Dto로 추가해주는게 좋습니다!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기도 마찬가지

Comment on lines +7 to +13
public class DateUtil {
public static DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss");
public static String formatTime(Instant timeStamp) {
if (timeStamp == null) return "N/A";
return timeStamp.atZone(ZoneId.systemDefault()).format(formatter);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋습니다 👍

import java.util.UUID;

@Getter
public class Channel implements Serializable {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

향후 네이밍 하실 땐 ChannelEntity 처럼 명시해주는게 좋습니다! 구분해주는게 좋거든요

import java.util.Optional;
import java.util.UUID;
@Repository
@ConditionalOnProperty(name = "discodeit.repository.type",havingValue = "file")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

조건에 따라 의존성 주입되도록 잘 지정해주셨네요

Comment on lines +5 to +8
discodeit:
repository:
type: jcf
file-directory: .discodeit

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋습니다!

if (user != null && user.getPassword().equals(autowired.password())) {
return user;
}
throw new RuntimeException("아이디 또는 비밀번호가 일치하지 않습니다.");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BadRequestException

Comment on lines +83 to +88
.filter(channel -> {
if (channel.getChannelType() == Channel.ChannelType.PUBLIC)
return true;
if (channel.getChannelType() == Channel.ChannelType.PRIVATE)
return readStatusRepository.existsByChannelIdAndUserId(channel.getId(), userId);
return false;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filter 연산 안에서 DB 조회 시, 부수효과가 발생하게 됩니다. 이 부분은 개선이 필요합니다!

Comment on lines +90 to +96
.map(channel -> {
UUID currentId = channel.getId();
Instant lastMessageAt = messageRepository.findByChannelId(currentId).stream()
.map(Message::getCreatedAt)
.max(Comparator.naturalOrder())
.orElse(null);
List<UUID> memberIds = new ArrayList<>();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기도 마찬가집니다 map 안에서 조회하시는 건 실행 보장도 100% 안될 뿐더러 순수함수가 아니게 됩니다.

Comment on lines +80 to +104
@Override
public List<ChannelResponse> findAllByUserId(UUID userId) {
return channelRepository.findAll().stream()
.filter(channel -> {
if (channel.getChannelType() == Channel.ChannelType.PUBLIC)
return true;
if (channel.getChannelType() == Channel.ChannelType.PRIVATE)
return readStatusRepository.existsByChannelIdAndUserId(channel.getId(), userId);
return false;
})
.map(channel -> {
UUID currentId = channel.getId();
Instant lastMessageAt = messageRepository.findByChannelId(currentId).stream()
.map(Message::getCreatedAt)
.max(Comparator.naturalOrder())
.orElse(null);
List<UUID> memberIds = new ArrayList<>();
if (channel.getChannelType() == Channel.ChannelType.PRIVATE) {
memberIds = readStatusRepository.findByChannelId(currentId).stream()
.map(ReadStatus::getUserId).toList();
}
return new ChannelResponse(channel, lastMessageAt, memberIds);
})
.toList();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@Override
public List<ChannelResponse> findAllByUserId(UUID userId) {
return channelRepository.findAll().stream()
.filter(channel -> {
if (channel.getChannelType() == Channel.ChannelType.PUBLIC)
return true;
if (channel.getChannelType() == Channel.ChannelType.PRIVATE)
return readStatusRepository.existsByChannelIdAndUserId(channel.getId(), userId);
return false;
})
.map(channel -> {
UUID currentId = channel.getId();
Instant lastMessageAt = messageRepository.findByChannelId(currentId).stream()
.map(Message::getCreatedAt)
.max(Comparator.naturalOrder())
.orElse(null);
List<UUID> memberIds = new ArrayList<>();
if (channel.getChannelType() == Channel.ChannelType.PRIVATE) {
memberIds = readStatusRepository.findByChannelId(currentId).stream()
.map(ReadStatus::getUserId).toList();
}
return new ChannelResponse(channel, lastMessageAt, memberIds);
})
.toList();
}
@Override
public List<ChannelResponse> findAllByUserId(UUID userId) {
// 1. 필요한 데이터를 미리 한 번에 조회
List<Channel> allChannels = channelRepository.findAll();
List<ReadStatus> userReadStatuses = readStatusRepository.findByUserId(userId);
// 2. 빠른 조회를 위해 Set/Map으로 변환
Set<UUID> joinedChannelIds = userReadStatuses.stream()
.map(ReadStatus::getChannelId)
.collect(Collectors.toSet());
// 3. 메모리에서 필터링 (DB 조회 없음)
List<Channel> accessibleChannels = allChannels.stream()
.filter(channel ->
channel.getChannelType() == Channel.ChannelType.PUBLIC
|| joinedChannelIds.contains(channel.getId()))
.toList();
// 4. map에 필요한 데이터도 미리 조회
List<UUID> channelIds = accessibleChannels.stream()
.map(Channel::getId).toList();
// channelIds 기반으로 메시지, ReadStatus 일괄 조회 후 Map으로 그룹핑
...
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

필요한 데이터를 미리 한 번에 조회해서 Set이나 Map으로 만든 뒤, Stream 안에서는 메모리 연산만 수행해야 합니다. 현재 코드는 성능 문제(N+1)와 디버깅 어려움이 생깁니다.

Comment on lines +64 to +70
.map(user -> {
UserStatus status = userStatusRepository.findByUserId(user.getId())
.orElse(null);
boolean isOnline = (status != null) && status.isOnline();
return new UserFind(
isOnline, user.getId(), user.getUserName(), user.getEmail()
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map 에서 조회행위를 하면 안됩니다!

Comment on lines +64 to +70
.map(user -> {
UserStatus status = userStatusRepository.findByUserId(user.getId())
.orElse(null);
boolean isOnline = (status != null) && status.isOnline();
return new UserFind(
isOnline, user.getId(), user.getUserName(), user.getEmail()
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map 에서 조회행위를 하면 안됩니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants