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

[2단계 - 웹 자동차 경주] 성하(김성훈) 미션 제출합니다. #134

Merged
merged 41 commits into from
Apr 23, 2023

Conversation

sh111-coder
Copy link

@sh111-coder sh111-coder commented Apr 18, 2023

안녕하세요 토니!!! 성하입니다! 😃

1단계 때 리뷰 너무 잘해주셔서 감사했습니다!!

2단계도 잘부탁드려요 ㅎㅎ 🙇🏻‍♂️


✅ 2단계 시작 전 리팩토링

2단계 구현을 진행하기 전에 1단계에서 부족했던 부분을 리팩토링했습니다.
리팩토링한 부분은 다음과 같습니다!

1. RaceResultService
2. Mapper 로직 -> DTO 안에서 정적 팩토리 메소드로 처리하면서 Mapper 삭제
3. Controller, DAO, Service 테스트 추
4. @Valid

✅ 2단계 구현 질문

Q1. 반환 시 List VS DTO

지금 현재는 index.html 구조에 맞춰서 GetMapping("/plays")의 반환을
'List로 직접 List로 반환하고 있는데,
설계 단계에서 List로 직접 반환할지,
List를 가지는 하나의 객체를 만들어서 해당 객체를 반환할지 고민이 들었습니다.

일반적으로는 어떤 방향으로 하는 것이 좋은지? 좋다, 안 좋다의 문제가 아니면
토니는 어떤 방식을 선호하시는지 궁금합니다!


Q2. 2단계 요구사항 질문

console application과 web application의 중복 코드를 제거합니다.
두 application은 입출력과 데이터 저장 방식을 제외하고는 내부 비즈니스 로직은 동일해졌습니다.
두 application의 비즈니스 로직은 새로운 객체를 도출 하여 중복 제거를 할 수 있습니다.

이러한 요구사항에서
두 application의 비즈니스 로직을 지금 구조에서 새로운 객체로 어떻게 중복 제거를 할 수 있을지
도저히 떠오르지가 않아서 중복 제거는 하지 못하고 제출하게 되었습니다 ㅠㅠ

중복 제거라고 해서 도메인 객체인 RacingCars와 NumberGenerator를 가지는 추상 클래스를 구현해서
해당 추상 클래스를 Console Application과 Web Application이 상속해서 중복을 제거하려고 했었는데,
RacingCars를 주입할 수 없어서 오류가 발생했었습니다,,
어떻게 중복제거를 할 수 있을까요??...

Copy link

@toneyparky toneyparky left a comment

Choose a reason for hiding this comment

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

안녕하세요 성하~ 토니입니다!

요청주신 리뷰에 대해 코멘트 남겼어요. 저도 조금 더 생각해볼 부분이 있어서 두번째 질문은 추가로 코멘트 남길게요. 우선은 다른 부분에 대해 남긴 내용을 확인 및 반영해주시면 좋겠습니다!

1단계에서 넘어온 리팩터링 내용도 좋네요 👍 이번 단계도 화이팅!

import racingcar.view.InputView;
import racingcar.view.OutputView;

import java.util.*;
import java.util.stream.Collectors;

public class RacingCarController {

Choose a reason for hiding this comment

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

콘솔 레이싱 게임을 실행하는 Application은 어디에 있을까요~? 요구사항을 보면 콘솔과 웹 모두를 실행할 수 있도록 제공해야할 것 같아요.

추가로 사용하지 않는 import문이 조금 있네요~

Copy link
Author

Choose a reason for hiding this comment

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

앗 두 부분 다 수정했습니다!! Console Application 만드는 걸 깜빡했네요!!
꼼꼼히 리뷰해주셔서 감사합니다! 🙇🏻‍♂️

gameOptionValidator.validateGameOption(gameInfoRequest);
public RaceResultResponse registerRaceResult(@Valid @RequestBody final GameInfoRequest gameInfoRequest) {

Choose a reason for hiding this comment

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

기존 직접 구현한 validator에 비해 @Valid를 사용하니 어떤 장/단점이 있으셨나요~?

Copy link
Author

Choose a reason for hiding this comment

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

제가 @Valid를 사용하면서 느낀 장단점을 적어보겠습니다!!

✅ 장점

  1. 코드가 간단해진다.
    자바 코드로 검증할 때보다 훨씬 코드가 간단해져서 가독성이 좋아진다고 느꼈습니다.
    if문과 throw new로 예외를 발생시키는 코드를 간단하게 어노테이션 네이밍 및 속성만으로도 표현이 가능해서 가독성이 좋아졌다고 느꼈습니다.

  2. 검증 객체 대신 DTO에 직접 검증 로직이 들어간다.
    이전 검증 객체를 사용했을 때는 Controller -> Validator(검증) -> RequestDto 흐름으로 이어졌는데
    @Valid를 사용하니 흐름이 Controller -> RequestDto(검증)이 되어서 검증 책임이 검증 대상 객체에 들어간 느낌이라 좋은 것 같습니다!

제가 생각한 것 이외의 다른 장점이 있는지 궁금합니다!

✅ 단점

  1. 스프링에 종속적인 검증이 된다.
    이 부분이 단점이 될지는 모르겠는데 자바의 검증 책임을 프레임워크에 위임한다고 생각해서
    프레임워크에 종속적인 코드가 되는 것 같습니다!

  2. 추가 설정을 해줘야한다.
    'org.springframework.boot:spring-boot-starter-validation' 의존성을 추가해야하는 작업이 추가로 든다는 것이
    단점이라고 생각했습니다.

Choose a reason for hiding this comment

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

말씀하신 장점이 맞다고 생각해요 👍
그리고 이 장점은 단점을 커버할 정도로 유용하구요. 잘 정리해주셨네요! 굳굳

int savedRaceResultId = raceService.saveRaceResult(gameInfoRequest);
return raceService.createRaceResult(savedRaceResultId);
}

@GetMapping("/plays")
public List<RaceResultResponse> searchAllRaceResult() {

Choose a reason for hiding this comment

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

Q1. 반환 시 List VS DTO

첫번째 질문에 대해 이야기해보죠! API를 설계할 때에 보통 프론트 개발자분들과 소통하여 작업해요. 그렇기에 둘간의 컨벤션을 정할 수 있다고 생각해요. 개인적인 선호로는 특별한 이유가 없다면 List<DTO>를 사용해요. 이유는 한번 더 래핑하면 프론트에서 한꺼풀 더 까서 사용해야하는 귀찮음(?)이 있기 때문이에요.

핵심은 협업자들간에 소통을 하여 일관된 규칙만 정하면 된다고 생각합니다!

Copy link
Author

Choose a reason for hiding this comment

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

그렇군요!!
처음에는 List가 있었을 때
일급컬렉션으로 감싸서 행위를 관리하는 한 곳에서 관리하는 느낌을 주고 싶었는데
생각해보니 비즈니스 로직이 없는 DTO 계층을 일급컬렉션으로 하는 의미가 없을 것 같네요!!

Choose a reason for hiding this comment

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

네 일급컬렉션으로 감싸는 데에는 그만한 이유가 있어야겠죠. 성하가 말씀해주신 비즈니스 로직이 그 이유가 될 수 있겠구요!


private static List<CarStatusResponse> mapToCarStatus(final List<Car> cars) {
return cars.stream()
.map(car -> new CarStatusResponse(car.getName(),

Choose a reason for hiding this comment

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

RaceResultResponse를 생성할 때에는 정적 팩터리 메서드, CarStatusResponse를 만들어 줄 때에는 생성자를 사용하셨네요..! 성하는 정적 팩터리 메서드를 사용하실 때와 생성자를 사용하실 때의 기준이 있으실까요?

Copy link
Author

Choose a reason for hiding this comment

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

주 생성자에는 단순한 초기화 로직만 들어가 있는 것이 생성자의 책임을 수행하는 것이라고 생각해서
생성자에 RaceResultResponse에서 변환 로직 등 초기화 로직 이외의 작업이 수행되면 해당 작업은
정적 팩토리 메소드에서 진행하고 주 생성자는 초기화 로직만 수행하게 하고 있습니다!!

Choose a reason for hiding this comment

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

좋네요. 성하의 기준을 공유해주셔서 감사해요. 저도 정적 팩터리 메서드는 이를 통해 내부에서 추가적인 로직을 수행한다거나 메서드의 이름을 통해 컨텍스트를 더 부여할 때에 사용한답니다.


import java.time.LocalDateTime;

public class RaceResultEntity {

Choose a reason for hiding this comment

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

해당 객체는 사용되지 않군요..! 어떤 이유로 추가되었나요?

Copy link
Author

Choose a reason for hiding this comment

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

아 사용하지 않는 객체를 지운다는 게 깜빡했네요!

원래는 DAO 단에서 메소드를 추가해서 파라미터나 반환으로 DTO를 추가해야하는 상황으로 설계를 했었어요!
그러다보니 DAO 단에서 DTO를 사용할 때 DAO에 메소드가 추가될 때마다 DTO도 새롭게 만들어줘야하는 것이 단점으로 느껴져서
아예 DB Table과 1:1로 맵핑되는 Entity로 생성해서 하나의 클래스로 전달 기능을 수행할 수 있게 하려고 했었습니다!

그런데 실제 구현 시에 DTO(Entity)를 사용하는 메소드가 없었어서 Entity를 사용하지 않았던 것 같아요!
삭제하겠습니다!!

Choose a reason for hiding this comment

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

DAO의 데이터와 매핑되는 영속성 객체를 만들어주셨군요. 하지만 처리해주신 것 처럼 사용하지 않는 경우는 제거하는 편이 혼란을 덜 야기시킬듯하네요. 굳


@AutoConfigureTestDatabase
@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
class RacingCarWebControllerTest {

Choose a reason for hiding this comment

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

컨트롤러 테스트에서 성하는 어떤 부분을 검증하고 싶으셨나요?

Copy link
Author

Choose a reason for hiding this comment

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

HTTP 요청이 수행돼서 실제 응답 바디에 값이 담기는지를 보고 싶었어요!

Choose a reason for hiding this comment

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

요 테스트는 통합 테스트의 개념으로 작성해주신걸까요? 그렇다면 학습 차원에서 슬라이스 테스트도 작성해보면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

기존 테스트 코드는 IntegrationTest으로 네이밍을 변경하고
슬라이스 테스트 클래스를 추가하는 방식으로 구현했습니다!

✅ 통합 테스트 VS 슬라이스 테스트

1. 통합 테스트

먼저 통합 테스트는 프로덕션 코드에 등록되어 있는 모든 스프링 빈을 테스트 시에 가져오기 때문에
모든 계층의 동작을 정확하게 테스트 할 수 있다는 장점이 있지만,
테스트 속도가 느려서 테스트 시 빠른 피드백을 받는다는 장점이 사라지는 단점이 있었습니다!

2. 슬라이스 테스트

슬라이스 테스트는 모든 계층의 동작을 정확하게 테스트할 수는 없지만
하나의 계층을 빠르게 테스트하기에 좋다는 장점이 존재하여
비즈니스 로직은 알 필요 없이 요청을 전달하고 응답을 반환받는 컨트롤러 레이어에 적합하다는 생각이 들었습니다!

Q : 현업에서는 통합 테스트와 슬라이스 테스트를 어떤 기준으로 나누어서 작성하는지 궁금합니다!!

Choose a reason for hiding this comment

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

굳굳 두가지 테스트의 차이를 잘 파악해주셨네요. 통합 테스트는 계층이나 다른 객체와의 동작을 테스트한다고 봐주시면 될 것 같습니다.

Q : 현업에서는 통합 테스트와 슬라이스 테스트를 어떤 기준으로 나누어서 작성하는지 궁금합니다!!

두 테스트의 목적이 다르므로 각 목적을 이루기 위해서 작성하겠죠? 일반적으로 통합 테스트는 테스트를 실행하는 시간이 길고 세팅해줘야할 게 많죠. 그래서 저희 팀에서도 잘 진행하지 않고 있어요. 대신 슬라이스 테스트를 꼼꼼하게 작성하려해요.

Comment on lines 15 to 17
@SpringBootTest
@Transactional
class CarDaoTest {

Choose a reason for hiding this comment

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

@SpringBootTest@JdbcTest의 차이에 대해 알고 계신가요?

Copy link
Author

@sh111-coder sh111-coder Apr 19, 2023

Choose a reason for hiding this comment

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

@SpringBootTest는 많이 들어봤는데 @jdbcTest는 이번에 처음 들어본 것 같아요!

@SpringBootTest는 스프링 컨테이너에 등록된 모든 빈을 등록하기 때문에 비용이 상당히 큰 테스트입니다!
그래서 모든 계층을 통합적으로 테스트할 때 많이 사용을 한다고 합니다!

부분적인 계층별로 테스트할 때는 '슬라이스 테스트'를 진행한다고 배웠습니다!
현재 DAO 계층에서는 Jdbc를 사용하므로 @jdbcTest 슬라이스 테스트를 하는 것이 더 좋아보이는 것 같은데
직접 해보니 CarDao가 주입이 안되더라고요!
그래서 @MockBean을 사용해서 CarDao를 주입했는데, 정상적으로 save가 작동하지 않았어요 ㅠㅠ 😭
그래서 일단은 @SpringBootTest로 놔두게 되었습니다!

추가로,
@jdbcTest를 들어가봤더니 @transactional이 내장되어 있는 것을 알 수 있었습니다!
DataSource, JdbcTemplate, EmbeddedDatabase 등의 Bean들도 자동으로 등록이 되어 사용이 가능하다고 배웠습니다!

Choose a reason for hiding this comment

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

잘 정리해주셨네요! 테스트에 대한 적절한 기술을 사용해야해요. 테스트를 구동하는 데에 시간이 오래 걸린다거나 불편한 부분이 많다면 테스트를 많이 돌리지 않게 되고 이로인해 죽은 테스트가 되기 때문이죠. 어노테이션의 내부까지 들어가서 확인해주신거 좋네요 👍

@MockBean을 사용해서 CarDao를 주입했는데, 정상적으로 save가 작동하지 않았어요 ㅠㅠ

이건 왜 작동하지 않았던걸까요? save에 대한 모킹도 해주셨을까요?

Comment on lines 23 to 24
@Autowired
private TruncateUtil truncateUtil;

Choose a reason for hiding this comment

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

기존 트랜잭셔널을 활용하는 방식에서 DB를 직접 날려주는 방식으로 수정해주신 이유가 궁금해요 😀

Copy link
Author

Choose a reason for hiding this comment

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

@SpringBootTest에 대해서 공부하다가, webEnviroment 속성에 대해 공부하게 됐어요!

학습 테스트에서 컨트롤러 테스트에
@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) 해당 코드를 붙여서
스프링 부트 테스트 환경을 RANDOM_PORT로 설정해줘서 따라하게 되었는데
RANDOM_PORT일 때는 @transactional이 작동하지 않는다고 하더라고요!

그 이유가 RANDOM_PORT 환경으로 실행하게 되면 내장 톰캣을 사용해서
내장 톰캣이 실행되는 스레드와 테스트가 실행되는 스레드가 다르기 때문에 트랜잭션을 공유할 수 없어서
@transactional이 작동하지 않는다고 합니다!

따라서 수동으로 매번 작업을 Truncate해주기 위해 TruncateUtil을 만들고 truncate 해주게 되었습니다!

Choose a reason for hiding this comment

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

세부적인 내용까지 잘 적어주셔서 좋네요~! 동작방식을 이해하고 계시군요! 유틸 클래스의 이름도 명확해서 이해하기 좋아요.

import racingcar.controller.dto.RaceResultResponse;

@SpringBootTest
class RaceServiceTest {

Choose a reason for hiding this comment

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

커밋을 확인해보니 TDD로 작업을 진행해주셨네요 💯

import racingcar.dao.raceresult.dto.RaceResultRegisterRequest;
import racingcar.domain.Car;
import racingcar.domain.RacingCars;
import racingcar.service.mapper.RaceResultMapper;

@Service
public class RaceService {

Choose a reason for hiding this comment

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

Q2. 2단계 요구사항 질문

두 번째 질문에 대해서는 조금 더 생각해보고 리뷰 나 DM 드릴게요!

Choose a reason for hiding this comment

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

DM으로 조언드린 구조도를 그려보고 리팩터링을 잘 적용해주셨군요!

해당 클래스를 추상 클래스로 만들었다가 다시 그냥 클래스로 바꾸신 이유는 뭘까요?

Copy link
Author

Choose a reason for hiding this comment

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

처음엔 관성적으로 추상 클래스로 만들어서 중복을 제거해야겠다고 생각해서 추상 클래스로 만들었었습니다.

그런데 만들고 보니 추상 메소드가 없는 추상 클래스가 이상하다고 생각해서 구체 클래스로 만들게 되었습니다.

그러나 지금 생각해보니 추상 클래스를 만들었을 때 인스턴스화가 되지 않는다는 장점이 있었습니다.
현재 RaceService는 인스턴스화가 필요 없는 클래스이기 때문에 추상 클래스가 어울린다고 생각합니다!!

그래서 다시 추상 클래스로 리팩토링하겠습니다!
그런데, 추상 클래스에 추상 메소드가 1개도 없어도 괜찮은 건지 궁금합니다!!

Choose a reason for hiding this comment

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

추상 클래스를 만들었다는건 이를 구현하는 클래스는 추상 클래스가 제공하는 시그니쳐를 따른다는걸 의미해요. 지금은 추상 메서드가 없어서 따로 구현할 게 없지만 디폴트 메서드가 있고 하위 클래스는 이를 이용할 수 있죠. 추상 메서드는 추후에 추가될 수도 있는거구요. 추상 메서드의 개수가 추상 클래스를 정하는 기준이 되기보다는, 코드 상에서 인스턴스화를 막고 시그니쳐를 제공하는 클래스임을 드러내는 추상 클래스 사용이 낫다 생각해요.

* CarService, RaceResultService 삭제
-> NumberGenerator를 WebRaceService로 위임
-> CarDao, RaceResultDao는 CarRepository, RaceResultRepository로 위임
자동차 move 기능이 Web과 Console에서 중복되어 추상 클래스에서 선언함으로써 중복 제거
@sh111-coder
Copy link
Author

sh111-coder commented Apr 19, 2023

안녕하세요 토니! 이번 2차 리팩토링은 이전에 하지 못했던
'Console & Web의 비즈니스 로직 중복 제거'에 초점을 맞춰 진행했습니다!

리팩토링한 전체 구조는 다음과 같습니다!
구조 계층

1. CarService, RaceResultService 삭제

-> NumberGenerator를 WebRaceService로 위임
-> CarDao, RaceResultDao는 CarRepository, RaceResultRepository로 위임

2. Web, Console Service가 RaceService를 상속하여 중복 제거

자동차 race 기능이 Web과 Console에서 중복되어 슈퍼 클래스에서 선언함으로써 중복을 제거했습니다!

import racingcar.dao.raceresult.dto.RaceResultRegisterRequest;
import racingcar.domain.Car;
import racingcar.domain.RacingCars;
import racingcar.service.mapper.RaceResultMapper;

@Service
public class RaceService {

Choose a reason for hiding this comment

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

DM으로 조언드린 구조도를 그려보고 리팩터링을 잘 적용해주셨군요!

해당 클래스를 추상 클래스로 만들었다가 다시 그냥 클래스로 바꾸신 이유는 뭘까요?


@AutoConfigureTestDatabase
@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
class WebConsoleRacingCarControllerTest {

Choose a reason for hiding this comment

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

이 테스트의 이름이 적절한가요?

Copy link
Author

Choose a reason for hiding this comment

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

앗.. 잘못 작성한 것 같아요!! WebRacingCarControllerTest로 수정했습니다!!

Comment on lines 63 to 71
String names1 = "성하,이오,코코닥";
RacingCars racingCars1 = RacingCars.makeCars(names1);
racingCars1.moveAllCars(5, new RandomNumberGenerator());
RaceResultRegisterRequest raceResultRegisterRequest1 = RaceResultRegisterRequest.create(5, racingCars1);

String names2 = "성하,이오,코코닥";
RacingCars racingCars2 = RacingCars.makeCars(names2);
racingCars1.moveAllCars(5, new RandomNumberGenerator());
RaceResultRegisterRequest raceResultRegisterRequest2 = RaceResultRegisterRequest.create(5, racingCars2);

Choose a reason for hiding this comment

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

테스트 픽스쳐라는 개념을 학습하고 테스트를 위해 값을 준비하는 단계를 추상화해볼 수 있겠어요.

Copy link
Author

@sh111-coder sh111-coder Apr 21, 2023

Choose a reason for hiding this comment

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

@BeforeEach를 통해 중복을 제거했습니다!!

Choose a reason for hiding this comment

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

좋네요~!!

import racingcar.domain.RacingCars;

@Repository
public class CarRepository {

Choose a reason for hiding this comment

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

레포지토리에 대한 테스트는 없어도 괜찮을까요?

Copy link
Author

@sh111-coder sh111-coder Apr 21, 2023

Choose a reason for hiding this comment

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

현재는 단순히 Dao의 로직만 호출하고 있기 때문에 Dao 테스트에서 충분히 테스트하고 있다고 생각해서
테스트를 만들지 않았습니다.

레벨 1에서도 네오의 강의 피드백에서 이와 비슷한 질문의 답변을 들었습니다!

중복(위임) 테스트의 장단점
테스트 코드가 많다는 것은 이해하기 어렵다 & 유지 보수 비용이 늘어난다.
하지만 위임 테스트를 하지 않으면 A -> B 위임 테스트일 때 B가 수정됐을 때 단위 테스트가 존재하지 않기 때문에 불안정하다.

이런 식으로 답변해주셨던 것 같아요!

저는 아직까지는 중복으로 테스트를 작성하는 리소스 낭비가 크다고 생각해서 하지 않을 것 같은데
지금은 학습 목적으로 리팩토링 할 생각입니다!

이와 관련해서 현업에서는 어떤지, 토니의 생각도 궁금합니다!!

Choose a reason for hiding this comment

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

좋은 포인트네요! 저도 모든 테스트를 다 해야한다고 생각하지 않아요. 특히 현업에서는 코드의 양이 방대하므로 필요 없는 테스트 코드는 코드 유지보수에 부정적인 영향을 끼칩니다. 다만 비즈니스적으로 의미있는 경우나 복잡한 로직에 대해서는 테스트가 필수적이라 생각해요. 지금의 경우에는 굳이 작성하지 않아도 되지만 학습차원에서 작성을 권유드리고 싶었는데 추가해주시는군요 굳굳

gameOptionValidator.validateGameOption(gameInfoRequest);
public RaceResultResponse registerRaceResult(@Valid @RequestBody final GameInfoRequest gameInfoRequest) {

Choose a reason for hiding this comment

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

말씀하신 장점이 맞다고 생각해요 👍
그리고 이 장점은 단점을 커버할 정도로 유용하구요. 잘 정리해주셨네요! 굳굳

Copy link

@toneyparky toneyparky left a comment

Choose a reason for hiding this comment

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

안녕하세요. 리뷰가 조금 늦었네요.

수정해주신 내용 확인했어요. 적절하게 중복이 제거 되었군요!
기존 리뷰의 답변과 추가 리뷰 확인 부탁드려요 😊

@sh111-coder
Copy link
Author

안녕하세요 토니!! 성하입니다! 😃

이번 주요 리팩토링은 다음과 같습니다!

* 모든 오류 처리하는 핸들러 에러 코드 500 변경 & 오류 메시지 수정
* 구체 클래스 -> 추상 클래스로 리팩토링
* 통합 테스트 -> 슬라이스 테스트로 리팩토링

리팩토링 시에 질문이 생겨서 질문 남깁니다!!

Q : Repository 계층을 추가한 이유 & Repository 계층에서 도메인을 받아도 되는가?

원래 기존 구조는 Service -> Dao로 전달되는 구조였습니다.
해당 구조에서 서비스에서 DB에 전달될 객체(DTO, Entity 등)을 생성하여 전달하는 것이
비즈니스 로직을 수행하는 서비스의 책임에서 벗어난다고 생각하여
Repository 계층을 추가해서 Service에서 Repository로 도메인을 넘기고,
DB에 전달될 객체는 Repository에서 생성해서 Dao에게 전달하는 구조를 만들기 위해
Repository를 추가했습니다!
결과적으로 Service는 DB단 로직과 한 계층 더 멀어져서 비즈니스 로직에 집중할 수 있는 느낌이 들었습니다!
제가 생각한 Repository 계층을 추가하는 이유가 타당한지 궁금합니다!!

�이 경우에 우려되는 점은
Repository에 바로 도메인을 전달하면 Repository에서 도메인이 오염될 우려가 있어보였습니다!
하지만 토니가 위의 Service, Repository, Dao 코멘트에도 달아주셨듯이
Repository는 도메인을 다루기 때문에 상관이 없는 걸까요?
Repository가 도메인을 다룬다는 말을 정확히 이해를 못해서 찝찝한 것 같습니다!!

이러한 부분을 토니는 어떻게 생각하시는지 궁금합니다!

Copy link

@toneyparky toneyparky left a comment

Choose a reason for hiding this comment

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

안녕하세요 성하! 토니에요~!

반영해주신 내용 잘 확인했어요! 잘 해주셨습니다! 항상 좋은 질문을 주셔서 저도 즐겁게 답변했어요 🙂
이번 미션 이제 마무리하겠습니다! 고생 많으셨어요!

@@ -5,6 +5,7 @@
import org.springframework.stereotype.Repository;
import racingcar.dao.raceresult.RaceResultDao;
import racingcar.dao.raceresult.dto.RaceResultRegisterRequest;
import racingcar.domain.RacingCars;

Choose a reason for hiding this comment

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

Q : Repository 계층을 추가한 이유 & Repository 계층에서 도메인을 받아도 되는가?

계층을 추가하신 이유는 적절해요 굳굳

레포지토리는 도메인을 다룰 수 있는 영역이기에 의존이 생겨도 된다 생각해요.

@toneyparky toneyparky merged commit ca5a3e3 into woowacourse:sh111-coder Apr 23, 2023
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.

None yet

2 participants