[사다리 타기] 김이화 미션 제출합니다. #6
Conversation
기능 완료, 테스트 코드 구현중
There was a problem hiding this comment.
고생하셨습니다. 리뷰가 늦어 죄송합니다!
상진님과 페어로 작업하셨기 때문에 코드가 겹쳐서
코드리뷰는 상진님과 동일하게 작성하고, 궁금한 점에 대한 질문만 별도로 작성했습니다.
코드는 같기 때문에 상진님의 문답도 도움이 될까 하여 링크 남겨봅니다.
읽어보시는 걸 추천해요!
#5
사다리 타기 로직을 구현하는 과정에서 어려움이 있었습니다.
제가 보기에도 이번 과제가 구현은 제일 어려웠을 것 같아요.
그래도 이런 문제일수록 깔끔하게 구현해내었을 때의 성취감도 큰 것 같아요!
TDD와 일급컬렉션을 사용하는 것에 어려움이 있었습니다.
일급 컬렉션에 대해서는 위 PR 링크에서 길게 서술해둔 바가 있으니 읽어보세요!
어려운 개념일수록 '왜 쓰는지'를 확실하게 이해하는 게 중요하다고 생각합니다.
한 번에 이해가 가지 않는다면, 일단은 실수할 여지를 틀어막는 코딩 테크닉정도로 알고 넘어가도 좋을 것 같아요.
TDD가 익숙하지 않아 시간이 오래 걸려 사다리 타기 부분에서 구현 후 테스트 코드를 작성한 것이 아쉽습니다.
저도 TDD를 수행해본 적은 없지만, '틀은 제대로 짜여 있어 컴파일은 가능하나, 구현부가 비어있어 테스트를 통과하지 못하는 코드를 작성하는 것' 부터 시작한다고 알고 있습니다.
코드 작성을 시작하기 전에 머릿속이든, 메모장이든, 종이와 펜이든 사용해서
확실히 설계의 틀을 잡는 습관을 들이면 좋을 것 같습니다!
| .map(i -> new User(i)) | ||
| .collect(collectingAndThen(toList(), i -> new Users(i))); |
There was a problem hiding this comment.
필수는 아니지만, 메서드 참조식의 사용도 고려해볼 만 합니다.
User::new로도 '스트림의 원소를 User 생성자의 유일한 파라미터로 넣어 생성자를 호출함'을 알 수 있으니까요!
| points = new ArrayList<>(); | ||
| createLine(personCount,points); |
There was a problem hiding this comment.
코드를 읽으면서 이 부분이 좀 의아했습니다.
아래의 createLint()에서 points 필드에 값을 할당하는 로직이 이미 존재합니다.
points를 빈 리스트로 초기화할 필요가 없어 보이네요!
또한 createLine()은 메서드 내부에서 값을 변경하고 있습니다.
이 메서드 내부에서 무슨 일이 일어났는지 구현부를 모두 들여다보지 않아도 되게끔
createLine()이 반환값을 가지게 하면 훨씬 읽기 좋을 것 같네요. 예를 들면
points = new ArrayList<>();
createLine(personCount, points);이거보다
points = createLine(personCount);이게 훨씬 이해하기 쉬워 보이네요.
createLine을 눌러서 무슨 일이 일어나는지 확인할 필요가 없기 때문이죠!
| private Direction updatePreviousDirection(Direction previousDirection, Direction newDirection) { | ||
| // 오른쪽으로 가는 경우 다음 포인트는 왼쪽으로 갈 수 없습니다. | ||
| if (newDirection == Direction.RIGHT) { | ||
| return previousDirection = Direction.LEFT; | ||
| } | ||
| return previousDirection = Direction.NEUTRAL; |
There was a problem hiding this comment.
여기서 첫째 파라미터인 previousDirection은 없어도 될 것 같아요!
return Direcetion.LEFT를 해도 똑같으니까요. 이 부분도 흐름을 이해하기 어려웠던 이유 중에 하나가 되었던 것 같아요!
| private void createLine(int personCount, List<Direction> points){ | ||
| Direction previousDirection = Direction.NEUTRAL; | ||
|
|
||
| for (int i = 0; i < personCount - 1; i++) { | ||
| Direction newDirection = createDirection(previousDirection); | ||
| points.add(newDirection); | ||
| previousDirection = updatePreviousDirection(previousDirection, newDirection); | ||
| } | ||
| // 마지막 포인트는 항상 NEUTRAL | ||
| points.add(Direction.NEUTRAL); | ||
| } | ||
|
|
||
| private Direction createDirection(Direction previousDirection) { | ||
| if (previousDirection == Direction.LEFT) { | ||
| return Direction.NEUTRAL; | ||
| } | ||
| if (random.nextBoolean()) { | ||
| return Direction.RIGHT; | ||
| } | ||
| return Direction.NEUTRAL; | ||
| } | ||
|
|
||
| private Direction updatePreviousDirection(Direction previousDirection, Direction newDirection) { | ||
| // 오른쪽으로 가는 경우 다음 포인트는 왼쪽으로 갈 수 없습니다. | ||
| if (newDirection == Direction.RIGHT) { | ||
| return previousDirection = Direction.LEFT; | ||
| } | ||
| return previousDirection = Direction.NEUTRAL; | ||
| } |
There was a problem hiding this comment.
- LEFT, NEUTRAL, RIGHT가 존재한다는 점,
- prev와 new를 사용해서 연속된 라인이 생기지 않게 검사하는 점,
- Direction 리스트의 변수명이 Point로 되어있는 점 등
천천히 읽으면서 분석해야 알 수 있는 부분이 많아서
제가 이 클래스를 처음 읽었을 때 이해하는 데 시간이 오래 걸렸었습니다.
다른 구현 예시를 보면서 좀 더 단순하게 만들 수 있을지 고민해보거나,
시간or아이디어가 충분하지 않을 경우에는 전체적인 흐름을 설명하는 주석을 달아주는 것도 좋을 것 같아요!
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
|
||
| public class LadderTest { |
There was a problem hiding this comment.
테스트 클래스는 public이 아니어도 괜찮습니다.
하나의 테스트는 그 자체만으로 독립적인 기능을 수행해야 하고,
이는 LineTest를 작성하다가 new LadderTest() 같은 걸 호출할 일은 없다는 것을 의미하니까요!
SonarLint 툴에서도 public keyword를 자제하라고 말하고 있네요. 툴 내부에서 그 이유 또한 설명하니 읽어보세요!
| .hasMessage("유저 이름은 빈칸이 들어갈 수 없습니다."); | ||
| } | ||
|
|
||
| @DisplayName("유저 이름 생성 시 글자 수 확인") |
There was a problem hiding this comment.
글자 수가 ~여야 한다, ~하지 않으면 예외가 발생한다와 같이 명시적인 이름을 권장합니다.
구현부를 하나하나 다 뜯지 않고 바깥부분만 읽어도 이해가는 코드를 만들어야 하는데, 이는 테스트 코드도 동일!
어려웠던 부분
아쉬웠던 부분
참고