[2단계 - 웹 자동차 경주] 테오(최우성) 미션 제출합니다. #133

woosung1223 · 2023-04-18T06:33:06Z

안녕하세요 영이!
2단계 미션 구현 완료했습니다~

처음으로 Spring, Web 개념을 접하다보니 혼동되는 개념이 많고 코드에서도 아직 어색한 부분이 많습니다.

특히 entity, repository, dao 등과 같은 개념이 아직 혼란스러워서 이에 관련해서 피드백을 많이 받고 싶습니다.

이번에도 좋은 피드백 부탁드립니다!
감사합니다!

choijy1705

안녕하세요 테오
2단계 정말 잘 구현해주셨네요👍
~~다만 요구사항중에 기존 콘솔 출력방식도 변경하라는 부분이 있는데 반영안된것 같아요~~(아 잘못봤네요 반영되있네요 ㅠㅠ)
몇 가지 코멘트 남겼으니 피드백 반영하면서 챙겨봐주세요!

choijy1705 · 2023-04-18T15:34:17Z

src/main/java/racingcar/controller/ExceptionController.java

+    @ExceptionHandler({
+            CarNameBlankException.class,
+            CarNameLengthException.class,
+            NoCarsExistException.class,
+            PositionInvalidException.class
+    })


위 예외가 아닌 경우에 대한 처리는 고려하지 않는걸까요?

반영했습니다 :)

말씀하신대로 처리되지 않는 예외가 시스템에 존재하는 경우, 시스템의 무결성을 보장하기가 어려울 것 같습니다.
사용자의 입력 한 번으로 시스템이 종료될 수도 있으니까요.

따라서 모든 checked/unchecked를 예외를 잡을 수 있도록 변경하고
이 과정에서 예외도 성격이 다르다고 생각하여 ControllerAdvice를 두 개로 분리했습니다.

그리고 @Order를 통해 예외가 예상하지 못한 곳에서 잡히지 않도록 변경했습니다.

이 부분 괜찮은지 한 번 봐주시면 감사하겠습니다. 😀

choijy1705 · 2023-04-18T15:39:58Z

src/main/java/racingcar/controller/WebRacingGameController.java

+
+    @PostMapping(path = "/plays")
+    public ResponseEntity<GameResultResponse> playRacingGame(
+            @Valid @RequestBody final RacingGameRequest racingGameRequest


valid 까지 좋네요!👍

choijy1705 · 2023-04-18T15:43:08Z

src/main/java/racingcar/controller/ExceptionController.java

+        return new ResponseEntity<>(
+                new ExceptionResponse(e.getMessage()),
+                HttpStatus.BAD_REQUEST
+        );


Suggested change

return new ResponseEntity<>(

new ExceptionResponse(e.getMessage()),

HttpStatus.BAD_REQUEST

);

return ResponseEntity.badRequest().body(new ExceptionResponse(e.getMessage()));

좀 더 간단하게 표현할 수 있겠네요

choijy1705 · 2023-04-18T15:44:14Z

src/main/java/racingcar/dto/ExceptionResponse.java

+
+    private final String message;
+
+    public ExceptionResponse(final String message) {


message를 받기보다 exception 자체를 받는게 더 좋지 않을까요?

감사합니다!

말씀해주신대로 Exception 객체를 가지도록 변경해보았는데요.
직렬화되는 과정에서 Exception 객체의 내부 정보가 모두 파싱되어 불필요한 정보까지 넘어가더라고요.

어떻게보면 클라이언트의 관심사는 예외 객체가 아닌 예외 객체의 메세지일 것 같은데,
영이가 말씀해주신 의도를 파악하는 데 어려움을 겪는 것 같습니다 😅

혹시 getMessage() 메소드를 내부적으로 getter에서 호출하라는 말씀이신가요?

클라이언트에만 보내는 목적이라면 그냥 string이 좋을것 같아요!

choijy1705 · 2023-04-18T15:45:25Z

src/main/java/racingcar/dto/GameRecordResponse.java

+
+import java.util.List;
+
+public class GameRecordResponse {


안쓰이는것 같아요!

choijy1705 · 2023-04-18T15:59:56Z

src/main/java/racingcar/service/RacingGameService.java

    }

-    public GameResultDto playRacingGame(final RacingGameRequestDto racingGameRequestDto) {
-        RacingGame racingGame = createRacingGame(racingGameRequestDto);
+    public GameResultResponse playRacingGame(final RacingGameRequest racingGameRequest) {


트랜잭션에 대해서도 한번 고민해보면 좋을것 같아요

choijy1705 · 2023-04-18T16:01:38Z

src/test/java/racingcar/controller/WebRacingGameControllerTest.java

+        @Test
+        @DisplayName("요청을 보낼 수 있다")
+        void shouldBeAbleWhenRequest() {
+            RestAssured.given().log().all()


log().all()을 무작정 붙이는것보다 붙였을때 이점이 있는지 고민해보면 좋을것 같아요

저도 모른 채로 사용하는 것 같아 조금 찾아보았는데요,

log()는 로그를 남기는 기능을 하며, (주로 테스트에서 디버깅으로 사용됩니다)
all() 혹은 body()를 통해 로깅의 범위를 제한할 수 있는 것 같습니다 😀

log가 필요한 시점에 사용해보면 좋을것 같습니다
이런 부분이 많아지면 애플리케이션이 커지면 테스트 시간을 길어지게 하는 원인이 될 수도 있을것 같아서요

choijy1705 · 2023-04-18T16:05:35Z

src/test/java/racingcar/controller/WebRacingGameControllerTest.java

+            Model model = new ConcurrentModel();
+            model.addAttribute("names", "브리,토미,브라운");
+            model.addAttribute("count", "3");


Suggested change

Model model = new ConcurrentModel();

model.addAttribute("names", "브리,토미,브라운");

model.addAttribute("count", "3");

RacingGameRequest request = new RacingGameRequest("브리,토미,브라운", 3);

controller에서 modelAttribute가 아닌 requestBody를 통해서 받고 있는데
테스트도 request 객체를 만들어 테스트하는게 더 좋지 않을까여?

감사합니다!

말씀해주신대로 변경하려고 해보았는데요,
다만 RacingGameRequest를 사용하는 경우 count필드가 보내지지 않거나
names 필드가 보내지지 않는 경우에 대한 테스트가 안되는 것 같습니다.

RacingGameRequest의 경우 객체이고 필드가 존재하지 않다는 것을 지정할 수 없으니까요.

이런 부분에서 Controller request에 대한 테스트는 Model로 순수하게
JSON 형식으로 보내는 게 좋다는 생각인데 영이는 이에 대해 어떻게 생각하시는지 궁금합니다!

그렇네요!
modelAttribute를 쓰는 경우는 거의 없어서
이건 조금만 더 고민해볼게요!

저도 한번 고민해보겠습니다 😀

choijy1705 · 2023-04-18T16:08:24Z

src/main/java/racingcar/service/RacingGameService.java

                mapWinnerNamesTextFrom(racingGame),
                mapCarDtosFrom(racingGame)
        );
-        save(gameResultDto, racingGameRequestDto.getCount());
-        return gameResultDto;
+        racingGameRepository.saveGameRecord(gameResultResponse, racingGameRequest.getCount());


save할때 view에 사용할 response를 넘겨주는게 조금 어색한것 같아요
response는 view로만 전달되면 좋을것 같습니다!

변경했습니다 :)

choijy1705 · 2023-04-18T16:11:25Z

src/test/java/racingcar/service/RacingGameServiceTest.java

+@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
 class RacingGameServiceTest {

+    @LocalServerPort
+    private int port;


service만 테스트하는데 port는 신경 안써도 되지 않을까여?

choijy1705

안녕하세요 테오!
피드백 반영 잘해주셨네요👍
몇가지 이야기 주고 받아보면 좋을 점들이 있어서
아직 머지하지는 않을게요
코멘트 남겼으니 확인해주세요

choijy1705 · 2023-04-21T07:20:45Z

src/main/java/racingcar/business/RacingGameService.java

+@Service
+public class RacingGameService {
+
+    private final GameRepository racingGameRepository;


컨벤션에 따라 다르겠지만 변수명도 gameRepository로 interface 명을 쓰는게 좋지 않을까요?

choijy1705 · 2023-04-21T07:21:38Z

src/main/java/racingcar/domain/CarName.java

 public class CarName {

    private static final int MAX_CAR_NAME_LENGTH = 5;

    private final String carName;

-    public CarName(String carName) {
+    public CarName(final String carName) {


final 붙이는걸 선택하신 이유가 궁금하네요

저는 개인적으로 저만의 컨벤션으로 사용하고 있습니다.

물론 협업 시에는 팀 규칙에 따라 달라지겠지만, 저는 최대한 final을 붙일 수 있는 곳이라면 붙이는 것이 좋다는 생각입니다!

메소드 입장에서 보면 파라미터의 의미가 외부로부터 전달받은 메세지에 가까운데,
이것을 굳이 재할당할 필요는 없다고 생각됩니다.

다만 final을 붙이게 되면서 코드가 필연적으로 길어지는 단점이 존재하는데요,
붙여서 얻는 이점에 비하면 미미하다고 판단해 계속 사용중입니다.

영이는 어떻게 생각하시나요?

재할당은 당연히 하면 안되는 것이고 불변이 되는게 아니라 이점이 없다고 생각합니다
final을 붙여서 얻는 이점은 별로 없다고 생각합니다. 오히려 말씀해주신 단점처럼 파라미터가 길어지는 단점이 저한테는 크게 느껴져서요

choijy1705 · 2023-04-21T07:22:56Z

src/main/java/racingcar/persistence/dao/GameResultDao.java

+            preparedStatement.setInt(2, gameResultEntity.getTrialCount());
+            return preparedStatement;
+        }, keyHolder);
+        return keyHolder.getKeyAs(Integer.class);


id는 Long으로 처리하는게 좋을것 같아요

저도 Long으로 받으려다가, 테이블의 id 필드가 INT인 것을 확인해 굳이 LONG일 필요는 없겠다고 생각했는데요,

혹시 영이가 그렇게 생각하신 이유를 여쭤봐도 될까요?

H2 데이터베이스의 자료형 범위가 Java의 Integer 범위와 다를 수도 있겠다 생각이 들어 찾아보니,

INTEGER -> 자바의 int 범위와 일치
BIGINT -> 자바의 long 범위와 일치

하는 것으로 확인했습니다!
h2 database: data scopes

int의 범위랑 long의 범위가 많이 차이나서 보통 long을 아이디로 사용하는걸로 알고 있습니다
int 범위는 조금만 규모가 커져도 쉽게 넘어버릴수 있을것 같아요

choijy1705 · 2023-04-21T07:29:04Z

src/main/java/racingcar/persistence/entity/GameResultEntity.java

+
+public class GameResultEntity {
+
+    private final int id;


wrapper class 인 Integer 혹은 Long을 사용하여
inward 의 경우 id를 null로 해주는게 어떨까요?
위에서도 언급했지만 id는 웬만하면 Long 타입이 좋을것 같아요

감사합니다!

생각해보니 Wrapper 타입으로 감싸는 경우 의미를 명확하게 전달할 수 있을 것 같습니다.
빠르게 적용하도록 하겠습니다!

choijy1705 · 2023-04-21T07:29:44Z

src/main/java/racingcar/persistence/entity/PlayerResultEntity.java

+            final int position,
+            final int gameResultId
+    ) {
+        return new PlayerResultEntity(-1, name, position, gameResultId);


마찬가지로 챙겨봐주세요

choijy1705 · 2023-04-21T07:39:53Z

src/main/java/racingcar/persistence/repository/RacingGameRepository.java

+        RacingGameConverter domainConverter = new RacingGameConverter();
+        return domainConverter.convertAll(gameResultDao.selectAll(), playerResultDao.selectAll());


conveter를 생성해서 처리하면 생성할때 gameResults, playerResults를 통해 생성하고
메서드만 호출하도록 하면 converter가 더 깔끔해지지 않을까요?

별도의 클래스에서 처리하는건 좋은것 같아요 다만 메서드명이 어떤 동작을 하는지 잘 알기 어려운것 같아요 이전에는 join이였던것 같은데 특정 도메인만을 convert하는거라면 covertToRacingGame과 같이 좀 더 구체적으로 적용해보면 어떨까요?

말씀해주신 내용 고민해보았는데요,

생성자를 통한 주입 방법은 결국 필드로 엔티티들을 가진다는 것을 의미하는데
과연 RacingGameConverter가 필드, 그리고 생성 책임이 중요한 객체인가? 를 생각해보니 그건 아니라고 생각되었습니다.

일단 RacingGameConverter는 유틸적인 성향이 매우 강하고 객체라기보다는 프로시저에 가깝기 때문에
따라서 static하게 사용할 수 있도록 변경했습니다.

이 접근방식에 대해서 영이는 어떻게 생각하시나요?
그리고 말씀해주신대로 메소드명은 변경했습니다!!

choijy1705 · 2023-04-21T07:42:52Z

src/main/java/racingcar/presentation/ClientResponseConverter.java

+        return new GameResultResponse(
+                toWinnerResponse(racingGame),
+                toCarDataResponse(racingGame)
+        );


Suggested change

return new GameResultResponse(

toWinnerResponse(racingGame),

toCarDataResponse(racingGame)

);

return new GameResultResponse(toWinnerResponse(racingGame), toCarDataResponse(racingGame));

100자를 안넘겨서 한줄로 해도 될것 같아요!

choijy1705 · 2023-04-21T07:51:40Z

src/main/java/racingcar/presentation/exception/DomainExceptionAdvice.java

+
+@Order(Ordered.HIGHEST_PRECEDENCE)
+@RestControllerAdvice
+public class DomainExceptionAdvice {


클래스를 나눈이유가 checked와 unchecked를 구분하기 위한것인가요?
지금 구조를 봤을때는 IllegalArgumentException일 경우에는 DomainExceptionAdvice의 handleException만 타고
나머지 RuntimeException은 모두 ExceptionAdvice의 unhandledException을 탈것 같네요

따로 order를 지정안해도 exception보다 IllegalArgumentException이 더 우선순위가 높아서 굳이 지정하지 않아도 될것 같습니다
(스프링은 항상 작은 집합이 더 우선순위를 가져서 더 작은 개념인 IllegalArgumentException이 우선적으로 적용)

맞습니다!

말씀하신대로 예외도 성격이 나뉘어질 수 있다고 생각해 분리하였는데요 (추후 확장성 고려)

말씀하신대로 Order가 없어도 잘 작동하는 것이 확인되었습니다.

빠르게 수정하였습니다!

choijy1705 · 2023-04-21T07:53:20Z

src/main/java/racingcar/presentation/exception/ExceptionAdvice.java

+    private final Logger logger = LoggerFactory.getLogger(getClass());
+
+    @ExceptionHandler({
+            MethodArgumentNotValidException.class


이 exception은 어느경우에 발생하나요?

Controller에서 @Valid 어노테이션이 붙은 객체가 검증 조건을 맞추지 못했을 때 반환합니다.

현재 RacingGameRequest에서 names, count 필드에 대해 각각 @NotBlank, @Positive 제약조건을 부여하고 있는데

이 조건을 맞추지 못하는 경우 던져지는 예외라고 알고 있습니다.

그리고 본문의 ExceptionHandler 메소드에서 stream을 연산을 하는 이유는 예외가 여러 개 발생할 수 있고
이를 병렬적으로 한번에 전달하기 위함입니다.

choijy1705 · 2023-04-21T07:53:57Z

src/main/resources/schema.sql

@@ -8,7 +8,7 @@ CREATE TABLE GAME_RESULT (

 CREATE TABLE PLAYER_RESULT (
    id          INT         NOT NULL AUTO_INCREMENT,
-    name        VARCHAR(50) NOT NULL,
+    name        VARCHAR(5) NOT NULL,


이렇게 하면 한글 5자도 저장이 잘되나요?

저장이 잘 되는 것 같습니다!

왜 그럴지 궁금해져서 H2 공식 문서를 찾아보았는데요, VARCHAR(CHARACTER_VARYING)에 대한 설명으로 다음과 같이 적혀 있었습니다.

The length is a size constraint; only the actual data is persisted.

따라서 5가 여기서는 5바이트가 아닌, 글자 수를 의미하는 것 같으며, 따라서 한글도 잘 저장되는 것 같습니다.

source

null 지정 가능해짐

스프링에서는 여러 ExceptionHandler가 있을 때, 가장 구체적인 ExceptionHandler를 선택함

choijy1705

안녕하세요 테오!
제가 리뷰요청온걸 놓쳤네요 죄송합니다!
피드백 잘 반영해주셨고 답변 코멘트도 남겼습니다
이번 미션은 머지할께요!

woosung1223 added 23 commits April 13, 2023 22:28

refactor: 컨트롤러를 RestController로 변경

cb28ec7

refactor: 일반 객체가 아닌 ResponseEntity로 응답하도록 변경

46cb85d

refactor: 변수명이 과도하게 의미를 내포하지 않도록 변경

77a351b

refactor: DTO 클래스명에서 보다 명확한 의미를 전달하도록 변경

c464ec5

refactor: 구현체 메소드에 Override 명시

521b739

fix: Multiple Key를 가져오던 문제 수정

6f24900

test: Service 객체의 생성 책임을 컨테이너에 위임, 더미 객체 삭제

5473e40

refactor: 테이블을 생성하는 sql 파일 네이밍 변경

930f4f2

refactor: SpringBootTest을 JDBCTest로 변경

bee4d4d

feat: 예외 핸들링 메커니즘 생성

3952d61

docs: 1단계 리팩토링 기록 작성

68e9bb4

docs: 2단계 요구사항 정리

837b87d

refactor: DAO가 하나의 테이블에 매핑되도록 변경

33e4908

feat: 플레이 이력 조회에 대한 웹 요청/응답이 가능하다.

8a8e89e

refactor: dto 패키지 이름 repository로 변경

6a8605a

chore: 기존 콘솔 애플리케이션 컨트롤러 복구

70f1128

refactor: 조인 기능을 하는 메소드를 stream을 활용하도록 변경

302c751

feat: Repository 추상화, 메모리 Repository 생성

8df1152

feat: 콘솔 어플리케이션이 기존 로직을 재사용하도록 변경

485cb34

chore: 패키지 추가 분리

42167bb

docs: 리팩토링 기록 작성

6db941c

refactor: 상수 분리

e0f0ee5

chore: 클래스명을 명확하게 변경

55ce85b

choijy1705 requested changes Apr 18, 2023

View reviewed changes

woosung1223 added 6 commits April 19, 2023 22:33

refactor: ExceptionAdvice 역할에 따라 추가 분리

87b59ca

refactor: ReponseEntity의 badRequest 메소드를 이용하도록 변경

23f75ca

chore: 패키지 구조 수정, entity를 persistence 하위로 변경

8019329

chore: 메소드명 수정

757fd4d

feat: @Valid를 통해 검출된 예외도 메세지를 반환하도록 수정

59cadb9

feat: Service의 메소드를 Transactional하게 변경

cfd87a6

woosung1223 added 20 commits April 20, 2023 15:32

refactor: Service에서 DTO가 아닌 도메인 객체를 반환하도록 수정

82b70a5

test: Repository Mock을 통한 테스트 개선

1a6fea9

chore: 사용되지 않는 메소드 제거

ab46881

docs: 리팩토링 기록 작성

326c302

feat: 예외처리 시 로깅 기능 추가

e81d24e

refactor: 쿼리를 매번 for문에서 생성하지 않도록 변경

66696f6

refactor: key를 Long으로 형변환할 때, getKeyAs 메소드를 사용하도록 변경

99b88b9

chore: 메소드명, 클래스명 수정

e21cc96

chore: DTO 패키지를 Presentation Layer로 이동

e4f1550

chore: Layered Architecture에 따라 패키지 이름 구성

dd5bd9e

chore: Layered Architecture에 따라 패키지 변경

e14cafb

chore: 도메인 제약조건에 맞게 스키마 재설계

67e9bb6

refactor: 도메인 코드 개선

9e6a397

chore: 메소드명 변경

c0c646d

refactor: 키를 Long이 아닌 Integer로 받도록 변경

a2169ca

chore: 커스텀 예외 제거에 따른 ExceptionHandler 변경

46ce961

fix: 컬럼 사이즈가 작아 우승자를 담지 못하는 문제 해결

7a47c31

fix: 게임 이력이 제대로 복원되지 않던 문제 해결

4786eae

test: 도메인 구조 수정에 따른 테스트 변경

895adfe

docs: 고민사항 및 리팩토링 기록 작성

87630d1

choijy1705 requested changes Apr 21, 2023

View reviewed changes

woosung1223 added 7 commits April 22, 2023 12:29

chore: 변수명 변경

a3ea6ea

refactor: 엔티티의 속성을 모두 Wrapper 타입으로 변경

d75c338

null 지정 가능해짐

refactor: 메소드 분리, 가독성 개선

27f3ed1

refactor: 불필요한 ControllerAdvice Order 지정 삭제

dab6b54

스프링에서는 여러 ExceptionHandler가 있을 때, 가장 구체적인 ExceptionHandler를 선택함

refactor: 프로시저의 성격을 가지는 객체를 static하게 변경, 메소드명 변경

be19af0

chore: 불필요한 개행 삭제

b6ca2b6

docs: 고민사항 및 리팩토링 기록 작성

b5e4c65

choijy1705 approved these changes Apr 26, 2023

View reviewed changes

choijy1705 merged commit 5e66441 into woowacourse:woosung1223 Apr 26, 2023


		private final String message;

		public ExceptionResponse(final String message) {

		RacingGameConverter domainConverter = new RacingGameConverter();
		return domainConverter.convertAll(gameResultDao.selectAll(), playerResultDao.selectAll());

[2단계 - 웹 자동차 경주] 테오(최우성) 미션 제출합니다. #133

[2단계 - 웹 자동차 경주] 테오(최우성) 미션 제출합니다. #133

Conversation

woosung1223 commented Apr 18, 2023

choijy1705 left a comment • 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

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

choijy1705 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

woosung1223 Apr 22, 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

woosung1223 Apr 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

choijy1705 left a comment

Choose a reason for hiding this comment

choijy1705 left a comment •

edited

Loading

woosung1223 Apr 22, 2023 •

edited

Loading

woosung1223 Apr 22, 2023 •

edited

Loading