인수 테스트와 리팩터링 회고

Step1 경로 조회 타입 추가

Step1 경로 조회 타입 추가 PR

 

package src/main/java/nextstep/subway/strategy/PathType.java

import nextstep.subway.controller.dto.PathResponse;
import nextstep.subway.controller.dto.StationResponse;

public enum PathType {
    DISTANCE(Section::distance, (stations, value) -> PathResponse.distanceOf(StationResponse.listOf(stations), value)),
    DURATION(Section::duration, (stations, value) -> PathResponse.durationOf(StationResponse.listOf(stations), value)),
    OTHER(section -> 0L, (stations, value) -> PathResponse.otherOf(StationResponse.listOf(stations)));

    private Function<Section, Long> type;
    private BiFunction<List<Station>, Integer, PathResponse> response;

 

PathType는 도메인 계층에 존재한다. PathType 값마다 PathResponse의 정적 팩토리 메서드를 사용하여 response를 다르게 만들었다. 

 

아래와 같은 피드백을 받았다.

💡PathType 은 현재 다양한 계층에서 참조되고 있습니다.
PathController -> PathService -> PathType -> PathResponse 와 같이 의존성이 생기는데, 이를 패키지 레벨로 확장해보면, controller -> service -> strategy -> controller(dto) 와 같이 의존성 사이클이 생겼습니다 😢
특히 PathFinder 에서도 참조하고 있는데, domain 계층에서 참조하는 클래스는 domain 패키지 레벨로 만들어 주시는 것이 의존성 관리에 좋다고 생각합니다. (domain 계층은 domain 계층 끼리만 참조한다. 라는 전제를 둔다면 말이죠)

 

domain 계층이 controller 계층을 바라보고 있었다. 너무 구현에만 몰두해서 개발해서 인지 import문을 볼 생각을 못했다.

PathFinder는 도메인 계층에 해당한다. PathType은 도메인이 아닌 별도의 strategy에 있다.

즉, 도메인 계층이 strategy 계층에 의존한다. stragety는 도메인과 관련된 로직이기 때문에 domain 계층의 하위 폴더로 응집시키면 도메인 계층끼리 의존하게 되어 의존성 관리에 좋다고 말씀하신 것 같다.

 

PathType에서 response를 생성하는 로직은 service 계층으로 옮기는 것으로 해결하였다.

@Getter
@AllArgsConstructor
public enum PathType {
    DISTANCE(Section::distance),
    DURATION(Section::duration);

    private Function<Section, Long> type;

 

 

Step2 요금 조회

Step2 요금 조회 PR

 

아래와 같은 요구사항이 있었다.

기본운임(10㎞ 이내) : 기본운임 1,250원
이용 거리초과 시 추가운임 부과
 - 10km초과∼50km까지(5km마다 100원)
 -  50km초과 시 (8km마다 100원)

이동한 거리마다 지하철 요금 정책이 변경된다.

 

책임연쇄 패턴을 사용하여 구현하였다.

책임연쇄 패턴은 연결된 체인을 따라 요청을 전달하는 행동 디자인 패턴이다.

각 핸들러는 요청을 받으면 요청을 처리할지 아니면 체인의 다음 핸들러로 전달할지 결정한다.

요구사항이 책임 연쇄 패턴을 적용하기 알맞다고 생각하였다.

 

 

1. 요금 계산 요청

2. 기본요금 계산

2-1. distance가 10km미만이면 기본 요금만 계산

2-2. distance가 10km 이상이면 다음 핸들러 호출

3. 10 ~ 50 KM 요금 계산

3-1. distance가 50km 미만이면 기본 요금 + (10 ~ 50KM 요금) 계산

3-2. distance가 50km 이상이면 다음 핸들러 호출

4. 50KM 이상 요금 계산

4-1. 기본 요금 + (10 ~ 50KM 요금) + (50KM ~ 요금) 계산

 

문득 리뷰어님은 실무에서 어떤 디자인 패턴을 사용하시는지 궁금해서 물어봤다.

 

아래와 같은 답변을 받았다.

전략 패턴이 많이 접하게 되는 패턴이긴 한 것 같아요 😅
어떤 패턴을 써야겠다 라고 생각하고 구현을 하는 경우는 많이 없는 것 같긴한데, 
템플릿 메소드 패턴 같은 경우도 꽤 자주 사용하는 것 같습니다.
떠오르는게 많진 않네요 ㅎㅎ
디자인 패턴을 사용하거나 학습하는 것은 물론 중요하고 좋은 것이긴 한데, 
그로 인해 소스코드가 너무 복잡해지는 것은 피해야 합니다. 
현재 팀에서 소화할 수 있는 만큼의 설계를 항상 고민해야합니다

 

 

클래스명을 아래와 같이 작성하였다.

Over10KmFareHandler
Over50KmFareHandler

 

클래스 명을 구체적으로 할지 범용적인 이름인 SecondPolicyFareHandler, ThirdPolicyFareHandler 로 할지 고민이였고 리뷰어님하테 여쭤봤다.

💡좋다 나쁘다의 기준이 사실 모호하긴 합니다. 🤔
개인적인 선호도는 SecondPolicyFareHandler 같은 이름인데, 이유는 세세한 정책보다는 조금 더 일반적인 이름을 갖는 게 아무래도 변경 영향에 대한 전파가 적다고 생각합니다.
물론 오른쪽 이름같이 하면 더 명확한 이름이라 무엇을 나타내는 지 더 이해가 쉬운 장점이 있다고 생각합니다 😄

 

각각의 장단점이 있고 팀원들간의 협의를 통해 정하면 될 것 같다.

 

기본 요금을 구성하는 클래스는 아래와 같다.

@Component
public class BasicFareHandler implements FareHandler {
    public static final long BASIC_DISTANCE = 10;
    public static final long BASIC_FARE = 1250;

    private FareHandler nextHandler;

    @Override
    public FareHandler setNextHandler(FareHandler fareHandler) {
        this.nextHandler = fareHandler;
        return this;
    }

    @Override
    public long calculate(long distance) {
        if (distance <= BASIC_DISTANCE) {
            return BASIC_FARE;
        }
        return BASIC_FARE + nextCalculate(nextHandler, distance);
    }

}

  

테스트 코드는 @SpringBootTest를 이용하였다.

아래와 같은 피드백을 받았다.

💡SpringBootTest 로 괜찮을까요? 슬라이스 테스트에 대해 찾아보고 고민해보시면 좋습니다 👍

 

슬라이스 테스트를 언급하셔서 아래와 같이 구현하였다.

@ExtendWith(SpringExtension.class)
@Import(BasicFareHandler.class)
class BasicFareHandlerTest {

 

아래와 같은 피드백을 받았다.

💡👍
어떤 차이가 있는지도 확인해보셨나요? 🤔
어쩌면 spring bean 으로 관리될 필요가 없을 수도 있는 클래스 같아요 😅
어떤 것이 맞다 틀리다라고 말씀드리려는 것은 아니고, 여러가지 관점에서 생각해보시면 좋을 것 같아서 피드백 드린 것이구요
어쩌면 이 클래스가 그냥 순수하게 자바만으로도 테스트 할 수 있지 않을까? 라는 생각까지 해보셨으면 좋겠습니다 😄

 

BasicFareHandler에 @Component가 달려있긴 하지만 별다른 의존성이 없었기에 테스트 시 new로 객체를 생성해서 주입 해줘도 테스트가 가능하였다. 

 

아래와 같이 수정하였다.

class BasicFareHandlerTest {

    private BasicFareHandler basicFareHandler;

    @BeforeEach
    void setUp(){
        basicFareHandler = new BasicFareHandler();
    }

 

Step3 요금 정책 추가

Step3 요금 정책 추가 PR

 

rest assured 로 구성된 API 요청을 도메인별로 나눴다.

public class LineSteps {
public class SectionSteps {
...

 

API 요청시 토큰을 세팅하는 로직이 중복되어 있었다.

private void setAuthorization() {
  if (this.accessToken != null && !this.accessToken.isEmpty()) {
    this.spec.header(AUTHORIZATION, "Bearer " + this.accessToken);
  }
}

 

아래와 같은 피드백을 받았다.

💡 매번 비슷한 설정을 추가해야할 것 같은데, 좀 더 좋은 방법은 없을까요? 🤔

 

추상 클래스를 생성하여 토큰을 세팅하는 공통 로직을 위로 올리고 각각의 Steps에선 상속을 받도록 구현하였다.

public abstract class RestAssuredRequestBuilder {

    private static final String BEARER = "Bearer ";

    protected RequestSpecification spec;
    protected String accessToken;

    public RestAssuredRequestBuilder() {
        this.spec = RestAssured.given().log().all();
    }

    public void setAuthorization() {
        if (accessToken != null && !accessToken.isEmpty()) {
            this.spec.header(AUTHORIZATION, BEARER + accessToken);
        }
    }
}

 

아래와 같은 요구사항이 있었다.

로그인 사용자의 경우 연령별 요금으로 계산
  - 청소년: 운임에서 350원을 공제한 금액의 20%할인
  - 어린이: 운임에서 350원을 공제한 금액의 50%할인

 

Member 도메인에서 age 필드를 가지고 있어 Member에서 age를 확인하여 요금을 할인하는 책임을 할당했다.

public long discountExtraFare(long fare) {
  FareAgeGroup fareAgeGroup = FareAgeGroup.of(age);
  return fareAgeGroup.calculateDiscountFare(fare);
}

 

아래와 같은 피드백을 받았다.

💡 이건 Member의 책임이라고 보기 힘든 것 같아요 😢
일반적으로 생각해보면, 회원 도메인은 굉장히 많은 곳에서 사용되는 공통적인 도메인이라고 할 수 있습니다.
요금 계산에 대한 것을 알필요가 없습니다

 

getter는 무조껀 지양해야 한다고 생각했다. 하지만 책임 관점에서 생각해보면 리뷰어님 말씀이 맞다. 회원이 요금 계산을 알 필요는 없기에 의존성을 끊어야 했다. 하지만 요금 계산을 하려면 age를 알아야 한다.

age를 Member 도메인에서 꺼낼지 아니면 LoginMember라는 객체에서 age 필드를 추가하여 꺼낼지 고민이였고, 전자로 구현하였다.

※ LoginMember는 로그인한 사용자고, 토큰의 payload에서 추출한 값을 알고 있었다.

Member member = memberRepository.getBy(email);
FareAgeGroup fareAgeGroup = FareAgeGroup.of(member.getAge());

 

아래와 같은 피드백을 받았다.

💡 딱히 어떤 코드가 더 좋다라고 하기엔 애매한것 같아요 😄
두 방법 모두 괜찮다고 생각하구요. 의존성을 어떻게 관리해야겠다라는 관점에서 고민해보면 좋을 것 같아요

 

의존성 관리 관점에선 별다른 차이는 없을 것 같다.

또한, 토큰이 유효한 상황에서 회원의 age가 수정되면 재 로그인을 하지 않으면 LoginMember에는 반영되지 않는다. 따라서 정확한 요금 계산을 위해 DB에서 조회한 Member 도메인에서 age를 추출하는게 좋다고 생각한다.

 

PathService에서 아래와 같은 코드가 있다.

long plusExtraFare = lines.calculatePlusExtraFare();

Member member = memberRepository.getBy(loginMember.getEmail());
long discountExtraFare = member.discountExtraFare(fare);

long totalFare = fare + plusExtraFare - discountExtraFare;

 

다음과 같은 피드백을 받았다.

💡 정책이 추가될때마다 PathService 가 영향을 받게 될 것 같아요 😢
다른 좋은 방법은 없을까요?

 

totalFare = 기본 요금 + 추가요금 - 할인 요금을 계산하고 있다. 만약 정책이 추가되거나 바뀌면 PathService에서 수정을 해야한다.

 

리뷰어님이 아래 영상을 힌트로 주셨다.

https://www.youtube.com/watch?v=26S4VFUWlJM

 

요금 책임을 담당하는 Fare 객체를 만들고 PathService는 Fare 객체에 요금 정책 계산 요청 후 최종 요금만 반환받도록 구현하였다.

Fare fare = new Fare(lines, fareAgeGroup);
long totalFare = fare.calculateTotalFare(fareAgeGroup, distance);

 

다음과 같은 피드백을 받았다.

💡 FareAgeGroup 이라는 정책까지 감춰지면 더 좋을 것 같습니다 😄

FareAgeGroup는 member의 age를 통해서 구할 수 있는 객체다.

그렇다면 Fare 생성자에 age를 넘겨서 Fare 내부에서 FareAgeGroup를 생성하라는 의미로 받아들였다.

 

 

Fare는 정책 관련된 책임인 FarePolicy랑 FareHandlerFactory를 알고 있다.

요금 정책 계산에 필요한 인자를 받고 순서에 맞춰 List<FarePolicy>에 객체를 등록한다.

public class Fare {

    private final List<FarePolicy> farePolicies;

    public Fare(Lines lines, FareAgeGroup fareAgeGroup) {
        this.farePolicies = List.of(
                new LineExtraFare(lines),
                new AgeDiscount(fareAgeGroup)
        );
    }

    public long calculateTotalFare(FareAgeGroup fareAgeGroup, long distance) {
        if (fareAgeGroup.isLineExtraFareFree()) {
            return 0;
        }
        long basicFare = new FareHandlerFactory().calculateFare(distance);
        for (FarePolicy policy : farePolicies) {
            basicFare = policy.calculateFare(basicFare);
        }
        return basicFare;
    }

}

 

Fare의 calculateTotalFare는 다음과 같다.

1. 연령대가 무료인 경우 0원 요금 반환

2. 거리별 요금 계산

3. 추가 요금 정책 계산

3-1. 추가 요금이 부과되는 노선

3-2. 연령별 할인

 

리뷰어님께 칭찬을 받았다.

💡 잘해주신것 같습니다 👍
조금 더 조언을 드리자면, FareHandlerFactory 에서 일어나는 행위도 FarePolicy 로 묶을 수 있을 것 같다는 아쉬움이 아주 조금 있습니다 😅
다만, 이런 설계에 관한 것들은 사람마다 의견이 다를 수 있기 때문에 고려하신 부분들에 부합하는 설계라면 좋은 설계라고 생각합니다

 

FareHandlerFactory는 거리별 요금계산 책임을 생성하는 클래스다.

생각해보니 요금 정책안에 거리별 요금계산도 포함관계이니 FarePolicy의 구현체로 등록했을 수도 있겠구나 라는 생각이 들었다.

 

코딩을 하면서 도메인 객체를 스프링 빈으로 관리하는게 맞는지 의구심이 들었다.

다음과 같은 질문을 하였다.

💡 Dijkstra.java, Lines.java 등 이러한 도메인 객체들은 스프링 빈으로 관리하지 않는게 좋을까요?
제가 생각한건 도메인 객체를 순수 자바 객체로 작성해야 테스트 코드 작성하기가 간편하다 정도만 알고 있었는데요.
도메인 객체를 스프링 빈으로 관리하면 멀티 스레드 환경에 안전하지 않아서 도메인 객체들은 되도록이면 순수 자바 객체로 관리해야 하는 걸까요?
스프링 빈으로 관리해야 하는 기준을 어떻게 잡아야 할지 모르겠네요 ㅠㅠ

 

다음과 같은 답변을 받았다.

💡스프링 빈은 아시다시피 대부분 싱글톤으로 만들어서 사용합니다.즉, application context 의 lifecycle 이 지속되는 동안 계속 같은 객체를 사용한다는 의미입니다. 대부분의 웹 애플리케이션은 멀티쓰레드를 사용하게 되고, 이는 동일한 객체에 여러 쓰레드가 동시에 접근할 가능성이 높다는 이야기입니다. 그렇다면 스프링 빈은 멀티쓰레드 환경에 안좋은 형태가 아니냐? 라는 의문이 들 수 있는데, 그렇기 때문에 항상 stateless 하게 만들어서 사용해야 합니다. stateless 하게 만든다는 것이 무슨 의미일까요? 변경 가능한 상태를 갖지 않게 하면 된다는 의미인데, 변경가능한 상태를 갖는 다는 것의 정의가 무엇인지 생각해보면, 인스턴스 변수 / 스태틱 변수 등을 final 이 아닌 상태로 가지는 것을 말합니다. (method block 안의 변수는 메모리 기준으로 stack frame 내에서만 공유하기 때문에 다른 쓰레드와 공유되지 않음)

길게 설명을 하였는데, 쉽게 말하면, 클래스의 멤버변수를 final 인 상태로만 구현 하면 됩니다. 바꿔말하면 final 이 아닌 멤버변수를 가져야 하는 클래스는 모두 bean 으로 만들 대상이 아닌것이 됩니다. 추가로, Dijkstra 클래스는 final 로 선언된 변수를 가지고 있지만 해당 변수의 내부가 자주 바뀌는 성질을 가지고 있기 때문에 bean 으로 적합하지 않다고 볼 수 있습니다.

보통은 싱글톤으로 관리하면 메모리 절약이 된다 등의 이야기도 있긴 한데, 개인적으로는 그런 부분에서는 큰 의미가 없다고 생각해요 😅

 

MVC의 controller, service, repository는 변하는 값을 멤버 변수로 가지는게 아니다 보니 bean으로 등록해도 stateless하다.

하지만 도메인 객체들은 엔티티에서 조회한 것을 멤버 변수로 가지고 있기 때문에 자주 바뀔 수 있다. 그러므로 bean으로 등록하면 멀티쓰레드에서 동시성 이슈가 발생할 수 있으니 빈으로 등록하는게 적합하지 않다.