1,2 단계 - 지하철역/노선 관리
리뷰어: 코니
고민한 내용
도메인과 Entity
이번 미션을 하며 궁금했던 점은 처음 레거시 코드를 받았을 때 Line 과 Station이 domain 패키지로 구분되어 있더라구요!
그런데 이 둘 모두에서 필드와 Getter만 가지고 있고, DB와 관련된 id를 가지고 있는데, 이런 경우에도 도메인이라고 부를 수 있는건지..궁금합니다..!!
Service 도입 이유
Service를 도입하기로 한 가장 큰 이유는 중복검증이라는 비즈니스 로직을 Service 단에서 해주기 위해서였는데, unique 컬럼에 대해서는 DuplicateKeyException이 던져지더라구요. 하지만 도메인과 뷰단의 의존성을 끊어주고, 서비스단에서 DTO를 생성해서 Controller로 넘겨준다는 점에서 Service를 유지하기로 하였습니다.
피드백 내용
Q. 디우가 생각하는 도메인이란 무엇인가요?😄
A. 저는 도메인이 비즈니스 로직과 관련된 책임을 가지는 객체들이라고 생각합니다. 그에 따른 상태(인스턴스 변수)를 가질 수 있다고 생각합니다. (더크게 보면 내가 해결하고자하는 문제 영역에 해당하는 객체들이라고 볼 수도 있을 것 같고, 그러한 관점이라면 Line과 Station을 domain 패키지로 구분할 수 있을 것 같습니다.)
코드를 처음 받았을 때 Line, Station은 데이터(인스턴스 변수)와 그에 따른 비즈니스 로직을 가지고 있지 않은 상태였으며, id라고 하는 DB와 관련된 인스턴스 변수 또한 가지고 있었습니다...그래서 이것 또한 도메인인가?? 도메인과 Entity의 경계는 무엇일까? 궁금해졌습니다.
제가 아는 Entity는 DB테이블의 컬럼을 필드로 가지는 클래스로 그에 따른 Getter를 가질 수 있는 것으로 이해하고 있습니다. 또한 지금과 같이 식별자(id, Primary Key값)을 가지는게 특징이라고 생각합니다. 그래서 Line과 Station은 도메인보다는 Entity에 가깝지 않나? 하는 생각을 하였습니다!
코니 : 충분히 잘 이해하고 있으신 걸로 보여요 👍
"도메인과 Entity의 경계는 무엇일까?" 하는 의문이 드셨다고 했는데, 이 경계는 분명한 경우보다 그렇지 않은 경우가 아마 훨씬 많을 거예요. 지금 우리 미션에서도 그런 것처럼요. 앞으로의 미션을 통해서, 그리고 학습을 통해서 배워나가야 할 부분인 것으로 이해해 주셔도 좋을 것 같습니다.
Q. 빈 문자열로도 역이 생성되고 있네요🤔
@PostMapping("/stations")
public ResponseEntity<StationResponse> createStation(@RequestBody StationRequest stationRequest) {
final StationResponse response = stationService.save(stationRequest);
return ResponseEntity.created(URI.create("/stations/" + response.getId())).body(response);
}A. 지하철역 생성시 공백인지, 그리고 노선을 생성하면서 노선의 이름과 색상이 공백인지를 검증하는 로직을 각 도메인에 추가하였습니다.
각 도메인에 추가한 이유는 이름이 공백인지, 색상이 공백인지에 대한 책임은 각 도메인에 있다고 판단하였기 때문입니다.
또한 IllegalArgumentException을 활용하였습니다. 즉, 공백인 경우 IllegalArgumentException을 던지도록 하였는데요, 허용하지 않는 값을 인자로 전달받았을 때 예외를 발생시키는 IllegalArgumentException 이라는 표준예외를 활용하여도 충분히 표현이 가능하기 때문입니다. 그리고 이 경우 bad request를 던지도록 하였습니다.
물론 밑에 코멘트 주신 것과 마찬가지로 IllegalArgumentException이 발생하는 상황이면 모두 BAD REQUEST가 아닐 수 있기 때문에 앞으로 IllegalArgumentException을 사용할 때 주의가 필요할 것으로 생각됩니다. 혹은 현재는 IllegalArgumentException을 사용하지만 좀 더 구체적인 또 다른 Custom Exception을 만들어 줄 수도 있을 것 같습니다. 하지만 현재는 IllegalArgumentException이 그 의미를 충분히 전달하고, 사용해도 적절하겠다는 판단을 하여 사용하였습니다!😃
Q. 기본 제공된 코드이긴 하지만, 혹시 모르고 있었다면 이번 기회에 학습해 보아요. poduces 설정은 어떤 역할을 할까요? 그리고 지금 이 설정을 지우면 어덯게 될까요?
@GetMapping(value = "/stations", produces = MediaType.APPLICATION_JSON_VALUE)A. RequestMapping에서 사용자 요청을 받고, 응답을 보내줄 때, MediaType을 통해서 데이터의 형식을 강제해줄 수 있습니다. consumes 의 경우에는 사용자 요청에 의해 들어오는 데이터의 데이터 형식을 정해줄 수 있고, 질문해주신 produces 의 경우에는 응답 데이터의 형식을 정해줄 수 있습니다.
언급해주신 코드 부분에서는 "/stations" Get 요청의 응답으로 나가는 List 가 JSON 형식이도록 강제해준다고 해석할 수 있습니다.
그리고 만약 해당 설정을 지우게 된다면 JSON 형식이 아닌 text/html, XML 등으로도 응답이 가능하게 될 것 같슽니다..!!
또한 @requestbody에서 HttpMessageConverter를 통해서 해당 json 데이터를 jackson 라이브러리를 통해서 변환해주는 것 같습니다..따라서 지금의 경우 굳이 사용할 필요가 없어보입니다!
Q. 사소한 것이긴 하지만, OK 응답의 경우 이렇게 shortcut을 지원하고 있어요!
return ResponseEntity.ok(response);A. A shortcut for creating a ResponseEntity with the given body and the status set to OK.와 같이 body()를 생략하는 shortcut을 제공하는지 몰랐네요..!! 감사합니다!!😁
Q. 컨트롤러에서 응답 객체를 만드는데, 요청 객체의 값을 바로 꺼내 생성하고 있네요. 이렇게 해도 괜찮을까요? 또한 응답 형태가 API 문서와는 달라요.
@PostMapping("/lines")
public ResponseEntity<LineResponse> createLine(@RequestBody LineRequest request) {
final Long savedId = lineService.save(request);
return ResponseEntity.created(URI.create("/lines/" + savedId))
.body(new LineResponse(savedId, request.getName(), request.getColor()));
}A. 컨트롤러에서 응답 객체를 만들 때, 요청 객체의 값을 그대로 사용하게 되면, 만약 잘못된 요청에 대한 검증이 제대로 처리되지 않는 경우(ex. 이전에 빈 문자열에 대한 검사가 없는 경우) 그대로 응답에 사용되게 될 것입니다.
또한 만약 Service단에서 예외에 대한 별도의 처리를 하고 그에 대한 값을 반환해준다 라고 할 때, (예를 들어 이름이 ""인 경우 "빈문자열" 이라고 바꿔서 응답해줘야한다.) 그에 대한 처리도 제대로 해주지 못하게 될 것이라고 생각됩니다. (요청값을 그대로 전달하기 때문에 "빈문자열"이 아닌 ""가 응답으로 나가게 될 것이다.)
따라서 lineService.save() 시에 LineResponse를 받고, 이를 body에 담아 응답하도록 수정하였습니다.
또한 API 문서와의 스팩이 일치하지 않았던 부분은 List<StationResponse> stations 필드를 제거함으로써 일치시켜 주었습니다.😃
@PostMapping("/lines")
public ResponseEntity<LineResponse> createLine(@RequestBody LineRequest request) {
final LineResponse response = lineService.save(request);
return ResponseEntity.created(URI.create("/lines/" + response.getId()))
.body(response);
}Q. 존재하지 않는 id로 요청을 보내면 어떤 응답이 오고 있나요?
@GetMapping("/lines/{id}")
public ResponseEntity<LineResponse> showLine(@PathVariable Long id) {
final LineResponse response = lineService.findById(id);
return ResponseEntity.ok().body(response);
}A. EmptyResultDataAccessException 예외가 발생할 수 있습니다!! 이 부분에 대한 처리를 추가하도록 하겠습니다😃
@ControllerAdvice
public class ExceptionControllerAdvice {
...
@ExceptionHandler
public ResponseEntity<ErrorResponse> handle(EmptyResultDataAccessException e) {
return ResponseEntity.badRequest().body(new ErrorResponse("해당 값이 존재하지 않습니다."));
}
...
}Q. IllegalArgumentException이 발생하는 상황이면 모두 BAD REQUEST인가요? 또한, 이 예외가 아닌 예상치 못한 다른 예외가 발생한다면 어떻게 될까요?
@ExceptionHandler
public ResponseEntity<ErrorResponse> handle(IllegalArgumentException e) {
return ResponseEntity.badRequest().body(new ErrorResponse(e.getMessage()));
}A. IllegalArgumentException이 모두 Bad Request는 아니라고 생각합니다..!! 현재는 중복된 이름인 경우(badRequest)와 존재하지 않는 경우(notFound) 모두 IllegalArgumentException을 던져 bad request로 처리하고 있습니다ㅠ.ㅠ
중복된 이름에 대해서는 DAO에서 던져주는 DuplicateKeyException 을 사용함으로써 Service에 별도의 검증로직을 두지 않고, 제거해주는 방향으로 개선해보았습니다!
또한 존재하지 않는 경우에 대해서는 NotExistException(custom exception)을 두어서 notFound로 응답할 수 있도록 수정해보았습니다!
@ControllerAdvice
public class ExceptionControllerAdvice {
@ExceptionHandler
public ResponseEntity<ErrorResponse> handle(IllegalArgumentException e) {
return ResponseEntity.badRequest().body(new ErrorResponse(e.getMessage()));
}
@ExceptionHandler
public ResponseEntity<ErrorResponse> handle(NotExistException e) {
return ResponseEntity.notFound().build();
}
@ExceptionHandler
public ResponseEntity<ErrorResponse> handle(EmptyResultDataAccessException e) {
return ResponseEntity.badRequest().body(new ErrorResponse("해당 값이 존재하지 않습니다."));
}
@ExceptionHandler
public ResponseEntity<ErrorResponse> handle(DuplicateKeyException e) {
return ResponseEntity.badRequest().body(new ErrorResponse("이미 존재합니다."));
}
}Q. Map 대신 LineRequest를 사용하는 건 어떨가요? 어떤 장점이 있을까요?
Map<String, String> params = new HashMap<>();
params.put("name", "신분당선");
params.put("color", "bg-red-600");A. Request DTO를 사용해볼 생각을 못했습니다. 지금 변경을 하고 나서 느끼는 장점으로는 테스트 코드에 요청에 대한 관리가 좀 더 편리할 것 같습니다!! 혹시 제가 모르는 또 다른 장점이 있을까요??
코니: 말슴해주신 것처럼 관리가 더 편하죠. 가독성도 높아지고, 또 오타 등으로 실수할 가능성도 확실히 줄어들 거에요. 혹시나 요청 객체의 변경이 이루어졌다면 테스트 코드에서도 어디가 수정되어야 하는지 빨리 파악할 수 있을 것 같습니다.
@Test
@DisplayName("노선을 등록한다.")
void createLine() {
// given
final LineRequest params = new LineRequest("신분당선", "bg-red-600");
// when
ExtractableResponse<Response> response = RestAssured.given().log().all()
.body(params)
.contentType(MediaType.APPLICATION_JSON_VALUE)
.when()
.post("/lines")
.then().log().all()
.extract();
// then
assertThat(response.statusCode()).isEqualTo(HttpStatus.CREATED.value());
assertThat(response.header("Location")).isNotBlank();
assertThat(response.as(LineResponse.class).getName()).isEqualTo("신분당선");
assertThat(response.as(LineResponse.class).getColor()).isEqualTo("bg-red-600");
}Q. 응답 바디까지 검증해보는 건 어떨까요?
assertThat(response.statusCode()).isEqualTo(HttpStatus.CREATED.value());
assertThat(response.header("Location")).isNotBlank();A. as 를 활용하여 테스트를 응답 바디까지 검증할 수 있도록 수정해보았습니다!😃
assertThat(response.as(LineResponse.class).getName()).isEqualTo("신분당선");
assertThat(response.as(LineResponse.class).getColor()).isEqualTo("bg-red-600");(지금와서 생각해보면 위와 같이 생성된 리소스에 대한 검증은 필수라는 생각이 든다.)
Q. DAO만 테스트하는데 @SpringBootTest를 이용해 모든 컨텍스트를 다 띄울 필요가 있을까요? 다른 방법은 없을지 학습해 보아요.
@SpringBootTest
@Transactional
class LineDaoTest {
...
}A. @SpringBootTest 대신 JDBC 기반의 컴포넌트들에 초점을 맞춘 @JdbcTest를 이용해서 좀 더 가볍게 테스트할 수 있습니다! 또한 @Transactional 어노테이션도 포함하고 있어 테스트 이후 롤백에 대한 문제도 없어보입니다!
Q. 같은 로직이 모든 테스트 메서드에 등장하고 있는데, 별도 메서드로 추출하여 중복을 제거하는 건 어떨까요? 어떤 장점이 있을까요?
ExtractableResponse<Response> response = RestAssured.given().log().all()
.body(params)
.contentType(MediaType.APPLICATION_JSON_VALUE)
.when()
.post("/lines")
.then().log().all()
.extract();A. RestAssured를 활용하여 API를 테스트하는 부분을 Fixture로 분리하였습니다.
이를 통해서 향후에 또 다른 인수테스트가 추가될 때도 현재 코드를 재활용할 수 있으며,
무엇보다 기존 LineAcceptanceTest나 StationAcceptanceTest의 각 단위테스트 메소드의 흐름을 한 눈에 알 수 있게 가독성이 향상되는게 느껴졌습니다!😃
public class AcceptanceFixture {
public static <T> ExtractableResponse<Response> post(T params, String path) {
return RestAssured.given().log().all()
.body(params)
.contentType(APPLICATION_JSON_VALUE)
.when()
.post(path)
.then().log().all()
.extract();
}
public static ExtractableResponse<Response> get(String path) {
return RestAssured.given().log().all()
.when()
.get(path)
.then().log().all()
.extract();
}
public static <T> ExtractableResponse<Response> put(T params, String path) {
return RestAssured.given().log().all()
.body(params)
.contentType(MediaType.APPLICATION_JSON_VALUE)
.when()
.put(path)
.then().log().all()
.extract();
}
public static ExtractableResponse<Response> delete(String path) {
return RestAssured.given().log().all()
.when()
.delete(path)
.then().log().all()
.extract();
}
}Q. 무엇을 검증하고 있는지 눈에 잘 들어오지 않는 것 같아요. 개선해 볼 수 있을까요?
assertThat(response.statusCode()).isEqualTo(HttpStatus.OK.value());
List<Long> expectedLineIds = Stream.of(createResponse1, createResponse2)
.map(it -> Long.parseLong(it.header("Location").split("/")[2]))
.collect(toList());
List<Long> resultLineIds = response.jsonPath().getList(".", LineResponse.class).stream()
.map(LineResponse::getId)
.collect(toList());
assertThat(resultLineIds).containsAll(expectedLineIds);A. 해당 부분은 응답 헤더와 바디로 부터 id값을 꺼내오는 부분입니다.
그런데 expectedLineIds 에서도 API 문서 를 보니 응답 바디에서 id 값을 꺼내와도 되겠다는 생각을 할 수 있었습니다.
따라서 두 부분모두 jsonPath()를 활용하여 응답 바디에서 id값을 가져오도록 하는 별도의 메소드를 두어 코드의 가독성을 개선해 보았습니다!
@DisplayName("노선 전체를 조회한다.")
void getLines() {
// given
final Long upStationId = extractIdByStationName("지하철역");
final Long downStationId = extractIdByStationName("새로운지하철역");
final Long anotherStationId = extractIdByStationName("또다른 지하철역");
final LineRequest params1 = new LineRequest("신분당선", "bg-red-600", upStationId, downStationId, 10);
ExtractableResponse<Response> createResponse1 = AcceptanceFixture.post(params1, "/lines");
final LineRequest params2 = new LineRequest("분당선", "br-green-600", upStationId, anotherStationId, 10);
ExtractableResponse<Response> createResponse2 = AcceptanceFixture.post(params2, "/lines");
// when
final ExtractableResponse<Response> response = AcceptanceFixture.get("/lines");
// then
assertThat(response.statusCode()).isEqualTo(HttpStatus.OK.value());
List<Long> expectedLineIds = List.of(extractId(createResponse1), extractId(createResponse2));
List<Long> resultLineIds = extractIds(response);
assertThat(resultLineIds).containsAll(expectedLineIds);
}
...
private List<Long> extractIds(ExtractableResponse<Response> response) {
return response.jsonPath().getList(".", LineResponse.class).stream()
.map(LineResponse::getId)
.collect(toList());
}Q. -ServiceTest인데 @SpringBootTest로 컨텍스트를 전부 띄워 테스트하고 있어요. 이렇게 했을 때의 장단점에는 어떤 것이 있을까요?
A. 컨텍스트를 띄우기 때문에 DAO에 대한 의존성 주입을 받아서 Service에서 필요한 DAO를 편리하게 주입 받을 수 있다는 장점이 있을 것 같습니다.
하지만 그에 따른 비용이 크다고 생각이 됩니다. 따라서 가짜 DAO객체, 즉 List를 사용하여 메모리에 데이터를 저장하는 방식을 사용하는 DAO를 사용하여 테스트를 진행해보았습니다. 이렇게 되면 컨텍스트를 띄우는데 필요한 시간과 비용을 줄일 수 있고, 원하는 테스트 결과를 얻을 수 있게 됩니다😄
class LineServiceTest {
private LineService lineService;
private static FakeLineDao lineDao = new FakeLineDao();
private static FakeSectionDao sectionDao = new FakeSectionDao();
private static FakeStationDao stationDao = new FakeStationDao();
@BeforeEach
void setUp() {
lineDao = new FakeLineDao();
sectionDao = new FakeSectionDao();
stationDao = new FakeStationDao();
lineService = new LineService(lineDao, sectionDao, stationDao);
}
@Test
@DisplayName("노선을 저장할 수 있다.")
void save() {
// given
final Long upStationId = stationDao.save(new Station("지하철역"));
final Long downStationId = stationDao.save(new Station("새로운지하철역"));
LineRequest request = new LineRequest("신분당선", "bg-red-600", upStationId, downStationId, 10);
// when
final LineResponse response = lineService.save(request);
// then
assertThat(response).extracting("name", "color")
.contains("신분당선", "bg-red-600");
assertThat(response.getStations()).hasSize(2)
.extracting("id", "name")
.containsExactlyInAnyOrder(
tuple(upStationId, "지하철역"),
tuple(downStationId, "새로운지하철역")
);
}
...
}Q. 프로그램이 계속 커지고 Line이 가지고 있는 정보가 늘어난다면, LineResponse 역시 수정될 확률이 높을 거예요. LineResponse를 생성할 때 이렇게 값을 일일이 꺼내 전달하는 방식을 사용해야 한다면, 수정이 필요할 때 LineResponse를 생성하는 모든 부분을 찾아 수정해야 하지 않을까요? 더 나은 방법은 없을까요?
final Line savedLine = lineDao.save(line);
return new LineResponse(savedLine.getId(), savedLine.getName(), savedLine.getColor());A. Line 자체를 넘기고 이를 생성자 내부에서 getter를 호출해 필요한 정보를 가져와 초기화를 진행한다면 수정이 필요할 때 생성하는 모든 부분을 찾아서 수정하지 않고, 생성자 부분만 수정하면 되므로 좀 더 유연하게 될 것 같습니다.😁
public LineResponse save(LineRequest request) {
Line line = new Line(request.getName(), request.getColor());
final Line savedLine = lineDao.save(line);
final Section section = new Section(savedLine.getId(), request.getUpStationId(), request.getDownStationId(),
request.getDistance());
sectionDao.save(section);
return new LineResponse(savedLine, makeStationResponseList(request));
}Q. 지정된 컬럼의 크기를 초과하는 값이 들어오면 어떻게 될까요?
private void validateArgument(String name, String color) {
if (name.isBlank() || color.isBlank()) {
throw new IllegalArgumentException("노선의 이름 혹은 색이 공백일 수 없습니다.");
}
}A. 500 응답 코드와 함께 JdbcSQLDataException이 발생합니다! 해당 검증 내용을 추가해보겠습니다.
private void validateArgument(String name, String color) {
if (name.isBlank() || color.isBlank()) {
throw new IllegalArgumentException("노선의 이름 혹은 색이 공백일 수 없습니다.");
}
if (name.length() > 255 || color.length() > 20) {
throw new IllegalArgumentException("노선의 이름이 255자 보다 크거나, 색이 20자 보다 큽니다.");
}
}**Q. name과 color만으로 동등성을 판단한다면 Line에서 id의 역할은 무엇일까요?
바로 위의 코멘트를 통해 말씀 드린 것처럼, 차근차근 배워나갈 부분이라고 생각하기 때문에 정확한 답을 요구하는 질문은 아니에요. 한번 고민해 보시면 좋을 것 같아 남겨 봅니다. 😊**
@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (!(o instanceof Line)) {
return false;
}
Line line = (Line) o;
return Objects.equals(name, line.name) && Objects.equals(color, line.color);
}A. 아직은 id의 역할이 DB의 pk를 자바 애플리케이션 상으로 가져오는 것 (ex. id를 통한 조회 등) 이외에는 잘 모르겠습니다....ㅠ.ㅠ 질문에 대해서 조금 더 고민해보도록 하겠습니다..!!
코니 : 하나 가정해 보아요. 노선의 이름과 색깔 모두 중복이 가능하도록 비즈니스 로직이 변경되었어요. 그렇다면 이름과 색깔만으로 두 객체가 동등하다고 판단해도 될까요?
나 : 만약 이름과 색깔 모두 중복이 가능하게 된다면 PK에 해당하는 id로 두 객체를 판별해야할 것 같아요!!
즉, 요구사항에 따라서 두 객체의 동등성을 비교할 때 id의 역할이 필요해질 수 있겠다는 생각을 합니다!!
Q. 지금 이 ExceptionControllerAdvice가 속한 패키지는 ui인데, 이 메서드들이 다루는 예외는 스프링의 dao 패키지에 속해 있네요. dao의 예외들은 어디서 처리하는 것이 좋을지 고민해 보아도 좋을 것 같아요.
@ExceptionHandler
public ResponseEntity<ErrorResponse> handle(EmptyResultDataAccessException e) {
return ResponseEntity.badRequest().body(new ErrorResponse("해당 값이 존재하지 않습니다."));
}
@ExceptionHandler
public ResponseEntity<ErrorResponse> handle(DuplicateKeyException e) {
return ResponseEntity.badRequest().body(new ErrorResponse("이미 존재합니다."));
}A. 음...해당 질문을 받고 나니, DAO쪽에서 다른 커스텀 예외나 자바 표준예외로 바꾸면 어떨까...하는 생각이 들어요!
아직 JPA에 대해서 학습이 되어 있는 상태는 아니지만! JdbcTemplate에서 JPA를 사용하기로 설계(?)가 변경된다면 지금 현재 사용하는 package org.springframework.dao 가 아닌 javax.persistence.PersistenceException 의 예외들을 사용하게 될 것 같아요!
따라서 해당 변경의 영향이 Service 계층이나 Controller, 특히 ControllerAdvice 쪽으로 영향을 미치는 것보다는 DAO 내에만 영향을 끼치도록 하는게 좋은 설계라는 생각이 들었습니다!
따라서 DAO쪽에서 예외를 catch하고 다른 커스텀 예외나 자바 표준예외를 활용해보면 어떨까하는 생각이 듭니다!😄
코니 : 이미 Service에서 DAO를 사용하고 있는데 이 부분에서의 설계 변경이 일어났을 때 Service에 영향을 미치지 않을 수 있을까요?
괜찮은 방향으로 접근하고 계신 것 같고, 과연 어디서 어떻게 처리하는 것이 더 좋은 방법인지는 다음 단계를 진행하면서 스스로 고민해 보아요 😎 물론 그 과정에서 궁금한 부분이 있으실 때 질문은 언제든 환영입니다.
3 단계 - 지하철 구간 관리
리뷰어: 코니
고민한 내용
BatchUpdate
3단계에서 Section(구간) 들을 관리하는 Sections를 두고, 해당 도메인에서 비즈니스 로직을 처리하게 하였습니다. 그러다 보니 DB에 추가하거나 할 때, Sections를 활용하여야 했고 결국 LineId에 해당하는 section들을 모두 지운 후 새롭게 저장하는 방법 밖에 떠오르지 않더라구요..그런데 각 데이터에 대해서 쿼리를 날리는 것이 부담스러워 batchUpdate를 활용하였습니다.
DAO의 예외처리
또한 1,2 단계에서 피드백 주셨던 부분(#208 (comment)) 에 대해서도 고민을 해보았는데...이전에 말씀 드렸던 것 처럼 DAO에서 다른 커스텀 예외나 표준예외를 던지는 것 이외에는 좋은 방법이 떠오르지 않더라구요...ㅠㅠ 조금만 더 힌트를 주시면 감사하겠습니다..!(EmptyResultDataAccessException에 대해서는 DAO에서 예외를 잡고, Optional을 활용해보았습니다!)
피드백 내용
Q. 처음에 메서드 이름만 보고서는 이 코드가 하는 역할을 이해하기 조금 어려웠는데요, 해당 메서드가 하는 본질적인 일이 extract일까요?
final Long upStationId = extractStationIdFromName("지하철역");
final Long downStationId = extractStationIdFromName("새로운지하철역");A. 1. 지하철역 이름(name)을 통해서 StationRequest 를 만들고, 요청을 보낸 이후 ExtractableResponse 를 만든다.
2. 해당 응답에서 id값을 추출한다.
이렇게 2가지 책임을 가지기 때문에 해당 메소드가 하는 일을 메소드 이름을 통해서 얻기가 어려웠다고 생각이 듭니다.
두 책임은 엄연히 다른 책임이고, makeStationResponse 와 extractStationId 두 가지 메소드로 분리하여 메소드 이름을 통해서 그 의미를 보다 명확히 알 수 있도록 개선해보았습니다!😃
Q. 그런데 이 두 메서드는 항상 같이 사용되고 있지 않나요? 이 둘을 분리했을 때의 장점을 느끼셨을까요?
이전에 제가 남긴 코멘트는, 메서드가 하는 본질적인 역할을 이름을 통해 잘 드러내면 좋다는 의미로 이해해 주시면 좋을 것 같아요!
final ExtractableResponse<Response> upStationResponse = makeStationResponse("지하철역");
final Long upStationId = extractStationId(upStationResponse);A. 이전에 말씀드린 것과 같이...메소드가 하는 역할을 이름을 통해서 잘 표현하지 못한 이유는 두 가지 역할을 하는 메소드를 하나의 이름으로 표현하려고 하였기 때문이라는 생각이 아직까지 들어요..그래서 저는 차라리 모호한 메소드 이름을 부여하는 것보다는 이렇게 구분하여 보다 명확하게 해주는 것이 맞다는 생각이 들었습니다..!
코니가 말씀해주신대로 항상 함께 사용되기 때문에 하나의 메소드로 표현할 수 있겠는데요! 이렇게 된다면 메소드명을 어떻게 해야할지 모르겠어요ㅠㅠ
지금 생각나는 메소드명은 extractStationIdFromResponse(String name) 정도인거 같아요..하지만 fromResponse 라는 이름에서 인자로 ExtractableResponse가 와야할 것 같아요🥲
Q. extract라는 맥락이 중요한지, 이 메서드를 가져다 사용하는 입장에서 꼭 알아야 하는 정보는 무엇인지 고민해 보면 좋을 것 같아요~
A. 사용하는 입장에서 보면 역 이름을 준다.(input), 해당 역의 id 값을 갖는다.(output) 이라는 정보가 중요할 것 같아요!
그러한 맥락에서 이전의 두 메소드를 합치고, 그 메소드의 이름으로 extractIdByStationName()는 어떨까하는 생각이 듭니다!
Q. 이 이름만 봐서는 무엇을 테스트하고 있는지 코드까지 다 봐야 알 수 있을 것 같아요. 작성할 때 조금만 더 신경 쓰면 나중에 정말 많은 시간을 아낄 수 있어요!
public class SectionAcceptanceTest extends AcceptanceTest {
@DisplayName("환승역(교점)에 대한 구간 테스트")A. 구현에 바쁘다보니...전체적으로 네이밍등에 신경을 못 쓴 것 같습니다..ㅠ.ㅠ
작성할 때 조금만 더 신경 쓰면 나중에 정말 많은 시간을 아낄 수 있어요! 말과 같이 가독성 있는 코드에 대해서는 정말 많이 공감합니다!
최초에 작성할 때에도 어느정도 신경을 써야하는데, 아직까지 이런 습관이 몸에 베지는 못한 것 같아요🥲
(테스트코드는 하나의 문서 역할을 하기 때문에 더욱 신경을 써야하는데, 특히 잘 못지키는 것 같아요 ㅠ.ㅠ)
앞으로 더욱 신경쓸 수 있도록 하겠습니다..!! 이러한 부분까지 꼼꼼히 봐주셔서 감사합니다🙏
Q. orElseThrow() 부분이 이 테스트 코드에서 꼭 필요한가요?
final Line findLine = jdbcLineDao.findById(savedLine.getId())
.orElseThrow(() -> new NotExistException("찾으려는 간선이 존재하지 않습니다."));A. 말씀해주신대로 굳이 예외를 던질 필요는 없어보입니다. 예외를 던지기 보다는 "실패"를 의미할 수 있는 객체를 만들어서 orElseGet()을 이용해 null인 경우에는 해당 객체를 반환하도록 함으로써 테스트를 해볼 수 있을 것 같습니다..!! 😃 수정해보도록하겠습니다.
final Line findLine = jdbcLineDao.findById(savedLine.getId())
.orElseGet(() -> new Line(FAIL_FIND_LINE, FAIL_FIND_LINE));Q. 여기서 Optional이 비었을 때 예외를 던지는 로직이 필요하냐고 코멘트를 남겼었지요. 그 이유는 테스트의 맥락상 절대 예외가 발생하지 않을 상황이기 때문이었어요. 그렇기 때문에 (싫어하실 수도 있지만) 바로 Optional.get()으로 꺼내도 되지 않을까 생각했어요. 테스트 코드가 잘못 수정되어 테스트가 실패하더라도 NoSuchElementException이 발생할 텐데 원인을 파악하기 어렵지 않겠죠. (물론 프로덕션 코드에서는 절대 그러면 안 됩니다.) 테스트 코드에 지금처럼 로직이 추가로 들어가면 어쨌든 읽고 이해하는 데 리소스가 필요하니까요.
물론 이건 제 의견이라 꼭 반영해달라는 말씀을 드리는 건 절대 아니에요!
A. 코니가 말씀해주신대로 지금처럼 로직이 추가되면 비용이 소모된다는데에도 동의하며, get() 을 활용하면 테스트가 실패하는 경우 NoSuchElementException이 발생하여 원인 파악에 도움이 될 것 같습니다!!
하지만 여전히 단순히 get()을 사용하는 것은..조금 마음에 걸리는 것 같아요..(IntelliJ 경고(?)도 그렇고...🥲)
혹시 그 비용을 조금 더 줄일 수 있게 다음과 같이 "부적절한 Line" 이라는 Line을 미리 생성해두고 활용한다면 지금보다 조금 더 괜찮을까요??
private static final String FAIL_FIND_LINE = "fail";
private static final Line INAPPROPRIATE_LINE = new Line(FAIL_FIND_LINE, FAIL_FIND_LINE); final Line findLine = jdbcLineDao.findById(savedLine.getId())
.orElse(INAPPROPRIATE_LINE);Q. Bool 변수 이름 제대로 짓기 위한 최소한의 영어 문법을 한번 읽어 보면 좋을 것 같아요!
❗주의❗is로 시작하는 변수명을 짓다가 범하는 흔한 실수가 바로 is + 동사원형 을 쓰는 것이다.
isAuthorize, isHide, isFind 등등.
public boolean isContainStation(Section section) {
return upStationId.equals(section.getDownStationId()) || downStationId.equals(section.getUpStationId());
}A. 해당 케스트는 "is + 동사원형" 을 사용한 경우입니다. 따라서 링크걸어주신 블로그 글의 마지막 내용인 "동사원형 용법" 을 참고하여 containsStation 으로 네이밍을 수정하여 "역을 포함하는가?" 라는 의미를 전달할 수 있도록 수정해보았습니다!
Q. 다른 예외 상황에서 같은 타입의 예외, 같은 메시지를 사용한다면 예외 발생 시 원인을 파악할 수 있을까요?
private void checkDistance(Section existingSection, Section newSection) {
if (existingSection.getDistance() <= newSection.getDistance()) {
throw new IllegalSectionException("구간 등록이 불가능합니다.");
}
}
private void isPossibleRegistration(Section section) {
sections.stream()
.filter(s -> s.isContainStation(section))
.findAny()
.orElseThrow(() -> new IllegalSectionException("구간 등록이 불가능합니다."));
}A. IllegalSectionException 은 구간(section)과 관련된 예외를 나타내도록 커스텀 예외를 만들었습니다. 또한 두 경우 모두 "구간 등록이 불가능한 상황"이라는 동일한 예외 상황이라고 생각이 듭니다.(예외의 발생 이유(길이 제한, 등록 조건)는 서로 다르지만, 동일하게 사용자의 잘못된 입력 요청, bad request)
따라서 예외 타입은 동일한 예외 타입을 사용하는 대신, "어떤 이유로" 구간 등록이 불가능한지를 명확히 하는 메시지로 변경해주었습니다..!
private void checkDistance(Section existingSection, Section newSection) {
if (existingSection.getDistance() <= newSection.getDistance()) {
throw new IllegalSectionException("등록하려는 구간 길이가 기존 구간의 길이와 같거나 더 길 수 없습니다.");
}
}
private void validateRegistration(Section section) {
sections.stream()
.filter(s -> s.containsStation(section))
.findAny()
.orElseThrow(() -> new IllegalSectionException("등록할 구간의 적어도 하나의 역은 등록되어 있어야 합니다."));
}Q. 이 메소드들은 무슨 일을 하는지 이름으로 알 수가 없네요.
sameUpstationInFork(existingSection, newSection);
sameDownStationInFork(existingSection, newSection);A. 이전처럼 "상행역이 같은 경우에 대해 처리", "하행역이 같은 경우에 대해 처리"로 분리하는 것이 아니라 "중간에 새로운 구간 추가", "새로운 구간 만들기" 라는 두 가지 로직을 메소드 분리를 통해 분리해냄으로써 의미를 보다 명확히 하도록 수정해보았습니다.
private void addSectionInMiddle(Section existingSection, Section newSection) {
Section section = createNewSection(existingSection, newSection);
sections.add(section);
}
private Section createNewSection(Section existingSection, Section newSection) {
if (existingSection.getUpStationId().equals(newSection.getUpStationId())) {
return new Section(existingSection.getId(), existingSection.getLineId(), newSection.getDownStationId(),
existingSection.getDownStationId(), existingSection.getDistance() - newSection.getDistance());
}
return new Section(existingSection.getId(), existingSection.getLineId(), existingSection.getUpStationId(),
newSection.getUpStationId(), existingSection.getDistance() - newSection.getDistance());
}Q. Line과 Section 모두 무사히 저장되어야 이 메서드의 임무가 완수되었다고 볼 수 있을 것 같고, 만약 Section을 저장하는 데 실패했다면 Line의 저장도 롤백되어야 하지 않을까요? 지금 LineRequest에 name, color만 채워서 요청을 보내 보고 어떤 일이 일어나고 있는지 확인해 볼까요?
public LineResponse save(LineRequest request) {
Line line = new Line(request.getName(), request.getColor());
final Line savedLine = lineDao.save(line);
final Section section = new Section(savedLine.getId(), request.getUpStationId(), request.getDownStationId(),
request.getDistance());
sectionDao.save(section);
return new LineResponse(savedLine, makeStationResponseList(request));
}A. 선언적 트랜잭션 처리를 지원해주는 어노테이션인 @Transactional 을 이용하여 해결해주었습니다.
트랜잭션은 하나의 논리적인 작업 단위로 말씀해주신 것과 같이 Line 과 Section이 모두 저장 되어야 하나의 논리적인 작업(위의 메소드)이 수행되었다고 이야기할 수 있습니다.
Transactional 어노테이션을 이용하면 데이터 추가, 갱신, 삭제 등 DB 컬럼에 변경이 이루어지는 작업 도중 오류가 발생했을 때 모든 작업들을 원상태로 되돌릴 수 있습니다. (@transactional이 붙은 메소드는 메소드가 포함하고 있는 작업 중에 하나라도 실패할 경우 전체 작업을 취소한다.) 이를 Service 클래스에 붙여주고, 읽기만 하는 경우(ex. findById, findAll)에는 readOnly = true 를 주어 해당 트랜잭션을 읽기 전용으로 설정해주도록 하였습니다.
(@transactional 은 클래스, 메소드 모두에 걸어줄 수 있고 메소드 레벨의 @transactional 선언이 우선 적용된다.)
다음의 블로그 글을 참고하여 공부하였습니다. Transactional 어노테이션
Q. // given & when & then 주석은 불필요해 보이네요😅
@Test
public void stationNameLengthTest() {
// given & when & then
assertThatThrownBy(() -> new Station(EXCESS_MAX_LENGTH_STRING))
.isInstanceOf(IllegalArgumentException.class);
}A. 이러한 경우에는 보통 테스트가 하나의 라인으로 끝나는데, given & when & then을 구분해줄 필요가 없으므로 해당 주석은 불필요해보이네요! 수정해보도록 하겠습니다!
Q. 이 설정을 켜는 건 어떨까요? 왜 필요한지는 혹시 알고 계신가요?
A. 넵..!! 이전에도 동일한 리뷰를 받았었습니다!
EOF 와 관련된 문제가 발생할 수 있을 것 같습니다..!
당시에 어떤 옵션을 적용해야할지 몰랐었는데, 코니 덕분에 설정 적용해볼 수 있었습니다!! 감사합니다.😃
Q. Section의 식별자는 이 맥락에선 불필요해 보이는데 제거해도 되지 않을까요? 비슷한 숫자들이 연달아 있다 보니 각 값의 의미를 이해하는 데 시간이 걸리는 것 같아요.
A. 아래와 같이 IDE의 도움을 받다 보니 고려해보지 못했던 부분이네요...😅 말씀해주신 것과 같이 해당 단위 테스트에서 Section의 식별자는 불필요하고, 이를 생략함으로써 코드를 읽는 사람으로 하여금 관심있는 부분만 집중하게 할 수 있을 것 같습니다! 수정해보겠습니다~_~
나머지 부분에서도 Section의 식별자는 불필요하므로 제거하였습니다! 하지만 예외 적으로 checkDistance() 와 sameSection() 에서는 무엇에 의해서 예외가 발생하는지 분명히 해주고 싶어 Section의 id 값을 그대로 사용하도록 유지시켜 주었습니다!
(지금와서 해당 코멘트를 다시 고려해보면 근본적으로 line, upStation, downStation 모두 Long 이라는 자료형을 통해서 표현하고 있기 때문에 발생하는 문제라고 생각한다. 원래는 Line 객체에 대한 참조, upStation, downStation은 Station 객체에 대한 참조를 가지고 있는 것이 보다 바람직하다는 생각이 든다.)
**Q. 1 - (7) - 3에서 1 - (3) - 2 - (4) - 3이 되는데, getSections()의 리턴 순서는 삽입 순서와 동일하여 거리가 순서대로 4, 3임을 검증하고 있네요.
그런데 Sections를 개념적으로 잘 생각해 보면, 본질적으로 순서가 있어야 하는 것 아닐까요? 지금은 LineService에서 노선을 리턴할 때만 getSortedSection() 메서드를 이용해 정렬을 하고 있는데 적절한 상황일까요?**
@Test
public void forkRodeSameUpStation() {
// given
List<Section> sectionList = new ArrayList<>();
sectionList.add(new Section(1L, 1L, 1L, 3L, 7));
final Sections sections = new Sections(sectionList);
// when
final Section section = new Section(2L, 1L, 1L, 2L, 4);
sections.add(section);
// then
assertThat(sections.getSections().size()).isEqualTo(2);
assertThat(sections.getSections()).extracting("distance")
.containsExactlyInAnyOrder(4, 3);
}A. Sections는 어떤 라인의 구간들을 나타내어 주는 도메인이기 때문에 말씀해주신대로 순서를 정렬한 상태로 유지하는게 맞는 것 같습니다..!! 따라서 Sections를 생성하면서 정렬된 형태를 유지하고, add, delete 시에도 정렬을 해주도록 하였습니다. 또한 테스트에서도 containsExactly(3, 4) 와 같이 수정하여 구간의 크기 뿐 아니라 그 순서도 정확하게 테스트 하도록 수정해보았습니다.😄
public class Sections {
private static final int MINIMUM_SECTION_COUNT = 1;
private List<Section> sections;
public Sections(List<Section> sections) {
this.sections = sortSections(sections);
}
...
}Q. 값을 가져와서 비교하지 않고 물어볼 수 있을 것 같아요.
private Section getPreviousSection(Long stationId) {
return sections.stream()
.filter(section -> section.getDownStationId().equals(stationId))A. isSameDownStation(), isSameUpStation() 이라는 메소드를 section에 만들어 값을 가져오는 것이 아니라 메시지를 보내도록 수정해보았습니다!!
(레벨1에서는 Getter 사용을 지양해야 하는 이유에 대해서 고민하면서 위와 같은 사항에 대해서 고민을 많이 하였음에도 불구하고 레벨2에 와서 이렇게 놓치는 부분이 있다는 것을 깨달을 수 있는 리뷰였다. 레벨1기간 약 2달 동안 잠깐 고민하면서 코딩했다고 이것이 자연스럽게 익혀질 수는 없다고 생각하며 앞으로도 계속해서 노력해야하는 점이라고 생각한다.)
public boolean isSameUpStation(Long stationId) {
return getUpStationId().equals(stationId);
}
public boolean isSameDownStation(Long stationId) {
return getDownStationId().equals(stationId);
}**Q. 이 메서드 로직을 보면, 상행 종점을 지우는 경우 laterSection만 찾을 수 있고, 하행 종점을 지우는 경우 previousSection만 찾을 수 있고, 중간에 있는 역을 지우는 경우 둘 다 찾을 수 있을 것 같은데요. 그러면 이미 previousSection과 laterSection의 존재 여부만으로 종점 여부를 알 수 있고 삭제할 구간도 바로 찾은 셈이니 이에 따라 로직을 전개하면 되지 않나요? findAndRemoveFirstSection() 등의 로직을 보면 내부에서 또 삭제할 구간을 다시 찾고 있는데 이렇게 할 필요가 있는 건가요?
제가 로직을 잘못 이해한 부분이 있다면 코멘트 부탁드려요!**
public List<Section> delete(Long stationId) {
validateThatHasMinimumSection();
final Section previousSection = getPreviousSection(stationId);
final Section laterSection = getLaterSection(stationId);
findAndRemoveFirstSection(stationId, previousSection);
findAndRemoveLastSection(stationId, laterSection);
removeMiddleSection(previousSection, laterSection);
return sections;
}A. getPreviousSection() 과 getLaterSection() 에서 지우려는 역이 만약 종점의 경우 둘 중 하나가 null이 되게 되는데요,
만약 getPreviousSection()이 null인 경우(상행 종점 제거) 즉, 1 - 2 - 3 - 4 에서 1을 지우려고 하는 경우, getPreviousSection이 null이기 때문에 첫번째 구간을 찾는 getFirstSection()이 필요하게 됩니다.
그런데 앞선 코멘트를 반영하면서 Sections가 항상 정렬된 상태를 유지하기 때문에 삭제할 구간을 찾을 필요 없이 상행 종점 제거의 경우 index: 0, 하행 종점 제거의 경우 index: sections.size()-1 을 이용할 수 있을 것 같습니다!
Q. 이렇게 두 번 할당할 필요가 있나요? 👀
public Sections(List<Section> sections) {
this.sections = sections;
this.sections = sortSections(sections);
}A. Sections의 정렬하는 부분에서, 특히 첫번째 section인지를 검사하는 부분에서 인스턴스 변수인 sections와 비교를 하기 때문에..먼저 할당한 이후 sortSections를 불러야 해서 이렇게 두 번 할당하게 되어 있습니다..!
private boolean isFirst(Long upStationId) {
return sections.stream()
.noneMatch(section -> upStationId.equals(section.getDownStationId()));
}저도 리팩토링 하면서 이부분이 찜찜하긴 했는데, 한 번에 전부 다 바꾸려고 하면 제대로 개선하지 못할 것 같아서 우선 이렇게 수정해보았는데요! 이 부분도 한 번만 할당해도 되게끔 리팩토링 해보도록 하겠습니다!😁
(다음 미션에서 본인의 코드를 이어서 작업하기로 페어였던 배카라와 결정을 하고, 해당 코드에 대한 리팩토링을 함께 진행하였는데, 걱정하였던 isFirst() 메소드에서 검사하는 로직에 복사한 sections를 넘김으로써 인스턴스 변수 할당 2번하는 문제를 해결할 수 있었다.)
Q. for문 안에서 DB에 접근하여 역의 정보를 하나하나 가져오고 있는데요, 한번에 가져올 수 있는 방법은 없는지 고민해 보면 좋을 것 같아요.
for (final Section section : sortedSections) {
final Station findStation = getStationById(section.getUpStationId());
stations.add(new StationResponse(findStation));
}A. 조금 오래 고민해보았는데요! section으로 부터 StationId들을 중복없이 TreeSet으로 받고, 이 id들을 IN 절을 사용해서 조회해볼 수 있을 것 같아요!!😃
@Service
@Transactional
public class LineService {
...
private List<StationResponse> sortedStations(Sections sections) {
final List<Section> sortedSections = sections.getSections();
final List<Station> stations = stationDao.findAllBySection(sortedSections);
List<StationResponse> stationResponses = new ArrayList<>();
for (Station station : stations) {
stationResponses.add(new StationResponse(station));
}
return stationResponses;
}
}@Repository
public class JdbcStationDao implements StationDao {
...
@Override
public List<Station> findAllBySection(List<Section> sections) {
List<Long> stationIds = getStationIds(sections);
final String inSql = String.join(",", Collections.nCopies(stationIds.size(), "?"));
String sql = String.format("SELECT * FROM station WHERE id IN (%s)", inSql);
return jdbcTemplate.query(sql, stationIds.toArray(), STATION_ROW_MAPPER);
}
}