refactor: letter패키지 정리하기 #513

chaewon121 · 2023-10-18T16:32:23Z

📌 관련 이슈

PR과 연관된 이슈의 번호를 작성해주세요.
closed #497

🛠️ 작업 내용

PR에서 작업한 주요 내용을 적어주세요.

LetterService, ReadLetterLogService, WriteLetterLogService 정리하기
해당 테스트 수정

🎯 리뷰 포인트

service와 controller에서 log가 따로 패키지로 분리되어있던것을 제거하고 모두 letterService와 letterController로 옮겨습니다.

이유: 괜히 클래스를 나누면서 의존성 문제만 발생시킬 뿐, 제 역량으로는 나눠야할 이유를 느끼지 못하였습니다.
-> 이부분에 대해서 아마 팀원들의 의견이 다 다를것같고 함께 고민하면 더 좋은 코드가 나올수있을것같아 많은 의견 환영입니다!

requestParam을 enum으로 받는것에대해서 컨트롤러 이후의 코드에서 처리하고싶지 않았습니다.
제가 원하는건 컨트롤러로 들어올때 리졸버에서 enum으로 매핑이 되어 넘어오는 것이었습니다.
Converter을 사용하여 구현하였습니다.
[참고 블로그]: (https://minkwon4.tistory.com/170)
-> 문제상황 발생 :
이넘으로 바인딩할때 해당 이넘값이 존재하지않으면 예외를 던지는데요. 이것이 핸들러에서 잡히지 않는것이었습니다.
이유는 리졸버에서 던지는 예외는 MethodArgumentTypeMismatchException.class
로 잡아야 하는데 저희 핸들러에서는 해당 클래스를 잡는 매서드가 구현되있지 않았습니다!
그래서 일단 매서드를 추가해놓긴했는데..(코드확인해주세용)
현재 코드로는 리졸버에서 발생하는 예외가 모두 CommonExceptionType.INVALID_REQUEST_PARAMETERS
으로 출력되는 문제점이 있습니다. 이문제에 대해서 함께 리팩터링 해나가고싶네요..!
잘부탁드립니다!

리뷰 포인트

⏳ 작업 시간

추정 시간: 3h
실제 시간: 4h
MethodArgumentTypeMismatchException 때문에 삽질 2시간 추가..

github-actions · 2023-10-18T16:49:07Z

Unit Test Results

186 tests 186 ✔️ 10s ⏱️
  37 suites     0 💤
  37 files     0 ❌

Results for commit f4bf86d.

♻️ This comment has been updated with latest results.

kokodak

안녕하세요 채채 !

오랜만에 프로젝트에 들어와서 코드리뷰를 남겨봤어요. PR 작업한다고 고생하셨씁니다

Converter 를 직접 다뤄서 Enum 매핑 문제를 해결하려한 점이 인상 깊었어요 !

이넘으로 바인딩할때 해당 이넘값이 존재하지않으면 예외를 던지는데요. 이것이 핸들러에서 잡히지 않는것이었습니다.
이유는 리졸버에서 던지는 예외는 MethodArgumentTypeMismatchException.class
로 잡아야 하는데 저희 핸들러에서는 해당 클래스를 잡는 매서드가 구현되있지 않았습니다!

채채가 말해주신 부분에서 생각해볼만한 점이 있는 것 같아요.

Enum 을 찾지 못했을 때 커스텀 예외를 던지는 것이 MethodArgumentTypeMismatchException 으로 변환되는 것이 좋을까?

채채가 말씀해주신대로, 커스텀 예외를 던지더라도 저 예외로 바뀌더라구요. 왜인지 궁금해서 디버깅을 쭉 해봤는데, 범인은 Converter 였습니다.

일단 MethodArgumentTypeMismatchException 라는 예외를 던지는 주체는, 리졸버가 아닌 컨버터입니다. 저희가 던진 커스텀 예외는 TypeConverterSupport 라는 클래스에서 TypeMismatchException 예외로 변환됩니다.

그후에, AbstractNamedValueMethodArgumentResolver 라는 이름의 리졸버에서 TypeMismatchException 을 잡고, 이를 MethodArgumentTypeMismatchException 예외로 변환시켜 클라이언트에게 던져주는 방식이네요.

만약 채채가 MethodArgumentTypeMismatchException 예외가 아닌, 저희가 커스텀한 예외가 터지길 원한다면 Converter 가 아닌, 커스텀 ArgumentResolver 를 만들어서 LetterLogType 으로 매핑시켜주는게 좋을 것 같아요. 이렇게 하면 커스텀 예외가 스프링 예외로 변환되지 않고, 저희가 의도한 커스텀 예외 그대로 나가게 될 것 같아요 !

그렇지 않고 만약 커스텀 예외가 아닌, 스프링에서 사용자 입력 값에 대해 추상화된 예외를 사용하고 싶다면, 지금처럼 MethodArgumentTypeMismatchException 을 잡아줘서 common 예외로 변환시켜 던져주는 것이 좋을 것 같네요.

채채는 어떤 것을 왜 선호하시는지 궁금해요 ㅎㅎ

kokodak · 2023-11-01T08:48:51Z

backend/src/main/java/com/now/naaga/common/exception/ControllerExceptionHandler.java

+    @ExceptionHandler(MethodArgumentTypeMismatchException.class)
+    public ResponseEntity<ExceptionResponse> handleArgumentTypeMismatchException(final Exception e) {
+        final CommonExceptionType commonExceptionType = CommonExceptionType.INVALID_REQUEST_PARAMETERS;
+        final ExceptionResponse exceptionResponse = new ExceptionResponse(commonExceptionType.errorCode(), commonExceptionType.errorMessage());
+
+        log.info("error = {}", exceptionResponse);
+
+        return ResponseEntity.status(commonExceptionType.httpStatus()).body(exceptionResponse);
+    }


P2:

클라이언트 단에서 들어오는 입력 값 검증에서 발생하는 예외는 바로 위 메서드에서 공통적으로 처리해주고 있어요! 그래서 MethodArgumentTypeMismatchException 예외를 위 메서드에 추가하는 게 좋을 것 같습니다 😁

인정...이지만 제가 던져줘야 하는 예외는 INVALID_REQUEST_BODY 가 아니라 INVALID_REQUEST_PARAMETERS여서 새로운 함수를 만들어서 던져주게 되었습니다 ㅠㅠ 사실 사용자에게 전달해야하는 예외가 요청바디가 잘못되었는지 요청 파라미터가 잘못되었는지 모두 알려줄 필요가 있을까 하는생각이어서 "잘못된 요청입니다"로 모두 보내주는것이 어떨가 생각했지만 논의가 필요하고 일단은 저희가 합의한 예외는 INVALID_REQUEST_PARAMETERS를 던져줘야해서 함수를 새로 만들게 되었네요

kokodak · 2023-11-01T08:52:47Z

backend/src/main/java/com/now/naaga/letter/presentation/LetterController.java

-import static com.now.naaga.letter.presentation.LogType.WRITE;
-
-@RequestMapping("/letters")
+@RequestMapping


P2:

중복되는 API URL 을 명시해줘서 중복 제거해주면 좋을 것 같아요 !!

letterlogs는 letters로 묶이지 않아서 따로두게 되었습니다!

kokodak · 2023-11-02T02:19:35Z

backend/src/main/java/com/now/naaga/letter/presentation/LetterLogType.java

@@ -0,0 +1,21 @@
+package com.now.naaga.letter.presentation;


P3:

제 개인적인 생각으로는 이 친구의 소속이 presentation 계층이 아닌 것 같아요. 뭔가 domain스러운 친구이지 않을까요 ?!

P2:

저도 코코닥 의견과 비슷하지만 저는 생각이 꽤 확고해서 단계를 올려보겠습니다.

일단 표현 계층에서 뷰에서 입력받을 때 사용하고 싶은 enum이 존재한다면 이곳(이 패키지 안에 dto 패키지 안)에 존재할 수 있다고 생각이 들어요.

하지만 이 enum이 표현계층이라는 생각으로 만들었다면 응용 계층에서 이 enum을 의존시키면 안될 것 같아요.
저희가 컨텍스트(도메인 패키지)별로는 의존성 순환을 관리하듯이,
레이어 별로는 표현 -> 응용 -> 도메인 -> 영속 로 의존성이 흐르게 하는데, 의존성이 꺼꾸로 가는 것 같아요.

어차피 표현 계층은 제일 상단에 있는 애니까 도메인을 의존해도 크게 상관이 없겠다라는 것이 저희의 결론이였고,
이걸 도메인으로 정의하자는 코코닥 의견에 동의합니다

루선생님 강의 잘 듣고 갑니다

확실히 서비스가 표현계층을 가지고 있는건 방향이 반대로 흐른다는 생각이드네요..

kokodak · 2023-11-02T02:20:58Z

backend/src/main/java/com/now/naaga/letter/presentation/LetterLogType.java

+        return Arrays.stream(values())
+                .filter(enumValue -> enumValue.name().equalsIgnoreCase(logType))
+                .findFirst()
+                .orElseThrow(() -> new CommonException(INVALID_REQUEST_PARAMETERS));


P2:

이 친구는 위 리뷰와 마찬가지로, 하나의 독립적인 도메인으로서 사용가능할 것 같아요.

만약 값을 찾아오지 못했을 때, 그에 대한 예외는 common 예외가 아닌 letter 예외가 되어야하지 않을까요 ?

저도 코코닥 말에 이어서 이걸 표현계층에 둘 것이라면 해당 에러가 적절할 수도 있지만
도메인으로 내릴거라면 LetterLog에 대한 NotFound예외 같은것이 필요할 것 같아요!

P3:

그리고 추가적으로 이 로직이 너무 뷰에 대한 로직이라는 생각이 든다면,
이 메서드는 삭제하고,
컨버터에서 직접 해당 값을 입력값에 따라서 enum을 할당해주는 것은 어떤가요??

해당 메서드의 역할은 컨트롤러 쿼리파라미터로 받은string을 enum으로 매핑해주는 역할입니다. 이것은 letter와 상관없이 요청 파라미터에 대한 예외라고 생각하는데, 해당 매서드를 이넘에 두지 않고 LetterLogTypeConverter로 이동하는 것이 맞을것같아서 수정하게 될것같네용 -> 수정 하다보니 이 방법도 아닌것같아서 다시 바뀔거같아요 호호..pr로 정리해놓겟습니다 -> 하다보니 코코닥의견이 맞다고 판단되네요 방어 실패 잘수정해보겟습니다

해당 메서드의 역할은 컨트롤러 쿼리파라미터로 받은string을 enum으로 매핑해주는 역할입니다. 이것은 letter와 상관없이 요청 파라미터에 대한 예외라고 생각하는데, 해당 매서드를 이넘에 두지 않고 다른 계층으로 이동하는 것이 맞을것같아서 수정하게 될것같네용

채채 이부분 정확히 이해하지 못해서 변경 후 알려주시면 감사하겠습니다

수정 후 pr에 남겨놓겠습니다!

kokodak · 2023-11-02T02:24:56Z

backend/src/main/java/com/now/naaga/letter/domain/LetterLogTypeConverter.java

+import com.now.naaga.letter.presentation.LetterLogType;
+import org.springframework.core.convert.converter.Converter;
+
+public class LetterLogTypeConverter implements Converter<String, LetterLogType> {


P2:

converter 라는게 있었군요 !! 채채 덕분에 하나 알아갑니다.

다만 하나 걸리는 점은 패키지인 것 같아요. Converter는 도메인이고, LetterLogType 은 presentation 계층에 속한 이유가 무엇인가요?

일단 Converter 는 저희 도메인으로서 사용을 하지 않을 것 같아요. 얘를 쓰는 주체는 결국 스프링 프레임워크이니까요. 어쩌면 이 친구는 common 패키지에 들어가야할 수도 있고, presentation 계층에 converter 패키지를 만들어서 거기에 두어도 괜찮을 것 같네요.

common 패키지에 들어가야할 수도 있고, presentation 계층에 converter 패키지를 만들어서 거기에 두어도 괜찮을 것 같네요.

인정

채채 컨버터라는 것은 RequestParam, RequestBody에 관계없이 동작하는 건가요?
채채 덕분에 배우고 갑니다!

코코닥의견 적극반영

kokodak · 2023-11-02T02:28:39Z

backend/src/main/java/com/now/naaga/like/application/PlaceLikeService.java

+        // 그래서 여기서 좋아요를 올려야하는거아님?
+        placeStatisticsService.plusLike(new PlusLikeCommand(placeId));


P1:

PR 주제와 맞지 않은 작업 내용이고, 잘 작동하고 있는 기능을 망가뜨리는 코드에요.

앞으로 PR 올릴때 이런 것들은 지워서 올려주세요 !

이게 왜올라갓죠..?ㅠㅠ 돌려놓겠습니다

kokodak · 2023-11-02T02:31:05Z

backend/src/test/java/com/now/naaga/letter/application/letterlog/ReadLetterLogServiceTest.java

+//package com.now.naaga.letter.application.letterlog;
+//
+//import com.now.naaga.common.builder.GameBuilder;
+//import com.now.naaga.common.builder.LetterBuilder;
+//import com.now.naaga.common.builder.PlaceBuilder;
+//import com.now.naaga.common.builder.PlayerBuilder;
+//import com.now.naaga.game.domain.Game;
+//import com.now.naaga.game.domain.GameStatus;
+//import com.now.naaga.game.exception.GameException;
+//import com.now.naaga.letter.application.letterlog.dto.LetterLogCreateCommand;
+//import com.now.naaga.letter.domain.Letter;
+//import com.now.naaga.letter.domain.letterlog.ReadLetterLog;
+//import com.now.naaga.letter.repository.letterlog.ReadLetterLogRepository;
+//import com.now.naaga.place.domain.Place;
+//import com.now.naaga.player.domain.Player;
+//import org.junit.jupiter.api.DisplayNameGeneration;
+//import org.junit.jupiter.api.DisplayNameGenerator;
+//import org.junit.jupiter.api.Test;
+//import org.springframework.beans.factory.annotation.Autowired;
+//import org.springframework.boot.test.context.SpringBootTest;
+//import org.springframework.test.context.jdbc.Sql;
+//
+//import java.util.List;
+//
+//import static com.now.naaga.common.fixture.PositionFixture.잠실_루터회관_정문_좌표;
+//import static com.now.naaga.common.fixture.PositionFixture.잠실역_교보문고_좌표;
+//import static com.now.naaga.game.exception.GameExceptionType.NOT_EXIST_IN_PROGRESS;
+//import static org.assertj.core.api.Assertions.assertThat;
+//import static org.assertj.core.api.SoftAssertions.assertSoftly;
+//import static org.junit.jupiter.api.Assertions.assertThrows;
+//
+//@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
+//@Sql("/truncate.sql")
+//@SpringBootTest
+//class ReadLetterLogServiceTest {
+//
+//    @Autowired
+//    private ReadLetterLogService readLetterLogService;
+//
+//    @Autowired
+//    private ReadLetterLogRepository readLetterLogRepository;
+//
+//    @Autowired
+//    private PlayerBuilder playerBuilder;
+//
+//    @Autowired
+//    private PlaceBuilder placeBuilder;
+//
+//    @Autowired
+//    private GameBuilder gameBuilder;
+//
+//    @Autowired
+//    private LetterBuilder letterBuilder;
+//
+//


P2:

모두가 함께 쓰는 dev 브랜치에 merge 될 PR인데, 전체 주석처리 같은 부분은 정리해서 올려주시면 감사하겠습니다 😊

허허 큰실수를 저질렀군요..

kokodak · 2023-11-02T02:31:58Z

backend/src/test/java/com/now/naaga/letter/presentation/LetterLogTypeTest.java

+    @Test
+    void ghgh() {
+        LetterLogType.from("하하하");
+    }


P1:

이에 대한 테스트 코드를 다시 작성해주시면 감사하겠습니다

이건 지워질거같아요 테스크를 오랜시간 작업하다보니 정리가 안되었네요..싹 수정하겠습니다

kokodak · 2023-11-02T02:55:29Z

backend/src/main/java/com/now/naaga/letter/application/LetterService.java

+                .toList();
+    }
+
+    // 여기 private인데 트렌젝셔이 어떻게 걸리지?


P2:

주석 제거해주시면 감사하겠습니다 !

dooboocookie

안녕하세요 채채

위에 Converter와 ArgumentResolver의 차이는 코코닥이 잘 설명해준 것 같은데요.
저는 추가적으로

ArgumentResolver: 특정 조건의 아규먼트를 해결하기 위해서 HttpServletRequest, ... 의 값을 이용해서 어떻게 해결할지 정한다.
Converter: 특정 타입을 특정 타입으로 컨버팅한다. 그리고 이것은 아규먼트를 리졸빙할 때보다 더 큰 범위에서 사용 가능하다.

전 개인적으로 해당 과정은 Converter가 적절하다고 생각합니다.

저는 해당 @RequestParam이 달린 아규먼트를 HTTP 요청의 요청 파라미터의 밸류를 가져온다는 것은 RequestParamMethodArgumentResolver에서 리졸빙을 해준다라고 생각합니다.

다만 타입컨버팅이 가능한 원시타입의 범위를 벗어난 enum타입으로 받으려는 것이 떄문에 추가적으로 타입컨버터만 필요하다고 생각합니다.

그런 이유로 채채가 한것처럼 컨버터를 추가로 한 것이 더 적절하다고 생각합니다.

그 이외에도 단 리뷰포인트들이 많아서 일단 RC합니다~

dooboocookie · 2023-11-02T09:11:44Z

backend/src/main/java/com/now/naaga/common/exception/ControllerExceptionHandler.java

+    @ExceptionHandler(MethodArgumentTypeMismatchException.class)
+    public ResponseEntity<ExceptionResponse> handleArgumentTypeMismatchException(final Exception e) {
+        final CommonExceptionType commonExceptionType = CommonExceptionType.INVALID_REQUEST_PARAMETERS;
+        final ExceptionResponse exceptionResponse = new ExceptionResponse(commonExceptionType.errorCode(), commonExceptionType.errorMessage());
+
+        log.info("error = {}", exceptionResponse);
+
+        return ResponseEntity.status(commonExceptionType.httpStatus()).body(exceptionResponse);
+    }


dooboocookie · 2023-11-02T10:22:31Z

backend/src/main/java/com/now/naaga/letter/presentation/LetterLogType.java

@@ -0,0 +1,21 @@
+package com.now.naaga.letter.presentation;


P2:

저도 코코닥 의견과 비슷하지만 저는 생각이 꽤 확고해서 단계를 올려보겠습니다.

일단 표현 계층에서 뷰에서 입력받을 때 사용하고 싶은 enum이 존재한다면 이곳(이 패키지 안에 dto 패키지 안)에 존재할 수 있다고 생각이 들어요.

하지만 이 enum이 표현계층이라는 생각으로 만들었다면 응용 계층에서 이 enum을 의존시키면 안될 것 같아요.
저희가 컨텍스트(도메인 패키지)별로는 의존성 순환을 관리하듯이,
레이어 별로는 표현 -> 응용 -> 도메인 -> 영속 로 의존성이 흐르게 하는데, 의존성이 꺼꾸로 가는 것 같아요.

어차피 표현 계층은 제일 상단에 있는 애니까 도메인을 의존해도 크게 상관이 없겠다라는 것이 저희의 결론이였고,
이걸 도메인으로 정의하자는 코코닥 의견에 동의합니다

dooboocookie · 2023-11-02T10:27:18Z

backend/src/main/java/com/now/naaga/letter/presentation/LetterLogType.java

+        return Arrays.stream(values())
+                .filter(enumValue -> enumValue.name().equalsIgnoreCase(logType))
+                .findFirst()
+                .orElseThrow(() -> new CommonException(INVALID_REQUEST_PARAMETERS));


저도 코코닥 말에 이어서 이걸 표현계층에 둘 것이라면 해당 에러가 적절할 수도 있지만
도메인으로 내릴거라면 LetterLog에 대한 NotFound예외 같은것이 필요할 것 같아요!

dooboocookie · 2023-11-02T10:40:36Z

backend/src/main/java/com/now/naaga/letter/application/LetterService.java

+    public void logWriteLetter(final Letter letter) {
+        final Game gameInProgress = getGameInProgress(letter.getRegisteredPlayer().getId());
+        final WriteLetterLog writeLetterLog = new WriteLetterLog(gameInProgress, letter);
+        writeLetterLogRepository.save(writeLetterLog);
    }


P2:

이 메서드를 public으로 열어놓으신 이유가 따로 있으신가요??

LetterSerivce의 기능 중 하나가 logWriteLetter과 logReadLetter인것은 어색한 것 같습니다.
LetterLogService가 따로 존재한다면, 그곳에 public으로 열릴 수도 있는 기능이긴 하지만,
이것은 LetterLog에 대한 응용계층이 독자적으로 존재할 가능성이 생겼을 때 같아요. 로그를 여러군데에서 사용된다면 letterLogService를 만들 것 같아요!!

지금은 LetterLog의 저장과 읽기에대한 로그 저장은 LetterService에 매우매우 강하게 결합되어있기 때문에 private 메서드로 바꿔야하지 않을까 합니다!!

밑에 logReadLetter(...)두요!!

루카의 의견대로 private으로 두는 것이 맞다고 생각합니다. 해당 메서드에 대해 테스트코트를 작성하다보니 자연스럽게 public으로 되어있던것같네요! private로직까지 함께 테스트가 되도록 수정해보겠습니당

dooboocookie · 2023-11-02T10:49:55Z

backend/src/main/java/com/now/naaga/letter/application/LetterService.java

+    // 여기 private인데 트렌젝셔이 어떻게 걸리지?
+    private Game getGameInProgress(final Long playerId) {
+        final FindGameByStatusCommand findGameByStatusCommand = new FindGameByStatusCommand(playerId, IN_PROGRESS);
+        final List<Game> gamesInProgress = gameService.findGamesByStatus(findGameByStatusCommand);
+        if (gamesInProgress.isEmpty()) {
+            throw new GameException(GameExceptionType.NOT_EXIST_IN_PROGRESS);
+        }
+        return gamesInProgress.get(0);
    }


private 메서드는 그것을 호출하는 쪽 메서드의 트랜잭션 범위 안에 있겠죠?
그리고 이 안에서 호출되는 gameService의 findGamesByStatus는 스프링부트 트랜잭션 전파 기본값인 REQUIRED에 의해서 부모 트랜잭션에 포함될 것입니다!

P2:

이 private 메서드가 저희 프로젝트 지금까지 책임분리를 하는 방법에 따르면 이쪽에 있는 것이 맞을까요?
제 생각에는 이곳보다는 GameSerivce에 의존하는 것이 맞지 않을까요?
지금 이 안에서 검증하는 것은

해당 플레이어가 진행중인 게임이 존재하는가와

그 게임이 1개인가
이 두가지가 있을 것 같은데, 그것은 GameService에서 검증해도될 내용인 것 같아요.

루카와 동일한 의견입니다.
이 부분 gameService 클래스에 아래와 같이 구현되어 있는데, 사용하면 좋을 것 같습니다!
```

@Transactional(readOnly = true) public Game findGameInProgress(final FindGameInProgressCommand findGameByStatusCommand) { List<Game> gameInProgress = gameRepository.findByPlayerIdAndGameStatus(findGameByStatusCommand.playerId(), IN_PROGRESS); if (gameInProgress.isEmpty()) { throw new GameException(NOT_EXIST_IN_PROGRESS); } return gameInProgress.get(0); }

dooboocookie · 2023-11-02T11:07:35Z

backend/src/main/java/com/now/naaga/letter/presentation/LetterController.java

-import static com.now.naaga.letter.presentation.LogType.WRITE;
-
-@RequestMapping("/letters")
+@RequestMapping


dooboocookie · 2023-11-02T11:32:49Z

backend/src/main/java/com/now/naaga/letter/domain/LetterLogTypeConverter.java

+import com.now.naaga.letter.presentation.LetterLogType;
+import org.springframework.core.convert.converter.Converter;
+
+public class LetterLogTypeConverter implements Converter<String, LetterLogType> {


common 패키지에 들어가야할 수도 있고, presentation 계층에 converter 패키지를 만들어서 거기에 두어도 괜찮을 것 같네요.

인정

dooboocookie · 2023-11-02T11:35:47Z

backend/src/main/java/com/now/naaga/letter/presentation/LetterLogType.java

+        return Arrays.stream(values())
+                .filter(enumValue -> enumValue.name().equalsIgnoreCase(logType))
+                .findFirst()
+                .orElseThrow(() -> new CommonException(INVALID_REQUEST_PARAMETERS));


P3:

그리고 추가적으로 이 로직이 너무 뷰에 대한 로직이라는 생각이 든다면,
이 메서드는 삭제하고,
컨버터에서 직접 해당 값을 입력값에 따라서 enum을 할당해주는 것은 어떤가요??

zillionme

채채~ 이미 코코닥과 루카가 좋은 리뷰 많이 남겨준 것 같습니다!
쪽지 로그 조회에서 P2 리뷰가 하나 있어 RC리뷰 남기고 갑니당

zillionme · 2023-11-06T12:09:07Z

backend/src/main/java/com/now/naaga/letter/application/LetterService.java

+
+    @Transactional(readOnly = true)
+    public List<Letter> findLetterLogInGame(final FindLetterLogByGameCommand findLetterLogByGameCommand) {
+        final Game gameInProgress = getGameInProgress(findLetterLogByGameCommand.playerId());


P1:
findLetterLogInGame를 호출하는 시점은 게임이 종료된 이후 아닐까요?
진행중인 게임이 레터 로그를 읽을 이유가 없을 것 같아서요.

이레천재..?

zillionme · 2023-11-06T12:16:36Z

backend/src/main/java/com/now/naaga/letter/application/LetterService.java

+    // 여기 private인데 트렌젝셔이 어떻게 걸리지?
+    private Game getGameInProgress(final Long playerId) {
+        final FindGameByStatusCommand findGameByStatusCommand = new FindGameByStatusCommand(playerId, IN_PROGRESS);
+        final List<Game> gamesInProgress = gameService.findGamesByStatus(findGameByStatusCommand);
+        if (gamesInProgress.isEmpty()) {
+            throw new GameException(GameExceptionType.NOT_EXIST_IN_PROGRESS);
+        }
+        return gamesInProgress.get(0);
    }


루카와 동일한 의견입니다.
이 부분 gameService 클래스에 아래와 같이 구현되어 있는데, 사용하면 좋을 것 같습니다!
```

@Transactional(readOnly = true) public Game findGameInProgress(final FindGameInProgressCommand findGameByStatusCommand) { List<Game> gameInProgress = gameRepository.findByPlayerIdAndGameStatus(findGameByStatusCommand.playerId(), IN_PROGRESS); if (gameInProgress.isEmpty()) { throw new GameException(NOT_EXIST_IN_PROGRESS); } return gameInProgress.get(0); }

zillionme · 2023-11-06T12:20:09Z

backend/src/main/java/com/now/naaga/letter/presentation/LetterLogType.java

@@ -0,0 +1,21 @@
+package com.now.naaga.letter.presentation;


루선생님 강의 잘 듣고 갑니다

zillionme · 2023-11-06T12:21:33Z

backend/src/main/java/com/now/naaga/letter/domain/LetterLogTypeConverter.java

+import com.now.naaga.letter.presentation.LetterLogType;
+import org.springframework.core.convert.converter.Converter;
+
+public class LetterLogTypeConverter implements Converter<String, LetterLogType> {


채채 컨버터라는 것은 RequestParam, RequestBody에 관계없이 동작하는 건가요?
채채 덕분에 배우고 갑니다!

zillionme · 2023-11-06T12:31:13Z

backend/src/main/java/com/now/naaga/letter/presentation/LetterLogType.java

+        return Arrays.stream(values())
+                .filter(enumValue -> enumValue.name().equalsIgnoreCase(logType))
+                .findFirst()
+                .orElseThrow(() -> new CommonException(INVALID_REQUEST_PARAMETERS));


해당 메서드의 역할은 컨트롤러 쿼리파라미터로 받은string을 enum으로 매핑해주는 역할입니다. 이것은 letter와 상관없이 요청 파라미터에 대한 예외라고 생각하는데, 해당 매서드를 이넘에 두지 않고 다른 계층으로 이동하는 것이 맞을것같아서 수정하게 될것같네용

채채 이부분 정확히 이해하지 못해서 변경 후 알려주시면 감사하겠습니다

chaewon121

수정한 부분

주석제거
LetterLogType -> 도메인으로 이동
LetterLogTypeConverter -> presentation/converter패키지로 이동
LetterLogType에서 예외를 LetterType으로 수정 (코코닥의견에 동의)
추가적인 의견도 반영

LetterLogType에서 예외를 LetterType으로 수정후 LetterLogTypeConverter에서는 따로 예외를 잡아주지 않았습니다.

어차피 LetterLogTypeConverter에서 에외가 잡히면 MethodArgumentTypeMismatchException로 예외가 바뀌기 때문인데요!

이유는 코코닥이 pr에서 잘 정리해주셨습니다!

또한 resolver로 수정하지 않고 converter로 가져가려는 이유는 루카가 pr에 남겨주신 의견과 같습니다.

string을 enum으로 바꿔주는것은 resolver보다 converter로 하였을때 그 역할이 더 잘 드러난다고 판단하였습니다.

또한 MethodArgumentTypeMismatchException를 잡아주는 함수를 따로 만든건 반환해주는 에러가 다르기 떄문입니다.

MethodArgumentTypeMismatchException로 잡았을때는 INVALID_REQUEST_BODY 가아닌

INVALID_REQUEST_PARAMETERS를 반환해주어야 하기때문에 함수를 따로 뺄수밖에 없는 상황입니다.

(handleTypeMismatchException에서 if 분기로 빼보았으나 코드가 꼴보기 싫어져서 함수로 뺐습니다..)

그럼 여기서 MethodArgumentTypeMismatchException로 잡힌 에러는 모두 INVALID_REQUEST_PARAMETERS로 반환해줘야 하는가?

네 일단 converter라는 애는 현재는 컨트롤러 매핑에서만 사용하며 이외의 상황에서는 어디서 추가로 사용될지 모르겠으나

당장은 문제가 없다고 판단합니다!

그렇게 생각하면 handleTypeMismatchException도 같은 상황이지 않을까요!?

…23-naaga into refactor/#497

dooboocookie

고생하셨어요 채채

제 생각에는 개선 여지가 조금 있을 것 같긴한데,
너무 큰 변경사항일 것 같아, 이 리뷰 저는여기서 어프루브 할게요.

생각한 개선점

LetterLog 부모 클래스를 둘 수 있지 않을까?
LetterLogType은 완전 뷰에 관련된 값 아닌가? (도메인이 아닌것같음)

chaewon121

LetterLog 부모 클래스를 둘 수 있지 않을까?
-> 이건 더생각해봐야할것같아요 부모클래스도 생각해봤는데 둘이 완전히 같지 않고 onetoone,mantoone으로 다른부분도 존재해서 시도했을때 좀 머리아팠던 기억이있습니다!
추후 고민해보죠..! -> 개선하고 싶다면 루카말처럼 크게 바뀔것같아 현재 pr말고 새로 이슈발급해서 수정하는것이 나을것같습니다
LetterLogType은 완전 뷰에 관련된 값 아닌가? (도메인이 아닌것같음)
->저도 이부분에 대해서 어제 하루종일 고민했는데 전 도메인으로 생각이 가까워지더라구요. 물론 컨트롤러매핑때문에 생겨난 enum이지만 서비스나 도메인에서 해당 이넘을 사용할일이 (아직은 없지만 충분히 사용가능하다고 생각) 생길 수있어 컨트롤러에 한정적으로 두는것보다 도메인으로 둬도 크게 문제 없다는 생각이 들었어요. 더 확장성이 있다고 판단되어 도메인으로 두었고, 도메인으로 두어도 controller에서 사용할때 전혀 문제없다고 생각하여 굳이 presentation계층에 두지 않아도 되겠다는 생각이 들었습니다! 그래서 예외도 Letter예외로 던지는 것으로 수정되었구요..! + 만약 presentation계층으로 이동된다면 예외가 바뀔것같고 presentation계층에서만 한정되어 사용할것같아요

chaewon121 added 3 commits October 18, 2023 01:23

refactor: letterlog 패치지 제거 및 letter와 합치기

094746f

refactor: enum값 converter 기능으로 리팩터링

56dca80

fix: 리졸버에서 터지는 에러 잡지 못하는 오류 수정

ccf030d

chaewon121 changed the base branch from main to dev_backend October 18, 2023 16:35

chore: 깃 충돌 해결

8458dbd

chaewon121 changed the title ~~Refactor/#497~~ refactor: letter패키지 정리하기 Oct 18, 2023

chaewon121 requested review from kokodak, dooboocookie and zillionme October 18, 2023 17:00

chaewon121 self-assigned this Oct 18, 2023

chaewon121 added 🍀 백엔드 백엔드 라벨 D-2 6차 데모데이 labels Oct 18, 2023

kokodak requested changes Nov 2, 2023

View reviewed changes

dooboocookie requested changes Nov 2, 2023

View reviewed changes

chaewon121 removed the D-2 label Nov 6, 2023

style: 주석 제거

cbc25ac

zillionme requested changes Nov 6, 2023

View reviewed changes

chaewon121 added 4 commits November 6, 2023 23:23

refactor: 위치 이동

4bca9e9

refactor: 필요없는 로직 제거

dce0561

refactor: 진행중인 게임 찾는 로직 수정

39e1e7c

refactor: 테스트 수정

c64fdb9

chaewon121 commented Nov 6, 2023

View reviewed changes

chaewon121 added 2 commits November 7, 2023 16:04

refactor: 서브모듈 업데이트

2e7dba4

Merge branch 'dev_backend' of https://github.com/woowacourse-teams/20…

f4bf86d

…23-naaga into refactor/#497

dooboocookie approved these changes Nov 7, 2023

View reviewed changes

chaewon121 commented Nov 7, 2023

View reviewed changes

dooboocookie merged commit ef598ef into dev_backend Nov 8, 2023
3 checks passed

dooboocookie deleted the refactor/#497 branch November 8, 2023 01:18

		// 그래서 여기서 좋아요를 올려야하는거아님?
		placeStatisticsService.plusLike(new PlusLikeCommand(placeId));

refactor: letter패키지 정리하기 #513

refactor: letter패키지 정리하기 #513

Conversation

chaewon121 commented Oct 18, 2023 • edited Loading

📌 관련 이슈

🛠️ 작업 내용

🎯 리뷰 포인트

⏳ 작업 시간

github-actions bot commented Oct 18, 2023 • edited Loading

Unit Test Results

kokodak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaewon121 Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaewon121 Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaewon121 Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dooboocookie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zillionme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaewon121 left a comment • edited Loading

Choose a reason for hiding this comment

수정한 부분

dooboocookie left a comment

Choose a reason for hiding this comment

chaewon121 left a comment • edited Loading

Choose a reason for hiding this comment

chaewon121 commented Oct 18, 2023 •

edited

Loading

github-actions bot commented Oct 18, 2023 •

edited

Loading

chaewon121 Nov 6, 2023 •

edited

Loading

chaewon121 Nov 6, 2023 •

edited

Loading

chaewon121 Nov 6, 2023 •

edited

Loading

chaewon121 left a comment •

edited

Loading

chaewon121 left a comment •

edited

Loading