[Spring 지하철 노선도 - 3단계] 오찌(오지훈) 미션 제출합니다. #323

Ohzzi · 2022-05-12T07:59:31Z

안녕하세요 던! :)

이번에 테코톡이 있어서 테코톡 준비랑 병행하다 보니까 PR도 늦고 코드 퀄리티도 굉장히 떨어지게 되었네요 😢
새로 추가된 로직들을 구현하는 것 부터 굉장히 힘든 3단계였습니다 ㅠㅠ
인수 테스트가 아직 부족해요! PR 요청 기간이 다가와서 우선 기능 구현을 다 하고 PR 드립니다(어쩌다보니 ATDD가 전혀 아니게 되었네요)
부족한 테스트는 리팩토링 하면서 추가해보도록 하겠습니다 :)

그리고 질문이 하나 있습니다!

Q. 서비스에서 어떤 메서드는 DTO를, 어떤 메서드는 도메인을 반환해도 될까요?

편의를 위해 LineService에서 StationSerivce를 단방향으로 참조하게 되어있습니다. 그런데 이 때 StationService의 다른 메서드들은 StationResponse를 반환하는데, LineService에서만 필요한 단건조회 메서드만 Station을 그대로 반환하고 있습니다. 이런 식으로 사용하지 말고 통일적으로 StationResponse를 반환해준 뒤 다시 Station으로 돌려줘야 할까요? 던의 생각은 어떤가요?? :)

ghost

안녕하세요 오찌!
3단계 미션 깔끔하게 잘 구현해주셨네요! 👍
테스트도 꼼꼼하게 잘 작성해주셨어요!
코멘트 남겼으니 확인부탁드릴게요~

ghost · 2022-05-14T04:46:26Z

src/main/java/wooteco/subway/service/StationService.java

+        if (!stationDao.existsId(id)) {
+            throw new DataNotFoundException("존재하지 않는 역입니다.");
+        }


도메인별로 데이터가 존재하지않을때 발생하는 예외를 만들어주는건 어떨까요?
StationNotFoundException 처럼요!
지금은 예외처리가 같기때문에 도메인별로 나누지않아도 되긴하지만, 예외가 발행했을때 어떤 예외가 발생했는지만 보고서도 더 명확하게 예외상황을 파악할 수 있습니다!
메시지로 예외가 발생한 상황을 구분하면 메시지를 잘 남겨야하는 부담도 있을 수 있겠어요.

안그래도 NotFound 쪽만 예외가 뭉뚱그려 만들어져 있어서 분리할까 생각했었는데 분리하는게 좋겠네요!

ghost · 2022-05-14T04:47:43Z

src/main/java/wooteco/subway/service/StationService.java

-            stationDao.findById(id);
-        } catch (EmptyResultDataAccessException e) {
-            throw new EmptyResultDataAccessException("존재하지 않는 역입니다.", 1);
+        if (!stationDao.existsId(id)) {


boolean을 반환하는 exitst메서드 사용 👍 👍

ghost · 2022-05-14T04:52:09Z

src/main/java/wooteco/subway/service/StationService.java

+    public Station findById(Long id) {
+        validateExist(id);
+        return stationDao.findById(id);
+    }


StationService의 다른 메서드들은 컨트롤러에서 호출하고 있기때문에 Response를 반환해주는게 자연스럽고, findById 메서드는 다른 서비스에서 호출하고 있기때문에 Response가 아닌 Station을 그대로 반환해줘도 괜찮아보입니다!
서비스에 메서드의 반환값이 무조건 Response일 필요는 없습니다. 메서드를 호출하는곳에 맞게 사용하면 될 것 같아요

ghost · 2022-05-14T04:54:23Z

src/main/java/wooteco/subway/service/LineService.java

+    private Line createLine(LineCreateRequest lineCreateRequest) {
+        Line line = new Line(lineCreateRequest.getName(), lineCreateRequest.getColor());
+        validateUnique(line);
+        return lineDao.save(line);
+    }


이 메서드에서 단순히 Line객체를 생성하는것 뿐만 아니라 db에 insert까지 하고 있으니 메서드 이름을 create-보다는 save-로 하는건 어떨까요?

항상 메서드 명에 고민이 많았는데 바꿔봐야겠네요! 감사합니다 :)

ghost · 2022-05-14T04:58:59Z

src/main/java/wooteco/subway/service/LineService.java

+        }
+    }
+
+    @Transactional(readOnly = true)


ghost · 2022-05-14T05:02:57Z

src/main/java/wooteco/subway/dao/SectionDao.java

+        String sql = "SELECT s.id AS id, s.line_id AS line_id, s.distance AS distance, us.id AS us_id,"
+                + " us.name AS us_name, ds.id AS ds_id, ds.name AS ds_name FROM SECTION AS s"
+                + " INNER JOIN STATION AS us ON s.up_station_id = us.id"
+                + " INNER JOIN STATION AS ds ON s.down_station_id = ds.id"
+                + " WHERE s.line_id = :line_id";


join 👍 👍
쿼리를 String으로 표현할때 FROM 앞에서 줄바꿈해주면 가독성이 더 올라갈것 같네요!

가독성을 높여보려고 고민을 되게 많이 했는데, 확실히 접속사 앞에서는 끊어줄 필요가 있겠네요...!

ghost · 2022-05-14T05:05:04Z

src/main/java/wooteco/subway/service/LineService.java

+        validateExist(id);
+        Line persistLine = lineDao.findById(id);
+        Sections sections = new Sections(sectionDao.findAllByLine(persistLine));
+        Line line = new Line(persistLine.getId(), persistLine.getName(), persistLine.getColor());


새로 Line객체를 만들어서 반환하는 이유를 알 수 있을까요?
persistLine을 그대로 사용해도 될 것 같아요

이 부분은 제가 제정신이 아닌 상태로 코드를 작성했나보네요 ㅋㅋㅋㅋ
persistLine을 그대로 사용해도 되는 코드여서 수정하겠습니다!

ghost · 2022-05-14T05:09:53Z

src/main/java/wooteco/subway/service/LineService.java

+    @Transactional
+    public void deleteSection(Long lineId, Long stationId) {
+        Station station = stationService.findById(stationId);
+        Line line = lineDao.findById(lineId);


lineId에 해당하는 Line이 없다면 예외가 발생할 것 같아요!

ghost · 2022-05-14T05:11:13Z

src/main/java/wooteco/subway/domain/Sections.java

+        validateContainsTargetStation(station);
+        List<Station> sortedStations = getSortedStations();
+        int index = sortedStations.indexOf(station);
+        if (index == 0 || index == sortedStations.size() - 1) {


이 로직을 메서드로 분리해서 의미를 더 드러내볼 수 있을까요?

체크하는 부분만 메서드로 빼 봐야 겠네요...!

ghost · 2022-05-14T05:12:19Z

src/main/java/wooteco/subway/exception/DuplicateLineException.java

@@ -0,0 +1,9 @@
+package wooteco.subway.exception;
+
+public class DuplicateLineException extends DuplicateDataException {


예외를 잘 쪼개셨네요! 👍

ghost

안녕하세요 오찌!
구현 잘해주셔서 코멘트 남길 부분이 딱히 없었네요 👍
가독성 관련해서 간단한 코멘트 남겼으니 확인부탁드릴게요!~
이번 미션 구현하시느라 수고많으셨습니다! 다음 미션도 화이팅입니다!

ghost · 2022-05-16T15:49:25Z

src/main/java/wooteco/subway/domain/Sections.java

+        return values.stream()
+                .noneMatch(section -> section.containStation(target.getUpStation()) && section.containStation(
+                        target.getDownStation()));


Suggested change

return values.stream()

.noneMatch(section -> section.containStation(target.getUpStation()) && section.containStation(

target.getDownStation()));

return values.stream()

.noneMatch(section -> section.containStation(target.getUpStation())

&& section.containStation(target.getDownStation()));

이렇게 변경하는건 어떠신가요? predicate 부분이 조금 더 명확하게 드러나도록 해보았어요.
개인적인 생각이니 오찌가 고민해주세요!ㅎㅎ

ghost · 2022-05-16T15:51:39Z

src/main/java/wooteco/subway/domain/Sections.java

+    private void validateDuplicate(Section newSection) {
+        if (values.stream()
+                .anyMatch(savedSection -> savedSection.hasSameStations(newSection))) {
+            throw new DuplicateSectionException("이미 존재하는 구간입니다.");
+        }
+    }


if문안에 stream전체를 넣으니 살짝 가독성이 떨어지는 느낌이 들어요!
stream 결과를 로컬변수에 저장한뒤에 처리하는건 어떨까요?

ghost · 2022-05-16T15:52:18Z

src/main/java/wooteco/subway/domain/Sections.java

+    private boolean canBeStartSection(Section newSection) {
+        return values.stream()
+                .anyMatch(savedSection -> savedSection.getUpStation().hasSameName(newSection.getDownStation()))
+                && values.stream()
+                .noneMatch(savedSection -> savedSection.getDownStation().hasSameName(newSection.getDownStation()));
+    }
+
+    private boolean canBeEndSection(Section newSection) {
+        return values.stream()
+                .anyMatch(savedSection -> savedSection.getDownStation().hasSameName(newSection.getUpStation()))
+                && values.stream()
+                .noneMatch(savedSection -> savedSection.getUpStation().hasSameName(newSection.getUpStation()));
+    }


여기도 stream 결과를 로컬변수에 저장해서 로직을 한번씩 끊어서 처리해주면 가독성이 더 좋아질 것 같아요!

ghost · 2022-05-16T15:53:06Z

src/main/java/wooteco/subway/domain/Sections.java

+                .orElseThrow(
+                        () -> new InvalidSectionCreateRequestException("구간 시작 역과 끝 역이 모두 노선에 존재하지 않아 추가할 수 없습니다."));


orElseThrow 부분은 한줄에 이어서 작성해도 될 것 같아요!

Ohzzi added 30 commits May 8, 2022 08:46

test: 테스트 시 데이터베이스 초기화를 DirtiesContext 대신 sql로 처리

c8c6deb

refactor: 데이터 중복, 없음 상황 처리 시 커스텀 예외를 만들도록 변경

3b50b0c

refactor: 서비스에서의 데이터 중복 및 존재 여부 검증 방법 변경

523cbfc

refactor: toEntity 제거

42c9e45

feat: 노선에 포함되는 구간을 받아서 역 목록을 돌려주는 기능 추가

25f2f19

feat: 데이터베이스에 노선을 저장하는 기능 추가

d9c34e8

feat: 데이터베이스에서 노선이 포함하고 있는 구간들을 조회해오는 기능 추가

698e179

feat: 데이터베이스에서 노선에 포함되면서 지정한 역을 포함하고 있는 구간을 지우는 기능 추가

f1e369c

test: 서비스 레이어의 테스트를 SpringBootTest로 변경

7f57b20

feat: 데이터베이스에서 동일한 노선이 존재하는지 확인하는 기능 추가

f6f449c

feat: Sections에서 하행선 기준 시점 -> 종점으로 정렬된 역 목록을 반환하는 기능 추가

a494541

refactor: 역 목록을 돌려주는 기능을 Sections에 위임

f0ffec8

refactor: 역 목록을 정렬하는 메서드 분리

c2bf25e

feat: Sections에 구간을 삽입하는 기능 추가

23449e1

feat: Sections에서 구간을 삭제하는 기능 추가

7051304

feat: Sections끼리 비교해서 비교 대상에서 포함하지 않는 구간들만 반환하는 기능 추가

59c0eec

test: 테스트들의 공통 변수 추출

94f4c9f

feat: 구간 생성 시 시점과 종점이 같은 역이면 예외를 반환하는 기능 추가

eaa430f

refactor: 객체를 넣어서 조회하는 경우의 메서드 이름 변경

fea92cf

feat: 노선 생성 요청을 받아서 노선과 구간을 저장하는 기능 추가

b5dc7c6

feat: 구간 생성 요청을 받아서 구간을 생성하는 기능 추가

f89001c

refactor: Sections의 중복 부분 메서드 추출

0c9d75c

refactor: 조회 메서드에 readOnly, 삭제 메서드에 @transactional 적용

859d2a1

test: extracting을 사용해 테스트 코드 리팩토링

ce002e0

refactor: 서비스 레이어 메서드 네이밍 변경

428a657

refactor: LineService에서 StationDao 대신 StationService를 사용하도록 변경

016aec1

feat: 요청으로 들어온 노선과 역 정보로 구간을 지우는 기능 추가

7148cae

test: LineService 의 중복 변수를 전역으로 빼고 일부 테스트 Disabled 제거

cec1f1a

test: 단건 조회에 대한 StationService 테스트 추가

0ebb0c4

refactor: 노선 생성과 수정 DTO 분리

d922604

Ohzzi added 9 commits May 12, 2022 11:43

test: 테스트에 쓰이는 지하철 역 및 노선 정보 변경

188148f

refactor: Line에서 Sections 제거

c8f8d24

refactor: LineResponse 생성 시 포함된 역 목록을 가지고 생성하도록 변경

94d402f

fix: 인수 테스트 실패 오류 수정

a433b03

test: 공통 테스트 코드를 AcceptanceTest로 이동

345b455

feat: 구간 추가 및 삭제 기능 컨트롤러에 매핑

38b4b26

fix: Seciton과 Line, Station 간에 외래키 추가

4dc57a5

feat: 해당 역을 포함하고 있는 구간이 있는 역을 삭제하려는 경우 예외를 반환하는 기능 추가

5522a65

fix: 테스트 실패 오류 수정

9f51cb7

ghost suggested changes May 14, 2022

View reviewed changes

Ohzzi added 10 commits May 14, 2022 22:06

refactor: 불필요한 객체 생성 부분 제거

3d7a03f

refactor: 메서드 명이 기능을 모두 담도록 변경

d5a3b59

fix: 구간 삭제 시 노선이 존재하는지 확인하도록 수정

971657a

refactor: 끝 구간인지 확인하는 메서드 추출

da954ea

refactor: 쿼리문 가독성 좋게 개행 추가

851a18b

chore: exception 디렉토리를 더 세세하게 분리

5fc553d

refactor: DataoNotFoundException을 도메인에 따라 상세하게 분리

f62a8e0

refactor: 노선 단 건 조회를 Optional 사용 방식으로 변경

12e0adf

refactor: 역 단 건 조회를 Optional 사용 방식으로 변경

acfb90a

test: 구간 추가 시 없는 역으로 생성하려는 경우 테스트 추가

4a33b5b

ghost approved these changes May 16, 2022

View reviewed changes

ghost merged commit cdf12ba into woowacourse:ohzzi May 16, 2022

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Spring 지하철 노선도 - 3단계] 오찌(오지훈) 미션 제출합니다. #323

[Spring 지하철 노선도 - 3단계] 오찌(오지훈) 미션 제출합니다. #323

Ohzzi commented May 12, 2022

ghost left a comment

ghost May 14, 2022

Ohzzi May 14, 2022

ghost May 14, 2022

ghost May 14, 2022

ghost May 14, 2022

Ohzzi May 14, 2022

ghost May 14, 2022

ghost May 14, 2022

Ohzzi May 14, 2022

ghost May 14, 2022

Ohzzi May 14, 2022

ghost May 14, 2022

ghost May 14, 2022

Ohzzi May 14, 2022

ghost May 14, 2022

ghost left a comment

ghost May 16, 2022

ghost May 16, 2022

ghost May 16, 2022

ghost May 16, 2022

		@@ -0,0 +1,9 @@
		package wooteco.subway.exception;

		public class DuplicateLineException extends DuplicateDataException {

		.orElseThrow(
		() -> new InvalidSectionCreateRequestException("구간 시작 역과 끝 역이 모두 노선에 존재하지 않아 추가할 수 없습니다."));

[Spring 지하철 노선도 - 3단계] 오찌(오지훈) 미션 제출합니다. #323

[Spring 지하철 노선도 - 3단계] 오찌(오지훈) 미션 제출합니다. #323

Conversation

Ohzzi commented May 12, 2022

Q. 서비스에서 어떤 메서드는 DTO를, 어떤 메서드는 도메인을 반환해도 될까요?

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment