[Review] ATDD 2주차 2차 PR

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

 

1. 질문

1-1) 일급 컬렉션에 대한 재질문

지난번 대화를 통해 sections을 방어적 복사를 해도, sections에 담긴 원소인 section은 변경의 위험이 있음을 인지하게 되었습니다.
따라서 리뷰어님의 의견을 수용하여 Sections.getSections() 메서드는 제거하였지만, "순서대로 역을 조회하는 기능" 때문에 어차피 역들은 변경의 위험에 있을 수 밖에 없지 않나? 란 생각이 들었습니다.

 

일급컬렉션은 컬렉션의 불변성만 보장하면 됐던 것 아닐까? 란 생각도 들구요 ㅎㅎ

변경 위험의 노출 정도를 줄였다 생각하면 될까요? 이에 대한 의견이 궁금합니다!!

 

답변:

 

1-2) 구간 길이 검증의 불가능

미션의 요구사항 중 다음과 같은 사항이 있습니다.

"구간을 중간에 추가할 때, 새로운 길이를 뺀 나머지를 새롭게 추가된 역과의 길이로 설정"

위 요구사항을 위해 sections를 반환받을 수 없다면, 추가된 구간 만이라도 가져오는 메서드를 만들어 변경된 거리를 확인해야 할까요?

근대 또 이게 Test 검증을 위한 메서드가 될 것 같아서요 ㅎㅎ
변경된 길이를 어떻게 검증할지 고민이 됩니다 ㅎㅎ

 

답변:

 

2. 리뷰 정리

2-1) if문 2개 합치기

당연한 리뷰였다 생각한다. 나 스스로 이러한 간단한 중복을 찾지 못했던 점에 대하여 반성해야겠다.

변경된 코드는 다음과 같다.

private void addSection(Section section) {
    if (isPossibleAddFrontOrBack(section)) {
        sections.add(section);
        return;
    }

    // 생략...
}

addSection의 if문 안에 isPossibleAddFrontOrBack()을 만들었다.

 

private boolean isPossibleAddFrontOrBack(Section section) {
    return isSectionsUpStation(section.getDownStation()) || isSectionsDownStation(section.getUpStation());
}

private boolean isSectionsUpStation(Station station) {
    return getFirstUpStation().equals(station);
}

private boolean isSectionsDownStation(Station station) {
    return getLastDownStation().equals(station);
}

메서드로 최대한 추출하여 리팩토링 하게 되었다.

 

2-2) 관련된 예외를 던지는 Domain

나의 코드는 Section 외부에서 값을 가져와 거리를 비교하고 있었다.

메시지를 전달해야 함을 인식하지 못하고 있었다... 일단 막 구현한 후 리팩터링 단계에서 이를 인지했어야 하는데...

생각을 좀더 세밀하게 하지 못한 것 같아 아쉽다.

 

(변명을 좀 하자만... 집중을 계속할 수가 없는 환경이라... 틈틈이 하다 보니.... 흐름이 끊어져 자꾸 리팩터링이 이상해지는 것 같다)

public void add(Section section) {

    // 생략...
    
    Section findSection = findTargetSection(section);
    // 문제의 지점
    if (section.isGreaterThanDistance(findSection.getDistance())) {
        throw new SectionsException(ErrorCode.SECTION_DISTANCE_EXCEPTION);
    }
    
    Section halfSection = findSection.divideSectionByMiddle(section);
    sections.remove(findSection);
    sections.add(halfSection);
    sections.add(section);
    
    if (!hasUpStation && !hasDownStation) {
        throw new SectionsException(ErrorCode.NOT_FOUND_BOTH_STATION_EXCEPTION);
    }
}

 

따라서 distance 를 가지고 있는 Section객체 스스로 판단하도록 다음과 같이 수정하였다.

@Entity
public class Section {

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    private Long id;
    
    private int distance;
    
    // 생략...

    private boolean isLessThanDistance(int distance) {
        return this.distance <= distance;
    }
}

 

2-3) Custom Exception에 대한 고민

 

이에 대하여 다음과 같이 생각하였다.

 

Sections 관련 예외 -> SectionsException
Station 관련 예외 -> StationException과 같이 처리하고,

세부적인 내용들은 errorMessage를 통해 사용하려 하였는데 이러한 방식보다는 세분화하는 것 이 더 좋을까요?

 

사실 기존의 만들어진 Java의 Exception을 잘 활용하는 편이 좋다고는 생각하는데,
CustomException을 잘 사용해보지 않아 적용시켜 봤습니다.

 

• 장점
  우선 ErrorCode와 같은 부분을 한곳에서 관리가 가능하기에 장점이 된다 생각합니다.
  또한 우리가 만든 Exception이기 때문에 도메인 별로 만들어 두면, 도메인과 관련된 내용 또한 해당 Exception안에 모아둘 수 있다 생각하였습니다.
• 단점
  단점으로는 직접 만들다 보니... 은근 귀찮은 작업인 것 같습니다??
  또한 신규로 프로젝트에 투입된 사람일 경우, 해당 팀의 Custom 예외 사용 방식에 적응기간이 필요하다는 단점이 있을 것 같습니다?

 

혹시 리뷰어님의 CustomException 사용방식을 공유해주실 수 있을까요?

 

답변:

 

2-4) 인수조건 주석의 삭제

이에 대하여 다음과 같이 생각하였다.

 

나:

음 이게 말씀하신 것 처럼 "단순 주석"에 해당되면 위 의견에 부합하다 생각하는데,
위 주석은 "인수조건"에 해당되기 때문에 일반적인 주석과는 다르다 생각합니다?

 

단순 코드의 설명을 위한 주석이라기 보단, 유저 스토리를 정리한 인수 조건을 단순히 표현 도구로 "주석"을 사용한 거라 생각돼서요 ㅎㅎ

꼭 지워야만 하는것 일까요?

인수조건 주석이 있다고 해서 시나리오 분석에 가독성이 떨어지지는 않는다고 생각돼서요 ㅎㅎ

 

답변:

 

주석을 잘못 적지만 않는다면, 인수조건 같은 경우 남겨주는 편이 좋을 것 같다 생각된다.

개인적으로 그렇게 생각한다.

 

2-5) 가독성이 이미 좋아 보이는데, 메서드로 추출해야 하는가?

이건 개인적인 질문에 가깝다?

위 코드의 31, 32, 33 행은 읽으면 바로 그 의미가 이해 간다.

상행 역과 하행 역이 모두 있는 경우를 찾는 부분이다.

 

이렇게 의미가 명확한 부분까지 메서드로 분리해야 하는 것 일까?

 

답변:

메서드를 최대한 10줄 정도로 줄이기 위해 의식적인 노력을 해야 할 것 같다.

따라서 위 의견을 받아들여 메서드로 분리한 모습은 다음과 같다.

 

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

    validateSection(section);
    addSection(section);
}

코드가 매우 깔끔해진 것 같다!!