[Spring 지하철 노선도 - 1,2단계] 기론(김규철) 미션 제출합니다. #191

Gyuchool · 2022-05-04T05:55:30Z

안녕하세요 소니!
기론이라고 합니다!😁

이번 미션을 진행하면서 궁금한 점이 있었습니다.

노선 정보를 update할 때, Repository의 update메서드에서 name과 color를 받아서 id를 기준으로 업데이트를 해줍니다.
그런데 여기서 update(id, name, color) 로 각각의 인자들을 받는 방식이 아닌, query를 날리는데 필요한 정보를 담는 dto를 만들어서 update(Dto dto)로 dto안에 id, name, color를 담도록 해보았습니다. 이렇게 클라이언트와 소통용 dto가 아닌 db와 소통용 dto를 만드는 것에 대해서 소니의 생각이 궁금합니다!

리뷰 잘 부탁드립니다🙂
감사합니다!

sonypark

기론 안녕하세요! 이번 미션 리뷰를 맡은 소니입니다.
지하철 노선도 미션 잘 구현해주셨네요~
질문 주신 내용과 작업한 코드에 대해 몇가지 코멘트 남겼으니 확인 부탁드려요~🙂

sonypark · 2022-05-05T17:07:24Z

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

+
+    private static final Long seq = 0L;
+    private static final List<Station> stations = new ArrayList<>();


사용하는 코드인가요?

1단계에서 2단계로 넘어가면서 불필요한 코드가 되었네요!
제거했습니다!

sonypark · 2022-05-05T17:11:25Z

src/main/java/wooteco/subway/domain/Station.java

 public class Station {
    private Long id;
    private String name;


기본 생성자는 필요한 코드인가요?

불필요한 코드 제거했습니다!

sonypark · 2022-05-05T17:16:09Z

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

+
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+        Line line = (Line) o;
+        return Objects.equals(name, line.name);
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(name);
+    }
+}


동등성의 기준을 name으로 잡으셨군요. 그렇다면 id는 어떤 역할을 할까요?

그 전에 1단계에서 dao 클래스에서 static List<Line> lines 와 같은 형태로 Line들을 저장하는 역할을 하는 상수가 있었습니다. 이 때, 들어오는 Line들마다 구별해주기 위해서 name을 기준으로 구별을 해주는데 이용했습니다.

그런데 현재는 db에 pk값으로 id를 자동으로 auto_increment 해주기 때문에 이를 통해서 식별할 수 있도록 되었습니다!

name은 unique이고 id는 pk이기 때문에 null을 허용하지 않는 id를 이용해서 식별하도록 하는 게 좋다고 생각합니다!

sonypark · 2022-05-05T17:20:35Z

src/main/java/wooteco/subway/ui/LineController.java

+@RestController
+@RequestMapping("/lines")


@RequestMapping 사용 👍

sonypark · 2022-05-05T17:21:17Z

src/main/java/wooteco/subway/dto/LineResponse.java

+    private LineResponse() {
+    }


기본생성자가 필요한가요? 없으면 어떻게 되나요?

LineAcceptanceTest에 response.jsonPath() 메서드를 통해서 json형식을 다시 Object로 역직렬화를 하기 때문에 기본생성자가 없다면 역직렬화가 실패해서 인수테스트가 실패하므로 추가를 했습니다!

즉, 테스트에서 json으로 받은 값을 다시 객체로 변환해서 비교하기 위해서 추가했습니다!

sonypark · 2022-05-05T17:37:02Z

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

+        Map<String, String> params = new HashMap<>();
+        params.put("name", "신분당선");
+        params.put("color", "bg-red-600");


Map이 아닌 LineRequest를 사용하면 더 편리하지 않을까요?

Map보다는 request이용이 더 편하네요! 수정했습니다

sonypark · 2022-05-05T17:39:57Z

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

+
+    @DisplayName("전체 노선을 조회하면 200 ok와 노선 정보를 반환한다.")
+    @Test
+    void getLines() {
+        Map<String, String> newBundangLine = new HashMap<>();
+        newBundangLine.put("name", "신분당선");
+        newBundangLine.put("color", "bg-red-600");


body에 대한 값도 검증하면 좋지 않을까요?

단순히 조회하는 로직이어서 조회한 id들만 잘 있는지 확인해도 충분하다고 생각했습니다. 왜냐하면 id가 잘 나온다면 같이 묶여있는 다른 데이터 또한 잘 올 것이라고 생각했기 때문입니다.

또한 조회하고 싶은 데이터가 변경 될 때마다 비교하는 코드가 추가되거나 삭제되므로 계속해서 테스트를 수정해야한다고 생각했습니다.

소니는 제 생각에 대해서 어떻게 생각하시나요?

다 적고 생각해보니 현재 테스트가 인수 테스트이기 때문에 요구사항에 맞는 body가 적절히 나왔는지도 검증하는게 맞다고 생각이 드네요..?

그렇다면 이러한 인수테스트에서는

또한 조회하고 싶은 데이터가 변경 될 때마다 비교하는 코드가 추가되거나 삭제되므로 계속해서 테스트를 수정해야한다고 생각했습니다.

이러한 이유때문에 잦은 수정이 발생할 것 같은데 어쩔수없는 부분일까요?

인수 테스트는 소프트웨어 내부 코드에 관심을 가지지 않는 블랙박스 테스트입니다. 개발자가 아닌 사람(일반 사용자, 기획자 등)도 이해할 수 있어야 합니다. 그래서 소프트웨어 내부 구조나 구현 방법을 고려하기보다는 실제 사용자 관점에서 테스트하는게 좋다고 생각합니다. 아이디가 같으면 다른 데이터도 동일할 것이라는 것은 내부 구조를 이해한 개발자에게는 당연한거지만 일반 사용자에게는 당연하지 않을 수 있을 것 같아요. 실제로 어떤 응답이 나왔는지 모두 검증해서 보여주는게 인수 테스트의 목적에 더 부합하지 않나 생각합니다. 그런 점에서 응답해주는 데이터에 변경사항이 생기면 인수테스트도 수정이 되어야하는 건 자연스러운 것 같습니다.

아이디가 같으면 다른 데이터도 동일할 것이라는 것은 내부 구조를 이해한 개발자에게는 당연한거지만 일반 사용자에게는 당연하지 않을 수 있을 것 같아요.

오.. 정말 바로 와 닿게 되는 멘트네욤. 이해했습니다!

sonypark · 2022-05-05T17:43:47Z

src/test/java/wooteco/subway/dao/LineRepositoryTest.java

+@Transactional
+@SpringBootTest
+public class LineRepositoryTest {
+
+    @Autowired
+    LineRepository lineRepository;


Repository를 테스트하는데 SpringBootTest를 이용해 모든 Bean을 띄울 필요가 있을까요?

테스트에서 Transational 어노테이션은 어떤 역할을 하나요? 이 어노테이션을 없애면 어떻게 되나요?

모든 빈을 끌고와서 저도 무겁다고 생각해서 @jdbcTest로 수정했습니다! @jdbcTest는 jdbc관련 빈만 끌고 오기 때문에 테스트 속도가 눈에 띄게 빨라졌습니다!
또한 @transactional로 테스트가 끝난 후, 롤백을 적용하려고 했는데 지금보니 JdbcTest안에 Trasanctional 어노테이션이 있었군요! 제거했습니다!

sonypark · 2022-05-05T17:46:09Z

src/test/java/wooteco/subway/service/LineServiceTest.java

+@Transactional
+@SpringBootTest
+public class LineServiceTest {
+
+    @Autowired
+    private LineService lineService;
+
+    @Autowired
+    private LineRepository lineRepository;
+


Service 테스트도 @SpringBootTest과 @Transactional을 사용하고 있네요

이 어노테이션 없이도 테스트 가능할까요?

현재 레포지토리가 Fake 객체를 사용하지 않고 빈으로 등록된 repository를 이용하기 떄문에 의존성 주입을 하지 않으면 동작하지 않습니다. 또한 Junit에서는 jupiter가 빈을 관리하게되므로 스프링 빈을 사용하려면 @Autowired를 명시하여 스프링 빈을 사용하겠다고 알려줘야한다고 알고 있습니다.
따라서 현재는 SpringBootTest에서 Jdbc관련 빈만 가져오는 JdbcTest로 수정했습니다!

sonypark · 2022-05-05T17:54:57Z

src/main/java/wooteco/subway/dao/dto/LineUpdateDto.java

+
+public class LineUpdateDto {
+
+    private final Long id;
+    private final String name;
+    private final String color;
+
+    private LineUpdateDto(Long id, String name, String color) {
+        this.id = id;
+        this.name = name;
+        this.color = color;
+    }
+
+    public static LineUpdateDto of(final Long id, final LineRequest lineRequest) {
+        return new LineUpdateDto(id, lineRequest.getName(), lineRequest.getColor());
+    }


질문주신 부분에 대해 답변 드리자면, 저라면 LineUpdateDto를 따로 만들지는 않을 것 같습니다.
LineUpdateDto는 사실상 Line과 다른점이 없어서 LineRequest.toLine(id) 과 같은 메서드를 만들어 Line을 update의 인자로 넘길 것 같습니다.

넵! 도메인으로 넘기면 됐겠네요..😂
지금은 save를 할때도 toLine이 있어서 오버로딩으로 toLine()과 toLine(id)를 만들까 했지만 막상 두개를 만들고 진행해보니 update메서드를 호출할때 toLine(id)를 이용해야하는데 toLine()을 이용하면서 헷갈리게 되더라구요!
그래서 그냥 toLine()으로 통일하고 update(id, toLine()) 으로 수정하였습니다!

sonypark

기론 안녕하세요!
피드백 잘 반영해주셨네요! 구조도 이전보다 개선된게 느껴집니다. 수고하셨어요!💯
이번 단계는 여기서 머지하겠습니다.
다음 단계 진행해주세요~💪

jayjaehunchoi · 2022-05-10T02:03:01Z

src/test/java/wooteco/subway/service/LineServiceTest.java

+@Transactional
+@SpringBootTest
+public class LineServiceTest {
+
+    @Autowired
+    private LineService lineService;
+
+    @Autowired
+    private LineRepository lineRepository;
+


@MockMvcTest? @WebMvcTest? class ServiceTest { @MockBean private Repository repository; private Service service; @BeforeEach void setUp() { service = new Service(repsitory) } }

jayjaehunchoi added 30 commits May 3, 2022 14:29

docs : 기능 구현 요구사항 추가

4f7f121

feat: 동일한 이름 등록시 에러가 발생한다.

6128090

refactor : validateDuplicateName 코드 분리

42d6ea5

feat : Line 등록 기능 추가

798c650

test: 중복 라인 중독시 에러발생 테스트 작성

984efe1

feat: 전체 노선 조회 기능 추가

7f43877

feat : 노선 단건 조회 기능 추가

f9bb7b3

feat: 노선 업데이트 기능 추가

3b9924f

feat : 노선 삭제 기능 추가

92edb79

refactor : test context를 최소화하도록 수정

863b410

feat : database 관련 설정 추가

99ac14f

refactor

92bc954

refactor: 전체 역 조회 jdbctemplate이용하여 수정

9087e58

feat : 역 삭제 쿼리 작성

8b0d071

feat: 역 등록기능 추가

f853b03

refactor : stationController save 코드 수정

c19ea41

feat: 모든 역 조회 기능 구현

8220b3f

refactor: stationController findAll 코드 수정

e7c33d8

feat : 역 삭제 기능 구현

8968a00

refactor : stationController delete 코드 수정

bb80722

refactor : 레거시 코드 삭제

0375539

refactor : schema 변경

ea1ce3e

feat : 노선 저장 쿼리 추가

469d1ef

feat: 모든 노선 조회 쿼리 추가

826c50a

feat : 단건 노선 조회 쿼리 추가

0b3f9f9

feat : 업데이트 쿼리 추가

2e3d2ff

feat : 삭제 쿼리 추가

e0f10b8

refactor : repository 인터페이스 분리

2641b69

feat : 노선 생성 기능 구현

50699ef

feat : 전체 노선 조회 기능 추가

4081552

jayjaehunchoi added 5 commits May 4, 2022 11:45

feat : 이름으로 노선 조회 쿼리 추가

ae97955

feat : 노선 명이 존재할 경우 예외 발생

25310f5

feat : 단건 노선 조회 기능 추가

908135f

feat : 업데이트 기능 추가

da6dc5b

feat : 삭제 기능 추가

88b66e3

sonypark requested changes May 5, 2022

View reviewed changes

Gyuchool added 14 commits May 6, 2022 12:16

refactor: 불필요한 코드 제거

cad767d

refactor: springBootTest에서 JdbcTest로 수정

ca9de3c

refactor: 생성자 체이닝 적용

21c064b

refactor: Optional을 반환하여 null처리문 제거

79d0468

refactor: dto에서 도메인으로 변환 로직 추가

63b6bd6

feat: 추가적인 예외 처리하도록 수정

87a1ed3

refactor: Map사용에서 request사용하도록 수정

7ec66ad

refactor: body도 검증하도록 수정

3d76cc2

refactor: test코드에서 @transactional 제거

b941468

refactor: 인수테스트 body도 검증하도록 수정

31b6c88

refactor: 서버 예외 사항 추가

bbaeb36

refactor: 예외처리 메서드 네이밍 수정

1b01580

refactor: 에러 처리 상태 코드 NotFound로 수정

2eba99f

refactor: 코드 포멧팅

339e7b3

sonypark approved these changes May 7, 2022

View reviewed changes

sonypark merged commit 8a7a5e1 into woowacourse:gyuchool May 7, 2022

jayjaehunchoi reviewed May 10, 2022

View reviewed changes

Gyuchool mentioned this pull request May 13, 2022

[Spring 지하철 노선도 - 3단계] 기론(김규철) 미션 제출합니다. #277

Merged


		private static final Long seq = 0L;
		private static final List<Station> stations = new ArrayList<>();

		@RestController
		@RequestMapping("/lines")

[Spring 지하철 노선도 - 1,2단계] 기론(김규철) 미션 제출합니다. #191

[Spring 지하철 노선도 - 1,2단계] 기론(김규철) 미션 제출합니다. #191

Uh oh!

Conversation

Gyuchool commented May 4, 2022

Uh oh!

sonypark 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!

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!

Gyuchool May 6, 2022 • 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!

sonypark left a comment

Choose a reason for hiding this comment

Uh oh!

jayjaehunchoi May 10, 2022 • 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!

Uh oh!

Gyuchool May 6, 2022 •

edited

Loading

jayjaehunchoi May 10, 2022 •

edited

Loading