[Review] ATDD 1주차 2차 PR

송용주 리뷰어 님께 감사의 말을 전하고 싶다!!

 

1. 질문

1-1) Dto -> Domain 변환의 장소는?

조금 인수테스트 와는 상관없는 Production Code에 대한 부분이기는 한데...
저의 Line 이라는 Entity 안에는 다음과 같이 edit 이라는 메서드가 구현되어 있습니다.

public void edit(Line line) {
    if (line.getName() != null && !line.getName().isBlank()) {
        name = line.getName();
    }
    if (line.getColor() != null && !line.getColor().isBlank()) {
        color = line.getColor();
    }
    if (line.getUpStationId() != null) {
        upStationId = line.getUpStationId();
    }
    if (line.getDownStationId() != null) {
        downStationId = line.getDownStationId();
    }
    if (line.getDistance() != null) {
        distance = line.getDistance();
    }
}

호출하는 쪽 에서는 다음과 같이 사용합니다.

public void editLine(Long lineId, LineRequest request) {
    Line findLine = lineRepository.findById(lineId)
            .orElseThrow(NoSuchElementException::new);
    findLine.edit(request.toDomain());
}

Line 도메인 자체에 DTO에 대한 의존성을 없에버리고 싶어, 원래는 인자를 하나하나 꺼내어, edit 메서드에 5개의 인자를 전달하였지만,
별로라 생각되어 DTO를 domain으로 변환 한 후, 이를 인자로 전달하고 있습니다.

따라서 Line 도메인 자체에서는 의도한 것 처럼 DTO에 의존하지 않게 됬으니 성공이라 생각은 하지만....?

이걸 DTO -> domain으로 어거지로 한번 더 변환한다? 라는 생각이 들었습니다.

리뷰어 님이라면 어떻게 처리하셨을지? 의견이 궁금합니다!!

 

▶ 답변 : 

약간 나의 생각을 적용시켜 보면, 

domain -> dto 변환의 메서드를 domain안에 만들어 두면 안된다 생각한다. domain이 dto에 의존해버리기 때문이다.

dto -> domain 에 대한 변환같은 경우 dto 내부에 만들어지는 메서드 이니 상관없지 않을까?

비지니스 로직과 관련된 것 들이 포함될 수 있다는 의미가 아직은 다가오지 않는다?

 

다른 사람들도 이에 대한 궁금증이 있었나 보다! 핵심은!

 

도메인 객체의 생성에 대한 책임을 어느 객체가 가질 것인가?

 

에 대한 이야기로 생각된다.

1. DTO
   객체 생성에 대한 책임을 DTO 로 준 경우인데요. 조졸두 님 책에 수록되어 있기도 해서 많이 쓰시는 것 같습니다.
   장점은 자주 변경되는 DTO 객체에서 변환을 담당하여 변경 시 수용이 편하다는 점이 있다고 생각합니다.(entity 를 만드는 방법 등을 쉽게 변경할 수 있음)
   단점으로는 요청이 항상 특정 Entity 에 강하게 결합될 가능성이 있을 것 같습니다.
2. Service
   서비스 레이어에서만 Entity 를 감싸고 있기 때문에, 안정적이라고 생각됩니다.
   다만, 객체 생성 로직이 복잡해질수록 서비스 레이어가 지저분해질 가능성이 있다고 생각합니다.
3. Factory
   객체 생성을 위해 따로 Factory 를 구축하는 방법인데, 직접 구현할 수도 있고, ModelMapper 나 MapStruct 등을 사용할 수 있습니다.
   간단하게 객체를 생성할 때는 과하다고 느껴질 수 있습니다.

 

1-2) 상태코드 반환의 방식

CQRS를 따르기 위해 command 성의 delete 메서드에서 딱히 뭔가를 반환할필요는 없다 생각하였습니다만,
적합한 상태코드를 반환하기 위해 명시적으로 고정된 @ResponseStatus 정도로 상태코드를 설정해주면 될까요?

이 방식은 처음 사용해보는 방식이라 신기했다. Void인데도 반환을 하는것이 신기했다!

코드의 통일성 또한 중요하기 때문에 리뷰어가 추천해주신 방식으로 일반화 시켰다!

 

2. 리뷰 정리

2-1) 호출할일 없는 생성자의 접근제어자를 protected로

JPA는 Entity 상에 기본생성자가 필요한데, 접근제어자로 protected까지는 허용해준다.

Private로 아예 막으면 더 좋겠지만, 기술적 한계로 protected를 사용해야 한다.

 

2-2) assertAll 의 사용

기존의 assertThat은 위 경우 43번에서 실패할 경우, 44, 45 문단은 확인하지 않게 된다.

이러한 단점을 극복한 assertAll을 사용하도록 변경하였다.

 

2-3) containsExactly 의 적용

원래 containsExactly가 원소들의 순서까지 동일해야 함은 알고 있었다.

하지만 이번 기회를 통해 중복 또한 되면 안되는 것을 알게되었다.

 

containsExactly 는 원소가 정확히 일치해야 합니다. 중복된 값이 있어도 안되고 순서가 달라져도 안됩니다.

 

2-4) 수정후 추가 검증

노선의 이름과 색상을 변경한 이후, 상태코드만 검증하고 있었다.

추가로 실제 색상이 변경되는지 API를 통해 확인해보도록 추가하였다.

/**
 * Given 지하철 노선을 생성하고
 * When 생성한 지하철 노선을 수정하면
 * Then 해당 지하철 노선 정보는 수정된다
 */
@Test
@DisplayName("단일 지하철 노선 편집")
public void editLine() {
    // given
    long lineId = 지하철_노선_생성("2호선", "bg-green-600").jsonPath().getLong("id");

    // when
    int editStatusCode = 지하철_노선_수정(lineId, "다른 2호선", "연두색");

    // then
    assertThat(editStatusCode).isEqualTo(HttpStatus.OK.value());

    // 추가된 부분
    JsonPath responseBody = 지하철_단일_노선_조회(lineId).jsonPath();
    assertAll(
            () -> assertThat(responseBody.getString("name")).isEqualTo("다른 2호선"),
            () -> assertThat(responseBody.getString("color")).isEqualTo("연두색")
    );

}

 

2-5) 중복 부분에 대한 리팩토링

이에 대하여 어떻게 처리할까 하다가... 메서드 오버로딩을 통해 인자를 2개만 받는 메서드를 만들었다.

public static ExtractableResponse<Response> 지하철_노선_생성(String lineName, String color, Long upStationId, Long downStationId, Integer distance) {
    HashMap<String, String> requestParam = new HashMap<>();
    requestParam.put("name", lineName);
    requestParam.put("color", color);
    requestParam.put("upStationId", upStationId.toString());
    requestParam.put("downStationId", downStationId.toString());
    requestParam.put("distance", distance.toString());

    return RestAssured
            .given().log().all()
            .contentType(MediaType.APPLICATION_JSON_VALUE)
            .body(requestParam)
            .when().post("/lines")
            .then().log().all()
            .extract();
}

public static ExtractableResponse<Response> 지하철_노선_생성(String lineName, String color) {
    Long upStationId = 지하철역_생성("강남역").jsonPath().getLong("id");
    Long downStationId = 지하철역_생성("건대입구역").jsonPath().getLong("id");
    return 지하철_노선_생성(lineName, color, upStationId, downStationId, 10);
}

다만 이 방식이 적합한 방식이였을까?

너무 특정 역(강남역, 건대입구역)에 의존적인 메서드가 아닐까? 란 생각이 들었다. 

 

 

리뷰어의 의견처럼 아예 특정 호선을 만드는 메서드를 추가적으로 작성하였다.

이럴 경우 메서드 안에 특정 호선에 의존적인 TestFixture를 작성할수도 있다.

 

2-6) JsonPath 의 getLong은 Primitive type을 반환한다.

처음알게된 내용이다. getLong()이길레 당연히 객체형 Long을 반환하는 줄 알았다.

리뷰어의 추천대로 isEqualTo 로 변경하였다.