✨ [Code Review] 산호 code review

[seoshinehyo]

week9 미션 코드입니다.

[junseokkim]

현재 Member 엔티티에서 양방향 매핑이 많아 보입니다. 양방향 매핑은 필요한 경우에만 신중히 사용하는 것이 좋습니다. 모든 연관 관계를 양방향으로 설정하면 순환 참조, N+1 문제, 불필요한 데이터 로드 등 성능 저하를 초래할 가능성이 있습니다.

각 관계마다 양방향 매핑이 꼭 필요한지 고민하고, 상황에 따라 단방향 매핑으로 전환하거나, 필요한 곳에서만 양방향 관계를 유지하면 코드의 복잡성과 성능 문제를 줄일 수 있습니다.

[junseokkim]

가게 별점이나 리뷰 수의 경우, 계산이 필요한 필드라고 생각됩니다.
현재처럼 해당 값을 엔티티에 필드로 저장하면 다음과 같은 장점과 주의사항이 있습니다.

장점

1. 빠른 조회 성능:

   • 미리 계산된 값을 저장하기 때문에, 가게 조회 시 별점이나 리뷰 수를 바로 가져올 수 있어 성능이 향상됩니다.
   • 별점이나 리뷰 수를 매번 계산하는 쿼리를 실행하지 않아도 되므로, 특히 대량 데이터를 처리할 때 유리합니다.

2. 간단한 로직:

   • 서비스 로직에서 별도의 계산 로직 없이 저장된 값을 활용하면 구현이 간단해지고, 코드 복잡성을 줄일 수 있습니다.

주의사항

1. 데이터 불일치 문제:

   • 리뷰가 추가되거나 삭제될 때, rating과 reviewCount를 함께 업데이트하지 않으면 데이터 불일치가 발생할 가능성이 있습니다.
   • 이를 방지하려면 리뷰 추가/삭제 시 트랜잭션 내부에서 필드를 동기화하는 로직이 필요합니다.

2. 실시간 데이터의 정확성:

   • 미리 저장된 값은 최신 데이터와 일치하지 않을 수 있습니다. 가게 조회 시 실시간 데이터가 필요한 경우, 별점이나 리뷰 수를 동적으로 계산하는 것이 더 적합할 수 있습니다.

3. 데이터 관리의 복잡성 증가:

   • 리뷰와 관련된 모든 동작(추가, 수정, 삭제)에서 rating과 reviewCount를 동기화해야 하므로, 로직이 복잡해질 수 있습니다.

상황에 따라 현재 방식처럼 필드로 유지하는 것이 적합할 수도 있고, 실시간 계산 방식으로 전환하는 것이 더 나을 수도 있습니다.
어떤 방식을 선택하든 주의사항을 충분히 고려하여 로직을 작성하면 데이터의 신뢰성과 코드의 유지보수성을 높일 수 있을 것입니다.

[junseokkim]

해당 설정의 경우 MySQL 등 특정 DB에서만 가능한 방식이므로, 나중에 다른 DB 사용 시 주의해야할 수도 있습니다.

[junseokkim]

해당 application.yml 파일이 깃허브에 올라와 있는데, 해당 설정 파일의 경우, DB 접속 정보나 추가 민감한 개인 설정이 포함되어 있을 경우가 높습니다. 현재도, 데이터베이스명, 비밀번호가 포함되어 있습니다.

이러한 정보가 깃허브에 올라갈 경우, 외부에 노출될 수 있으니 .gitignore설정을 추가해주세요!!!

.gitnore 파일에

application.yml

을 추가해주시면 됩니다!!

[junseokkim]

해당 application.yml 파일이 깃허브에 올라와 있는데, 해당 설정 파일의 경우, DB 접속 정보나 추가 민감한 개인 설정이 포함되어 있을 경우가 높습니다. 현재도, 데이터베이스명, 비밀번호가 포함되어 있습니다.

이러한 정보가 깃허브에 올라갈 경우, 외부에 노출될 수 있으니 .gitignore설정을 추가해주세요!!!

.gitnore 파일에

application.yml

을 추가해주시면 됩니다!!

[junseokkim]

DTO의 멤버 변수에 private 과 final을 붙이는 것을 권장합니다!! 이유는 다음과 같습니다.

1. private 키워드가 없으면 외부 클래스에서 변수에 직접 접근하거나 변경할 수 있습니다.
2. DTO는 값을 전달하는 역할만 수행하므로 생성 이후 값이 변경되지 않아야 합니다. 이를 보장하기 위해서 final을 붙이는 것이 좋습니다.

주로, private은 대부분 필수로 붙이나, final은 권장 느낌이라 꼭 둘다 사용할 필요는 없습니다!!! 다만 요러한 장점이 있다만 인지하고 계시면 좋을 거 같네요

[junseokkim]

현재 코드는 Optional의 .get() 메서드를 사용하여 storeId로 가게를 조회하고 있습니다:
이 방식은 가게가 존재하지 않을 경우 NoSuchElementException을 발생시킬 수 있으므로 적절한 예외 처리가 필요합니다.
orElseThrow() 메서드를 사용해 명시적인 예외를 던지거나, Optional이 비어 있는 경우에 대한 처리를 추가하면 안전성을 높일 수 있습니다:

Suggested change

	Store store = storeRepository.findById(storeId).get();
	Store store = storeRepository.findById(storeId)
	.orElseThrow(() -> new StoreHandler(ErrorStatus.STORE_NOT_FOUND));

[junseokkim]

예외처리를 잘 해주신거 같은데 이 부분을 주석 처리하고, 예외처리를 뺀 이유가 있을까요?

[seoshinehyo]

스웨거에서 예외 처리 테스트를 하기 위해 잠시 주석 처리를 해놓는다는게 주석을 해제 안 하고 push 한 것 같습니다!

[junseokkim]

이 부분은 디버깅용을 작성하신 걸까요?

[seoshinehyo]

넵 로그를 사용하지 않고 간단하게 콘솔창에 결과 확인용으로 작성했습니다.

[junseokkim]

특정 엔티티를 id로 조회하는 로직을 함수화한 것은 너무 좋은 것 같습니다.

다만, Optional로 묶지 않고, null인 경웨 대해서 예외처리까지 함수화하는 게 더 좋을 것같습니다

또한, 아래에 여러 부분에 똑같이 id로 엔티티를 조회하는 부분이 있는데 코드가 중복되어 가독성이 떨어질 수 있으니 이 함수를 호출하는 형태로 하는 것이 더 좋아보이네요!!!

[junseokkim]

현재 MemberQueryServiceImpl에서 Review, Mission, MemberMission과 관련된 레포지토리를 직접 접근하여 DB를 조회하고 있습니다.
이 방식은 에러가 발생하거나 즉각적인 문제는 없지만, 서비스 간 책임 분리 원칙을 고려할 때 개선이 필요합니다.

문제점

1. 결합도 증가

   • MemberQueryServiceImpl이 여러 레포지토리에 직접 접근하면서 서비스 간 결합도가 높아집니다.
   • 이는 유지보수성과 테스트 용이성을 저하시킬 수 있습니다.

2. 책임 분리 부족

   • 현재 구조에서는 각 레포지토리에 대한 로직이 여러 서비스에 분산되어 있어, 특정 도메인의 책임이 명확하지 않습니다.
   • 예를 들어, Review와 관련된 로직은 ReviewQueryService에서 관리되어야 하지만, 현재는 MemberQueryServiceImpl에서도 직접 관리하고 있습니다.

따라서 각 레포지토리의 접근은 관련된 도메인 서비스에서만 이루어지도록 설계하는 것이 좋습니다.

예를 들어,
ReviewQueryService에 다음과 같이

public Page<Review> getReviewsByMember(Member member, Integer page) {
        return reviewRepository.findAllByMember(member, PageRequest.of(page, 10));
}

이렇게 함수를 만들어주고 해당 서비스에서는 이 review 서비스 함수를 호출하는 방식으로 하는 것이 좋습니다.

        Page<Review> reviewPage = reviewQueryService.getReviewsByMember(member, page);

[junseokkim]

지금과 같이 memberMissionList를 전역 변수로 사용할 경우 다음과 같은 문제가 발생할 수 있습니다.

1. Thread-Safety 문제:
   • Spring의 서비스 클래스는 기본적으로 Singleton으로 동작합니다. 따라서 여러 요청이 동시에 들어올 경우, memberMissionList가 공유되어 데이터 오염 문제가 발생할 수 있습니다.
     -> 즉, 다른 유저끼리 공유될 수도 있습니다.
2. 의도치 않은 데이터 누적:
   • memberMissionList가 요청 간에 데이터를 유지하므로, 이전 요청의 데이터가 다음 요청에 영향을 미칠 수 있습니다.
     -> 서로 공유가 되므로, 미션 리스트가 섞일 가능성이 있죠.
3. 비효율적인 설계:
   • 전역 변수로 상태를 유지하기보다는, 필요한 경우 메서드 내부에서 List를 생성하여 처리하는 것이 더 적합합니다.
     따라서 아래 방식처럼 메서드 내부에서만 가능한 로컬 변수로 사용하는 것을 권장드립니다.

[junseokkim]

현재 MissionConverter.toMission 메서드를 통해 Mission 엔티티를 생성하고 있으며, 그 과정에서 이미 store와 memberMissionList를 설정하고 있습니다.

하지만 setStore와 setMemberMissionList 메서드에서도 동일하게 store와 memberMissionList 값을 수정하고, 양방향 매핑을 위해 연관관계를 설정하고 있습니다.

문제점

1. 중복 코드

   • MissionConverter.toMission에서 이미 store와 memberMissionList가 설정되었음에도, 이후 setStore와 setMemberMissionList를 통해 다시 설정하고 있습니다.
   • 이는 불필요한 중복을 초래하며, 코드의 유지보수성을 저하시킬 수 있습니다.

2. 책임 분리 부족

   • 연관관계 설정의 책임이 toMission과 setStore, setMemberMissionList 메서드 간에 분산되어 있어, 로직이 복잡해지고 실수로 인해 데이터 불일치가 발생할 가능성이 있습니다.

따라서

1. 중복 제거

   • toMission에서 이미 store와 memberMissionList를 설정하고 있으므로, 이후 setStore와 setMemberMissionList에서 이를 다시 설정하지 않도록 수정해야 합니다.
   • 연관관계 설정 역시 toMission에서 처리하도록 일원화하여, 코드의 중복을 제거하고 책임을 명확히 분리합니다.

2. toMission 내부에서 양방향 연관관계 설정

   • toMission 메서드에서 Mission 생성 시 양방향 매핑을 설정하도록 수정합니다. 예를 들어:

     public static Mission toMission(AddMissionDTO request, Store store, List<MemberMission> memberMissionList) {
         Mission mission = Mission.builder()
             .reward(request.getReward())
             .deadline(request.getDeadline())
             .missionSpec(request.getMissionSpec())
             .store(store)
             .memberMissionList(memberMissionList)
             .build();
     
         // 양방향 연관관계 설정
         store.getMissionList().add(mission);
         memberMissionList.forEach(memberMission -> memberMission.setMission(mission));
     
         return mission;
     }

그리고

Suggested change

	Mission newMission = MissionConverter.toMission(request, store, memberMissionList);
	newMission.setStore(store);
	newMission.setMemberMissionList(memberMission);
	Mission newMission = MissionConverter.toMission(request, store, memberMissionList);

이렇게 set 메서드를 제거해줄 수 있습니다.

다른 방법으로는,
set 함수를 유지하고 toMission을 다음과 같이 수정하여 중복 코드를 제거할 수 있습니다.

    public static Mission toMission(MissionRequestDTO.AddMissionDTO request) {

        return Mission.builder()
                .reward(request.getReward())
                .deadline(request.getDeadline())
                .missionSpec(request.getMissionSpec())
                .build();
    }

[junseokkim]

해당 기능의 경우 memberMission의 상태를 COMPLETE로 변경해주는 기능이기 때문에, MissionQueryServiceImpl에 정의하기보다 MissionCommandServiceImpl에 정의하는게 더 적합해보입니다!!!

[junseokkim]

이부분도 mission과 마찬가지로 코드 중복이 존재하는 것 같습니다!!!

양방향 매핑을 수행하면서 중복으로 코드가 생기는 부분이 많아보이네요!! 양방향 매핑이 편리하긴 하지만, 설정 부분에서 신경써야 되는 부분이 많기 때문에 사용할 때는 주의해야 합니다!!

[junseokkim]

오타인 거 같긴한데, 이 에러 부분만 소문자가 섞여 있네요!!!

[junseokkim]

우선, 미션 수행하시느라 정말 고생 많으셨습니다! 😊

대부분은 공통적이고 대중적인 관점에서 작성하려고 했지만, 일부 제 개인적인 의견이 포함되어 있을 수 있습니다. 따라서 코드 리뷰를 참고하시되, 무조건적으로 수용하기보다는 “왜 이렇게 해야 할까?”를 고민하고 찾아본 후 본인의 코드 스타일에 맞게 적용하는 것을 추천드립니다!

아래는 코멘트를 달앗지만 공통적으로 수정이 필요한 부분을 작성했습니다!!!

1. 코드 스타일

   • 코드 한 줄의 길이가 너무 긴 경우가 있습니다. 줄바꿈을 통해 가독성을 개선하고, 메서드 선언부나 Builder 패턴 등에서 구조를 명확히 표현하면 더욱 좋습니다.

2. DTO 설계

   • DTO는 불변 객체로 설계하는 것해야 합니다. 따라서,
     @AllArgsConstructor와 final을 사용하면 객체의 안정성과 가독성을 높일 수 있습니다.
   • 또한, DTO 멤버 변수에는 private 접근 제한자를 사용하여 캡슐화를 보장하는 것이 좋습니다.

3. createdAt 필드 처리

   • 엔티티 생성 시 createdAt 필드를 LocalDateTime.now()로 설정하고 있는데, 이는 DB에 저장되면서 자동으로 설정된 값과 다를 수 있습니다.
     DB에 저장된 실제 createdAt 값을 그대로 사용하도록 수정하는 것을 권장드립니다.

4. 서비스와 레포지토리 간의 책임 분리

• 현재 Mission 서비스에서 여러 레포지토리(Store, User, UserMission)에 직접 접근하고 있어 결합도가 높아지고 중복 코드가 발생하고 있습니다.
• 한 서비스에서 여러 레포지토리에 접근하기보다, 각 도메인의 로직은 해당 도메인 서비스(StoreService, UserService 등)에서 처리하도록 분리해주세요.
  이를 통해 코드 중복을 줄이고, **책임 분리 원칙(SRP)**을 준수할 수 있습니다.
  다만, 서비스 간 호출 시 순환 참조가 발생하지 않도록 의존성 방향을 잘 설계해야 합니다.

5. 양방향 매핑 주의
   • 양방향 매핑은 양쪽에서 데이터를 참조할 수 있어 편리하지만, 순환 참조 문제와 N+1 문제, 불필요한 복잡성을 초래할 수 있습니다.
   • 꼭 필요한 경우에만 사용하고, 가능하다면 단방향 매핑으로 설계를 단순화하는 것도 좋은 선택입니다.
   • 특히, MissionConverter.toMission에서 이미 store와 memberMissionList를 설정하고 있음에도, 이후 setStore와 setMemberMissionList 메서드에서 이를 다시 설정하는 문제는 중복 코드와 책임 분리를 저해합니다.
     → 연관관계 설정의 책임을 toMission에 일원화하거나, 필요 없는 중복 설정을 제거하세요.

혹시나 코드리뷰에 대해서 궁금한 점이 있다면 답변 달아주시면 확인해볼게요!! 미션 수행하시느라 정말 고생 많으셨습니다! 🎉

[seoshinehyo]

제가 오늘 종강을 해서 달아주신 리뷰를 이제야 확인했습니다. 코드 읽고 하나하나 정성스럽게 리뷰 달아주셔서 정말 감사합니다. 리뷰에서 말씀해주신 수정이 필요한 부분에 대해 더 공부하고 단일 책임과 유지보수 관점에서 코드를 작성할 수 있도록 노력하겠습니다. 다시 한 번 감사합니다!!