[1단계 - 사다리 생성] 제이(이재윤) 미션 제출합니다. #112
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sonypark
requested changes
Feb 16, 2023
There was a problem hiding this comment.
안녕하세요 제이, 리뷰어 소니 입니다.😊
사다리게임 미션 잘 구현하셨네요! 커밋로그를 보니 TDD로 구현하려고 노력한게 보입니다💯
리드미도 잘 작성하셨구요.👍
질문 주신 부분과 코드상 약간의 코멘트 남겼으니 확인 부탁드릴게요~
- 도메인에서 로직을 구현하고 toString() 메서드를 통해 OutputView로 전달해도 되나요?
출력 로직을 toString()에 넣는건 적합하지 않다고 생각합니다. (그 이유는 무엇일까요? toString의 용도는 무엇일까요?) 현재는 OutputView라는 console을 통해 보여주지만 View가 달라지게 될 경우도 생각해봅시다. 그런 경우는 어떻게 될까요?
참고: toString의 정의
- 정말 중요한 로직이 있는데 private 메서드라서 테스트를 할 수 없다면, 이를 테스트하기 위해서 setter() 혹은 테스트를 위한 생성자를 만들어도 될까요?
- private 메서드를 사용하는 public 메서드를 테스트 하신다면 private 메서드도 수행될테니 별도로 private 을 public 으로 열어서 테스트를 작성하실 필요는 없습니다. 다만 private 메서드가 지나치게 많아질 경우 이는 해당 클래스가 지나치게 많은 역할과 책임을 하고 있다는 증거가 될 수도 있습니다. Should I test private test? 라고 구글링 해보세요😄
- Height.getHeight().getHeight()와 같은 로직을 작성하였습니다. 구조가 더욱 복잡해져서 객체 안에 객체의 필드를 가져오는 것을 상위 객체에서 진행하고 있는데, 이런 경우 설계가 잘못된 것일까요? 이 부분에서 의문이 생겼습니다.
설계가 잘못되었다고 생각하진 않습니다. 원시값을 값객체로 잘 포장하셨고 해당 객체를 사용하는 클래스에서 getter로 호출하는건 자연스러운것 같습니다. 다만, 외부에서 get().get()으로 호출하기보다 현재 작성하신 것처럼 상위 클래스에서 원하는 값을 get해서 외부로 반환하는게 적절하다고 생각합니다. 잘 하셨어요👍
src/main/java/domain/Ladder.java
Outdated
Comment on lines
2
to
11
|
|
||
| public class Ladder { | ||
|
|
||
| private final Lines lines; | ||
| private final Height height; | ||
|
|
||
| public Ladder(final int numberOfPlayer, final int height) { | ||
| this.lines = new Lines(numberOfPlayer, height); | ||
| this.height = new Height(height); | ||
| } |
src/main/java/domain/Lines.java
Outdated
Comment on lines
32
to
35
|
|
||
| public List<Line> getLines() { | ||
| return lines; | ||
| } |
There was a problem hiding this comment.
Line이 외부에서 수정될 수도 있을까요?
방어적 복사란 무엇일까요?
There was a problem hiding this comment.
와 이 부분에 대해 처음 알았습니다!
Collection이라서 외부에서 충분히 getter()를 이용해서 수정될 수 있겠군요!
Comment on lines
+2
to
+24
|
|
||
| public class PlayerName { | ||
|
|
||
| private static final int MINIMUM_LENGTH_OF_NAME = 1; | ||
| private static final int MAXIMUM_LENGTH_OF_NAME = 5; | ||
|
|
||
| private final String name; | ||
|
|
||
| public PlayerName(final String name) { | ||
| validateLengthOfName(name); | ||
| this.name = name; | ||
| } | ||
|
|
||
| private void validateLengthOfName(final String name) { | ||
| if (isNotPermittedLengthOfName(name)) { | ||
| throw new IllegalArgumentException("이름의 길이는 최소 1자 이상, 5자 이하입니다."); | ||
| } | ||
| } | ||
|
|
||
| private boolean isNotPermittedLengthOfName(final String name) { | ||
| return (name.length() < MINIMUM_LENGTH_OF_NAME) || (name.length() > MAXIMUM_LENGTH_OF_NAME); | ||
| } | ||
|
|
There was a problem hiding this comment.
@sonypark
추가 질문을 어디에 남길지 몰라서 여기에 남깁니다..!
테스트에 대한 질문입니다!
자동차 경주 결과를 출력하는 로직이 있다면, 이를 테스트 하기 위해서 Car 클래스에 move()를 사용해야합니다. 경주 결과는 Cars 클래스에서 결과를 리턴해준다고 가정했을 때,
Cars.경주결과() 를 테스트 하기 위해서 테스트에서 car.move()와 같은 작동을 미리 해줘야합니다.(경주 결과 판단을 위해서) 이런 경우 Cars 테스트가 Car 도메인 메서드를 의존하게 되는데 이런 경우는 어떻게 하면 좋을까요?
소니의 생각이 궁금합니다.
There was a problem hiding this comment.
네 제이 말대로 Cars가 Car를 가지고 있고 Car의 move 메서드를 통해야 경기 결과를 도출할 수 있습니다.
이건 도메인의 의존성 때문에 테스트에서도 동일한 의존성이 생기는것으로 자연스러운 흐름이라 생각합니다.
레이어별로 controller -> service -> domain -> repository 와 같이 의존성을 갖게 되는데 가장 하위의 레이어의 단위 테스트가 잘 작성되어 있어야 합니다. 하위 레이어의 정상 작동이 보장되어야 그것을 사용하는 상위 레이어도 로직의 안정성을 보장할 수 있습니다. 그렇기 때문에 의존성을 어디에 얼마나 두느냐가 중요한 것 같습니다. 따라서 하위 레이어일수록 꼼꼼히 테스트를 작성합니다. 테스트의 중요도와 우선순위도 (repository) -> domain -> service -> controller 순입니다. (repository는 이미 검증된 라이브러리를 사용하기 때문에 잘 테스트하진 않습니다.🙃)
Comment on lines
+8
to
+20
| public class Players { | ||
|
|
||
| private static final int MINIMUM_LENGTH_OF_PLAYER = 2; | ||
| private static final int MAXIMUM_LENGTH_OF_PLAYER = 10; | ||
|
|
||
| private final List<Player> players; | ||
|
|
||
| public Players(final List<String> playerNames) { | ||
| validateNumberOfPlayers(playerNames); | ||
| validateDuplicatedPlayers(playerNames); | ||
| this.players = makePlayers(playerNames); | ||
| } | ||
|
|
src/main/java/domain/Players.java
Outdated
Comment on lines
63
to
66
|
|
||
| public List<Player> getPlayers() { | ||
| return players; | ||
| } |
Comment on lines
+4
to
+12
|
|
||
| public class RandomFootholdGenerator implements FootholdGenerator { | ||
|
|
||
| private final Random random = new Random(); | ||
|
|
||
| @Override | ||
| public boolean generate() { | ||
| return random.nextBoolean(); | ||
| } |
Comment on lines
+7
to
+17
|
|
||
| public class OutputView { | ||
|
|
||
| private static final int MAXIMUM_LENGTH_OF_NAME = 5; | ||
| private static final int BOUNDARY_OF_NAME_LENGTH = 4; | ||
| private static final String BLANK = " "; | ||
| private static final String FOOTHOLD = "-"; | ||
| private static final String BAR = "|"; | ||
| private static final String NEW_LINE = System.getProperty("line.separator"); | ||
| private static final StringBuilder ladderOutput = new StringBuilder(); | ||
| private static final StringBuilder playerNamesOutput = new StringBuilder(); |
src/test/java/domain/HeightTest.java
Outdated
Comment on lines
7
to
18
|
|
||
| public class HeightTest { | ||
|
|
||
| @ParameterizedTest | ||
| @ValueSource(ints = {0, 11}) | ||
| @DisplayName("높이는 1이상 10이하가 아니면 예외를 던진다.") | ||
| void throws_exception_when_invalidate_range_of_height(int height) { | ||
| // when & then | ||
| Assertions.assertThatThrownBy(() -> new Height(height)) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessageContaining("사다리의 높이는 최소 1이상 최대 10이하입니다."); | ||
| } |
Comment on lines
+13
to
+39
| @Test | ||
| @DisplayName("사람의 수가 1명 이하이면 예외를 던진다.") | ||
| void throws_exception_number_of_player_under_two() { | ||
| // given | ||
| List<String> playerNames = List.of("pobi"); | ||
|
|
||
| // when & then | ||
| assertThatThrownBy(() -> new Players(playerNames)) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessageContaining("참여 가능한 플레이어의 수는 2명이상 10명이하 입니다."); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("사람의 수가 10명 보다 크면 예외를 던진다.") | ||
| void throws_exception_number_of_players_over_ten() { | ||
| // given | ||
| List<String> playerNames = new ArrayList<>(); | ||
| for (int i = 0; i < 11; i++) { | ||
| playerNames.add("abc" + i); | ||
| } | ||
|
|
||
| // when & then | ||
| assertThatThrownBy(() -> new Players(playerNames)) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessageContaining("참여 가능한 플레이어의 수는 2명이상 10명이하 입니다."); | ||
| } | ||
|
|
sonypark
reviewed
Feb 16, 2023
src/main/java/domain/Ladder.java
Outdated
Comment on lines
3
to
8
| public class Ladder { | ||
|
|
||
| private final Lines lines; | ||
| private final Height height; | ||
|
|
||
| public Ladder(final int numberOfPlayer, final int height) { |
sonypark
approved these changes
Feb 17, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
안녕하세요~ 소니😀
백엔드 크루 제이입니다!
이번 미션은 페어와 함께 정말 재밌게 진행 했습니다.
저희는 구현을 할 때, "최대한 도메인을 통해서 기능을 동작할 수 있게 설계해보자!"라는 생각으로 구현을 했습니다.
그래서 도메인 설계를 할 때, 일급 컬렉션을 사용했고, Name 같은 값들도 원시 값 포장으로 객체로 분리했습니다.
사실 이번 1단계에서는 Player 도메인 자체가 name 변수 밖에 없기에 Player 자체가 name에 대한 원시 값 포장으로 볼 수 있었지만, 확장성을 고려하여 페어와 Player, PlayerName 객체를 따로 만들었습니다.
이번에 느낀 점은, 도메인을 최대한 분리를 하면서 validate처리도 도메인 내부에서 쉽게 할 수 있었고, 관리가 편했습니다.
그리고 프로그래밍을 하면서 몇 가지 궁금증이 생겼습니다.
도메인에서 로직을 구현하고 toString() 메서드를 통해 OutputView로 전달해도 되나요?
정말 중요한 로직이 있는데 private 메서드라서 테스트를 할 수 없다면, 이를 테스트하기 위해서 setter() 혹은 테스트를 위한 생성자를 만들어도 될까요?
클래스 분리 과정에서, 다음과 같은 문제점이 발생했습니다.
먼저 클래스 구조는 다음과 같습니다.
Ladder <- Height(Ladder가Height를 가지고 있는 경우) 이 경우에Height.getHeight()를 통해 높이를 가져오고 싶습니다. 하지만, 직접적으로 접근을 막아 놨기에Height.getHeight().getHeight()와 같은 로직을 작성하였습니다.구조가 더욱 복잡해져서 객체 안에 객체의 필드를 가져오는 것을 상위 객체에서 진행하고 있는데, 이런 경우 설계가 잘못된 것일까요? 이 부분에서 의문이 생겼습니다.
소니의 의견은 어떠신가요?!
감사합니다 :)
감사합니다!