[1단계 - 사다리 생성] 페드로(류형욱) 미션 제출합니다

[hw0603]

안녕하세요 피케이🙂 6기 BE 페드로입니다. 리뷰 잘 부탁드립니다!

이번 미션을 진행하면서 생긴 궁금한 점들을 정리해 보았습니다.

DTO 저장 구조 관련

지난 미션에서 DTO를 사용한 크루들도 많았는데, 아직까지 DTO가 등장할 수준의 규모는 아니라고 판단하여 저번 미션에서는 DTO를 사용하지 않고 중첩 Collection 형태로 값을 전달하였습니다.
이번 미션에서 DTO 없이 구현하려다 보니 가독성이 더 떨어지는 것 같아 View 단에 전달하기 위한 LineDto를 만들어 사용하였습니다.

특정 지점에서 연결되어 있는 방향을 나타내기 위해 enum LadderPath를 사용하였고,
Line은 List<LadderPath>를 가집니다. 차후 사다리를 타는 기능을 구현하려면 이러한 형태로 저장하는 것이 유리할 것 같다고 판단했습니다.
반면 dto로 사용하는 LineDto의 경우, 출력 로직에서 사용하기 편하도록 특정 지점에서의 방향값을 저장하는 것이 아닌, 두 지점 사이에 경로가 있는지를 저장하고 있는 List<Boolean> 형태로 구현하였습니다.
DTO와 모델 간 저장하는 구조 뿐만 아니라 저장하는 관점 또한 다른데, 단순히 "데이터를 잘 전달" 할 수 있으면 상관없는 것인지 궁금합니다.

정적 팩토리 메서드에서 사용하는 static 메서드

정적 팩토리 메서드에서 객체를 생성하기 전, 인자로 전달받은 정보를 검증하는 로직을 구현할 때 메서드 분리를 통해 한 번에 하나의 조건만 검증하도록 했더니 static 메서드가 너무 많아지는 문제가 있었습니다. 메모리 사용의 측면에서 static 메소드를 많이 선언하는 것은 권장되지 않는 것으로 알고 있는데, 다른 방법으로 구현할 수 있을지 궁금합니다.

LineStringFormatter의 위치

LineStringFormatter는 연결 여부를 가지고 있는 LineDto를 전달받아 사다리 문자열로 변환하는 역할을 합니다. 이전 미션에서 '출력에 관여하는 메서드들은 뷰에 위치해야 한다'라는 리뷰를 받았었는데, 해당 클래스의 경우 뷰 클래스에서 사용하는 클래스이기도 하고 출력에 관여하는 클래스인 것도 맞지만 InputView나 OutputView 처럼 사용자와 직접 상호작용하지는 않고 있어서 같은 패키지에 위치해도 되는 것인지 의문이 남습니다. 피케이는 이에 대해 어떻게 생각하시나요?

[pkeugine]

안녕하세요 페드로!
구현 잘 해주셨네요! TDD 는 해보니 어떠셨나요?
결과물은 정말 잘 만들어주셔서 재밌게 리뷰했네요
리뷰 반영하다가 궁금하신 점 있으면 언제든 DM 주세요!

[pkeugine]

LadderController 의 구현을 이렇게 둘로 나눈 이유가 궁금하네요!
Application 클래스를 만드는 사람 입장에서는 굳이 init() 과 printResult() 를 둘 다 호출해야한다는 것을 알아햐하니까요

[hw0603]

컨트롤러가 하는 일이 초기값 설정 -> 로직 수행 -> 결과 출력 의 과정을 담당한다고 생각해서 이때까지 항상 세 단계로 나누어 구현했었는데 생각해 보니 그런 문제가 있겠네요. 외부로는 start() 메서드만 노출하고 나머지 메서드들은 해당 메서드 안에서 호출하는 형태로 변경하였습니다!

[pkeugine]

enum! 요구사항을 잘 지켜주셨네요 👍

[pkeugine]

constant 만 별도의 패키지에 보관하는 이유가 있나요?
특히 LadderPath 의 경우는 도메인 로직에서 굉장히 중요하게 사용되는 enum 이라고 생각해요

[hw0603]

단순히 상수라는 이유로 별도의 패키지를 만들었던 것 같아요. 애플리케이션 전반에 사용되는 상수라기보다는 말씀하신 대로 도메인 로직과 관련이 깊은 값인 것 같아 model 패키지로 이동시켰습니다.

[pkeugine]

dto 에는 필드에 접근제한자를 붙이지 않은 이유가 있나요?

[hw0603]

실수...였습니다ㅎㅎ 수정했습니다!

[pkeugine]

DTO 저장 구조 관련

오 저번 미션에서는 dto 를 사용하지 않으셨군요! 저도 저번 미션의 규모가 dto 를 사용하지 않아도 괜찮은 크기였다고 봐요.
그리고 이번 미션도 사용하지 않아도 된다고도 생각해요.
하지만 지금 만드신건 유지하셔도 괜찮아요!

차후에 사다리를 타는 기능이 요구사항으로 추가될 거라는 사실을 이미 알고 계셨군요!
어쩐지 그걸 알아야 좌, 우, 중간 개념이 나올텐데 라는 생각을 했었어요.
추후에 새로 생길수도 있는 기능을 고려한 구조 좋네요 👍

Q) DTO와 모델 간 저장하는 구조 뿐만 아니라 저장하는 관점 또한 다른데, 단순히 "데이터를 잘 전달" 할 수 있으면 상관없는 것인지

질문 그 자체에만 답변하자면, 네 상관없습니다. dto 는 말 그대로 데이터를 전달하기 위해 있는 객체라, 다양한 형태로 만들어질 수 있어요. 만드신 dto 는 필드 접근 제한자 외엔 문제가 없어보이네요.

그런데 이렇게 말하면 속이 시원한 답변이 아니죠? 😅

Q) DTO와 모델 간 저장하는 구조 뿐만 아니라 저장하는 관점 또한 다른데...

이걸보며 "페드로가 dto 라는 것을 도입하고나서 그 개념에 대해 너무 많은 고민을 하고 있구나" 라고 생각했어요.

dto 는 데이터를 전달하기 위한 객체니까, 그냥 만들어서 데이터를 넘길 때 사용하면 돼요. 그 값을 어떻게 전달해야 사용하는 곳에서 편할까 라는 생각을 너무 많이 하지 않아도 괜찮아요. dto 안에서 값을 추출하는 getter 를 사용하는 것 이에의 로직을 웬만하면 담지 않아보는 것은 어떨까요? 예를 들어 아래와 같이 Line 의 getter 를 사용하는건 괜찮아요 :

    public static LineDto from(Line line) {
        return new LineDto(line.getConnected());
    }

저는 오히려 '방향' 과 '연결 여부' 를 어떻게 관리할 것인지 고민해보는게 이 미션의 핵심 포인트 중 하나라고 생각했어요.
그 해결책이 dto 에서의 전환은 아니라고도 생각했구요.

• 오른쪽, 왼쪽, 유지 (3개)
• 연결됨, 연결 안 됨 (2개)

이렇게 같은 사다리라도 나타낼 수 있는 형태가 다를 때 써볼 수 있는 무언가가 있을 것 같네요 :)

[hw0603]

속 시원한 답변 감사합니다!
DTO를 최대한 도입하지 않고 작성해 보려다가 이번에 처음 도입한 것이라 그 파급효과에 대해서 고민이 많았던 것 같아요😅

dto 안에서 값을 추출하는 getter 를 사용하는 것 이에의 로직을 웬만하면 담지 않아보는 것은 어떨까요?

이 접근이 훨씬 좋은 것 같아요. Line.toLineDto() 보다 LineDto.From(line) 형태를 유지하고 싶었는데, 작성하면서도 DTO에 변환하는 (복잡한) 로직이 들어가는 것 같아 찜찜했었거든요. List<Boolean> 을 LineDto가 아니라 Line에서 만들어 반환하도록 하니 애매하게 필요한 getter였던 Line.getRow()도 제거할 수 있고 좋네요!

저는 오히려 '방향' 과 '연결 여부' 를 어떻게 관리할 것인지 고민해보는게 이 미션의 핵심 포인트 중 하나라고 생각했어요.
그 해결책이 dto 에서의 전환은 아니라고도 생각했구요.
오른쪽, 왼쪽, 유지 (3개)
연결됨, 연결 안 됨 (2개)
이렇게 같은 사다리라도 나타낼 수 있는 형태가 다를 때 써볼 수 있는 무언가가 있을 것 같네요 :)

이 부분은 아직 생각이 잘 떠오르지 않아 좀더 고민해 보겠습니다! 🤔

[hw0603]

이렇게 같은 사다리라도 나타낼 수 있는 형태가 다를 때 써볼 수 있는 무언가가 있을 것 같네요 :)

[질문] 현재 수정된 구현과 같이 Line에서는 3개의 상태값을 가지고, Line.getConnected() 에서 요청을 받을 때 마다 연결 여부를 나타내는 boolean 배열을 리턴하는 방식 외에는 생각이 잘 나지 않는데, 설명에서 의도하신게 이 방식이 맞을까요? 아니라면 힌트를 하나만 더..ㅎㅎ 원합니다!

[pkeugine]

제가 생각해봤던건 enum 을 활용해보는 거였어요. 대략 이런 방식으로요 :

public enum LadderPath {
  LEFT(true, -1),
  STAY(false, 0),
  RIGHT(true, 1);

  final boolean connected;
  final int direction;

  // 생성자 및 로직...
}

이러면 방향과 연결 유무를 한 곳에서 관리할 수 있거든요!

[hw0603]

감사합니다!

그 해결책이 dto 에서의 전환은 아니라고도 생각했구요.

이 'dto에서의 전환' 이라는 것이 'dto와 도메인이 데이터를 바라보는 관점이 다른 상태'가 아니라 'DTO 내부에서 변환하는 로직이 들어간 상태'를 말씀하시는게 맞겠죠?!

현재 변경된 로직은 enum을 다음과 같이 선언하고,

public enum LadderPath {
    LEFT(true, -1),
    STAY(false, 0),
    RIGHT(true, 1);

    final boolean connected;
    final int direction;

    LadderPath(boolean connected, int direction) {
        this.connected = connected;
        this.direction = direction;
    }

    public static boolean isPathExist(LadderPath leftPath, LadderPath rightPath) {
        return leftPath.equals(RIGHT) && rightPath.equals(LEFT);
    }

    public boolean isConnected() {
        return connected;
    }
}

컨트롤러에서 뷰로 데이터를 넘길 때, LineDto를 생성하며, LineDto의 정적 팩토리 메서드에서 Line.getConnected()를 호출하면,

// LineDto.from()
public static LineDto from(Line line) {
    return new LineDto(line.getConnected());
}

// Controller.printResult() 내부
List<LineDto> lineDtos = ladder.getLadder().stream()
        .map(LineDto::from)
        .toList();
OutputView.printLadder(lineDtos);

Line에서 실제로 매핑(방향 -> 연결여부)을 해 주는 형태로 변경하였습니다.

// Line.getConnected()
public List<Boolean> getConnected() {
    return IntStream.range(0, row.size() - 1)
            .mapToObj(idx -> LadderPath.isPathExist(row.get(idx), row.get(idx + 1)))
            .toList();
}

이 구현에서는 DTO에서는 도메인을 알고 있지만, 도메인에서는 DTO를 모르고, enum을 통해 연결 여부와 방향을 한 번에 관리할 수 있게 되었습니다.

혹시 어색한 부분이 있을까요? 검증 부탁드립니다!

[hw0603]

+) DTO를 이번 미션에서 처음 써서 질문이 많네요..ㅎㅎ 피케이가 해 주신 말씀을 제가 이해한 대로 정리해 봤는데 혹시 틀린 부분이 있을까요?

• 도메인에서 DTO에 의존하는 것은 좋지 않다. DTO에서는 도메인을 알아도 된다(?)

• 도메인 to DTO 변환 시 Domain.toDto() 보다는 DTO.from(Domain domain) 내부에서 domain.getXXX()를 통해 필요한 데이터를 가져와서 변환한다.

  • 이때, DTO로의 변환을 위한 Domain의 게터는 트레이드오프이지만, 게터를 열어서 얻을 수 있는 이득(도메인에서 DTO 의존 x)이 게터 없이 도메인에서 DTO에 의존하는 손해보다 더 크다.

• 그럼 Ladder처럼 별개의 DTO 없이도 List<LineDto> 형태로 나타낼 수 있는 객체들은 DTO로의 변환을 어디서 해야 하는가?

  • -> '뷰'와 '모델'을 이어주는 책임은 컨트롤러에 있으므로, 변환은 컨트롤러에서 이루어지는 것이 자연스럽다.

[pkeugine]

이 테스트는 어떤것을 확인하기 위한 테스트인가요? Ladder 객체에게 7 과 5 가 주는 특별한 의미가 있을까요?

[hw0603]

특별한 의미 없이 TDD 과정 중 지정된 높이와 폭의 사다리를 생성하는 기능이 그대로 테스트 케이스가 된 경우였는데, 현재 구현에서 의미가 퇴색된 것 같아 삭제했습니다. (해당 테스트만을 위해 사용되던 getSize() 또한 삭제할 수 있었습니다)

[질문] 제가 이해한 TDD에서는 각 요구사항(=기능) 이 하나의 테스트 코드로 변환되고, 해당 테스트 케이스를 만족하도록 코드를 작성하며 설계를 이어나가는 것으로 이해했습니다. 이것이 맞다면, 지금과 같이 (비록 해당 기능이 삭제되지 않았음에도) 필요 없어진 테스트 케이스를 삭제하는 경우는 그냥 리팩토링 사이클에 해당한다고 이해하면 될까요?

TDD가 아직 낯설어서 그런지,
기능이 필요하네 -> 테스트코드 만들자 -> 실패하네? -> 구현하자 -> 필요 없어진 테스트코드가 있네? -> 삭제하자

의 과정이 아직 조금 어색한 것 같아요.

[pkeugine]

네 그렇다고 볼 수 있죠. 그리고 굳이 TDD 를 하지 않더라도 테스트 역시 프로덕션 코드만큼이나 (어쩌면 그 이상으로) 유지보수의 대상이 돼요. 리팩토링을 하는 과정에서, 또는 새로운 기능이 추가되는 과정에서 테스트가 변경되거나 삭제되는 경우가 자주 발생해요. 그리고 지금처럼 나중에 보니 그 테스트가 로직을 효과적으로 검증하는건지 의문이 들어서 삭제하는 경우도 있고, 테스트가 잘못 만들어져서 잘못 검증을 하고 있다는 사실을 발견할 때도 있어요 (false positive 또는 false negative 상황). 제가 질문을 제대로 이해한건진 모르겠지만요 😅 저는 참고로 작년 4분기에 테스트 관련 일만 계속 했답니다...! 그만큼 관리 포인트가 많았어요.

[pkeugine]

TDD 도 해보셨으니 다양한 상황에 대한 테스트도 추가해보죠!
예를 들어 Ladder 의 경우, 생성에 관련된 테스트는 있지만 row 나 path 가 만들어지는 과정에 대한 테스트는 없어요.
random 한 요소가 포함이 되어 있어서 테스트하기가 좀 까다로울 수 있는데, 저번 미션에서 자동차의 이동 조건을 테스트 했던 것을 떠올려서 적용해봐도 좋을 것 같네요 :)

[hw0603]

[질문]

처음 설계에서는 Line에서 Random을 사용하여 무작위 경로를 만들어냈었는데요, 랜덤 요소 때문에 테스트가 어려워 Line의 생성자에서 boolean list를 전달받고, Random은 Ladder에서 사용하도록 변경한 것이 현재 설계입니다.
덕분에 Line의 생성자에서 여러 검증 로직을 작성하고, 이를 테스트할 수 있었구요.

이미 Line에 대한 테스트가 있고(=생성자에 테스트 완료된 검증 로직이 있으므로 생성된 Line은 항상 유효한 정보를 담고 있다는 것을 보장할 수 있고), 외부(Ladder)에서는 랜덤한 리스트를 생성하여 전달만 해 주는데, row 나 path 가 만들어지는 과정에 대한 테스트가 별도로 필요할까요..?
Ladder를 생성할 때 Line을 생성해야 하므로, Line의 테스트만으로 제 기능을 한다고 생각하기도 했고, Ladder에서 row, path가 만들어지는 과정을 테스트한다 해도 Random이 잘 작동하는지, 내지는 리스트에 요소가 잘 추가되는지를 테스트하는 것 밖에 되지 않을 것 같아서요.

+) 저번 미션에서 DTO를 사용하지 않았던 이유와 유사하게, 이번 미션에서 randomGenerator 같은 것을 만들지 않았고, 인터페이스 등으로 추상화하지도 않았습니다. 만약 정말 Ladder도 테스트를 해야 한다면 랜덤적 요소를 Ladder의 외부로 분리해야 할 것이고, 그럼 인터페이스를 만들고 컨트롤러 단에서 Ladder로 랜덤 생성기를 주입해 줄 수 밖에 없을 것 같은데 피케이는 이에 대해 어떻게 생각하시나요?

[pkeugine]

맞아요. 저도 거의 동일한 생각을 가지고 있어요. 특히 randomGenerator 만들지 않고 구현하신 점도 개인적으로 아주 잘하셨다고 생각해요. 제가 자동차의 이동 조건 이라고 해서 인터페이스를 만들어보자는 말처럼 들렸을 수도 있을 것 같네요 🤔 자동차 미션에서는 특정 상황에 자동차가 움직이고, 어쩔 땐 움직이지 않는 특수한 값(경계값)을 가지고 테스트 할 수 있어요. 페드로의 Ladder 역시 말해주신 것처럼 거의 대부분 List 에 LadderPath 를 넣는 행위가 거의 대부분이기도 하고, random 한 요소는 검증하기 어렵기도 하죠. 그런데 저 어떤 특별한 값이 들어왔을 때는 확실하게 검증해볼 수 있는데요, makeRandomRow 메서드에 있는 이 로직이에요 :

        if (randomPath.size() < width) {
            randomPath.add(LadderPath.STAY);
        }

이 로직이 코멘트를 작성하게 된 이유이기도 한데, 저는 보면서 이게 무슨 역할을 하나 꽤 오래 생각해야했거든요. 아마 제 생각이 맞다면 플레이어가 단 한 명일 때, 일직선의 사다리를 만들기 위해 있는 로직일거에요. 무작위적이기도 하고 List 를 만드는 로직 치고는 이런 나름 중요한? 로직이 숨어있는 클래스라서 최소한의 테스트가 필요하다고 느꼈던 것 같아요.

[hw0603]

음.. 말씀하신 상황도 역시 포함되지만 정확히는 아래와 같아요.
N명의 Player가 있을 때,
사다리의 연결 여부를 저장 시 각 지점에서의 방향을 저장하고 있으므로 N 길이의 LadderPath(Enum) 리스트가 필요합니다.

이때 RIGHT 오른쪽에는 항상 LEFT가 와야 하므로 연결된 지점에 대해서는 RIGHT, LEFT를 쌍으로 한 번에 .add() 하게 되는데요.

사람이 5명인 경우를 예시로 들어 볼게요.
만약 R L S R L 와 같은 Line 이 구성될 때는 큰 문제가 없지만,
R L R L S 와 같은 Line이 구성될 때는 R L R L 이 결정된 상태에서 마지막 칸은 항상 S인 것이 고정되어야 하므로 더 이상 경로를 랜덤하게 생성하면 안 됩니다.

따라서, 아래 코드와 같이

        while (randomPathWithPair.size() < maxWidth - 1) {
            randomPathWithPair.addAll(generateRandomPath());
        }

루프 내에서 경로를 랜덤하게 생성하는 최대 길이를 (플레이어 수)-1로 제한하고, 루프 탈출 이후 충분한(Player 수 만큼의) 길이의 경로가 생성되지 않았다면 마지막 경로를 항상 STAY로 채우는 함수였습니다.

피케이의 말대로 한 번에 이해하기 난해한 것 같아 메서드를 분리하고 메서드명을 통해 의도를 드러내도록 수정해 봤어요.

private static List<LadderPath> makeRandomRow(int width) {
    List<LadderPath> randomPath = new ArrayList<>(generatePairableRandomPath(width));
    fillPathifNotEnough(randomPath, width);

    return randomPath;
}

private static void fillPathifNotEnough(List<LadderPath> createdPath, int width) {
    if (createdPath.size() < width) {
        createdPath.add(LadderPath.STAY);
    }
}

하지만 이후에도 두 가지 의문점이 남습니다.

1. 메서드의 반환값을 잘 활용해 보라는 리뷰를 받은 적이 있어요. 위 사례의 fillPathifNotEnough() 와 같이 이미 존재하는 리스트에 조건에 따라 값을 추가하거나, 추가하지 않아야 하는 함수의 경우 반환값을 어떻게 활용할 수 있을까요?

private static List<LadderPath> fillPathifNotEnough(List<LadderPath> createdPath, int width) {
    if (createdPath.size() < width) {
        createdPath.add(LadderPath.STAY);
    }
    return createdPath;
}

이렇게 변경할 경우, 의미 없는 값(이미 호출자가 createdPath의 주소값을 가지고 있으므로)을 반환하게 되고

private static LadderPath getAdditionallyNeededPath(List<LadderPath> createdPath, int width) {
    if (createdPath.size() < width) {
        return LadderPath.STAY;
    }
    return null; // 여기서 뭘 반환..???
}

와 같이 메서드를 추출하고 호출자에서 .add(getAdditionallyNeededPath(...))와 같이 사용하려면 더 이상 추가할 필요가 없을 경우에 주석 처리한 부분처럼 어떤 값을 반환해야 할지 애매해지는 문제가 생깁니다.

2. fillPathifNotEnough() 와 같이 메서드를 분리한 이후에도 여전히 의미가 불분명하다는 이유로 테스트가 필요할까요? 랜덤적인 요소가 처리된 이후에 나오는 로직이라 외부에서 테스트하기가 애매해요.

랜덤 요소 없이 외부에서 해당 메서드만을 테스트하기 위해서는 public으로 선언 후 List<LadderPath> createdPath의 인자를 직접 전달하는 방법을 사용할 수 있지만 테스트를 위해 프로덕션 코드의 메서드가 public으로 변경되어야 하고, 그것을 감수하고 진행한 테스트 자체도 "if문이 잘 동작하는가?" 정도의 의미밖에 가지지 못할 것 같아요.
private 메서드를 꼭 테스트해야 한다면 객체 설계가 잘못되었다는 신호라고 했는데 그렇다고 더 나은 설계가 떠오르지도 않네요..ㅋㅋㅋ😅
위에서 언급했던 것 처럼 Ladder와 Line의 관계와 역할을 생각해 봤을 때, 이미 Line의 테스트에서 충분한 검증을 하고 있으므로 Ladder에서의 테스트는 이정도로 충분하지 않을까요?!

[pkeugine]

import 문에 wildcard (*) 를 사용하면 어떤 장점과 단점이 있을까요?

[hw0603]

장점으로는 한 라인으로 모든 값들을 import 해서 네임스페이스 없이도 바로 사용할 수 있다는 점이 있을 것 같고, 해당 객체 내에 아주 많은 값(혹은 메서드)가 존재할 경우 사용할 일이 없는 것들까지도 같이 임포트된다는게 단점으로 작용할 것 같아요.

LadderPath는 제가 작성한 enum 이고, 내부 값이 3개밖에 없다는 것을 인지하고 있기에 와일드카드를 사용하여 임포트했습니다.

[pkeugine]

👍 다른 사람들이랑 협업하다보면 누군가는 wildcard 로 불러오고 누군가는 다 하나씩 가져오는 사람들도 있어서 불필요한 변경점이 PR 에 많이 잡힌다는 특징도 있어요. 이건 wildcard 이기 때문에 생기는 단점이라기보단 wildcard 임포트에 대한 컨벤션이 없어서 그런거긴해요!

[pkeugine]

Line 의 개념이 솔직히 코드로만 봤을 때 어려운 개념인데, 테스트가 있어서 쉽게 이해했어요 👍 👍 👍
이렇듯 테스트는 검증 역할도 하지만, 기능, 로직에 대한 명세서 역할도 해요!

[pkeugine]

Players 의 경우도 정상적으로 객체가 생성되는 상황에 대한 테스트가 추가되면 좋을 것 같아요

[hw0603]

추가했습니다!

[hw0603]

안녕하세요 피케이! 상세한 리뷰 빠르게 달아 주셨는데 수정이 늦어 죄송합니다. 졸업식 참석 일정이 있어 며칠간 코드를 제대로 보지 못했네요..

리뷰 주셨던 부분 반영해 두었고, 궁금한 점은 [질문] 말머리로 질문 남겨 두었습니다! 시간 나실 때 답변 부탁드릴게요🙂

[pkeugine]

안녕하세요 페드로! 졸업... 축하(?)합니다! 🥳 🥳 🥳
바쁘셨다고 하는데, 그 와중에 정말 열심히 작업해주셔서 코드도 정말 깔끔하네요.

이번 리뷰는 별거 없어서 그냥 가볍게 봐주시고, 남기신 질문에 대한 답변 다 달았으니 한 번 확인 부탁해요.
이번 step 은 여기까지 하고 다음 step 진행해보죠!
만약 코멘트에 관련해서 이해 안되거나 더 대화하고 싶은 점이 있다면 언제든지 DM 주세요.
다음 step 도 파이팅입니다!

[pkeugine]

프로젝트 전반에 걸쳐 상수 네이밍 컨벤션을 전체 대문자로 해보는건 어떨까요?

[hw0603]

오... 한번도 상수화된 인스턴스를 상수라고 생각해 본 적이 없었는데 생각해보니 상수가 아닐 이유가 없네요.

상수 변수에 대해서는 전체 대문자 네이밍 컨벤션을 지키는 편인데, 이와 같은 경우는 사람마다 의견이 갈리는 것 같아요.

https://stackoverflow.com/questions/7259687/java-naming-convention-for-static-final-variables/7259738

로거를 LOGGER로 쓸지 logger로 쓸지 결정하는 것이 비슷한 상황일 것 같은데, 저는 소문자가 편한 것 같아요!

피케이는 static final 로 선언된 인스턴스 변수들도 모두 uppercase로 작성하시나요?

[hw0603]

크루들과 이야기해보다가 이런 글을 발견했어요!
https://google.github.io/styleguide/javaguide.html#s5.2.4-constant-names

// Constants
static final int NUMBER = 5;
static final ImmutableList<String> NAMES = ImmutableList.of("Ed", "Ann");
static final Map<String, Integer> AGES = ImmutableMap.of("Ed", 35, "Ann", 32);
static final Joiner COMMA_JOINER = Joiner.on(','); // because Joiner is immutable
static final SomeMutableType[] EMPTY_ARRAY = {};

// Not constants
static String nonFinal = "non-final";
final String nonStatic = "non-static";
static final Set<String> mutableCollection = new HashSet<String>();
static final ImmutableSet<SomeMutableType> mutableElements = ImmutableSet.of(mutable);
static final ImmutableMap<String, SomeMutableType> mutableValues =
    ImmutableMap.of("Ed", mutableInstance, "Ann", mutableInstance2);
static final Logger logger = Logger.getLogger(MyClass.getName());
static final String[] nonEmptyArray = {"these", "can", "change"};

[pkeugine]

Line.toLineDto() 를 사용하기를 지양하셨듯, List 를 만들어주는 로직을 Ladder 가 아닌 다른 곳에 넣어보면 어떨까요? 도메인은 최대한 다른 클래스에 의존성을 가지지 않는 것이 좋아요. 도메인을 '외부 요소의 변경'으로부터 최대한 지키는거죠.

[hw0603]

반영했습니다! 이 변경과 관련하여 궁금한 점이 생겼는데, 2단계 PR때 정리해서 질문드릴게요!

[pkeugine]

Suggested change

	@DisplayName("연속된 경로가 있다면 가로줄 생성에서 예외가 발생한다.")
	@DisplayName("STAY 가 아닌 경로가 연속된다면 예외가 발생한다.")

그냥 제안인데 이 이름 어때요? DisplayName 안에는 도메인 관련 명칭 (STAY, LEFT, RIGHT, Ladder 등) 을 사용하지 않으려고 하시는 것 같은데 그 이유가 궁금해요!

[hw0603]

/docs/README.md 에 기능 요구사항을 작성할 때, 요구사항 명세서는 개발자 뿐만 아니라 모든 사람이 쉽게 이해할 수 있어야 한다고 생각해서 최대한 개발 관련 용어나 도메인 명칭을 사용하지 않으려고 했었는데, TDD 중 각 기능들을 하나씩 구현하다 보니 @DisplayName 안의 설명 또한 그렇게 따라갔던 것 같아요.

테스트코드도 '코드' 라는 말을 생각해 봤을 때 어노테이션 안에서는 오히려 도메인 관련 명칭을 직접 명시해 주는 게 좀 더 명확한 의미 전달이 가능하겠네요! 수정해 보겠습니다🙂