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

[또링] 지하철 경로 조회 - TDD 미션 제출합니다! #35

Merged
merged 37 commits into from
May 31, 2020

Conversation

jnsorn
Copy link

@jnsorn jnsorn commented May 15, 2020

안녕하세요 구구님!
잘부탁드립니다😄

Copy link

@kang-hyungu kang-hyungu left a comment

Choose a reason for hiding this comment

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

안녕하세요 또링 👋 다시 만나서 반가워요
구현한 코드 확인하고 피드백 남겼어요
확인해보시고 궁금한 점 생기면 코멘트 남기거나 dm 주세요

@kang-hyungu
Copy link

피드백 반영하신 내용 아직 다 확인 못했네요
내일 오전에 마무리할게요

@kang-hyungu
Copy link

#35 (comment)

db 조회하는 리소스 비용을 줄이기 위해 source, target에 대한 유효성 검사를 먼저 하는 방법도 괜찮네요
도메인 객체에서 유효성 검증 할 수도 있지만 서비스 객체에서 유효성 검사하는 방법도 있으니 참고하세요

그리고 Graphs를 싱글톤으로 만들고 경로 없데이트 하는 방법은 좋지 않아요
대량의 요청을 처리하거나 서버 장애가 발생하는걸 대비해서 최소 2대 이상의 서버를 운영합니다.
현재 구조는 각 서버의 싱글톤 객체에 업데이트된 내용이 공유될 방법이 없어요
이런 문제 때문에 싱글톤 객체는 변경사항이 생길 멤버를 갖지 않는게 좋습니다.
그리고 싱글톤을 직접 구현하면 몇 가지 문제가 있어서 보통 스프링에 빈으로 등록해서 사용합니다.
싱글톤을 구현할 때 생기는 문제는 이펙티브 자바 3판 아이템 3 참고하세요

어떻게 하면 효율적인 구조로 만들지 고민하는 모습이 멋지네요 👍
이런 고민이 쌓이면 또링님 실력 향상에도 많은 도움이 될거에요!

Copy link

@kang-hyungu kang-hyungu left a comment

Choose a reason for hiding this comment

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

또링, 피드백 반영한 내용 확인했어요
질문 주신 내용에 답변 남겼으니 같이 보시고 수정해보시면 좋을 것 같네요
수정하면서 추가로 궁금한 점 생기면 코멘트 남기거나 dm 주세요

private String backgroundColor;
private LocalDateTime createdAt;
private LocalDateTime updatedAt;
private Set<LineStation> stations = new HashSet<>();

Choose a reason for hiding this comment

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

Set를 일급 컬렉션으로 만드는걸 추천드려요

Copy link
Author

Choose a reason for hiding this comment

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

좋은 지적 감사합니다 😊 이전에 일급컬렉션으로 만들었던 부분인데, 이번 미션에서는 다른 것들에 집중하다보니 놓치게 되었습니다..!ㅠㅠ 수정하였습니다 ㅎㅎ

import wooteco.subway.domain.Station;
import wooteco.subway.exception.InvalidPathException;

public class Graphs {

Choose a reason for hiding this comment

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

싱글톤으로 메모리에 유지하기보단 db에서 조회해서 그때그때 객체 만들어서 처리하는게 좋습니다
질문 주신 내용에 남긴 코멘트 참고하세요

Copy link
Author

@jnsorn jnsorn May 28, 2020

Choose a reason for hiding this comment

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

답변 감사합니다! 정말 큰 도움이 되었고, 생각도 못했던 여러 문제들에 대해 생각해볼 수 있는 시간이었습니다😊 특히, 이펙티브자바와 다른 자료들을 통해 singleton의 문제점들을 인지할 수 있었습니다!

코드는 다시 예전 방식으로 돌려놓았습니다. 다만 한 가지 드는 궁금증은
실제 큰 서비스에서는 대부분 서버를 분산시켜놓았을텐데, 그럼 이 때 모든 데이터를 다 DB를 통해 들고있는지 궁금합니다. 제가 singleton으로 들고있던 것과 같이 메모리상에 들고 있을 법한 데이터들을 어디서 관리하는지 궁금합니다(Singleton이 아닌 Spring bean 으로 등록했다 하더라도 분산 서버끼리 Spring context를 공유하는 건 아닌 것 같은데....)

대략적으로 검색을 해보니 memcached/redis 같은 것이 나오는데.. 이런 기술을 통해 위와 같은 문제를 해결하는 걸까요?

처음과는 좀 다른 주제의 이야기가 되었는데.. 😅 궁금해서 코멘트 남겨봅니다..ㅎㅎ

Choose a reason for hiding this comment

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

디비에서 필요한 정보만 인덱스로 필터해서 가져오면 됩니다.
전체 역 정보는 데이터가 많지 않으니 꼭 디비를 활용하지 않더라도 찾아보신 것처럼 캐시에 올려서 사용하는 방법도 있겠네요

src/main/java/wooteco/subway/service/LineService.java Outdated Show resolved Hide resolved
import java.util.Map;
import io.restassured.RestAssured;
import io.restassured.specification.RequestSpecification;
import wooteco.subway.dto.LineDetailResponse;

Choose a reason for hiding this comment

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

api가 늘어나면 AcceptanceTest 클래스에 계속 메서드가 추가 되겠네요
아래 샘플 코드처럼 get, post, put, delete 호출에 대한 중복 코드를 제거하면 AcceptanceTest가 변경될 일은 없지 않을까요?

    <T> T post(String path, Map<String, String> params, Class<T> responseType) {
        return
                given().
                        body(params).
                        contentType(MediaType.APPLICATION_JSON_VALUE).
                        accept(MediaType.APPLICATION_JSON_VALUE).
                when().
                        post(path).
                then().
                        log().all().
                        statusCode(HttpStatus.CREATED.value()).
                        extract().as(responseType);
    }

Copy link
Author

Choose a reason for hiding this comment

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

좋은 지적 감사합니다! 인수테스트 짜기에만 바빠서 중복 제거 할 생각도 못했던 것 같습니다....ㅠㅠ 중복되는 부분들을 분리하여 메서드로 추출하였습니다. 감사합니다😀

Copy link

@kang-hyungu kang-hyungu left a comment

Choose a reason for hiding this comment

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

또링 피드백 반영 잘 하셨어요
추가로 남긴 피드백은 다음 미션에 적용해보면 좋겠네요
코드 구현하느라 수고하셨어요
pr은 머지 처리할게요!

RestAssured.port = port;
}

protected StationResponse createStation(String name) {

Choose a reason for hiding this comment

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

createStation, getStations, createLine 처럼 특정 인수 테스트에서 사용하는 메서드는 해당 테스트 클래스로 옮겨주세요
AcceptanceTest는 모든 인수테스트에서 공통적으로 사용할 메서드만 남겨두는게 좋습니다

@kang-hyungu kang-hyungu merged commit 01ba824 into woowacourse:jnsorn May 31, 2020
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