[Review] ATDD 2주차 1차 PR

권승청 리뷰어 님께 감사의 말을 전하고 싶다!!

 

1. 질문

1-1) Section의 생성을 어디서?

예를 들어 다음과 같이 section을 추가하는 메서드가 있다고 해봅시다!

LineService 내부

    @Transactional
    public void addSection(Long lineId, SectionRequest sectionRequest) {
        Station upStation = stationService.findById(sectionRequest.getUpStationId());
        Station downStation = stationService.findById(sectionRequest.getDownStationId());
        Line line = lineRepository.findById(lineId).orElseThrow(IllegalArgumentException::new);
        line.addSection(new Section(line, upStation, downStation, sectionRequest.getDistance()));
    }

이때 addSection의 인자부분에서 Section을 생성하여 전달하는 방식을 사용하고 있습니다.
즉, LineService에서 Section을 생성하고 있습니다.

 

이와는 달리

line.addSection(line, upStation, downStation, sectionRequest.getDistance());

위와 같이 값만 전달 한 후, line 내부에서 section을 생성하는 방법도 가능할 것 입니다.

 

저는 개인적으로 전자의 방식이 적합하다 생각하는데... Section의 생성은 어디가 적합할까요?
Service일까요? 아니면 Line과 같은 class 내부 일까요?

 

답변:

흠 리뷰어는 Line내에서 Section을 생성하는 방법을 추천하셨다.

하지만 나의 생각은 조금 다르다.

 

애당초 Section class 내부에 서 자신의 생성을 담당하는 것 이 아니라면, 서비스 계층에서 생성해서 넘겨주는것이 더 적합하지 않을까?

위에서 언급한 방식처럼

line.addSection(line, upStation, downStation, sectionRequest.getDistance());

인자를 전부 전달하게 되면, 세부값들이 service, Line 둘다 알게되는 것 아닐까?

 

오히려 Serviec 에서 생성해서 전달하면 Line은 Section만 알게되는것 아닐까?

이를 도표로 보면 다음과 같다.

	Service의 의존성	Line의 의존성
Service에서 생성할 경우	세부 정보, Section	Section
Line에서 생성할 경우	세부 정보	Section, 세부 정보

흠.. 일단 해당 부분은 Service에서 생성하도록 두었다?

 

2. 리뷰 정리

2-1) 메서드 추가와 비어있는 List 반환

우선 말씀해주신 것 처럼 첫 상행역을 구하는 메서드가 필요함을 느껴 구현하였다.

원래 service 계층에서 Sections이 비어있는 경우 empty list를 반환하도록 했었는데, 이를 Sections.add 내부로 로직을 이동시켰습니다!

public void add(Section section) {
    if (sections.isEmpty()) {
        sections.add(section);
        return;
    }
  
    // ... 생략
}

 

2-2) 부정 조건을 피해라

즉 다음과 같음을 의미한다.

따라서 코드를 다음과 같이 변경하였다.

if (lastSection.hasNotDownStation(station)) {
    throw new SectionsException(ErrorCode.NOT_SAME_DOWN_STATION_EXCEPTION);
}

 

2-3) 테스트를 위한 메서드

Sections이 일급컬렉션이기 때문에 외부에서 List<Section>에 접근할수가 없다.

따라서 테스트 작성시에도 검증 부분에서 문제가 있었다. 변경된 Line내부의 section들을 어떤 방법으로 확인할 것 인가?

 

이에 대한 해결로, 사이즈를 반환하는 메서드를 만들어 줬었다.

리뷰어 께서 한번 생각해보라 말씀해주셨는데, 사이즈 메서드 정도는 열어도 상관없을것 같다.

 

원래 Collection 자체도 Size를 반환하는 메서드들이 다 있다!

또한 size 정보는 객체의 불변성에 영향을 주지 않는다고 생각한다. 열어줘도 좋을것 같다.

 

2-4) 일급컬렉션에 대한 고찰

리뷰어님과 가장 많은 의견을 나눈 주제인 것 같다.

 

나 :

안녕하세요 리뷰어님!
https://github.com/next-step/atdd-subway-path/pull/290#discussion_r922761641
위 리뷰에 대한 질문이 있습니다!사실 개인적으로는 1급컬렉션에서 다음과 같이 sections를 반환하면 테스트에도 용의 하고, 다른쪽에서도 사용할일이 많다 생각됩니다.

public List<Section> getSections() {
    return new ArrayList<>(sections);
}

직전 리뷰에서 말씀해주신 new ArrayList를 잘못 wrapping한 부분도 원래는 위 getter에 적용하려 했었습니다.

이러한 사용은 일급컬렉션의 불변객체또한 지킬 수 있으니 getSectionsSize()보다 좋은 대안이라 생각되는데 어떻게 생각하시나요? 

---

리뷰어:

안녕하세요
https://github.com/next-step/atdd-subway-path/pull/290#issuecomment-1186217489
내용과 이어지는 것 같은데요.
Section 객체를 공개해도 상관없다라고 생각하시면 말씀 하신데로 하는 것도 가능은 할 것 같은데요.
List<Section> 이 불변이라고 그 안의 Section 도 불변일까요?? 새로 wrapping 한 것은 컬렉션 그 자체 아닐까요? 

---

나:

우선 답변 감사합니다 리뷰어님!!말씀해주신 것 처럼 wrapping 한 컬렉션은 새롭게 방어적으로 복사된 컬렉션이니, 원본 Sections에 영향을 주지 않는 불변객체라 생각했습니다.

 

하지만 Sections 내부의 Section들이 문제가 될수 있음을 학습하게 되었습니다.
그럼 혹 unmodifiableList()를 통해 요소의 수정까지 막도록 한다면 적합한 방법이 될수 있을까요?

(저의 생각을 요약하면, Line.getSections()를 하면 일급컬렉션 내부의 sections과는 완전 별도의, 값만 같은 리스트를 얻어오고 싶었습니다)

=> deep copy 하고 싶다는말 ㅎㅎ

---

리뷰어:

해당 메서드는 add remove 등의 컬렉션에 대한 수정을 시도하면 UnsupportedOperationException 예외를 발생시키는 wrapper 컬렉션을 반환할 뿐입니다.

따라서 Section 의 수정을 완벽히 막으려면 관계가 완전히 끊어진 형태로 깊은 복사를 하는 등의 복잡한 행위를 해야겠죠?

따라서, 일반적으로 사용하는 가장 좋은 방법은 해당 객체를 공개 안하는 것이라고 생각합니다.
혹은 외부에서 수정할 수 있는 방법을 다 막는 방법이 있을 것 같은데(공개 범위를 package-private 으로 설정하고, 패키지 관리를 잘한다던가?) 공개를 결정해서 얻을 수 있는 이득이 무엇일까를 먼저 고민해봐야 할 것 같아요

---

나:

답변 감사드립니다 ㅎㅎ!! 좀더 스스로 고민을 더해본후 Step2에서 반영하도록 해보겠습니다!!