[사다리 타기] 김동균 미션 제출합니다. #4
Conversation
- 참여자 생성자를 protected로 변경합니다.
- 이름 목록을 받아서 참여자를 생성하고 관리합니다.
- 랜덤으로 생성되는 값을 테스트 할 수 없기 때문에, 생성전략을 상속받아 고정된 값으로 테스트 할 수 있도록 도움
- 사람의 수를 받아 Boolean타입의 리스트를 반환하는 메소드를 가진다.
- 다리가 문제의 규칙을 여기지 않으며 생성되지 않는지 확인
- 0과 1을 랜덤으로 생성하여 0이면 false, 1이면 true 반환하는 메소드 - 이전에 다리가 연결된적 있는지 확인하는 메소드 - 위 2개 메소드를 활용하여 다리 생성 여부를 가지고 있는 Boolean타입 리스트 반환
- 생성 전략을 활용하여 행 생성
- 읽기 쉽게 변수명을 수정합니다. - 더 의미있는 테스트를 할 수 있게 수정합니다.
- 관심사 기준으로 패키징합니다.
- 사다리가 알고 있는 행의 정보들을 반환합니다.
- 관련 모델과 화면을 사용하여 사다리 게임을 구현합니다.
- 구현한 기능 목록 체크
There was a problem hiding this comment.
안녕하세요, 동균! 👋
중간 고사 이후에 놀고 싶었을 텐데 미션하느라 고생 많았어요~
1. 랜덤은 어떻게 테스트 해야 하는가?
랜덤에 의해 좌우되는 기능에 대한 테스트가 힘들었군요. 저도 처음에 그 문제를 마주했을 땐 눈 앞이 깜깜했습니다. 프로그램을 매번 실행할 때마다 달라지는, 통제할 수 없는 상태에 대한 테스트는 사실상 불가능하다고 봐야 합니다.
그렇다면 어떻게 해야 무작위 값에 대해 테스트할 수 있을까요? 방법은 이미 동균이 활용했습니다! 바로, 사다리 생성 전략 추상화를 통한 테스트 더블 도입입니다. 테스트 더블에 대해 학습해보시고, 남겨드린 리뷰 코멘트를 활용해서 리팩토링 해봅시다. 힌트를 드리자면, 테스트를 위해 가짜 생성 전략 객체를 만들고, 원하는 값만 반환하도록 하는 것입니다.
2. View에 대한 테스트?
다음으로, View에 대한 테스트에 대해서 질문주셨는데요. 결론부터 말씀드리자면, 필요하지 않습니다. View를 테스트해서 얻을 수 있는 이점이 무엇이 있나요? 무엇을 테스트하고 싶으신가요? 신뢰할 수 있는 코드를 위해 TDD를 하는 것도 중요하지만, 테스트 작성에서 가장 중요한 것은 "왜, 무엇을 테스트하려 하는가?"를 명확하게 결정하는 것입니다.
Java 표준 입출력을 사용하는데 해당 기능이 잘 동작하는지 테스트할 필요가 있을까요? 언어에 대한 도전이 과연 꼭 필요한 테스트일까요?
3. 원시값 포장은 어디까지 해야 할까?
@Override
public List<Boolean> generate(int countOfPlayer) {
if (countOfPlayer < 2) {
throw new IllegalArgumentException("플레이어는 최소 2명 이상이어야 합니다.");
}
int countOfBridge = countOfPlayer - 1;
List<Boolean> bridges = new ArrayList<>();
for (int i = 0; i < countOfBridge; i++) {
if (wasPreviousBridgeConnected(bridges, i)) {
bridges.add(false);
continue;
}
bridges.add(decideBridgeConnection());
}
return bridges;
}제 생각에도 이 코드에서 2 정도는 포장하는게 좋아보이지만 1까지는 굳이 필요 없어 보입니다! 꼭 모든 원시값을 포장해야하는 것은 아닙니다. 변경 될 여지가 많고 많은 곳에서 활용되는 값은 포장해두는 것이 좋지만, 지금은 그렇지 않아 보이네요.
리팩토링 파이팅입니다!
| ## 구현할 기능 목록 | ||
|
|
||
| - [x] 참여할 사람 이름을 입력 | ||
| - [x] 참여할 사람 입력을 위한 안내 출력 | ||
| - [x] 참여할 사람 이름을 쉼표(,)를 기준으로 분리 | ||
| - [x] 참여할 사람 이름을 입력받아 유효성 검사 | ||
|
|
||
| - [x] 사다리 높이 입력 | ||
| - [x] 사다리 높이 입력을 위한 안내 출력 | ||
| - [x] 사다리 높이를 입력받아 유효성 검사 | ||
|
|
||
| - [x] 결과 출력 | ||
| - [x] 참여한 사람의 이름 출력 | ||
| - [x] 사다리 결과 출력 | ||
|
|
||
| ## 우리가 생각한 사다리 | ||
|
|
||
| - 하나의 행은 여러 개의 연결로 이루어져 있고, 사다리는 여러 개의 행으로 이루어져 있다. | ||
|
|
||
| ## 우리가 필요하다고 생각하는 객체 | ||
|
|
||
| - 참여자 | ||
| - 해야할 일: 참여할 사람 이름을 입력받아 유효성 검사 | ||
| - 참여자들 | ||
| - 해야할 일: 참여할 사람들을 생성하고 중복 여부를 검사 | ||
| - 기준 | ||
| - 해야할 일: 사다리 생성 전략을 담당하고 기준에 맞게 연결 여부를 생성 | ||
| - 행 | ||
| - 해야할 일: 사다리의 가로줄을 담당하고 기준에 맞게 무작위로 연결 | ||
| - 연결 | ||
| - 해야할 일: 연결 여부를 판단하고 연결된 결과를 반환 | ||
| - 사다리 | ||
| - 해야할 일: 사다리 높이에 맞게 행을 생성 | ||
| - 입출력 뷰 | ||
| - 해야할 일: 사용자 입력을 받고 결과를 출력 |
There was a problem hiding this comment.
구현 기능 목록 작성 👍
다만, "우리가"나 "해야할 일:" 같은 표현은 해당 README가 기능 명세라기보단 메모에 가까운 느낌을 줄 수 있습니다.
| public List<List<Boolean>> getLadderInformation() { | ||
| return rows.stream() | ||
| .map(row -> row.getBridges().stream() | ||
| .map(Bridge::isExist) | ||
| .toList()) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
getLadderInformation이라는 이름은 어떤 정보를 얻을 수 있는지 알기 어려운 다소 모호한 이름이라고 생각됩니다.
getLadderShape나 오히려 getLadder가 더 이해하기 쉬울 수 있습니다.
| public interface GenerateStrategy { | ||
| List<Boolean> generate(int countOfPlayer); | ||
| } |
| public Row(int countOfPlayer, GenerateStrategy generateStrategy) { | ||
| this.generateStrategy = generateStrategy; | ||
| generateRow(countOfPlayer); | ||
| } | ||
|
|
||
| private void generateRow(int countOfPlayer) { | ||
| generateStrategy.generate(countOfPlayer) | ||
| .forEach(bridge -> bridges.add(Bridge.of(bridge))); | ||
| } |
There was a problem hiding this comment.
전략 패턴을 활용한 것이 눈에 띄네요.
혹시 전략 패턴에 대해서 학습한 것이 있다면 말씀해주실 수 있나요?
There was a problem hiding this comment.
이번에 전략 패턴을 처음으로 사용해보면서 공부해봤습니다!
전략 패턴은 어떤 일을 수행하는 알고리즘이 여러 개일 때 적합한 패턴으로, 이번 미션에서는 랜덤으로 값을 생성하는 객체와 고정된 값을 생성하는 객체 2개가 있으므로 전략 패턴을 활용했습니다.
| @Override | ||
| public List<Boolean> generate(int countOfPlayer) { | ||
| if (countOfPlayer < 2) { | ||
| throw new IllegalArgumentException("플레이어는 최소 2명 이상이어야 합니다."); | ||
| } | ||
| int countOfBridge = countOfPlayer - 1; | ||
|
|
||
| List<Boolean> bridges = new ArrayList<>(); | ||
| for (int i = 0; i < countOfBridge; i++) { | ||
| if (wasPreviousBridgeConnected(bridges, i)) { | ||
| bridges.add(false); | ||
| continue; | ||
| } | ||
| bridges.add(decideBridgeConnection()); | ||
| } | ||
| return bridges; | ||
| } |
There was a problem hiding this comment.
요구 사항에도 있지만 메소드의 최대 indent(들여쓰기)는 1단계로 제한하고 있습니다.
해당 메소드의 indent가 1이 될 수 있도록 리팩토링 해봅시다!
| checkNameIsNotNull(name); | ||
| checkNameIsNotEmpty(name); | ||
| checkNameLength(name); | ||
| checkNameIsNotNumber(name); | ||
| checkNameIsNotKorean(name); | ||
| checkNameIsNotSpecialCharacter(name); |
There was a problem hiding this comment.
사용자 이름에 대한 메소드들을 잘 분리해두셨군요.
다만, 생성자에서는 validatePlayerName 메소드만 두고, 해당 메소드 안에 이 코드들을 넣어두는 것이 추상화 수준이 잘 맞을 것 같습니다. 개발자가 생성자를 읽었을 때, 어떤 검증을 하는지 상세하게 아는 것보단, '사용자의 이름은 검증을 거친다.' 수준의 정보만 알아도 충분합니다.
| playerNames.stream() | ||
| .collect(Collectors.groupingBy(name -> name, Collectors.counting())) | ||
| .forEach((name, count) -> { | ||
| if (count > 1) { | ||
| throw new IllegalArgumentException("중복된 플레이어가 존재합니다."); | ||
| } | ||
| }); |
There was a problem hiding this comment.
stream에서 forEach는 지양하는 편이 좋습니다.
Stream의 forEach는 스트림 종료 연산이며, 로직을 가지기 보다는 스트림 결과의 출력 등의 결과 보고에 활용되어야 합니다.
또한, Stream 병렬화 공식 문서에서는 forEach 내부에 로직이 하나라도 추가되면 동시성 보장이 어려워지며, 가독성이 떨어진다고 말하고 있습니다.
위 코드는 다른 스트림 연산이나 로직 리팩토링으로 충분히 대체할 여지가 많은 것 같습니다!
리팩토링 해봅시다.
src/test/java/model/LadderTest.java
Outdated
| class LadderTest { | ||
| @Test | ||
| @DisplayName("정상적으로 사다리 객체 생성") | ||
| void should_CreateLadderObjectSuccessfully() { |
There was a problem hiding this comment.
테스트 메소드 이름에 스네이크 케이스와 카멜 케이스를 혼용한 이유가 있을까요?
There was a problem hiding this comment.
페어와 테스트 네이밍 컨벤션을 "Should_예상되는결과_When_테스트중인상태"으로 정했습니다. 각 과정을 구분할 때는 스네이크 케이스로 구분하였고, 예상되는 결과와 테스트 중인 상태에는 각 단어를 구분할 수 있도록 카멜 케이스를 사용했습니다.
src/test/java/model/LadderTest.java
Outdated
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
|
|
||
| class LadderTest { |
There was a problem hiding this comment.
이 테스트는 랜덤 요소에 의해 좌우되는 부분이 많아서 테스트가 크게 의미가 없어보여요.
assertNotNull은 각 사다리가 요구 사항에 맞게 생성 되었는지를 확인할 수는 없을 겁니다.
그래서 한가지 팁을 드리자면, RandomGenerateStrategy를 사용하기 보단 FixedGenerateStrategy를 활용해
원하는 대로 생성 되었는지를 확인하는 것도 방법일 수 있습니다.
리펙토링 내용
궁금한 점OutputView에서 StringBuilder 타임의 output을 전역 변수로 선언하여 관리했는데요! 사실 전역 변수로 사용해도 되는지에 대한 고민을 좀 해봤습니다. 저는 이런 이유로 static을 사용했습니다.
과연 output을 static으로 관리해도 될까요? 만약 안된다면 이유가 무엇인가요? |
StringBuilder는 대안으로는 StringBuilder 대신에 thread-safe한 // 예시
public void printResult(List<String> names, List<List<Boolean>> ladder) {
StringBuilder output = new StringBuilder();
// 메서드 내에서 생성 후 사용 ...
}위와 같은 방식을 통해 전역 상태를 관리하는 어려움과 thread 안전성 문제를 피할 수 있을 것 같아요. |
사실 저도 이런 부분을 생각해봤었는데, 이번 미션은 싱글 스레드 환경이라 굳이 StringBuffer가 필요하지 않겠다 생각하여 성능이 더 우수한 StringBuilder를 사용했습니다. 하지만 예혁님 말대로 향후 멀티스레드 환경으로 확장 시 문제가 발생할 수 있을 것 같네요. 좋은 의견 감사합니다! |
우리가 생각한 사다리
우리가 미션을 진행한 방식
어려웠던 부분
궁금한 부분
결과