[Review] ATDD 3주차 2차 PR

 

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

 

1. 질문

1-1) 공통의 테스트를 만들었어야 하는가?

미션중 TokenAuthenticationInterceptor와 UsernamePasswordAuthenticationFilter 를 추상화 하는 단계에서는,
두 필터에 공통적으로 적용 가능한 테스트를 작성한 후, 공통 부분을 추상화 했어야 할까요?

두 필터의 공통으로적용할 테스트를 작성하기 어렵다 생각되어 새로운 테스트는 작성하지 못하고, 기존의 테스트 코드로 구현하게 된것 같아 아쉽습니다.

 

리뷰어 님이라면 어떻게 하셨을까요? 의견 궁금합니다!!

 

답변: 

공통의 테스트를 작성하기시 보다는, 구현체 위주의 테스트를 작성한다고 하셨다.

추상 클래스를 기반으로 하는 테스트는 인수테스트 선에서 처리하시는 것 같다.

 

다만 수업중에 배웠던 내용인 TDD를 활용한 리팩터링 방법이 있었기 때문에 공통의 테스트를 작성했어야 한다고 생각된다.

추상화 시켜야 하는 A, B 기능이 있다. 각각에 대한 테스트는 이미 작성되어 있다.

따라서 1번에 해당되는 새로운 테스트를 만들기 를 진행했어야 할것 같은데...... (난 작성하지 못해버린 상황)

만약 새로운 테스를 작성했다면, 추상화 후, 3번 과정을 통해 기존 테스트를 모두 삭제했을것 이다...

 

공통의 테스트를 어떤 방식으로 작성했어야 할까... 아직도 고민이다.

 

2. 리뷰 정리

2-1) Exception의 메시지 분리

 

나: 

아! public 상수로 만들어서 assert문에서 비교할때 BuildResponseTokenFailException.MESSAGE 와 같이 사용 가능하다는 말씀이시죠?

혹 다음과 같이 상수 이름을 MESSAGE 로만 적어도 충분할 것 같은데 리뷰어님의 의견이 궁금합니다.

public class BuildResponseTokenFailException extends RuntimeException {

    public static final String MESSAGE = "해당 데이터를 Token으로 만들 수 없습니다";

    public BuildResponseTokenFailException() {
        super(MESSAGE);
    }
}

이러면 테스트에서 실패 메시지를 검증할때도 BuildResponseTokenFailException.MESSAGE로 사용가능할것 같습니다!

 

답변:

상수명을 뭐로짓는지는 적절히 의미만 전달되면 상관없을 것 같기는합니다.
저는 with parameter(적절한 예외에 필요한 자료들) 케이스에 나눠서 메세지가 추가되기도해서 DEFAULT_MSG나 DEFAULT_MSG_FORMATTED 이런식으로 나누기는 하는데, 이건 취향문제같네요!

 

Exception이 인자를 받을수도 있기 때문에 DEFAULT prefix를 사용하시는 것 같다.

이러한 방식도 좋은것 같다!

 

2-2) Private 메서드의 인자 검증

좋은 방식인것 같다.

사실 private 메서드는 테스트 하기도, 디버깅 하기도 어렵다.

이런 private메서드에 인자를 전달받는지 검증하는 과정 하나만 더 있어도 진짜 많은 도움이 될것이다.

 

다음 코드와 같이 Assert.notNull() 을 추가하게 되었다!

private void afterAuthentication(UserDetails userDetails) {
    Assert.notNull(userDetails);
    Authentication authentication = new Authentication(userDetails.getPrincipal(), userDetails.getAuthorities());
    SecurityContextHolder.getContext().setAuthentication(authentication);
}

 

2-3) Java docs 주석

솔직하게 java docs 주석 은 처음 들어봤다.... 아닌가? 예전에 자바의 정성 어딘가에서 잠시 봤던거 같기도?.....

 

주석도 표준화된 docs 방식으로 작성하면 좋다는 의미이신 것 같다...

하지만 이건 어디까지나 회사에서 협업을 할때의 이야기가 아닐까? 개인 프로젝트나, 개인 공부 과정에서는 이정도로 주석을 작성할 필요는 없는것 같다.

 

RestDocs를 사용한 문서화 정도는 작성하는게 좋다고 생각하지만, 주석같은 경우 의미가 명확하면 대부분 지워버리기 때문에 

개인 프로젝트에서 적용은 음? 고민을 더 해봐야 겠다.

 

2-4) Command, Query 서비스 분리

 

나:

CQRS에 따라 Command, Query Service를 분리하였습니다!
음 그 혹시 위에 게이트웨이를 하나 두라는 말씀이? => 패키지 하나 추가하여 그안에 Command, Query Service 를 두라는 의미 이신가요??

 

답변:

좀 더 상위의 XXXService 인터페이스와 이를 구현하는 Impl이 있는데 이는 게이트웨이 역할을 겸임하죠.
그리고그 하위에 Command와 Query를 담당하는 각각의 서비스가 있고, 게이트웨이에서는 적절히 요청에 맞는 로직을 가지고있는 서비스 계층에게 책임을 위임하는것이죠. 규모가 작은 경우에는 불필요한 오버엔지니어링으로 보일 수 있는데,
프로젝트가 커질수록 고려해보는것도 나쁘지 않은 방식입니다.

 

사실 여기까지 듣고 나서 한번에 의도를 파악할수가 없었다.

리뷰어 님께 DM을 보냈는데, 코드로서 답변을 주셨다. 역시 코드로 대화하니 한번에 이해가 되버렸다!

좋은 방식을 하나 알게되어 좋다!

 

예를 들어 다음과 같은 코드 구조가 가능할것 이다.

public interface ACommandService {
    void update();
    void create();
    void delete();
}

public interface AQueryService {
    A find(Long id);
}

public class AServiceGateway implements ACommandService, AQueryService {
    private final AQueryService aQueryService;
    private final ACommandService aCommandService;

    public AServiceGateway(AQueryService aQueryService, ACommandService aCommandService) {
        this.aQueryService = aQueryService;
        this.aCommandService = aCommandService;
    }


    @Override
    public void update() {
        aCommandService.update();
    }

    @Override
    public void create() {
        aCommandService.create();
    }

    @Override
    public void delete() {
        aCommandService.delete();
    }

    @Override
    public A find(Long id) {
        return aQueryService.find(id);
    }
}

두 개의 나눠진 서비스 인터페이스가 있고, 이를 정의한 인터페이스를 구현하는 게이트웨이가 있고 여기서 커맨드와 쿼리를 주입받아 위임하죠.

 

상당히 흥미로운 방식이였다.

평소 나는 CRQS를 개념적으로 지키려 노력하는데 한가지 구체적인 방식에 대하여 알게된것 같다!