Skip to content
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

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

Merged
merged 11 commits into from
Nov 8, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,12 @@
import com.now.naaga.auth.presentation.interceptor.ManagerAuthInterceptor;
import com.now.naaga.common.presentation.interceptor.RequestMatcherInterceptor;
import java.util.List;

import com.now.naaga.letter.domain.LetterLogTypeConverter;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.context.annotation.Configuration;
import org.springframework.format.FormatterRegistrar;
import org.springframework.format.FormatterRegistry;
import org.springframework.http.HttpMethod;
import org.springframework.web.method.support.HandlerMethodArgumentResolver;
import org.springframework.web.servlet.HandlerInterceptor;
Expand Down Expand Up @@ -39,6 +43,11 @@ public WebConfig(final PlayerArgumentResolver playerArgumentResolver,
this.managerAuthInterceptor = managerAuthInterceptor;
}

@Override
public void addFormatters(final FormatterRegistry registry){
registry.addConverter(new LetterLogTypeConverter());
}

@Override
public void addInterceptors(final InterceptorRegistry registry) {
registry.addInterceptor(mapAuthInterceptor());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.springframework.web.bind.MethodArgumentNotValidException;
import org.springframework.web.bind.annotation.ExceptionHandler;
import org.springframework.web.bind.annotation.RestControllerAdvice;
import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException;

@RestControllerAdvice
public class ControllerExceptionHandler {
Expand Down Expand Up @@ -38,6 +39,16 @@ public ResponseEntity<ExceptionResponse> handleTypeMismatchException(final Excep
return ResponseEntity.status(commonExceptionType.httpStatus()).body(exceptionResponse);
}

@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);
}
Comment on lines +42 to +50
Copy link
Collaborator

Choose a reason for hiding this comment

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

P2:

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

인정

Copy link
Collaborator Author

@chaewon121 chaewon121 Nov 6, 2023

Choose a reason for hiding this comment

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

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


@ExceptionHandler(InternalException.class)
public ResponseEntity<ExceptionResponse> handleInternalException(final InternalException e) {
final BaseExceptionType internalExceptionType = e.exceptionType();
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,23 +1,32 @@
package com.now.naaga.letter.application;

import static com.now.naaga.letter.exception.LetterExceptionType.NO_EXIST;

import com.now.naaga.game.application.GameService;
import com.now.naaga.game.application.dto.FindGameByStatusCommand;
import com.now.naaga.game.domain.Game;
import com.now.naaga.game.exception.GameException;
import com.now.naaga.game.exception.GameExceptionType;
import com.now.naaga.letter.application.dto.CreateLetterCommand;
import com.now.naaga.letter.application.letterlog.ReadLetterLogService;
import com.now.naaga.letter.application.letterlog.WriteLetterLogService;
import com.now.naaga.letter.application.letterlog.dto.LetterLogCreateCommand;
import com.now.naaga.letter.application.letterlog.dto.WriteLetterLogCreateCommand;
import com.now.naaga.letter.domain.Letter;
import com.now.naaga.letter.domain.letterlog.ReadLetterLog;
import com.now.naaga.letter.domain.letterlog.WriteLetterLog;
import com.now.naaga.letter.exception.LetterException;
import com.now.naaga.letter.presentation.LetterLogType;
import com.now.naaga.letter.presentation.dto.FindLetterLogByGameCommand;
import com.now.naaga.letter.presentation.dto.FindNearByLetterCommand;
import com.now.naaga.letter.presentation.dto.LetterReadCommand;
import com.now.naaga.letter.repository.LetterRepository;
import com.now.naaga.letter.repository.letterlog.ReadLetterLogRepository;
import com.now.naaga.letter.repository.letterlog.WriteLetterLogRepository;
import com.now.naaga.player.application.PlayerService;
import com.now.naaga.player.domain.Player;
import java.util.List;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

import java.util.List;

import static com.now.naaga.game.domain.GameStatus.IN_PROGRESS;
import static com.now.naaga.letter.exception.LetterExceptionType.NO_EXIST;

@Transactional
@Service
public class LetterService {
Expand All @@ -26,43 +35,85 @@ public class LetterService {

private final LetterRepository letterRepository;

private final ReadLetterLogService readLetterLogService;
private final WriteLetterLogService writeLetterLogService;
private final ReadLetterLogRepository readLetterLogRepository;

private final WriteLetterLogRepository writeLetterLogRepository;

private final PlayerService playerService;

private final GameService gameService;

public LetterService(final LetterRepository letterRepository,
final ReadLetterLogService readLetterLogService,
final WriteLetterLogService writeLetterLogService,
final PlayerService playerService) {
final ReadLetterLogRepository readLetterLogRepository,
final WriteLetterLogRepository writeLetterLogRepository,
final PlayerService playerService,
final GameService gameService) {
this.letterRepository = letterRepository;
this.readLetterLogService = readLetterLogService;
this.writeLetterLogService = writeLetterLogService;
this.readLetterLogRepository = readLetterLogRepository;
this.writeLetterLogRepository = writeLetterLogRepository;
this.playerService = playerService;
this.gameService = gameService;
}

public Letter writeLetter(final CreateLetterCommand createLetterCommand) {
final Player player = playerService.findPlayerById(createLetterCommand.playerId());
final Letter letter = new Letter(player, createLetterCommand.position(), createLetterCommand.message());
letterRepository.save(letter);

logWriteLetter(letter);
return letter;
}

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

Choose a reason for hiding this comment

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

P2:

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

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

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


public Letter findLetter(final LetterReadCommand letterReadCommand) {
final Player player = playerService.findPlayerById(letterReadCommand.playerId());
final Letter foundLetter = letterRepository.findById(letterReadCommand.letterId())
.orElseThrow(() -> new LetterException(NO_EXIST));

readLetterLogService.log(new LetterLogCreateCommand(player.getId(), foundLetter));
logReadLetter(player, foundLetter);
return foundLetter;
}

public void logReadLetter(final Player player,
final Letter letter) {
final Game gameInProgress = getGameInProgress(player.getId());
final ReadLetterLog readLetterLog = new ReadLetterLog(gameInProgress, letter);
readLetterLogRepository.save(readLetterLog);
}

@Transactional(readOnly = true)
public List<Letter> findNearByLetters(final FindNearByLetterCommand findNearByLetterCommand) {
return letterRepository.findLetterByPositionAndDistance(findNearByLetterCommand.position(), LETTER_RADIUS);
}

public Letter writeLetter(final CreateLetterCommand createLetterCommand) {
final Player player = playerService.findPlayerById(createLetterCommand.playerId());
final Letter letter = new Letter(player, createLetterCommand.position(), createLetterCommand.message());
letterRepository.save(letter);

final WriteLetterLogCreateCommand writeLetterLogCreateCommand = new WriteLetterLogCreateCommand(letter.getId());
writeLetterLogService.log(writeLetterLogCreateCommand);
return letter;

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이레천재..?

if (findLetterLogByGameCommand.letterLogType() == LetterLogType.WRITE) {
final List<WriteLetterLog> writeLetterLogs = writeLetterLogRepository.findByGameId(gameInProgress.getId());
return writeLetterLogs.stream()
.map(WriteLetterLog::getLetter)
.toList();
}
final List<ReadLetterLog> readLetterLogs = readLetterLogRepository.findByGameId(gameInProgress.getId());
return readLetterLogs.stream()
.map(ReadLetterLog::getLetter)
.toList();
}

// 여기 private인데 트렌젝셔이 어떻게 걸리지?
Copy link
Collaborator

Choose a reason for hiding this comment

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

P2:

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

제거완!

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);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

P2:

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

  1. 해당 플레이어가 진행중인 게임이 존재하는가와
  2. 그 게임이 1개인가
    이 두가지가 있을 것 같은데, 그것은 GameService에서 검증해도될 내용인 것 같아요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

루카와 동일한 의견입니다.
이 부분 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);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

인정!

}

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.now.naaga.letter.domain;

import com.now.naaga.letter.presentation.LetterLogType;
import org.springframework.core.convert.converter.Converter;

public class LetterLogTypeConverter implements Converter<String, LetterLogType> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

P2:

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

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

인정

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

코코닥의견 적극반영


@Override
public LetterLogType convert(final String source) {
return LetterLogType.from(source);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import jakarta.persistence.ManyToOne;
import java.util.Objects;


@Entity
public class ReadLetterLog extends BaseEntity {

Expand Down
Loading
Loading