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 지하철 노선도 - 3단계] 후디(조동현) 미션 제출합니다. #312

Merged
merged 27 commits into from
May 15, 2022

Conversation

devHudi
Copy link

@devHudi devHudi commented May 12, 2022

안녕하세요, 앨런!
이번 미션은 살짝 시간에 쫒기면서 만들게 되어 조금 아쉽네요 🥲
각 레이어간 책임을 어떻게 만들고, 도메인 객체간의 의존을 직접참조로 해야할지 데이터베이스를 통한 간접참조로 해야할지가 가장 핵심적인 고민이었습니다. 질문이 조금 많을 것 같은데 나머지는 코멘트로 남겨두겠습니다!

Comment on lines 16 to 22

public void createSection(Long lineId, SectionRequest sectionRequest) {
Sections sections = sectionDao.findSectionsByLineId(lineId);
sections.addSection(sectionRequest.toSection());
sectionDao.saveSections(lineId, sections);
}

Copy link
Author

Choose a reason for hiding this comment

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

이 도메인 객체를 포함하여 많은 객체의 의존관계를 직접 참조 (객체를 직접 필드로 들고있음) 대신 간접 참조 (데이터베이스의 ID값을 들고있음) 를 통해 이루어지고 있습니다.

예를 들어 Line 으로부터 Sections 를 가지고 오고 싶을때, 객체지향적인 코드라면 line.getSections() 등을 통해 Line 이 직접 들고있는 Sections 를 가지고 오면 됩니다.

하지만 지금 Line 클래스는 아래와 같이 Sections 에 대해 전혀 모르고 있습니다. 반대로 SectionsLine 을 전혀 모르고 있는 상황이구요.

public class Line {
    private Long id;
    private String name;
    private String color;

// ...

Line과 Section 사이의 의존 관계는 데이터베이스에서'만' 찾아볼 수 있습니다.

create table if not exists SECTION
(
    id bigint auto_increment not null,
    line_id bigint not null, /* <== */
    up_station_id bigint not null,
    down_station_id bigint not null,
    distance int,
    primary key(id)
);

이게 과연 객체지향적인 코드라고 할 수 있을지 코드를 짜며 많은 의문이 들었습니다. 진짜 객체지향적인 코드를 짜려면 이미 Line 의 인스턴스의 필드로 Sections 가 생성되어 메모리에 로드되어 있는 상태여야한다고 생각했습니다. 데이터베이스는 그런 LineSections 의 상태를 저장만 하는 역할을 수행하구요.

하지만 생각해보니 이런 구조도 문제가 있습니다. 지금이야 역이 많아봤자 100개 남짓될 것이지만, 예를 들어 나중에 관리해야할 역이 100만개가 된다면 이 많은 데이터를 한번에 메모리에 올리는 것은 성능상의 많은 이슈를 일으킬 것 같습니다.

메모리 효율과 객체지향적인 구조 둘중 하나는 일부분 포기해야하는 트레이드 오프인걸까요? 아니면 더 좋은 설계 방법이 있을까요? 🤔

앨런의 의견이 궁금합니다!

Choose a reason for hiding this comment

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

후디가 line가 sections 갖는게 자연스럽다면 도메인객체를 바꿔볼 수 있지않을까요?

line 도메인객체에 sections를 추가해볼 수 있을 것 같아요. 데이터를 가져오는건 DAO단에 맡겨보시죠!

또 성능문제관련인데요. 정말 노선에 비즈니스적으로 100만개의 섹션이 필요한가? 라는 생각도 들었습니다 👀

Copy link
Author

Choose a reason for hiding this comment

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

앨런 말씀대로 지금의 도메인에서는 100만개의 섹션이 등록될일은 없을 것 같아요. 일단 현재 주어진 도메인에 집중해서 개발해보도록 할게요 🙂

Comment on lines 5 to 12

public class Line {
private Long id;
private String name;
private String color;

private Line() {
}
Copy link
Author

Choose a reason for hiding this comment

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

지금 코드에서 많은 도메인 객체가 Id 라는 필드를 가지고 있습니다. Id 는 데이터베이스에서 각 행을 식별하기 위해 PK 로 사용되고 있는 값 입니다. 도메인 객체가 데이터베이스의 내부 구현을 알고 있는 모양새인 것 같습니다.

객체지향적으로 프로그래밍 하기 위해서는 도메인 객체가 데이터베이스 내부 구현에는 전혀 관심갖지 않고, 다른 방법으로 객체간의 식별을 해야한다고 생각합니다. 예를 들어 VO라면 들고있는 값으로만 서로를 식별하고, 그렇지 않다면 서로 다른 메모리 주소를 갖는지 여부를 통해 서로를 식별해야한다고 생각합니다.

또, Id 라는 값이 비즈니스적으로 의미가 있는 값 인지도 잘 모르겠습니다.

지금이야 도메인 객체를 DTO로 매핑할 때 Id 값이 필요하기 때문에, 그리고 아직 더 나은 설계 방법을 정하지 못하였기 때문에 편의를 위해 도메인 객체가 Id 를 필드로 들고있는 구조로 만들었습니다. 하지만 아직 어딘가 기분이 개운치는 않아요.

이에 대한 앨런의 의견이 궁금합니다! (이럴때 엔티티 클래스 도입을 고려하는 것인가요? 🤔)

Copy link

@hongbin-dev hongbin-dev May 13, 2022

Choose a reason for hiding this comment

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

도메인 객체가 데이터베이스의 내부 구현을 알고 있는 모양새인 것 같습니다.

이건 생각하기 나름인 것 같습니다. 개인적으로 도메인의 식별자는 데이터베이스의 의존하지 않는다고 생각해요. (물론 id의 생성은 데이터베이스의 의존적이긴합니다.)

객체지향적으로 프로그래밍 하기 위해서는 도메인 객체가 데이터베이스 내부 구현에는 전혀 관심갖지 않고, 다른 방법으로 객체간의 식별을 해야한다고 생각합니다.

동일한객체의 기준이 꼭 메모리의 주소가 같아야 동일한 객체일까요? 그렇다면 제약되는부분이 많을 것 같아요.
비즈니스적으로 동일한객체를 만들어야할 수도 있을 것 같고, 도메인객체내에서 불변객체로 만들 수도 없을 것 같아요.

또 DAO 같은 외부리소스에서 findById() 메서드를 두번 호출한 값은 다르다고 판단하지않을까요?

엔티티클래스를 도입하면 어떻게 달라질까요🤔 도메인객체에 id가 불필요해질까요?

Copy link
Author

Choose a reason for hiding this comment

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

동일한객체의 기준이 꼭 메모리의 주소가 같아야 동일한 객체일까요?

지금 다시 생각해보니 꼭 그렇지는 않은 것 같아요. 심지어 모든 필드가 다르지만, id 가 같다면 같은 객체라고 식별할수도 있지 않을까? 라는 생각도 드네요! 오래 생각해보았는데 아직 스스로 명확하게 답을 내리기 어려운 것 같아요. 미션을 좀 더 진행해보며 계속 고민해보겠습니다. 💪

Comment on lines 117 to 145
@DisplayName("노선 정보를 수정한다.")
@Test
void updateLineById() {
// given
String name = "1호선";
String color = "bg-green-600";
Long upStationId = createdStationId1;
Long downStationId = createdStationId2;
Integer distance = 10;

ExtractableResponse<Response> createResponse = requestCreateLine(name, color, upStationId, downStationId,
distance);
Long createdId = getIdFromResponse(createResponse);

// when
String newName = "2호선";
String newColor = "bg-green-600";
requestUpdateLine(createdId, newName, newColor);
ExtractableResponse<Response> response = requestGetLineById(createdId);

// then
String responseName = response.jsonPath().getString("name");
String responseColor = response.jsonPath().getString("color");

assertAll(
() -> assertThat(responseName).isEqualTo(newName),
() -> assertThat(responseColor).isEqualTo(newColor)
);
}
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 테스트인데, 수정이 잘 되었는지 노선 조회 API도 추가로 호출합니다.

사실 관련된 테스트는 service 와 dao 에서 어느정도 커버가 되었다고 생각되어 '굳이 조회를 한번 더 해야하나?' 라는 생각이 듭니다. 앨런은 어떻게 생각하시나요?

Copy link

@hongbin-dev hongbin-dev May 13, 2022

Choose a reason for hiding this comment

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

단위테스트라면 수정하는 API만 호출하는게 좋겠지만, 시나리오기반인 인수테스트라서 크게 어색하진 않은 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

그렇군요, 감사합니다!

Comment on lines 37 to 58
@Test
void findAll() {
Line line1 = new Line("신분당선", "bg-red-600");
Line line2 = new Line("분당선", "bg-green-600");

lineDao.save(line1);
lineDao.save(line2);

List<Line> lines = lineDao.findAll();
List<String> lineNames = lines.stream()
.map(Line::getName)
.collect(Collectors.toList());
List<String> lineColors = lines.stream()
.map(Line::getColor)
.collect(Collectors.toList());

assertAll(
() -> assertThat(lineNames).containsAll(List.of("신분당선", "분당선")),
() -> assertThat(lineColors).containsAll(List.of("bg-red-600", "bg-green-600"))
);
}

Copy link
Author

Choose a reason for hiding this comment

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

현재 도메인 객체는 id 라는 필드를 가지고 있고, 데이터베이스에 저장되지 않은 객체는 id == null, 저장된 객체는 id != null 입니다. 이런 구조를 갖게되어 겪은 문제점이 두가지 있습니다.

  1. 테스트가 어렵다.
  2. NPE 위험성을 갖는다.

첫번째로는 저장할 때 생성한 Line 인스턴스와 데이터베이스에서 조회하여 Dao 가 반환해준 Line 인스턴스는 값이 같을 수 있으나 id 필드만 달라 위 코드처럼 필드만 따로 꺼내와서 비교해야한다는 단점이 존재합니다. 물론 equals, hashCodeid 를 제외하고 비교하도록 재정의할 수는 있습니다. 하지만 다른 개발자가 직접 동등성 비교를 할때 equals 의 비교 대상에 id가 포함되는지 포함되지 않는지 코드를 봐야 알 수 있기 때문에 좋은 방법은 아니라고 생각합니다.

두번째로는 getId() 등을 호출해 그 값을 사용하는 코드는 모두 잠재적으로 NPE위험성을 갖는다는 점 입니다. 실제로 개발하면서 NPE 문제를 조금 겪었습니다. (Optional 로 반환할까 생각도 했지만, 일단은 생각에서 멈췄습니다.)

결론적으로 null 이 될 수 있는 필드를 만드는 것이 바람직한 방법인가? 에 대한 의문점이 아직 풀리지 않았습니다. 이에 대한 앨런의 의견이 궁금합니다!

Choose a reason for hiding this comment

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

결론적으로 null 이 될 수 있는 필드를 만드는 것이 바람직한 방법인가? 에 대한 의문점이 아직 풀리지 않았습니다. 이에 대한 앨런의 의견이 궁금합니다!

개인적으로 바람직하진 않다고 생각합니다. null이 있다는 건 불안정한 객체라고 생각해요.

후디는 �어떤 게 문제고, 어떻게 해결할 수 있는지 생각해보셨을까요? 의견 말씀해주시면 좋을 것 같아요 🙂

Copy link
Author

Choose a reason for hiding this comment

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

  1. id 필드가 있는 객체와 없는 객체를 별개로 만든다.
  2. id 필드의 getter 의 반환 타입을 Optional 로 한다.

첫번째는 해결하려는 문제는 DRY 하지 않으므로 좋은 방법이 아니라고 생각합니다.

두번째는 적어도 외부에서 객체의 id 필드를 사용할 때에는 NPE의 위험에서 벗어날 수 있다고 생각합니다. 하지만 객체 내부는 마찬가지로 null 체크를 철저히 하지 않으면 NPE가 발생할 위험이 그대로 존재합니다. 단, 이 문제도 객체 내부에서 id 필드를 직접 참조하지 않고 getter 를 사용해서 참조하면 Optional 로 사용이 가능하기 때문에 어느정도 커버가 될 것 같네요!

@devHudi
Copy link
Author

devHudi commented May 12, 2022

인수 테스트와 E2E 테스트의 의미가 다소 모호하게 다가오는데요!

그래서 한번 찾아보니 (https://www.quora.com/What-is-the-difference-between-user-acceptance-testing-and-end-to-end-testing) 인수 테스트는 프로그램이 개발자의 손을 떠나서, 실제 프로그램을 사용하는 대상이 직접 프로그램을 사용하는 테스트 과정을 의미한다고 합니다. E2E 테스트는 유닛, 통합 테스트보다 더 넓은 범주에서 기능 단위로 처음부터 끝까지 테스트하는 것이라고 합니다.

그렇다면 지금 AcceptanceTest 라고 되어있는 것들은 명확하게는 E2E 테스트인건가요? 아니면 제가 잘 못 이해하고 있는 것 일까요?

@devHudi
Copy link
Author

devHudi commented May 12, 2022

생성자에 Autowired를 적어야하는 번거로움과 프레임워크에 의존하는 코드를 줄일 수 있는 장점이 있는 것 같은데요.
명시성은 어떤 걸 의미하는지 알 수 있을까요?
#184 (comment)

지난 미션에 처음으로 스프링을 접해보고, IoC와 Spring Bean에 대해서도 알게되었습니다. 생성자가 하나일 경우 자동으로 빈이 주입되는 것은 알고있었지만, 다소 헷갈릴수도 있다고 생각하여 '이 메소드는 스프링 빈을 주입받는 메소드야' 라고 명시하고 싶어서 굳이 @Autowired 를 적어두었습니다!

@devHudi
Copy link
Author

devHudi commented May 12, 2022

이 부분은 상수로 관리할 수 있겠네요~
#184 (comment)

private static final 로 변경하였고, 네이밍도 CONSTANT_CASE 로 변경했습니다!

@devHudi
Copy link
Author

devHudi commented May 12, 2022

id, name, color 세 필드로 동일성을 구분하게 하신 이유가 있을까요?
#184 (comment)

Line은 동등성 비교를 할 필요가 없으므로 equals, hashCode 메소드 재정의를 제거하였습니다!

@devHudi
Copy link
Author

devHudi commented May 12, 2022

후디가 deleteById의 정책을 어떻게 세우셨나 궁금했습니다! 네이밍을 봤을 때, ID로 row를 삭제할 책임을 가져야한다고 생각이 들었습니다.
정책을 어떻게 세우느냐에 따라 다르겠지만, id가 존재하지않으면 예외를 줘서 빠르게 피드백받는게 좋다고 생각했습니다.
update()에서 row를 지운 갯수를 반환하니, 그 값을 활용할 수 있을 것 같아요.
#184 (comment)

저는 Dao 의 책임은 쿼리를 캡슐화하고, 실행하는 수준이라고 생각했어요. SQL 문법에 어긋나거나, 데이터베이스단의 문제가 생기지 않은 이상 쿼리가 데이터베이스단의 오류 없이 문제없이 동작했다면 (설령 DELETE 쿼리가 아무 행도 지우지 않더라도) 정상 동작이라고 생각했습니다!

따라서 예외처리의 책임을 서비스 레이어에게 할당했는데요! 제가 Dao의 책임범위를 잘 못 이해하고 있는 것인지 궁금합니다!

@devHudi devHudi changed the base branch from master to devhudi May 12, 2022 04:51
}

public void saveSections(Long lineId, Sections sections) {
removeAllSectionsByLineId(lineId);

Choose a reason for hiding this comment

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

기존에 line_id를 모두 비우고, sections를 저장하는 것 같은데요. 이 부분이 DAO에 넣으신 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

DAO의 메소드를 단일쿼리만 담당하도록 최대한 단순하게 변경하였고, saveSectionSectionService 로 옮겼습니다!

Comment on lines 6 to 9
public Distance(int value) {
this.value = value;
validatePositive(value);
}

Choose a reason for hiding this comment

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

개인적으로 값이 할당되기 전에 검증하는걸 선호하는 편입니다!
원자성을 갖을 수 있을 것 같아요.

Suggested change
public Distance(int value) {
this.value = value;
validatePositive(value);
}
public Distance(int value) {
validatePositive(value);
this.value = value;
}

Copy link
Author

Choose a reason for hiding this comment

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

동시성 이슈를 해결하기 위해 코드를 위와 같이 작성했는데, 지금 생각해보니 참조값이 아니라 원시값이라서 검증과 할당 사이에 값이 바뀔 걱정을 하지 않아도 괜찮군요 😅

Comment on lines +86 to +103
@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
Section section = (Section) o;
return Objects.equals(id, section.id) && Objects.equals(upStationId, section.upStationId)
&& Objects.equals(downStationId, section.downStationId) && Objects.equals(distance,
section.distance);
}

@Override
public int hashCode() {
return Objects.hash(id, upStationId, downStationId, distance);
}

Choose a reason for hiding this comment

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

👍

Comment on lines +20 to +24
private void validateEmptySections(List<Section> sections) {
if (sections.isEmpty()) {
throw new IllegalArgumentException("비어있는 구간 목록은 생성할 수 없습니다.");
}
}

Choose a reason for hiding this comment

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

👍

Comment on lines 30 to 32
if (isNeededToInsert(newSection)) {
insertSection(newSection);
}

Choose a reason for hiding this comment

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

왜 insert가 필요한지 코드에서 설명해줘야한다고 생각해요.
메서드 네이밍을 객체에게 메시지를 보낸다고, 생각하고 작성하면 좋을 것 같습니다.

Suggested change
if (isNeededToInsert(newSection)) {
insertSection(newSection);
}
if (갈래길이 생길 있는가?(newSection)) {
중간역수정(newSection);
}

Comment on lines +99 to +110
public void deleteStation(Long stationId) {
validateHasSingleSection();

Optional<Section> upSection = getUpSection(stationId);
Optional<Section> downSection = getDownSection(stationId);

upSection.ifPresent(sections::remove);
downSection.ifPresent(sections::remove);

if (upSection.isPresent() && downSection.isPresent()) {
addMergedSection(upSection.get(), downSection.get());
}

Choose a reason for hiding this comment

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

비즈니스가 굉장히 깔끔하네요 💯💯

@@ -54,19 +80,35 @@ public void delete(Long id) {
lineDao.deleteById(id);

Choose a reason for hiding this comment

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

만약 line이 삭제되면 어떻게될까요?

Copy link
Author

Choose a reason for hiding this comment

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

질문을 잘 이해하지 못했어요 🥲

Choose a reason for hiding this comment

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

앗! 제가 말을 잘못썼군요 😮
'line을 삭제하면 section은 어떻게되나요?' 를 여쭤보고 싶었습니다

Copy link
Author

Choose a reason for hiding this comment

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

아하! 지금 확인해보니 schema.sql 에 cascade 제약 조건이 없네요! line 이 제거되었을 때 관련된 section 도 함께 제거하기 위해서는

  1. SQL 단에서 SECTION 테이블에 cascade 제약조건을 추가한다. (그 전에 외래키 설정도 해야할 것 같습니다.)
  2. 이는 비즈니스 로직 중 일부이므로 LineService 에서 직접 SectionDao 를 호출하여 관련된 행을 모두 제거한다.

이 두가지 방법이 생각나요.

첫번째 방법은 비즈니스 로직의 구현을 데이터베이스 단에서 구현하는 모양새여서 레이어간 책임 할당이 제대로 되지 않은 느낌이고, 두번째 방법은 비즈니스 로직이 제대로 수행이 되지 않았을 때 디비의 무결성이 깨질 것 같은 불안감이 다소 존재하는 것 같아요.

첫번째 방법과 두번째 방법을 둘다 하는 방법도 생각이 났는데, 그러기에는 cascade 제약사항 때문에 이미 line 이 제거된 상태에서 sectionDao 가 삭제할 Section 이 없다는 문제점이 있는 것 같습니다.

앨런이 보시기에는 어떤 방식으로 구현하는게 가장 바람직하다고 생각되시나요? 🤔

Choose a reason for hiding this comment

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

앗 제가 답변을 못드렸군요 🥲 DM으로 리마인드주셔서 감사해요~

Comment on lines 95 to 99
return stationIds.stream()
.map(stationDao::findById)
.filter(Optional::isPresent)
.map(Optional::get)
.map(StationResponse::from)
.collect(Collectors.toList());

Choose a reason for hiding this comment

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

여기서 N번이상의 쿼리로 성능이슈가 발생할 수 있겠네요.

Copy link
Author

Choose a reason for hiding this comment

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

StationDao에 아래와 같이 메소드를 추가하고 LineService 에서 호출하는 방식으로 변경하였습니다.

    public List<Station> findAllByIds(List<Long> ids) {
        String inSql = String.join(",", Collections.nCopies(ids.size(), "?"));
        String sql = String.format("SELECT id, name FROM STATION WHERE id IN (%s)", inSql);
        return jdbcTemplate.query(sql, STATION_ROW_MAPPER, ids.toArray());
    }

Choose a reason for hiding this comment

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

Collections.nCopies()는 처음보는군요!

알아보니 NamedParameterJdbcTemplate를 사용하면 SELECT id, name FROM STATION WHERE id IN (:ids) 형태로 쿼리를 풀어낼 수 있어보이네요.

https://www.baeldung.com/spring-jdbctemplate-in-list

Copy link
Author

Choose a reason for hiding this comment

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

좋은 레퍼런스 감사합니다! NamedParameterJdbcTemplate 한번 공부해봐야겠네요 😄

Comment on lines 104 to 109
Sections sections = sectionDao.findSectionsByLineId(lineId);

List<Long> stationIds = new ArrayList<>();
for (Section section : sections.getValue()) {
stationIds.add(section.getUpStationId());
stationIds.add(section.getDownStationId());

Choose a reason for hiding this comment

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

stationIds가 가변인데 stream으로 처리해볼 수 없을까요?

Copy link
Author

@devHudi devHudi May 14, 2022

Choose a reason for hiding this comment

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

    private List<Long> getStationIdsByLineId(Long lineId) {
        List<Section> sections = sectionDao.findSectionsByLineId(lineId).getValue();

        return Stream.concat(
                sections.stream().map(Section::getDownStationId),
                sections.stream().map(Section::getUpStationId)
        ).distinct().collect(Collectors.toList());
    }

위와 같이 변경했습니다! 확실히 가독성이 증가한 것 같습니다.

for 로 하면 한번의 루프지만, stream 으로 하면 두번의 루프로 돌아갈 것 같습니다. 현재 코드에서 stream 을 사용했을 때 제가 알지 못하는 이점이 있을까요?

Choose a reason for hiding this comment

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

return sections.stream()
            .flatMap(section -> Stream.of(section.getDownStationId(), section.getUpStationId()))
            .distinct()
            .collect(Collectors.toList());

flatMap을 이용하면 조금 더 나은방향으로 변경할 수 있을 것 같아요.

아래 가벼운 예제가 있으니 참고하면 좋을 것 같아요~
https://www.baeldung.com/java-difference-map-and-flatmap

Copy link
Author

Choose a reason for hiding this comment

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

좋은 레퍼런스 감사드립니다 🙇‍♂️ flatMap 에 대해서 공부해봐야겠군요

Comment on lines 16 to 22

public void createSection(Long lineId, SectionRequest sectionRequest) {
Sections sections = sectionDao.findSectionsByLineId(lineId);
sections.addSection(sectionRequest.toSection());
sectionDao.saveSections(lineId, sections);
}

Choose a reason for hiding this comment

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

후디가 line가 sections 갖는게 자연스럽다면 도메인객체를 바꿔볼 수 있지않을까요?

line 도메인객체에 sections를 추가해볼 수 있을 것 같아요. 데이터를 가져오는건 DAO단에 맡겨보시죠!

또 성능문제관련인데요. 정말 노선에 비즈니스적으로 100만개의 섹션이 필요한가? 라는 생각도 들었습니다 👀

@hongbin-dev
Copy link

hongbin-dev commented May 13, 2022

인수 테스트와 E2E 테스트의 의미가 다소 모호하게 다가오는데요!
그래서 한번 찾아보니 (https://www.quora.com/What-is-the-difference-between-user-acceptance-testing-and-end-to-end-testing) 인수 테스트는 프로그램이 개발자의 손을 떠나서, 실제 프로그램을 사용하는 대상이 직접 프로그램을 사용하는 테스트 과정을 의미한다고 합니다. E2E 테스트는 유닛, 통합 테스트보다 더 넓은 범주에서 기능 단위로 처음부터 끝까지 테스트하는 것이라고 합니다.
그렇다면 지금 AcceptanceTest 라고 되어있는 것들은 명확하게는 E2E 테스트인건가요? 아니면 제가 잘 못 이해하고 있는 것 일까요?

인수테스트의 의미는 사용자기반으로 시나리오를 테스트하는게 관점인데요. 두 개의 관점이 다른 것 같습니다. 테스트 환경, 테스트 목적의 차이랄까요?
후디는 시나리오기반 테스트 + SpringBootTest로 실제와같은 E2E 테스트를 하고 있는 것 같습니다.

https://tecoble.techcourse.co.kr/post/2021-05-25-unit-test-vs-integration-test-vs-acceptance-test/

@hongbin-dev
Copy link

지난 미션에 처음으로 스프링을 접해보고, IoC와 Spring Bean에 대해서도 알게되었습니다. 생성자가 하나일 경우 자동으로 빈이 주입되는 것은 알고있었지만, 다소 헷갈릴수도 있다고 생각하여 '이 메소드는 스프링 빈을 주입받는 메소드야' 라고 명시하고 싶어서 굳이 @Autowired 를 적어두었습니다!

개인적으로 프레임워크의 의존을 줄이는게 좋다고 생각합니다. 생성자에 @Autowired를 생략해도 되는 것도 비슷한의미의 �feature가 아니었을까요?

Copy link

@hongbin-dev hongbin-dev left a comment

Choose a reason for hiding this comment

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

안녕하세요 후디!
미션이 어려운 것 같은데 잘 구현해주셨군요! 특히 도메인쪽 코드가 잘 정리되어있어서 쉽게 이해할 수 있었습니다. 💯💯
코멘트와 답변 남겼으니 확인해주세요~

@devHudi
Copy link
Author

devHudi commented May 14, 2022

지난 미션에 처음으로 스프링을 접해보고, IoC와 Spring Bean에 대해서도 알게되었습니다. 생성자가 하나일 경우 자동으로 빈이 주입되는 것은 알고있었지만, 다소 헷갈릴수도 있다고 생각하여 '이 메소드는 스프링 빈을 주입받는 메소드야' 라고 명시하고 싶어서 굳이 @Autowired 를 적어두었습니다!

개인적으로 프레임워크의 의존을 줄이는게 좋다고 생각합니다. 생성자에 @Autowired를 생략해도 되는 것도 비슷한의미의 �feature가 아니었을까요?

아하 그러네요! @Autowired 를 붙이지 않아도 자동으로 의존성 주입이 되므로, 어노테이션을 제거하고 POJO로 만들 수 있겠군요. 😁😁

Copy link

@hongbin-dev hongbin-dev left a comment

Choose a reason for hiding this comment

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

안녕하세요 후디!
피드백 반영 잘 해주셨군요 💯💯
전반적으로 도메인과 테스트코드가 잘 작성되어서 읽기 좋았습니다 🙂
머지해도 좋을 것 같네요! 3단계 미션 고생 많으셨습니다

Comment on lines +53 to +57
public List<Station> findAllByIds(List<Long> ids) {
String inSql = String.join(",", Collections.nCopies(ids.size(), "?"));
String sql = String.format("SELECT id, name FROM STATION WHERE id IN (%s)", inSql);
return jdbcTemplate.query(sql, STATION_ROW_MAPPER, ids.toArray());
}

Choose a reason for hiding this comment

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

@hongbin-dev hongbin-dev merged commit 7f09931 into woowacourse:devhudi May 15, 2022
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.

2 participants