[Spring 지하철 노선도 관리 - 1, 2단계] 케빈(박진홍) 미션 제출합니다. by xlffm3 · Pull Request #84 · woowacourse/atdd-subway-map

xlffm3 · 2021-05-05T16:08:09Z

안녕하세요.
테스트 코드를 깔끔하게 작성하는 것이 어렵네요. 최대한 중복을 제거해보려고 노력했습니다...!

(학습 로그 작성 중입니다.)

- 기능 구현과 테스트 작성

- 기능 구현 및 테스트 작성

- 특정 아이디로 조회 - 기능 구현 및 테스트 작성

- 기능 구현 및 테스트 작성 - 중복되는 이름 확인 및 테스트

- 지하철 노선 삭제 기능 구현 및 테스트 작성

- 노선 생성, 조회 db 접근하도록 변경 - DB 설정 파일 추가 - line 생성자 추가

- 수정, 삭제 기능 변경

- h2 DB 적용

seok-2-o

안녕하세요 케빈. 이번 미션을 함께할 미립입니다. 반갑습니다.

전체적으로 구현 정말 잘해주셨네요 💯
구현 관련하여 몇가지 코멘트 남겨놓았는데요, 확인 후 리펙토링 해보면 좋을 것 같아요.

저와 의견이 다르거나 궁금한점이 있으면 편하게 DM 주시면 좋을 것 같아요.
코멘트는 제가 간혹 누락 하는 경우가 있으니 혹시 누락된다면 이부분도 말씀해주세요.

이번 미션 잘 부탁드릴게요 🙇

seok-2-o · 2021-05-06T01:57:14Z

src/main/java/wooteco/subway/controller/ExceptionController.java

+    @ExceptionHandler(EmptyResultDataAccessException.class)
+    public ResponseEntity<String> handleEmptyResultDataAccessException(EmptyResultDataAccessException emptyResultDataAccessException) {
+        return ResponseEntity.badRequest().body("해당 ID의 엔티티가 존재하지 않습니다.");
+    }


특정 익셉션에 종속된 핸들러가 많아질 수록 해당 어드바이져를 많이 만들어줘야 할 것 같아요. 케빈 생각은 어떠한가요?

제가 질문을 잘 이해했는지 모르겠네요...! 이와 관련해서는 comment에 구체적으로 더 작성했습니다.

EmptyResultDataAccessException보다는 DataAccessException같은 상위 예외를 바인딩하는게 다양한 예외를 처리할 수 있어 유리할 것 같습니다. 유일키 제약을 위배한 경우 DuplicatedKeyException이 발생하는데 이 또한 상위 DataAccessException을 상속받기 때문에, 하나의 핸들러로 sql 관련 여러 예외들을 잡을 수 있겠네요.

혹은 커스텀 예외와 같은 구조를 사용한다면, 특정 익셉션에 종속되지 않고도 ExceptionController가 비즈니스상에서 발생할 수 있는 다양한 예외들(서비스 레이어, 데이터 레이어 등)을 한 번에 처리할 수 있을 것 같아요.

SubwayException.java

public class SubwayException extends RuntimeException { public SubwayException(SubwayExceptionCode subwayExceptionCode) { super(subwayExceptionCode.getDescription()); } }

SubwayExceptionCode.java

public enum SubwayExceptionCode() { IdNotFound(400,"Id Not Found"), DuplicatedName(400, "Duplicated Name"), //... UnrecognizedRole(40x,"Unrecognized Role"); private final int errorCode; private final String description; //... }

StationDao.java

try { return jdbcTemplate.queryForObject(query, rowMapper, id); } catch (SQL관련 예외) { throw new SubwayException(SubwayExceptionCode.IdNotFound); }

ExceptionController.java

@ExceptionHandler(StationException.java) public ResponseEntity<String> handle(StationException e) { return ResponseEntity.status(HttpStatus.valueOf(e.getCode()).body(e.getDescription); }

seok-2-o · 2021-05-06T01:57:21Z

src/main/java/wooteco/subway/controller/apis/LineController.java

+
+    private final LineService lineService;
+
+    public LineController(LineService lineService) {


생성자 주입 👍

seok-2-o · 2021-05-06T01:58:43Z

src/main/java/wooteco/subway/controller/apis/LineController.java

+    public ResponseEntity<LineResponse> createLine(@RequestBody LineRequest lineRequest) {
+        Line savedLine = lineService.createLine(lineRequest.getName(), lineRequest.getColor());
+        LineResponse lineResponse = new LineResponse(savedLine.getId(), savedLine.getName(), savedLine.getColor(), new ArrayList<>());
+        return ResponseEntity.created(URI.create("/lines/" + savedLine.getId())).body(lineResponse);


Suggested change

return ResponseEntity.created(URI.create("/lines/" + savedLine.getId())).body(lineResponse);

return ResponseEntity.created(URI.create("/lines/" + savedLine.getId()))

.body(lineResponse);

메서드 체이닝은 내려쓰는것이 가독성과 디버깅 측면에서 모두 좋은 것 같아요.
27 라인에서 에러가 발생한다면 어떤 행위에서 에러가 발생하는지 확인하기 어려울 것 같아요. 🤔

그런 의미에서 URI.create("/lines/" + savedLine.getId()) 도 변수로 분리되면 장/단점이 있겠네요.

케빈은 어떻게 생각하나요?

네, 확실히 디버거 돌려보는데 line by line으로 분리되어 있는게 가독성이 훨씬 좋네요.
디버거는 1줄을 계속 표시하고 있는데 현재 어디 메서드를 작업 중인지 잘 보이지가 않네요!
단점이라고 한다면 코드의 길이가 늘어난다는 점이겠지만, 오히려 더 명확한 변수명을 통해 빠른 이해가 되는 것 같아요.

seok-2-o · 2021-05-06T01:59:37Z

src/main/java/wooteco/subway/controller/apis/LineController.java

+    public ResponseEntity<List<LineResponse>> showLines() {
+        List<LineResponse> lineResponses = lineService.findAll()
+                .stream()
+                .map(line -> new LineResponse(line.getId(), line.getName(), line.getColor(), new ArrayList<>()))


LineResponse 가 Line 을 받아서 LineResponse를 생성하는것에 대해서 어떻게 생각하시나요?
LineResponse 가 Line와 의존성이 생기긴 하지만, 이점이 더 클 것 같아요.

항상 마지막 필드는 비어 있는게 맞을까요 ? 그럼 인자로 받을 필요가 있나요 ?

예전에는 Entity를 DTO로 변환할 때 정적 팩토리 메서드 public static LineResponse from(Line line) 형태를 사용했었는데요.
Line domain model의 속성값들이 변한다면, 현재처럼 new 연산자를 통해 변환하는 부분들이 모두 다 변경을 수반하게 될 것 같습니다.

현재 마지막 필드 부분은 3단계 미션에서 사용하는 부분들이라 지금은 empty list를 주입해주었는데 일단은 수정해놓겠습니다!

seok-2-o · 2021-05-06T02:00:37Z

src/main/java/wooteco/subway/controller/apis/LineController.java

+    @PutMapping("/{id}")
+    public ResponseEntity<Void> editLine(@PathVariable long id, @RequestBody LineRequest lineRequest) {
+        lineService.editLine(id, lineRequest.getName(), lineRequest.getColor());
+        return ResponseEntity.ok().build();


응답이 없다면. noContent 가 더 명확하게 의미르 전달 할 수 있을 것 같아요.

seok-2-o · 2021-05-06T02:02:16Z

src/main/java/wooteco/subway/dao/LineDao.java

+    public void update(long id, String name, String color) {
+        String query = "UPDATE LINE SET name = ?, color = ? WHERE ID = ?";
+        int affectedRowCounts = jdbcTemplate.update(query, name, color, id);
+        if (affectedRowCounts == 0) {


매직 넘버 사용하셨네요. 비교적 명확한 의미 전달이 되는 코드긴 하지만, 연습과정이니만큼 상수로 분리해보면 좋을 것 같아요.

seok-2-o · 2021-05-06T02:02:24Z

src/main/java/wooteco/subway/dao/LineDao.java

+            return prepareStatement;
+        };
+        jdbcTemplate.update(preparedStatementCreator, keyHolder);
+        return keyHolder.getKey().longValue();


seok-2-o · 2021-05-06T02:19:40Z

src/main/java/wooteco/subway/domain/line/Line.java

+
+public class Line {
+
+    private long id;


id가 0보다는 null 이 의미상 정확해 보이는데 어떻게 생각하시나요 ?

네, db에서 조회할 때는 null이 들어갈 일은 없고 Station, LineDTO를 entity로 변경시킬 때 id가 필요없는 경우 0을 줬는데... 혹여 잘못 사용할 여지가 있네요. 더 확실하게 null을 주는 것이 의미상 정확한것같습니다.

seok-2-o · 2021-05-06T02:20:17Z

src/main/java/wooteco/subway/dao/StationDao.java

+    }
+
+    public List<Station> findAll() {
+        String query = "SELECT * FROM STATION";


현재는 필드가 몇개 없지만 * 를 사용하면 불필요한 필드도 계속 조회 될 것같아요.
이부분에 대해서 어떻게 생각하시나요?

넵, 지금 Line과 Station 모두 필드가 많이 없어서 무의식적으로 사용했었는데요.

Why is SELECT * considered harmful?

제가 생각하는 것 이상으로 생각할 점들이 많네요. 단순히 당장 어플리케이션 기능에 필요없는 컬럼까지 조회하는 것(그로 인한 속도 저하) 말고도 인덱싱 이슈 등등... 이번 미션 하면서 학습 내용을 정리해봐야겠습니다.

seok-2-o · 2021-05-06T02:26:54Z

src/main/java/wooteco/subway/service/LineService.java

+    }
+
+    public Line createLine(String name, String color) {
+        validateDuplication(name);


데이터베이스에 제약 조건을 줄 수도 있다고 생각하는데,
어플리케이션에서 처리했을때 장,단점에는 어떤것들이 있을까요?

Should check for duplicate or catch exception from database?

어플리케이션에서 처리할 때의 단점으로는

a 유저의 요청에 대해 이름 중복을 검사하는 중에

중복된 이름의 b 유저의 요청이 db에 반영되면

결과적으로 db에는 중복된 이름의 Station Entity가 존재하게됩니다.

또 매번 데이터를 삽입하기 전에 전체 목록에 대한 쿼리를 날리고 검사를 하는 것이 성능상 유리할 것 같지 않구요...

지금 보니 schema에 이미 unique가 작성되어 있네요...

xlffm3 · 2021-05-07T04:17:43Z

안녕하세요 미립. 피드백 주신 내용을 다른 크루(중간곰, 제이온) 등과 함께 많이 고민해보고 반영해보았어요.
부족한 점이 많아서 미리 리뷰 감사드립니다!!

요청으로 들어온 값의 중복 검사 및 id에 해당하는 엔티티 조회 검사 로직을 어디에?

처음에는 Dao가 findById()로 무조건 Optional을 반환하도록 하고, Service Layer에서 엔티티가 있는지 검증했습니다. 또한 중복 검사도 dao.findAll()을 호출해서 Service Layer에서 중복되는 값이 있는지 체크했구요.

미립의 피드백을 보면서 어플리케이션 단에서 처리하는 방법의 장단점에 대해 고민해보았어요.

장점(인지는 명확하게 모르겠습니다.)
- 도메인의 로직(?)이 Dao 레이어까지 내려가지 않는다.
단점
- 매번 중복검사를 할 때마다 findAll() 쿼리를 날려서 값들을 가져오고, 이를 또 순회하면서 값을 비교해야 한다.
- 동시성의 문제
  - 어느 클라이언트의 이름 등록 요청에 대한 중복 검사를 진행 중일 때, 해당 이름과 중복되는 이름으로 등록 요청을 날린 다른 클라이언트의 처리가 먼저 처리되면 DB에 결국 중복된 값이 존재하게 된다.
  - db에 unique 제약을 걸어두면 DuplicatedKeyException이 호출되겠지만, 그렇다면 서비스 레이어에서 검증하는 로직은 사실상 무용지물이 아닐까?
- id에 해당하는 엔티티 조회 쿼리를 날렸을 때 존재하지 않는다면 자동으로 예외가 호출되는데, 굳이 비용이 존재하는 Optional 객체를 사용해서 반환한 다음 서비스 레이어에서 검증해야 할까?

이런 점들을 고려해 보았는데, 아이디 존재 유무나 유일키 제약 등은 db에서 검증하는 것이 낫겠다고 생각했습니다.

예외 처리

특정 익셉션에 종속된 핸들러가 많아질 수록 해당 어드바이져를 많이 만들어줘야 할 것 같아요.

라고 피드백 주셨는데요. Dao 단에서 발생할 수 있는 예외는 많은데, 구체적인 예외를 하나 하나 ExceptionHandler에 등록하는 것 보다 상위 추상화된 예외(DataAccessException)를 잡는게 효율적일 것 같다고 생각했어요.

@ExceptionHandler(DataAccessException.class)
public ResponseEntity<String> handle(DataAccessException e) {
      return ResponseEntity.badRequest().body(e.getMessage());
}

문제는 dao를 조작하면서 발생하는 DataAccessException의 예외 메시지는 대부분이 클라이언트에게 크게 도움이 되지 못하는 내용입니다. (제가 제어할 수 없는 영역에서 발생되기 때문에 예외 메시지를 세밀하게 조작이 힘들고, 메시지 내용 또한 개발자가 이해할법한 영어)

또한 sql, db관련 DataAccessException은 잘못된 유저 입력값으로 인해 발생할 수도 있겠지만, 아닌 경우도 있을 테구요. (일부 예외 메시지는 클라이언트에게 노출되면 안 되는 내용도 있을 수도 있구요.)

이런 상황을 고민하다가 커스텀 예외를 활용해서

Dao

    public long save(Line line) {
        String query = "INSERT INTO LINE (name, color) VALUES (?, ?)";
        KeyHolder keyHolder = new GeneratedKeyHolder();
        PreparedStatementCreator preparedStatementCreator = getPreparedStatementCreator(line, query);
        try {
            jdbcTemplate.update(preparedStatementCreator, keyHolder);
            return keyHolder.getKey().longValue();
        } catch (DuplicateKeyException duplicateKeyException) {
            throw new SubwayException(ExceptionStatus.DUPLICATED_NAME);
        }
    }

ExceptionHandler

    @ExceptionHandler(SubwayException.class)
    public ResponseEntity<String> handleSubwayException(SubwayException subwayException) {
        int statusCode = subwayException.getStatusCode();
        String message = subwayException.getMessage();
        return ResponseEntity.status(statusCode)
                .body(message);
    }

DAO 단에서 클라이언트에게 유의미한 정보를 제공할 수 있는 Controllable한 예외를 try-catch로 잡아 커스텀 예외를 호출하도록 했습니다. 다만 dao에 try-catch가 너무 많아지는 것 같아 고민이긴 합니다... 😔

또한 테이블에 유일키 칼럼이 추가되는 경우, 현재 어떤 칼럼에서 중복이 발생했는지 구분하는 것도 쉬워보이지 않구요.

seok-2-o

피드백 반영 잘 해주셨네요 👍
테스트코드도 분리가 잘 되어 있어 인상적이네요 💯
테스트 부분에 크게 피드백 드릴 부분이 없어요 👏

매우 소소한 질문 몇가지를 마지막으로 이번 미션은 여기서 마무리 하도록 할게요.

seok-2-o · 2021-05-07T04:29:07Z

src/main/java/wooteco/subway/controller/ExceptionController.java

+@ControllerAdvice
+public class ExceptionController {
+
+    @ExceptionHandler(SubwayException.class)


seok-2-o · 2021-05-07T04:29:52Z

src/main/java/wooteco/subway/controller/apis/LineController.java

+    public ResponseEntity<List<LineResponse>> showLines() {
+        List<LineResponse> lineResponses = lineService.findAll()
+                .stream()
+                .map(LineResponse::from)
+                .collect(Collectors.toList());


LineResponse.fromList(lines) 와 같이 리스트도 캡슐화 할 수 있어요.

seok-2-o · 2021-05-07T04:30:29Z

src/main/java/wooteco/subway/controller/apis/StationController.java

+    public ResponseEntity<StationResponse> createStation(@RequestBody StationRequest stationRequest) {
+        Station savedStation = stationService.createStation(stationRequest.getName());
+        StationResponse stationResponse = StationResponse.from(savedStation);
+        URI uri = URI.create("/stations/" + savedStation.getId());


seok-2-o · 2021-05-07T04:31:27Z

src/main/java/wooteco/subway/dao/LineDao.java

+
+@Repository
+public class LineDao {
+    private static final RowMapper<Line> ROW_MAPPER = (resultSet, rowNumber) -> {


라인은 하나의 매퍼만 가질 수 있나요? 그게 아니라면 좀 더 의미를 나타낼 수 있는 이름이 좋을 것 같아요.

seok-2-o · 2021-05-07T04:33:56Z

src/main/java/wooteco/subway/dao/LineDao.java

+        }
+    }
+
+    public void update(long id, String name, String color) {


update 의 인자가 Line 로 만드는것에 대해서는 어떻게 생각하시나요 ?

seok-2-o · 2021-05-07T04:34:43Z

src/test/java/wooteco/subway/controller/LineAcceptanceTest.java

+        }
+    }
+
+    private ValidatableResponse postLineApi(LineRequest lineRequest) throws JsonProcessingException {


👍 👍

이름은 createLine 어 좀 더 적합해보이는데 케빈은 어떻게 생각하시나요?

xlffm3 · 2021-05-11T15:25:15Z

학습로그

유효성 검사의 위치

https://xlffm3.github.io/spring%20&%20spring%20boot/how-to-handle-exception/
도메인 유효성 검사를 어플리케이션단과 db단에서 했을 때는 각각 장단점이 존재한다.
db 제약조건을 활용하는경우 동시성 이슈가 해결될 수 있으나 자바 코드만을 읽고 프로젝트를 이해하기 어려워진다 (스키마를 확인해야한다.)
어플리케이션단에서 수행하는 경우 동시성 이슈가 발생할 수 있다.

Spring IoC와 DI

https://xlffm3.github.io/spring%20&%20spring%20boot/toby-spring-chapter1/

Select 쿼리와 *

https://xlffm3.github.io/database/select-asterisk-anti-pattern/
Why is SELECT * considered harmful?
- 불필요한 데이터까지 조회해야 하며, 여러 테이블에서 조인하여 동일 칼럼이 존재할 때 충돌이 발생, 인덱싱 이슈가 발생한다.

DTO의 사용 범위에 대해

https://woowacourse.github.io/javable/post/2021-04-25-dto-layer-scope/

단위 테스트

통제할 수 없는 리소스에 의존하는 테스트는 단위 테스트가 아니다.
Sociable하지 않는 테스트를 보장하기 위해 테스트 진행 후 조작된 db를 롤백한다.
대표적으로 @SQL 애너테이션을 통해 truncate시킨다.

DAO vs Repostiory 패턴

https://xlffm3.github.io/spring%20&%20spring%20boot/dao-and-repository-pattern/
DAO는 DB의 CRUD쿼리와 1:1 매칭
Repository는 DDD에서 나온 개념으로 어그리게이트 루트를 저장 및 조회하는 역할을 함.

Insert시 PK 반환

https://xlffm3.github.io/spring%20data/simplejdbcinsert/


		private final LineService lineService;

		public LineController(LineService lineService) {

	return ResponseEntity.created(URI.create("/lines/" + savedLine.getId())).body(lineResponse);
	return ResponseEntity.created(URI.create("/lines/" + savedLine.getId()))
	.body(lineResponse);

Conversation

xlffm3 commented May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seok-2-o left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xlffm3 May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xlffm3 May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xlffm3 May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xlffm3 commented May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

요청으로 들어온 값의 중복 검사 및 id에 해당하는 엔티티 조회 검사 로직을 어디에?

예외 처리

Uh oh!

seok-2-o left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xlffm3 commented May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

학습로그

유효성 검사의 위치

태그

Spring IoC와 DI

태그

Select 쿼리와 *

태그

DTO의 사용 범위에 대해

태그

단위 테스트

태그

xlffm3 commented May 5, 2021 •

edited

Loading

xlffm3 May 6, 2021 •

edited

Loading

xlffm3 May 6, 2021 •

edited

Loading

xlffm3 May 6, 2021 •

edited

Loading

xlffm3 commented May 7, 2021 •

edited

Loading

xlffm3 commented May 11, 2021 •

edited

Loading