중복 제거 및 메소드 분리

2주차 미션에는 지하철 노선에 새로운 구간을 추가하는 로직에 대한 코드를 작성하였다.
비즈니스 로직을 잘 모르는 분들을 위해 예를 들어 설명을 하고자 한다.

지하철 노선 2호선이 있다고 하자. 현재 2호선에는 상행종점역인 신도림역과 하행종점역인 영등포구청역 딱 2개의 역만 존재한다. 이 때 새로운 구간을 추가하는 방법은 2가지가 존재한다.

상행역을 기준으로 새로운 구간 추가

• 신도림역은 2호선의 상행종점역이다.
• 새로운 구간(신도림역 ~ 문래역)을 추가한다.
• 2호선은 (신도림역 ~ 문래역)구간과 (문래역 ~ 영등포구청역)구간을 가지게 된다.
  
  

하행역을 기준으로 새로운 구간 추가

• 영등포구청역은 2호선의 하행종점역이다.
• 새로운 구간(문래역 ~ 영등포구청역)을 추가한다.
• 2호선은 (신도림역 ~ 문래역)구간과 (문래역 ~ 영등포구청역)구간을 가지게 된다.
  
  

그 외 여러가지 비즈니스 로직이 있지만 이 포스트와는 관련성이 없으므로 패스하겠다.
상행역을 기준으로 구간을 추가하는 것이 addSectionBasedOnUpStation이고
하행역을 기준으로 구간을 추가하는 것이 addSectionBasedOnDownStation이다.

자 이제 비즈니스 로직에 대한 설명은 했으니 코드를 보자.

private void addSectionBasedOnUpStation(Section section) {
    Section findSection = findSectionByUpStation(section.getUpStation());
    if(Objects.isNull(findSection)) {
        sections.add(section);
        return;
    }
	
    =================================================================
	Line line = findSection.getLine();
    int distance = findSection.getDistance() - section.getDistance();
    validateDistance(distance);

	Section nextSection = Section.builder()
				  .line(line)
				  .upStation(section.getDownStation())
				  .downStation(findSection.getDownStation())
				  .distance(distance)
				  .build();
    =================================================================          
    
    int index = sections.indexOf(findSection);
    sections.set(index, section);
    sections.add(index + 1, nextSection);
}

private void addSectionBasedOnDownStation(Section section) {
    Section findSection = findSectionByDownStation(section.getDownStation());
    if(Objects.isNull(findSection)) {
        sections.add(section);
        return;
    }
	
    =================================================================
	Line line = findSection.getLine();
    int distance = findSection.getDistance() - section.getDistance();
    validateDistance(distance);

    Section nextSection = Section.builder()
				   .line(line)
				   .upStation(findSection.getUpStation())
                   .downStation(section.getUpStation())
				   .distance(distance)
				   .build();
    =================================================================
    
    int index = sections.indexOf(findSection);
    sections.set(index, section);
    sections.add(index + 1, nextSection);
}

=============== 부분을 유심히 잘 봐주기를 바란다.
일단은 다른 로직임은 알 수 있을 것이다. 그래서 나는 딱히 중복코드라 생각하지 않고 가볍게 넘겼었다. 그런데 또 다시 살펴보면 형태가 비슷하다는 건 부정을 못할 것이다. 🤔🤔🤔

그래서 리뷰어님께서 중복코드를 없애보라고 하셨고 고민을 해서 낸 코드는 다음과 같다.

private Section createSectionForAdd(Section findSection, 
                                    Section section, 
                                    boolean isBasedOnUpStation) {
    Line line = findSection.getLine();
    int distance = findSection.getDistance() - section.getDistance();
    validateDistance(distance);

    if(isBasedOnUpStation) {
        return Section.builder()
                .line(line)
                .upStation(section.getDownStation())
                .downStation(findSection.getDownStation())
                .distance(distance)
                .build();
    }

    return Section.builder()
            .line(line)
            .upStation(findSection.getUpStation())
            .downStation(section.getUpStation())
            .distance(distance)
            .build();
}

맨 처음에 로직이 다르다고 여긴 부분은 Section을 생성하는 부분이었다. 그래서 Boolean값을 두어서 구분을 해서 return을 반환하도록 로직을 짰고 위와 같은 메소드를 추출하였다.

그리고 이를 이용하여 다시 리팩토링한 코드는 다음과 같다.

private void addSectionBasedOnUpStation(Section section) {
    Section findSection = findSectionByUpStation(section.getUpStation());
    if(Objects.isNull(findSection)) {
        sections.add(section);
        return;
    }

    Section nextSection = createSectionForAdd(findSection, section, true);

    int index = sections.indexOf(findSection);
    sections.set(index, section);
    sections.add(index + 1, nextSection);
}

private void addSectionBasedOnDownStation(Section section) {
    Section findSection = findSectionByDownStation(section.getDownStation());
    if(Objects.isNull(findSection)) {
        sections.add(section);
        return;
    }

    Section prevSection = createSectionForAdd(findSection, section, false);

    int index = sections.indexOf(findSection);
    sections.set(index, prevSection);
    sections.add(index + 1, section);
}

훨씬 깔끔해졌음을 알 수있다 👍👍👍

이를 통해 2가지의 지식을 얻을 수 있었다.

1. 다른 로직이더라도 비슷한 형태라면 중복코드임을 인지하고 공통 메소드를 추출하는 노력을 해보자.

2. 핵심 로직과 그 외 로직을 분리하자.

sections.set(index, prevSection);
sections.add(index + 1, section);

addSectionBasedOnUpStation의 핵심로직은 위의 코드이다.
즉 구간리스트에서 기존의 구간을 없애고 새로만든 2개의 구간을 넣는 것이 핵심로직이다.

즉 추가할 새로운 구간을 만드는 것이 핵심로직이 아니라는 말이다. 메소드명에서 알 수 있듯이 구간을 추가하는 것이 목적이지 구간을 새로 만드는 것은 핵심 목적이 아니다. 따라서 새로운 구간을 만드는 것은 따로 메소드를 만들어서 처리해주는 것이 옳다고 본다.