ATDD, 클린 코드 with Spring 피드백 정리

1월 22일 생일 기념으로 강의를 신청했다. 오프라인 모임을 가졌을 때 이 강의를 왜 듣게 되었는지 말하는 시간이 있었는데 그때는 대답을 성장을 위해라고 했다. 이어서 왜 성장을 하고 싶은지? 성장을 위한 원동력이 무엇인지?에 대해 이야기를 나누었는데 대답을 잘 못했다. 강의를 신청한 이유를 생각해보면 그냥 미션을 하고 피드백 받는 과정이 재밌어서 신청했던 것 같다. 미션을 끝냈을 때 성취감이랑 개선하기 위해 고민하는 시간들이 좋았다. 왜 성장하고 싶은지에 대해서는 좀 더 생각해봐야할 것 같다. 전에 `위대한 나의 발견 강점혁명`이라는 책에서 배움이 내 강점이라고 생각했는데 새로운 걸 배우는 게 재밌다. 요즘에는 꾸준히 공부를 하다보면 결과는 언젠가 자연스럽게 따라오지 않을까라는 생각으로 조급함에서 벗어나게 된 것 같다

그리고 이번 과정에서는 오프라인으로 페어프로그래밍을 통해 미션을 진행하는 모짝미(모여서 짝 미션)도 참여했다. `내가 너무 못해서 피해를 주면 어떡하지`라는 생각이 들어 갈지말지 고민이 되었지만 이런 경험을 해볼 수 있는 기회가 많지 않기 때문에 용기를 냈다. 시작하기 전 페어프로그래밍을 어떻게 하는지 간략하게 설명해주셨는데 내가 했던 고민들도 적혀있었다. 잘하는 사람도 설명을 하면서 정리하는 시간을 가질 수 있기 때문에 적극적으로 설명을 요청하라고 하셨던 것 같다. 덕분에 많이 배울 수 있었다. 네트워킹 시간에는 개발자로서 하는 고민들에 대한 이야기를 들을 수 있어서 좋았다.

끝나가는 시점에서의 후기
내가 하는 질문들이 너무 사소하진 않을까? 내가 너무 안찾아보고 질문하는 거는 아닐까? 그런 생각이 들기도 했는데 일단 돈을 냈으니 궁금한건 여쭤보자라는 생각으로 임했고 리뷰어분들의 경험을 들을 수 있어서 좋았다. 막 짜던 습관을 버리고 + 인수테스트 작성까지 난이도를 높여 수련의 과정을 겪은 느낌이다. 인수테스트 작성, 클린코드, 페어프로그래밍까지 많은 걸 경험할 수 있었고 아는 것에 그치지 않고 활용할 수 있도록 해봐야겠다.

PR링크

인수 테스트와 E2E 테스트
인수 테스트와 TDD
인수 테스트와 인증
인수 테스트와 리팩터링

인수 테스트와 E2E 테스트

Location헤더를 이용

ExtractableResponse<Response> response = createStation("강남역");
long id = response.jsonPath().getLong("id");

String locationHeader = response.header("Location");

Q. createStation 메소드는 ExtractableResponse 로 반환하고 getStationNames 메소드는 .jsonPath().getList("name", String.class) 를 추가해서 List 으로 반환하고 있습니다. 반환 타입을 통일성있게 가져가는게 좋은지, 사용하는 곳의 편의를 위해서 다르게 작성해도 되는 건지?

A. 도메인 객체 같은 경우는 좀 더 고민을 해야겠지만(정말 필요한 메소드만 노출하도록 노력), 테스트 클래스나 dto 같은 곳에서는 여러개 노출해도 상관없다고 생각하는 편

Q. 기능 단위로 커밋을 하려고 보니 생성에 관한 인수테스트가 제대로 동작하는지 확인하려면 조회를 구현하게 되어 생성과 조회를 함께 커밋하게 되는데 커밋 단위를 어떻게 해야 할까요?

A. 커밋 단위는 팀원간의 협의 - 기준은 구현도 많이 해보고 협업도 해보면서 가져가기! 첫번째 방법 - 일단은 등록에 대한 응답만 확인하는 테스트를 구현하고 성공시킨다. 이후 조회를 구현한 후 생성 테스트 쪽에 검증을 추가한다. 두번째 방법 - 등록과 조회가 꽤나 밀접한 관계에 있다고 보고 한번에 구현한다.

Q. Line(entity) 에서 값을 가지고 있기 때문에 이곳에서 LineResponse를 생성하는 메소드를 작성하는게 깔끔하다고 느껴지는데 어디서 response를 생성하는게 좋을까요?

A. 이런 경우에 가장 우선적으로 생각해야 하는 것은 의존성입니다. 비즈니스 룰을 가지고 있는 도메인 객체(이번 강의에서는 Entity)는 외부 의존성이 없어야 합니다. 외부 의존성이란 도메인 객체를 제외한 객체입니다. 이런 변화에 비즈니스 로직이 영향을 되도록 영향을 받지 않게 하려면 격리 시키는 것이 유리합니다. 따라서, Response 로 변환하는 로직은 LineResponse 의 생성자로 Line 을 받거나, 혹은 중간에 Factory 등을 두어 변환 시키는 것이 좋습니다.

개선

//변경 전
public LineResponse findLineById(Long id) {
    return lineRepository.findById(id)
            .orElseThrow(EntityNotFoundException::new)
            .createLineResponse();
}

public StationResponse createStationResponse() {
    return new StationResponse(id, name);
}

//변경 후
public LineResponse findLineById(Long id) {
    Line line = lineRepository.findById(id)
            .orElseThrow(EntityNotFoundException::new);
    return new LineResponse(line);
}

public LineResponse(Line line) {
    this.id = line.getId();
    this.name = line.getName();
    this.stations = List.of(new StationResponse(line.getDownStation()), new StationResponse(line.getDownStation()));
}

Q. sql 파일을 통해 DB에 넣어두는 방식과 테스트 실행 시에 직접 요청을 보내는 방식을 고민하였습니다. 테스트를 위한 사전 데이터를 어떻게 처리하면 좋을지

A. 실무에서는 사실 다양한 방법을 선택. 지금 상황에서는 시나리오의 모든 상황을 api 호출할 수 있기 때문에 api 호출해서 생성하는 것을 추천

Q. StationAcceptanceTest 에 있던 createStation 메소드를 중복해서 작성하였는데 중복코드를 어떻게 분리할지 고민

A. StationSteps 같이 역에 관련된 네이밍으로 추출

개선

public class StationSteps {

    public static ExtractableResponse<Response> createStation(String name) {
        Map<String, String> params = new HashMap<>();
        params.put("name", name);

        return RestAssured.given().log().all()
                .body(params)
                .contentType(MediaType.APPLICATION_JSON_VALUE)
                .when().post("/stations")
                .then().log().all()
                .statusCode(HttpStatus.CREATED.value())
                .extract();
    }
}

method reference 사용

.orElseThrow(() -> new EntityNotFoundException())

.orElseThrow(EntityNotFoundException::new)

Q. N+1 문제 해결을 위해 EntityGraph를 사용했는데 만들고 보니 이게 맞는지, 어떻게 쓰는게 좋은지 의견을 듣고 싶습니다!

 @EntityGraph(attributePaths = {"upStation", "downStation", "sections", "sections.upStation", "sections.downStation"})
 List<Line> findAll();

A. 실제 생성되는 쿼리도 항상 같이 확인하면 좋음. 아예 간접 참조를 통해 실제로 필요할때만 쿼리를 하도록 하는 방법이나, 조회 성능을 위한 쿼리는 아예 따로 만드는 등의 방법도 있음. 항상 어떤 방법이 정답이라고 하기는 힘들고, 특히나 성능을 위해 최적화 하는 부분은 예상과는 다르게 작동하는 경우가 많으니 테스트를 많이 해보는 것이 좋음!

Q. 지하철 역의 순서를 보장하고자 상행을 기준으로 다음 역을 찾는 로직을 작성을 했는데

응답DTO에서 이 로직을 짜는게 맞는 건지
순서를 보장하는 로직을 좀 더 깔끔하게 짜고 싶은데 어떻게 하면 좋을지

A. 순서를 보장하는 것도 비즈니스 룰이라고 생각. 따라서 Entity 내에서 구현. Comparable 을 Section 이 구현하도록 구성해보는 방법이 있음. null 반환 가능성이 있다는 것을 알리기 위해 Optional 을 사용하는 편이 좋음

개선

//기존
public List<Station> getOrderedStations() {
    List<Station> stations = new ArrayList<>();
    Station upStation = getStartStation();
    stations.add(upStation);

    for (int i = 0; i < sections.size(); i++) {
        upStation = findNextStation(upStation);
        stations.add(upStation);
    }

    return stations;
}

private Station findNextStation(Station upStation) {
    return sections.stream()
            .filter(section -> section.equalsUpStation(upStation))
            .map(Section::getDownStation)
            .findFirst()
            .orElseThrow(NoSuchElementException::new);
}

private Station getStartStation() {
    return sections.stream()
            .map(Section::getUpStation)
            .filter(upStation -> isStartStation(upStation))
            .findFirst()
            .orElseThrow(NoSuchElementException::new);
}

private boolean isStartStation(Station upStation) {
    return sections.stream()
            .noneMatch(section -> section.equalsDownStation(upStation));
}

private Station getEndStation() {
    return sections.stream()
            .map(Section::getDownStation)
            .filter(downStation -> isEndStation(downStation))
            .findFirst()
            .orElseThrow(NoSuchElementException::new);
}

private boolean isEndStation(Station downStation) {
    return sections.stream()
            .noneMatch(section -> section.equalsUpStation(downStation));
}

//변경
public class Section implements Comparable<Section> {

    @Override
    public int compareTo(final Section section) {
        if (this.equals(section)) {
            return 0;
        }
        if (this.downStation.equals(section.upStation)) {
            return -1;
        }
        return 1;
    }
}

private Station getEndStation() {
    return getOrderedStations().get(sections.size());
}

public List<Station> getOrderedStations() {
    return sections.stream()
            .flatMap(section -> Stream.of(section.getUpStation(), section.getDownStation()))
            .distinct()
            .collect(Collectors.toList());
}

Comparable로 어떻게 구현해야하는지 감을 못잡았었는데 다른분 코드를 보고 개선할 수 있었다.. 엄청 뚱뚱했던 코드가 간결하게 바뀌는 놀라운 경험이었다...

Q. 구간에 대한 요청은 LineController에서 처리하는게 적합한지 아니면 따로 컨트롤러를 만드는게 좋은지

@PostMapping("/lines/{id}/sections")
public ResponseEntity<LineResponse> createSection(@PathVariable Long id, @RequestBody SectionRequest sectionRequest) {
    LineResponse line = lineService.saveSection(id, sectionRequest);
    return ResponseEntity.created(URI.create("/lines/" + line.getId())).body(line);
}

A. 개발하다보면 항상 고민하게 되는 부분. 실무에서 개발할때는 어떤 것이 일관성을 가져가기 더 좋을까를 고민. 클래스가 너무 커지지 않도록 하려는 편인데, 구간에 대한 처리가 노선 에 대한 처리에 포함될 수 있는 개념인지 고민해보고 포함된다면 같은 클래스에 먼저 둬보고, 너무 커진다는 생각이 들면 나눌 것.

일급컬렉션 사용

//기존
@OneToMany(mappedBy = "line", cascade = CascadeType.PERSIST, orphanRemoval = true)
List<Section> sections = new ArrayList<>();

//변경 후 - 관련 로직 이동
@Embedded
private Sections sections = new Sections();

@Embeddable
public class Sections {
	@OneToMany(mappedBy = "line", cascade = CascadeType.PERSIST, orphanRemoval = true)
    List<Section> sections = new ArrayList<>();

    public Sections() {
    }

    public void addSection(Section section, Station downStation) {
        if (sections.size() > 0) {
            validateNextSection(section, downStation);
            validateDuplicateStation(section);
        }
        this.sections.add(section);
    }

    private void validateNextSection(Section section, Station downStation) {
        if (!downStation.isEquals(section.getUpStation())) {
            throw new SubwayException("구간의 상행역은 해당 노선에 등록되어있는 하행 종점역이 아닙니다.");
        }
    }

    private void validateDuplicateStation(Section section) {
        if (isContains(section.getDownStation())) {
            throw new SubwayException("이미 등록되어있는 역입니다.");
        }
    }
}

인수 테스트와 TDD

Q. 테스트를 위해서 코드를 추가하거나 변경하는 것을 지양해야 한다고 알고있어 최대한 바꾸지 않으려고 하였는데 테스트 하기 위해서 SectionRequest를 만들기 위한 생성자를 추가. 생성자를 추가하지 않고 테스트하는 방법이 있을지 아니면 이 정도는 만들어도 괜찮은지

SectionRequest sectionRequest = new SectionRequest(역삼역아이디, 선릉역아이디, 10L);
lineService.addSection(이호선아이디, sectionRequest);

A. 지양하는 부분이긴 하지만, 간단한 생성자이니 크게 문제는 없다고 생각. 아래와 같은 방법을 활용하면 생성자 추가없이도 private변수에 추가가 가능

ReflectionTestUtils.setField();

Q. 현재 로직과 (중간 구간인지 체크하는 로직을 명시하고 한번 더 조회) getNextSection메소드에서 Optional을 반환하여 null이 아닌 경우 nextSection.update(section);를 실행하는 것 중에서 어떤 로직이 더 나은지 고민이 되었습니다! 전자의 경우 가독성이 좋은 반면 찾는 로직을 두번 실행하게 되고, 후자의 경우 getNextSection메소드 한번으로 조회할 수 있는 것이 장점이라고 생각됩니다. 어떤 방식을 선호하시나요?

if(isMiddleSection(section)){
    Section nextSection = getNextSection(section);
    nextSection.update(section);
}

A. 개인적으로는 찾는 로직 자체가 크게 무겁지 않은 것으로 생각되어서 현재 로직을 더 선호하지만, 추후에 찾는 로직이 무거워진다면 한번더 고민해볼 필요는 있어보임

Q. 인수테스트에서 검증 되었다고 생각하였는데 서비스에 대한 단위 테스트를 작성 해야할까?라는 고민이 들었습니다! 어떤 기준으로 인수테스트, 서비스 단위테스트, 객체 단위테스트를 진행하면 좋을까요?

A. 인수 테스트 > 도메인 레이어 테스트 > 서비스 레이어 테스트 순으로 작성해보는 것 추천. 서비스 단위 테스트는 사실 작성이 쉽지 않기도 하고, 변경이 잦기도해서 도메인 단위 테스트 대비는 중요도가 낮다고 생각. 일반적으로 객체 내부에 핵심 비즈니스 로직이 위치하고 있고 서비스에서는 호출만 하는 경우는 과감히 생략해도 큰 문제는 아닐 것 같다고 생각. 다만, 서비스 레이어에 뭔가 로직이 존재해서 확인이 필요하다고 느끼실 때는 작성 하길 추천.

Q. 노선의 가운데 역을 삭제하는 과정에서

앞 구간을 업데이트 하고 뒤에 구간을 삭제한다.
두 구간을 삭제하고 새 구간을 만든다.

두가지 로직을 고민했습니다. 테스트가 쉬운 방식을 생각하다보니 전자를 구현했습니다.
어떤 방식을 선호하시는지 궁금합니다!

if(sectionByUpStation.isPresent() && sectionByDownStation.isPresent()){
    Section downSection = sectionByUpStation.get();
    Section upSection = sectionByDownStation.get();

    upSection.join(downSection);
    this.sections.remove(downSection);
    return;
}

A. 현재 서비스에서는 특별히 트래픽을 고려하고 있지는 않기 때문에 두가지 방식중에 더 마음에 드시는 방향으로 작성. 실제 동시성이 요구되는 환경에서라면 각각의 경우 쿼리가 어떻게 나가는지, 트랜잭션을 어떻게 묶어서 진행할지 등등에 대해 더 고려는 해봐야될 것.

예외케이스 검증
단위 테스트와 인수테스트 둘중에 한곳에서만 예외케이스를 검증한다고 하면, 인수테스트는 아무래도 단위테스트 대비 실행 시간이 오래걸리기때문에 인수테스트보다는 단위테스트에서 작성하시는 걸 추천

Q. 간접 참조와 직접 참조에 대해서 고민해본 결과 모두 직접 참조로 구현했습니다.

직접참조의 경우

조회 시에 객체에 담긴 정보를 가져오기 편리했습니다.
즐겨찾기 삭제 시에 객체 비교를 통해 본인의 즐겨찾기인지 확인하는 것이 id 없이 도메인 단위테스트 하기 편리했습니다.
결론적으로 좀 더 객체지향적으로 구현할 수 있어서 편하다는 느낌을 받았습니다.
반면 따로 구현하진 않았지만 n+1 문제가 있기 때문에 fatch join이 필요하고,
Favorite이라는 객체를 생성하기 위해 Station, Member를 조회하는 쿼리가 추가된다는 단점이 있었습니다.

간접참조의 경우

Favorite 객체를 생성할 때는 편리하지만
조회할 때 FavoriteRepository에서 FavoriteResponse DTO 객체로 변환해서 받아야 한다고 알고 있습니다.
결론적으로 최적화해서 받기에는 좋지만 번거롭다는 느낌을 받았습니다.

실무에서 간접 참조와 직접 참조를 결정하는 기준이 무엇인지 궁금합니다!

@ManyToOne
@JoinColumn(name = "sourceStationId")
private Station sourceStation;

@ManyToOne
@JoinColumn(name = "targetStationId")
private Station targetStation;

@ManyToOne
@JoinColumn(name = "memberId")
private Member member;

A. 직접 참조로 구현할 경우에는 jpa를 사용한다고 가정했을 때 의도치 못한 수정이 발생할 수도 있을 것. 예를 들어 Favorite을 조회하면 member도 함께 가져올 수 있는데, member에 이름을 바꾸는 로직이 들어간 경우 favorite을 persist 하면 member도 함께 변경되거나 하는 식. 그런데 간접참조를 하면 말씀해주신 대로 번거롭기도 하고, N+1이 발생할 수도 있을 것 같음. 보통은 생명주기를 같이한다거나 무조건 함께 조회되어야하는 경우에 직접참조를 사용.
-> member는 생명주기를 같이하지 않기 때문에 간접 참조가 낫겠다는 생각이 들었다. 간접 참조는 DTO로 받아야한다고 생각했는데 생각해보니 service에서 id를 통해 repository에서 조회도 가능했다.

Q. findPath의 목적은 경로를 찾는건데, 검증을 위해 재활용해도 되는지? return new Path(path.getVertexList(), path.getWeight()); 이 부분 제외하고는 검증을 위한 로직과 경로 찾는 로직이 거의 같아서 고민

A. 그대로 사용해도 괜찮다고 생각 대신 가독성을 위해서 한번 더 wrapping할 순 있을 것. 예를 들자면
isValidateRoute(lines, sourceStation, targetStation) 을 호출하면 isValidateRoute 메서드 내부에서 PathFinder.findPath를 호출

본인이 아닌 경우나 토큰이 없는 경우에 대한 요청도 테스트

/**
 * Given 즐겨찾기를 생성하고
 * When 토큰 없이 즐겨찾기를 삭제하면
 * Then 에러를 반환한다.
 */
@DisplayName("토큰 없이 즐겨찾기를 삭제하면 에러를 반환한다")
@Test
void deleteFavoriteWithoutToken() {
    // given
    ExtractableResponse<Response> createResponse = 즐겨찾기_생성_요청(accessToken, 교대역, 양재역);

    // when
    ExtractableResponse<Response> response = 토큰_없이_즐겨찾기_삭제_요청(createResponse);

    // then
    assertThat(response.statusCode()).isEqualTo(HttpStatus.UNAUTHORIZED.value());
}

public static ExtractableResponse<Response> 토큰_없이_즐겨찾기_삭제_요청(ExtractableResponse<Response> response) {
    String uri = response.header("Location");
    return RestAssured.given().log().all()
            .when().delete(uri)
            .then().log().all().extract();
}

/**
 * Given 다른 회원이 즐겨찾기를 생성하고
 * When 본인이 작성하지 않은 즐겨찾기를 삭제하면
 * Then 에러를 반환한다.
 */
@DisplayName("본인이 작성하지 않은 즐겨찾기를 삭제하면 에러를 반환한다")
@Test
void deleteFavoriteByOther() {
    // given
    회원_생성_요청(OTHER_EMAIL, PASSWORD, AGE);
    String otherAccessToken = 토큰_생성(OTHER_EMAIL, PASSWORD);
    ExtractableResponse<Response> createResponse = 즐겨찾기_생성_요청(otherAccessToken, 교대역, 양재역);

    // when
    ExtractableResponse<Response> response = 즐겨찾기_삭제_요청(createResponse, accessToken);

    // then
    assertThat(response.statusCode()).isEqualTo(HttpStatus.UNAUTHORIZED.value());
}

Q. oauth 로그인을 통해 회원을 생성할 때 비밀번호는 어떻게 처리하면 좋을지

A. 보통 Oauth 로그인을 활용할 때엔 oauth에서 식별할 수 있는 식별자(email)만 관리, null 또는 빈 스트링을 넣어주어도 될 것 같음 (정책적으로 관리되어야 할 부분이라고 생각)

@Transactional
save 메서드는 자체적으로 SimpleJpaRepository 의 구현체를 사용하는거라 자동으로 트랜잭셔널 애노테이션이 붙는데, 그렇지 않은 경우도 있을 수 있어서 트랜잭션이 꼭 걸려야하는 부분에는 작성해주시는 게 좋음. update의 findById 또한 SimpleJpaRepository를 사용해서, 클래스 레벨에 걸려있는 readOnly 트랜잭션이 동작
TODO: @Transactional, SimpleJpaRepository의 구현체 공부하기

findOrCreate
find만 주로 사용되는 빈도가 높은 경우 사실 트랜잭션을 걸고 조회하지 않아도 되는데 create의 가능성 때문에 트랜잭션을 여는 것이 불필요하다고 느껴짐

'테스트코드' 카테고리의 다른 글

TDD, 클린 코드 with Java 피드백 정리 (1)	2022.12.15

개발 글쓰기

ATDD, 클린 코드 with Spring 피드백 정리

PR링크

인수 테스트와 E2E 테스트

인수 테스트와 TDD

직접참조의 경우

간접참조의 경우

'테스트코드' 카테고리의 다른 글

티스토리툴바

ATDD, 클린 코드 with Spring 피드백 정리

PR링크

인수 테스트와 E2E 테스트

인수 테스트와 TDD

직접참조의 경우

간접참조의 경우

'테스트코드' 카테고리의 다른 글

관련글

티스토리툴바