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.

Sign up for GitHub

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

Jump to bottom

🚀 1단계 - 구간 추가 기능 변경 #225

Open

sanhee wants to merge 34 commits into next-step:sanhee from sanhee:step1

sanhee commented Feb 5, 2022

뒤늦게 리뷰 요청드립니다.

요구사항에 있는 Line 단위테스트와 구간관리 인수테스트를 완료했습니다.

시간내어 리뷰해주셔서 감사합니다.

sanhee added 30 commits

February 2, 2022 15:29


          chore: 윈도우 파일 시스템에서 인식하지 못하는 경로 수정

6b26f4a

https://jkim83.tistory.com/148


          test: 노선을 생성한다.

7bb6b7c


          test: 모든 노선을 출력한다.

41e128f


          test: 특정 노선을 검색한다.

68c3026


          test: 존재하지 않는 노선을 검색한다.

1c8d780


          test: 노선 정보를 수정한다.

dc13b4b


          test: 노선을 삭제한다.

a88d7d9


          test: 노선에 구간을 추가한다.

136bacf


          test: Line 엔티티를 LineResponse로 변환한다.

5ea7fe7


          test: LineServiceMockTest - 가짜 협력 객체 사용 연습

64f3bb2


          refactor: 구간 추가 로직 Line 도메인으로 이전


          refactor: LineResponse, StationResponse 생성 책임 자신이 갖도록 변경

16c15d6


          chore: LineServiceTest, LineServiceMockTest 패키지 이동

73ef079


          feat: 노선에 속해있는 역 목록 조회

d4c6841


          feat: 구간이 목록에서 마지막 역 삭제

a0dcf15


          refactor: 노선 생성시 구간도 동시에 생성 가능하게끔 변경

497bdf2


          feat: 구간 추가 - 상행역이 같은 경우 / 새로운 구간의 거리가 작은 경우

b8ec505


          feat: 구간 추가 - 상행역이 같은 경우 / 기존 구간의 거리가 작은 경우


          feat: 구간 추가 - 새로운 역을 상행 종점으로 등록할 경우

cf3985c


          test: 새로운 역을 하행 종점으로 등록할 경우

58da291


          refactor: 노선 조회시 응답되는 역 목록 수정

e296504

상행 종점이 상행역인 구간을 먼저 찾는다.
그 다음, 해당 구간의 하행역이 상행역인 다른 구간을 찾는다.
2번을 반복하다가 하행 종점역을 찾으면 조회를 멈춘다.


          feat: 역 사이에 새로운 역을 등록할 경우 기존 역 사이 길이보다 크거나 같으면 등록을 할 수 없음

ca081f9


          feat: 상행역과 하행역이 이미 노선에 모두 등록되어 있다면 추가할 수 없음

80ade28


          feat: 상행역과 하행역 둘 중 하나도 포함되어있지 않으면 추가할 수 없음

41a1510


          test: 구간 추가 - 상행역이 같은 경우에 새로운 역을 등록

3d7384a

오늘의 교훈
- 함수 오버라이드를 조심하자..

List<Section> addSection(Section newSection)

List<Section> addSection(Station upStation, Station downStation, int distance)


          test: 새로운 역을 하행 종점으로 등록할 경우

1dd43a5


          test: 역 사이에 새로운 역을 등록할 경우 기존 역 사이 길이보다 크거나 같으면 등록을 할 수 없음

c9c1f1c


          test: 상행역과 하행역이 이미 노선에 모두 등록되어 있다면 추가할 수 없음

17f5336


          test: 상행역과 하행역 둘 중 하나도 포함되어있지 않으면 추가할 수 없음

dad7461


          refactor: 지하철 구간 관리 기능 인수테스트

34c2341

kang-hyungu requested changes

View reviewed changes

kang-hyungu left a comment

안녕하세요 노을님, 만나서 반갑습니다. 리뷰어 강현구입니다.
먼저 몇 가지 피드백 남겼습니다. 코드 리팩터링 해보시고 리뷰 재요청주세요.

src/main/java/nextstep/subway/applicaion/LineService.java

+                      Line line = lineRepository.save(new Line(
+                              request.getName(),
+                              request.getColor()
+                      ));
                       if (request.getUpStationId() != null && request.getDownStationId() != null && request.getDistance() != 0) {

kang-hyungu Feb 6, 2022

if절 조건이 복잡하니 메서드로 추출해보는게 좋겠네요

src/main/java/nextstep/subway/applicaion/LineService.java Outdated

                   }
                   @Transactional(readOnly = true)
                   public LineResponse findById(Long id) {
-                      return createLineResponse(lineRepository.findById(id).orElseThrow(IllegalArgumentException::new));
+                      Line findLine = lineRepository.findById(id)

kang-hyungu Feb 6, 2022

findById(id).orElseThrow 이 코드가 중복해서 발생하고 있으니 메서드로 추출해주면 어떨까요

src/main/java/nextstep/subway/applicaion/LineService.java Outdated

                       Station station = stationService.findById(stationId);
-                      if (!line.getSections().get(line.getSections().size() - 1).getDownStation().equals(station)) {
+                      if (!line.getSections()

kang-hyungu Feb 6, 2022

객체지향 생활체조
4. 한 줄에 점을 하나만 찍는다.

line 객체 내부로 조건을 옮겨보시기 바랍니다.

src/main/java/nextstep/subway/applicaion/LineService.java Outdated

                           throw new IllegalArgumentException();
                       }
-                      line.getSections().remove(line.getSections().size() - 1);
+                      line.getSections()

kang-hyungu Feb 6, 2022

객체지향 생활체조
4. 한 줄에 점을 하나만 찍는다.

src/main/java/nextstep/subway/applicaion/dto/ErrorResponse.java Outdated


		import java.time.LocalDateTime;

		public class ErrorResponse{

kang-hyungu Feb 6, 2022

Suggested change

      
            public class ErrorResponse{
          
            public class ErrorResponse {

src/main/java/nextstep/subway/domain/Line.java Outdated


		private List<Section> addSection(Section newSection) {

		AtomicBoolean normalCondition = new AtomicBoolean(true);

kang-hyungu Feb 6, 2022

Atomic 클래스를 사용하신 이유가 있을까요? 동시성 문제가 없는 걸로 보이네요
각 메서드들이 이 변수에 의존하는 로직을 만드신 것 같네요. 외부 조건에 의존하지 않는 독립적인 메서드로 만들 방법은 없을까요?

src/main/java/nextstep/subway/domain/Line.java Outdated

+                                   .filter(station -> !getStations().contains(newSection.getUpStation()))
+                                   .filter(section -> !getStations().contains(newSection.getDownStation()))
+                                   .findAny()
+                                   .ifPresent(unused -> {

kang-hyungu Feb 6, 2022

ifPresent는 예외 처리할 때 사용하지 않습니다.
orElseThrow로 적용해보시기 바랍니다.

src/main/java/nextstep/subway/domain/Line.java Outdated

+                      for (Section section : sections) {
+                          Station upStation = section
+                                  .getUpStation();

kang-hyungu Feb 6, 2022

엔터로 구분해야 될 만큼 복잡한 코드가 아닌 것 같으니 불필요한 엔터는 지양합시다.

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

+                          }
+                          if (upStation.equals(prevDownStation)) {
+                              if (downStation.equals(newSection.getDownStation())) {

kang-hyungu Feb 6, 2022

for, if로 이어지는 2뎁스 코드들이 보이네요

1뎁스만 사용하도록 메서드를수정해보시고
메서드를 10줄 이내로 줄여보세요.

src/main/java/nextstep/subway/domain/Line.java Outdated

+                                              newSection.getUpStation(),
+                                              (oldSection.getDistance() - newSection.getDistance())
+                                      ));
+                                  } else if (newSection.getDistance() > oldSection.getDistance()) {

kang-hyungu Feb 6, 2022

else if를 사용하지 않도록 바꿔보시기 바랍니다. else if가 많아지면 코드 복잡도가 올라갑니다.
dto로 값을 전달할 때 외에는 getter를 사용하지 마세요. section 객체 내부에서 처리하도록 코드를 수정하면 line 클래스의 복잡도를 낮출 수 있을겁니다.

sanhee added 4 commits

February 7, 2022 15:59


          refactor: 노선 저장시 추가로 구간을 저장하는 로직 메서드로 분리

0a252c0

next-step#225 (comment)


          refactor: 객체지향 생활체조 참고하여 lineService 리팩토링

cd6e402

next-step#225 (comment)

next-step#225 (comment)

next-step#225 (comment)

next-step#225 (comment)


          refactor: DTO에서 spring 의존 애노테이션 제거

13e4d71

next-step#225 (comment)


          refactor: Line 구간 추가 로직 의존 제거 시도 (아직 진행중)

7ada152

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet