[사다리 타기] 김우진 미션 제출합니다. #8
Conversation
- 입력, 출력, 기능으로 분류하여 과제를 진행합니다.
- 테스트 코드를 작성합니다. - 각종 기능 구현 -> 이사 필요 - StringBuiler 부분 view로 이동 필요
- Line 랜덤 값 Test 코드 작성 - Test 코드를 위한 FootholdGenerator 생성
There was a problem hiding this comment.
안녕하세요, 우진! 👋
중간 고사 끝나고 바로 미션하느라 고생 많았습니다!
1. TDD 왜 이렇게 어렵나요?
이번 미션의 핵심 주제가 TDD였고, 생소한 개념이었기 때문에 많이 어려웠으리라 생각합니다. 저도 가끔은 TDD를 통해 개발하지 못하기도 합니다. 어려웠다고 해서 위축될 필요는 없습니다. 😁 처음에는 무엇부터 개발해야할지 몰라서 TDD를 실천하기 힘든데요, 그래서 Top-Down 방식과 Bottom-Up 방식을 적절하게 혼용하곤 합니다.
Top-Down 방식은 도메인 지식이 거의 없을 때, 사용자와 가장 가까운 부분 부터 차근차근 구현해나가는 방식이며, Bottom-Up 방식은 도메인 지식이 풍부하거나 잘 설계 되었을 때, 도메인부터 구현하는 방식입니다. 정답은 없으며, 우진이 편한 방식으로 개발하면 됩니다!
2. MVC 패턴의 흐름
크게 문제 될 만한 부분은 보이지 않으나, 한가지 걸리는게 있다면, model 패키지의 LadderSymbol 클래스는 View에서 관리하는게 더 나은 것 같아 보입니다. MVC 패턴을 활용한 이유는 결국 패키지 의존성을 포함한 객체 의존관계의 방향에 통일성을 주기 위함이기도 합니다.
각 계층이 적절하고 완벽하게 분리 되었는지 한번 고민 해봅시다!
3. 사다리 모양 그리기
2번과 마찬가지일 수 있는데, 우진의 생각대로 Model에서부터 View에 보여주기 위한 모양을 그리는 것은 적절하지 않은 것 같습니다. StringBuilder를 활용한 것 자체는 문제가 되지 않지만, 해당 객체를 필드로 두거나, 모델에서부터 생성해야 했는지는 조금 의문이 드네요..
충분히 앞 계층으로 분리할 수 있을 것 같습니다. Boolean 리스트를 활용한다거나, 발판 정보를 담은 컬렉션을 컨트롤러에서 변환한다던지 하는 방식으로요.
리팩토링 파이팅입니다!
docs/README.md
Outdated
| # 구현할 기능 목록 | ||
|
|
||
| ## 입력 | ||
| - [x] 사용자 이름 입력 | ||
| - [ ] 쉼표로 구분 | ||
| - [ ] 최대 5글자까지 가능 | ||
| - [x] 사다리 높이 입력 | ||
| - [ ] 자연수 | ||
|
|
||
|
|
||
| ## 출력 | ||
| - [x] 실행 결과 | ||
| - [ ] 첫번째 줄 : 사람 이름 | ||
| - [ ] 그 아래 줄 : 사다리 그리기 | ||
|
|
||
| ## 기능 | ||
| - [ ] 사람의 이름을 받고 , 기준으로 나누기. | ||
| - [ ] 한 줄에 | 개수를 사람 이름 개수만큼 반환. | ||
| - [ ] 랜덤 값 기능 구현 | ||
| - [ ] 랜덤에 충족하면 ----- 그리기. | ||
| - [ ] 한줄의 |에 ----- 연결선이 동시에 나오지 않게 | ||
| - [ ] 테스트 코드 작성 | ||
|
|
||
|
|
||
| ## TDD 객체 명세서 | ||
| - [x] Height | ||
| - [x] 객체 생성 가능 | ||
| - [ ] Ladder | ||
| - [x] 객체 생성 가능 | ||
| - [ ] 전체적인 값이 맞는지 확인 | ||
| - [ ] Line | ||
| - [x] 객체 생성 가능 | ||
| - [ ] 예상되는 사다리 라인 문자열 | ||
| - [x] Player | ||
| - [x] 객체 생성 가능 | ||
| - [x] Players | ||
| - [x] 객체 생성 가능 | ||
| - [x] util | ||
| - [x] LadderHeightValidator | ||
| - [x] 정상_입력 | ||
| - [x] 정수가_아닌_입력 | ||
| - [x] 음수_입력 | ||
| - [x] 공백_입력 | ||
| - [x] PlayerNamesValidator | ||
| - [x] 정상_입력 | ||
| - [x] 공백_포함 | ||
| - [x] 공백 | ||
| - [x] 이름_길이_초과 | ||
| - [x] 중복_입력 | ||
| # 1차 리뷰 후 개선 방향 |
| } | ||
|
|
||
| private Players createPlayersFromInput() throws IOException { | ||
| String[] playerNamesArray = inputView.readPlayerNames().split(","); |
There was a problem hiding this comment.
split까지 InputView에서 해주면 어떨까요?
View의 요구 사항이라고 볼 수 있을 것 같습니다.
| public static class DuplicatedPlayerNameException extends IllegalArgumentException { | ||
| public DuplicatedPlayerNameException() { | ||
| super(ErrorMessage.DUPLICATED_CAR_NAME.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Custom Exception을 구현하셨네요! Nested Class로 구현하신 특별한 이유가 있으실까요?
IllegalArgumentException으로 충분히 처리 가능한 것 같은데 Custom Exception을 구현한 이유는 무엇인가요?
src/main/java/model/Ladder.java
Outdated
| private Line line; | ||
|
|
||
| public Ladder() { | ||
| this.lines = new StringBuilder(); |
There was a problem hiding this comment.
StringBuilder를 생성자를 통해 필드에 주입할 이유가 있었나요?
makeLines를 generateLadder 같은 이름으로 바꾸고 바로 return 해주는 편이 낫지 않았을까요?
클래스의 필드가 늘어났다는 것은 그만큼 의존하는 객체가 늘었다는 의미입니다!
src/main/java/model/Ladder.java
Outdated
| public StringBuilder getLines() { | ||
| return lines; | ||
| } | ||
| } |
|
|
||
| @Test | ||
| void makeLineWithNotFoothold() { | ||
| TestFootholdGenerator generator = new TestFootholdGenerator(false); |
There was a problem hiding this comment.
테스트용 Fake 객체를 활용한 테스트 더블!
테스트 더블에 대해 학습하시고 사용하신 건지 직접 떠올린 아이디어로 작성하신 건지는 모르겠으나,
매우 훌륭한 랜덤한 조건에 대한 테스트 방법입니다!
src/test/java/model/LadderTest.java
Outdated
| @Test | ||
| @DisplayName("makeLines 메소드가 정상적으로 작동하는지 테스트 합니다.") | ||
| void Test_MakeLines_Operation() { | ||
|
|
||
| ladder.makeLines(height, players); | ||
|
|
||
| String[] lines = ladder.getLines().toString().split("\n"); | ||
|
|
||
| assertEquals(5, lines.length); | ||
| } |
src/test/java/model/PlayersTest.java
Outdated
| @Test | ||
| @DisplayName("리스트를 사용해 정상적으로 Players 객체를 생성하고, 값이 같은지 확인") | ||
| void Players_Object_Create_Success_Test() { | ||
| // given | ||
| List<String> playerNames = new ArrayList<>(); | ||
|
|
||
| playerNames.add("Player1"); | ||
| playerNames.add("Player2"); | ||
| playerNames.add("Player3"); | ||
|
|
||
| // when | ||
| Players players = new Players(playerNames); | ||
|
|
||
| // then | ||
| assertThat(players.getPlayers()).hasSize(playerNames.size()); | ||
|
|
||
| List<String> extractedNames = players.getPlayers().stream() | ||
| .map(Player::getName) | ||
| .toList(); | ||
|
|
||
| assertThat(extractedNames).containsExactlyElementsOf(playerNames); | ||
| } |
| @Test | ||
| void ladderHeightValidator_정수가_아닌_입력() { | ||
| String ladderHeight = "abc"; | ||
| assertThrows(LadderGameValidationException.NotNumericException.class, () -> LadderHeightValidator.validateLadderHeightNumberIsCorrect(ladderHeight)); |
There was a problem hiding this comment.
Java 컨벤션에는 한 줄에 최대 글자 제한도 있습니다.
적절하게 개행해볼까요?
| @Test | ||
| void playerNameValidator_정상_입력() { | ||
| String[] playerNames = {"pobi", "honux", "crong", "jk"}; | ||
| assertDoesNotThrow(() -> PlayerValidator.validatePlayerNameIsCorrect(playerNames)); | ||
| } |
There was a problem hiding this comment.
다른 테스트들 처럼 @DisplayName을 활용해보는 건 어떨까요?
코드의 통일성이 다소 떨어져 보입니다. 서로 다른 드라이버가 작성한 코드라고 할지라도
통일된 컨벤션을 통해 유지보수성을 높이는 것이 훨씬 좋습니다.
|
어려웠던 점
이 부분에 있어서 피드백이 궁금합니다!
고민했던 부분 / 질문할 부분
하지만 StringBuilder가 멀티스레드 환경에서 사용하기에 그리 적합한 객체가 아니라고 하더라구요
그럼에도 StringBuilder말고는 사용할 방법을 찾지 못해서 고민하다가 결국은 StringBuilder를 채용했습니다
view에서 처리를 해야할거 같은 생각이 계속 들었음에도 기능을 옮기는데 어려움이 있어서 이전은 하지 않았습니다.