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

[Spring 지하철 노선도 관리 - 1, 2단계] 아마찌(신지혜) 미션 제출합니다. #86

Merged
merged 35 commits into from May 13, 2021

Conversation

NewWisdom
Copy link

안녕하세요 게이츠! 아마찌입니다!
레벨 1때도 만났었는데, 레벨 2에서 지하철 미션으로 다시 만나뵙네요!
다시 만나뵙게되어 정말 반갑습니다 👏

이전 미션까지는 MockMVC로 단위 테스트를 진행했는데,
이번 미션에서는 RestAssured로 end to end 테스트를 진행했어요!
MockMVC와 RestAssured로 테스트 하는 것이 어떤 차이가 있는지도 학습할 수 있었던 시간이었습니다!

이번 미션, 잘 부탁드립니다 🙌
리뷰에 귀한 시간 내주셔서 정말 감사드려요 🙇‍♂️

Livenow14 and others added 29 commits May 4, 2021 14:05
Copy link

@serverwizard serverwizard left a comment

Choose a reason for hiding this comment

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

아마찌! 다시 한번 리뷰를 맡은 (빌)게이츠입니다. 😄

요구사항에 맞춰 기능 구현을 잘 해주셨어요. 👍
간단한 코멘트 몇 가지 남겼는데, 확인 부탁드릴게요!

코멘트 참고해보시고 추가적으로 궁금한 부분 있으시면 언제든 DM 주세요

import java.util.stream.Collectors;

@RestController
@RequestMapping("/lines")

Choose a reason for hiding this comment

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

참고로 실무에서는 /api/v1(버전)/lines 식으로 버전 정보도 명시해준답니다.

API URI에 버전 정보를 붙여주는 이유는
v1으로 만든 API를 운영에 배포한 상태에서, 요구사항이 변경된 경우 v1을 수정하는게 아니라(행여 기존에 해당 API를 사용하고 있는 서비스에 영향을 주지 않기 위함) v2를 새로 만드는 식으로 하기 위함이에요.

Copy link
Author

Choose a reason for hiding this comment

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

이 부분은 구현 단계에서 고민했었던 부분이었는데,
미션에서 주어진 프론트엔드 API 요청이 이미 있기 때문에
컨트롤러 부분만 API를 변경할 수는 없었어요 🥲
앞으로 API를 설계하는데 참고하겠습니다 감사합니다!


@Override
public Line save(final Line line) {
if (findByName(line.name()).isPresent()) {

Choose a reason for hiding this comment

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

Line의 이름과 색상(Line의 데이터) 중복 여부를 Line에게 위임해서 처리하게는 할 수 없을까요?

Copy link
Author

Choose a reason for hiding this comment

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

Line 객체가 가지고 있는 것은 자신의 이름과 색상인데,
중복 여부를 Line에게 위임한다는 것은 Line이 List Line에 있는
다른 Line들의 이름을 알아야한다는 것으로 게이츠의 리뷰를 이해했어요!ㅎㅎ
현재 이름이 주어졌을 때 line 객체 내부에서 sameName으로 판단하고 있어 객체에게 메시지를 통해
같은 이름인지 확인하고 있다고 생각했는데 다른 의도를 말씀하신걸까요???

Choose a reason for hiding this comment

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

아마찌가 이해한 것이 맞아요.
Line에게 다른 Line들의 이름을 전달해서 중복 여부를 판단할 수는 없는지를 말씀드린 거였어요.
이렇게 함으로써 장점은, Dao에 비즈니스 로직이 없다는 거에요~

그리고 최종적으로 도달했음 했던 부분은 JPA를 활용한 아래 코드였어요!

@Column(unique = true)
private String name;


assertThat(line.id()).isEqualTo(id);
assertThat(line.name()).isEqualTo(name);
assertThat(line.color()).isEqualTo(color);

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.

hashCode, equals를 통해 객체를 비교할 수 있도록 하였습니다!

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

Choose a reason for hiding this comment

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

데이터를 Map타입이 아니라 DTO타입으로 전달하면은 안되는 걸까요?

Copy link
Author

Choose a reason for hiding this comment

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

컨트롤러에서는 DTO를 받았는데, 테스트 코드에서는 그냥 Map을 전달하고 있었네요
ObjectMapper를 사용하여 DTO를 파싱해 테스트하도록 변경하였습니다!

.when()
.post("/lines")
.then().log().all()
.extract();

Choose a reason for hiding this comment

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

아래와 같은 코드가 반복적으로 사용되고 있는 것 같은데요.
메소드로 추출하여 중복을 제거해보는 건 어떨까요?

    RestAssured.given().log().all()
                .body(파라미터)
                .contentType(미디어타입)
                .when()
                .post("경로")
                .then().log().all()
                .extract();

Copy link
Author

Choose a reason for hiding this comment

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

테스트코드의 중복된 메서드를 추출할 수 있다는 사실을 간과하고 있었던 것 같아요!
중복된 부분은 메서드로 추출하였습니다! 감사합니다

import static org.assertj.core.api.Assertions.assertThatCode;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

class LineNameTest {

Choose a reason for hiding this comment

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

도메인객체 테스트 👍


@Override
public void delete(Long id) {
Line findLine = findById(id).orElseThrow(() -> new IllegalArgumentException("없는 노선임!"));

Choose a reason for hiding this comment

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

현재 아래와 같은 코드가 반복되고 있는데요.
차라리 findById( ) 내부에서 Exception을 발생시키는건 어떨까요?

findById(id).orElseThrow(() -> new IllegalArgumentException("없는 노선임!"));

Copy link
Author

Choose a reason for hiding this comment

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

dao에서는 optional로 싼 객체를 반환하고,
서비스단에서 이에 대한 처리를 해주도록 변경하였습니다 !
감사합니다!


import java.util.regex.Pattern;

public class StationName implements Name {

Choose a reason for hiding this comment

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

원시값 포장 👍

import java.util.Optional;

public class MemoryStationDao implements StationDao {
private static Long seq = 0L;

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.

이부분은 미션 기본 템플릿으로 주어진 부분인데요,
저는 Static 메모리를 사용할 때 여기서는 DB처럼 index가 설정되지 않으니,
주어진 seq가 DB의 index역할을 한다고 생각했어요!
객체를 저장할 때도 seq 값을 증가시키구요!!
MemoryDB를 구현하기 위해서는 이러한 부분에서 필요한 값이라고 생각했는데,
불필요한 값일까요???

Choose a reason for hiding this comment

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

제가 2단계 미션 요구사항인 DB에서 데이터를 관리한다만 보고 말씀드린 것 같아요.
이 부분은 그냥 skip하셔도 될 것 같아요.
혼란을 드려 죄송해요!

Copy link

@serverwizard serverwizard left a comment

Choose a reason for hiding this comment

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

피드백 반영을 잘해주셨네요. 👍

이번 단계는 여기서 머지할게요. 추가로 남긴 피드백은 다음 단계때 같이 반영해주시면 됩니다!

궁금한건 언제든 질문 주세요. 😉


@Override
public Line save(final Line line) {
if (findByName(line.name()).isPresent()) {

Choose a reason for hiding this comment

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

아마찌가 이해한 것이 맞아요.
Line에게 다른 Line들의 이름을 전달해서 중복 여부를 판단할 수는 없는지를 말씀드린 거였어요.
이렇게 함으로써 장점은, Dao에 비즈니스 로직이 없다는 거에요~

그리고 최종적으로 도달했음 했던 부분은 JPA를 활용한 아래 코드였어요!

@Column(unique = true)
private String name;

@serverwizard serverwizard merged commit c4459b4 into woowacourse:newwisdom May 13, 2021
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

3 participants