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

[Step1] 🚀 1단계 - 지하철 구간 추가 기능 개선 #681

Open
wants to merge 7 commits into
base: oh980225
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import nextstep.subway.line.dto.CreateLineRequest;
import nextstep.subway.line.dto.LineResponse;
import nextstep.subway.line.dto.UpdateLineRequest;
import nextstep.subway.line.repository.Line;
import nextstep.subway.line.service.LineService;
import nextstep.subway.section.dto.CreateSectionRequest;
import org.springframework.http.ResponseEntity;
Expand Down
9 changes: 6 additions & 3 deletions src/main/java/nextstep/subway/line/service/LineService.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import nextstep.subway.line.repository.Line;
import nextstep.subway.line.repository.LineRepository;
import nextstep.subway.section.dto.CreateSectionRequest;
import nextstep.subway.section.policy.add.AddSectionPolicy;
import nextstep.subway.section.repository.Section;
import nextstep.subway.station.repository.Station;
import nextstep.subway.station.service.StationFindable;
Expand All @@ -21,6 +22,7 @@
class LineService implements LineFindable, LineSavable, LineUpdatable, LineDeletable {
Copy link

Choose a reason for hiding this comment

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

어떤 장점이 있어서 인터페이스를 implements 하신걸까요? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

사용하는 클라이언트 입장에서, 필요한 부분만 인터페이스로 사용할 수 있도록하는 것이 의도였습니다.
또한 필요한 기능만 인터페이스로 의존할 수 있기에 테스트시 fake 객체를 만들거나 할 때에도 테스트 코드를 작성할 때 더 용이하지 않을까 싶은 생각으로 이렇게 구현하였습니다.

private final LineRepository lineRepository;
private final StationFindable stationFindable;
private final AddSectionPolicy addSectionPolicy;

@Transactional
public Line saveLine(CreateLineRequest request) {
Expand Down Expand Up @@ -56,9 +58,10 @@ public void deleteLineById(Long id) {
@Transactional
public Line addSection(Long id, CreateSectionRequest request) {
Line line = findLineById(id);
System.out.println(line.getSections().size());
line.getSections().addSection(createSection(request.getUpStationId(), request.getDownStationId(), request.getDistance()));
System.out.println(line.getSections().size());
line.getSections().addSection(
createSection(request.getUpStationId(), request.getDownStationId(), request.getDistance()),
addSectionPolicy
);
return line;
}

Expand Down
19 changes: 19 additions & 0 deletions src/main/java/nextstep/subway/section/config/BeanConfig.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package nextstep.subway.section.config;

import nextstep.subway.section.policy.add.*;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;

import java.util.List;

@Configuration
public class BeanConfig {
@Bean
AddSectionPolicy addSectionPolicy() {
return new IntegrationAddSectionPolicy(List.of(
new CheckRegisterPolicy(),
new CheckNotRegisterPolicy(),
new CheckDistancePolicy()
));
}
}
21 changes: 0 additions & 21 deletions src/main/java/nextstep/subway/section/policy/AddSectionPolicy.java

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package nextstep.subway.section.policy.add;

import nextstep.subway.section.repository.Section;
import nextstep.subway.section.repository.Sections;

public interface AddSectionPolicy {
void validate(Sections sections, Section section);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package nextstep.subway.section.policy.add;

import nextstep.subway.section.repository.Section;
import nextstep.subway.section.repository.Sections;

import java.util.Optional;

public class CheckDistancePolicy implements AddSectionPolicy {
@Override
public void validate(Sections sections, Section section) {
Optional<Section> sectionEqualUpStation = sections.getSectionByUpStation(section.getUpStation());
Optional<Section> sectionEqualDownStation = sections.getSectionByDownStation(section.getDownStation());

if ((sectionEqualUpStation.isPresent() && (sectionEqualUpStation.get().getDistance() <= section.getDistance()))
|| (sectionEqualDownStation.isPresent() && (sectionEqualDownStation.get().getDistance() <= section.getDistance()))) {
Comment on lines +14 to +15
Copy link

Choose a reason for hiding this comment

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

조금 더 가독성 있게 바꿔볼 수 있을 것 같아요 😅

throw new RuntimeException("Section's distance too long");
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package nextstep.subway.section.policy.add;

import nextstep.subway.section.repository.Section;
import nextstep.subway.section.repository.Sections;
import nextstep.subway.station.repository.Station;

import java.util.Objects;
import java.util.Optional;

public class CheckNotRegisterPolicy implements AddSectionPolicy {
@Override
public void validate(Sections sections, Section section) {
Optional<Station> upStation = sections.getAllStation().stream()
.filter(station -> Objects.equals(station, section.getUpStation()))
.findFirst();
Optional<Station> downStation = sections.getAllStation().stream()
.filter(station -> Objects.equals(station, section.getDownStation()))
.findFirst();

if (upStation.isEmpty() && downStation.isEmpty()) {
throw new RuntimeException("Section's stations not exist in sections");
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package nextstep.subway.section.policy.add;

import nextstep.subway.section.repository.Section;
import nextstep.subway.section.repository.Sections;
import nextstep.subway.station.repository.Station;

import java.util.Objects;
import java.util.Optional;

public class CheckRegisterPolicy implements AddSectionPolicy {
@Override
public void validate(Sections sections, Section section) {
Optional<Station> upStation = sections.getAllStation().stream()
.filter(station -> Objects.equals(station, section.getUpStation()))
.findFirst();
Comment on lines +13 to +15
Copy link

Choose a reason for hiding this comment

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

중복되는 부분은 메서드 추출하는 게 어떨까요? 😄

Optional<Station> downStation = sections.getAllStation().stream()
.filter(station -> Objects.equals(station, section.getDownStation()))
.findFirst();

if (upStation.isPresent() && downStation.isPresent()) {
throw new RuntimeException("Section's stations already registered");
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package nextstep.subway.section.policy.add;

import lombok.RequiredArgsConstructor;
import nextstep.subway.section.repository.Section;
import nextstep.subway.section.repository.Sections;

import java.util.List;

@RequiredArgsConstructor
public class IntegrationAddSectionPolicy implements AddSectionPolicy {
Copy link

Choose a reason for hiding this comment

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

흥미로운 구조 👍

다만, IntegrationAddSectionPolicy 가 꼭 AddSectionPolicy 를 구현해야만 했나? 에 대해서는 의문이 듭니다.
오히려 구조가 너무 복잡해진 것 같아요 😢
동료가 유지보수 할 때, 직관적일지 고민해보셨을까요? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

저는 오히려 AddSectionPolicy 인터페이스를 구현함으로써 언제든 그 정책을 바꿀 수 있고, 테스트가 더 용이해졌다고 생각하였습니다.

또한 유지보수 측면에서도, 정책이 많아지고 복잡해져도, 각 정책이 섞일 일 없이 각자의 정책 객체로써 존재하게 될 것이고, 각 객체별로 테스트되고 있을 것이기에 괜찮지 않을까라고 생각하였습니다..! 만약 정책이 지금은 3개뿐이지만, 10개가 된다면? 그 이상이 된다면? 하나의 클래스가 그 정책을 다 담고 있는 것이 과연 단일 책임을 지킨 것인가? 그 코드는 유지보수하기 쉽고 이해하기 편한가? 오히려 모든 로직을 다 봐야 그제서야 이해할 수 있지 않을까? 라고 생각했던 것 같습니다.

물론 구조가 인터페이스를 두고 각 구현체들이 생기고 주입되는 과정이 생겨 더 복잡하고 오버엔지니어링이라고 볼수도 있을 것 같긴 합니다..! 😓 다만 정말 정책이 많았다면? 이와 반대로 그냥 모든 정책이 도메인 클래스 또는 하나의 클래스에 몰려있었다면? 과연 유지보수하기 쉬웠을까, 그 클래스의 테스트 코드는 읽기 쉬웠을까, 그 클래스를 주입받는 다른 클래스는 테스트하기 쉬웠을까를 생각했을 때는 잘 모르겠네요.. 물론 지금은 정책이 몇개 없긴합니다.. 🤣🤣

private final List<AddSectionPolicy> policyList;

public void validate(Sections sections, Section section) {
policyList.forEach(addSectionPolicy -> addSectionPolicy.validate(sections, section));
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package nextstep.subway.section.policy;
package nextstep.subway.section.policy.delete;

import nextstep.subway.section.repository.Sections;
import nextstep.subway.station.repository.Station;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
package nextstep.subway.section.repository;

import lombok.AccessLevel;
import lombok.Builder;
import lombok.Getter;
import lombok.NoArgsConstructor;
import lombok.*;
import nextstep.subway.station.repository.Station;

import javax.persistence.*;
Expand Down
69 changes: 57 additions & 12 deletions src/main/java/nextstep/subway/section/repository/Sections.java
Original file line number Diff line number Diff line change
@@ -1,34 +1,53 @@
package nextstep.subway.section.repository;
Copy link

Choose a reason for hiding this comment

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

의도는 이해가 가지만 현재 상황에서 repository 라는 패키지 분류는 오해를 부르기 쉬울 것 같아요 🤔
현재 사용하신 것은 domain 이나 entity 와 같은 패키지 이름이 더 어울릴것 같습니다

Copy link
Author

Choose a reason for hiding this comment

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

넵넵, entity 패키지에 별도로 넣어두겠습니다!


import lombok.AccessLevel;
import lombok.Getter;
import lombok.NoArgsConstructor;
import nextstep.subway.section.policy.AddSectionPolicy;
import nextstep.subway.section.policy.DeleteSectionPolicy;
import nextstep.subway.section.policy.add.AddSectionPolicy;
import nextstep.subway.section.policy.delete.DeleteSectionPolicy;
import nextstep.subway.station.repository.Station;

import javax.persistence.CascadeType;
import javax.persistence.Embeddable;
import javax.persistence.JoinColumn;
import javax.persistence.OneToMany;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.stream.Collectors;
import java.util.*;

@Embeddable
@NoArgsConstructor(access = AccessLevel.PROTECTED)
public class Sections {
@OneToMany(cascade = {CascadeType.PERSIST, CascadeType.REMOVE})
@OneToMany(cascade = {CascadeType.PERSIST, CascadeType.REMOVE}, orphanRemoval = true)
@JoinColumn(name = "line_id")
private List<Section> sections = new ArrayList<>();

public void addSection(Section section) {
AddSectionPolicy.validate(this, section);
@Getter
private Long firstStationId;

@Getter
private Long lastStationId;

public void addSection(Section section, AddSectionPolicy policy) {
policy.validate(this, section);
Optional<Station> upStation = getAllStation().stream()
.filter(station -> Objects.equals(station, section.getUpStation()))
.findFirst();
Optional<Station> downStation = getAllStation().stream()
.filter(station -> Objects.equals(station, section.getDownStation()))
.findFirst();
Comment on lines +31 to +36
Copy link

Choose a reason for hiding this comment

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

중복으로 보입니다 👀


if (upStation.isPresent() && upStation.get().getId().equals(lastStationId)) {
lastStationId = section.getDownStation().getId();
Copy link

Choose a reason for hiding this comment

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

디미터법칙을 보고 개선할 방법을 찾아보시면 좋을 것 같아요 😄

}
if (downStation.isPresent() && downStation.get().getId().equals(firstStationId)) {
firstStationId = section.getUpStation().getId();
}

this.sections.add(section);
}

public void deleteSectionByLastStation(Station station) {
DeleteSectionPolicy.validate(this, station);
this.lastStationId = getLastSection().getUpStation().getId();
this.sections.remove(getLastSection());
}

Expand All @@ -40,6 +59,14 @@ public Section getLastSection() {
return this.sections.get(this.sections.size() - 1);
}

public Optional<Section> getSectionByUpStation(Station upStation) {
return this.sections.stream().filter(section -> Objects.equals(section.getUpStation(), upStation)).findFirst();
}

public Optional<Section> getSectionByDownStation(Station downStation) {
return this.sections.stream().filter(section -> Objects.equals(section.getDownStation(), downStation)).findFirst();
}

public Station getDownEndStation() {
return this.sections.get(this.sections.size() - 1).getDownStation();
}
Expand All @@ -49,15 +76,33 @@ public Long getTotalDistance() {
}

public List<Station> getAllStation() {
List<Station> totalStation = this.sections.stream().map(Section::getUpStation).collect(Collectors.toList());
totalStation.add(this.sections.get(this.sections.size() - 1).getDownStation());
return Collections.unmodifiableList(totalStation);
List<Station> result = new ArrayList<>();
Long targetStationId = firstStationId;

while (targetStationId != null && !targetStationId.equals(lastStationId)) {
for (Section section : sections) {
if (Objects.equals(section.getUpStation().getId(), targetStationId)) {
Comment on lines +83 to +84
Copy link

Choose a reason for hiding this comment

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

객체지향생활체조

규칙 1: 한 메서드에 오직 한 단계의 들여쓰기(indent)만 한다.

result.add(section.getUpStation());
targetStationId = section.getDownStation().getId();
}
if (targetStationId.equals(lastStationId)) {
result.add(section.getDownStation());
break;
}
}
}

return Collections.unmodifiableList(result);
}

public Sections(List<Section> sections) {
Copy link

Choose a reason for hiding this comment

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

일반적으로 생성자는 메서드들중 클래스의 가장 상단에 위치할 것을 기대합니다

if (sections.isEmpty()) {
throw new RuntimeException("sections: at least one section is required");
}
this.sections = sections;
this.firstStationId = sections.get(0).getUpStation().getId();
Copy link

Choose a reason for hiding this comment

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

생성자 파라미터를 List 로 받지 않아도 될 것 같아요 😢

this.lastStationId = sections.get(sections.size() - 1).getDownStation().getId();
}


}
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
package nextstep.subway.unit.fixture;
package nextstep.fixture;

import nextstep.subway.line.repository.Line;

import static nextstep.subway.unit.fixture.SectionFixture.강남역_TO_신논현역;

public class LineFixture {
public static final Long 신분당선_ID = 1L;

Expand All @@ -12,7 +10,7 @@ public class LineFixture {
.builder()
.name("신분당선")
.color("bg-red-600")
.initSection(강남역_TO_신논현역())
.initSection(SectionFixture.강남역_TO_신논현역())
.build();
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package nextstep.subway.unit.fixture;
package nextstep.fixture;

import nextstep.subway.section.repository.Section;

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package nextstep.subway.unit.fixture;
package nextstep.fixture;

import nextstep.subway.station.repository.Station;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ void init() {
CREATE_LINE_URL = createdResponse.header("Location");
}

// TODO: 구간 추가에 대한 상세 사항은 Policy 의 단위 테스트에서 검증한다.

/**
* When 지하철 구간을 추가하면
* Then 지하철 노선 조회시, 추가된 구간을 확인할 수 있다
Expand All @@ -49,32 +51,6 @@ void createSection() {
추가한_구간이_노선에_포함된다(createdResponse, selectedResponse);
}

/**
* Given 하행역이 지하철 노선에 등록된 구간으로
* When 구간을 등록하면
* Then 에러가 발생한다
*/
@DisplayName("하행역이 이미 지하철 노선에 등록된 지하철 구간을 추가하면 에러가 발생한다.")
@Test
void createSection_already_register() {
CreateSectionRequest request = 하행역이_지하철_노선에_등록된_구간에_대한_요청이_존재한다();

구간을_등록하면_에러가_발생한다(request);
}

/**
* Given 상행역이 노선의 하행 종점역이 아닌 구간으로
* When 구간을 등록하면
* Then 에러가 발생한다
*/
@DisplayName("상행역이 노선의 하행 종점역이 아닌 지하철 구간을 추가하면 에러가 발생한다.")
@Test
void createSection_not_include() {
CreateSectionRequest request = 상행역이_노선의_하행_종점역이_아닌_구간에_대한_요청이_존재한다();

구간을_등록하면_에러가_발생한다(request);
}

/**
* Given 새로운 지하철 구간을 추가하고
* When 해당 노선의 구간을 제거하면
Expand All @@ -83,11 +59,8 @@ void createSection_not_include() {
@DisplayName("지하철 구간을 삭제한다.")
@Test
void deleteSection() {
ExtractableResponse<Response> selectedResponse = 지하철_노선을_조회한다();
ExtractableResponse<Response> createdResponse = 지하철_구간을_추가한다(UP_STATION_ID, DOWN_STATION_ID, DISTANCE);

지하철_구간을_삭제한다(createdResponse.header("Location") + "/sections?stationId=" + DOWN_STATION_ID);

삭제한_구간의_역이_노선에서_제외된다();
}

Expand Down
Loading