[2단계 - 사다리 게임 실행] 성하(김성훈) 미션 제출합니다. #208
Conversation
| List<Player> gamePlayers = new ArrayList<>(); | ||
| for (int generateIndex = 0; generateIndex < names.size(); generateIndex++) { | ||
| Name name = new Name(names.get(generateIndex)); | ||
| StartIndex startIndex = new StartIndex(generateIndex); |
There was a problem hiding this comment.
🎯 참여자의 시작 순서 의미 고민
참여자의 시작 순서를 위의 코드에서는 '인덱스' 개념으로 생각해서 0번으로 시작하고 있는데,
개발자 입장에서는 첫번째 순서가 '0'번이 되는 것이 코드 상으로도 쉬울 것 같아
위처럼 구현을 했습니다.
하지만 '참여자의 시작 순서'의 객체 필드의 의미로 보면 첫번째 순서가 '1'번이 되는 것이 의미상으로 좋아보이는데 소니는 어떻게 생각하시는지 궁금합니다!
There was a problem hiding this comment.
시작 인덱스를 0으로할지 1로 할지는 성하의 판단대로 해주시면 됩니다 ㅎㅎ
다만 0,1과 같은 값을 그대로 사용해서 매직넘버를 만들기 보다는 상수로 선언해 네이밍을 통해 의미를 명확히 해주면 좋을 것 같습니다.
There was a problem hiding this comment.
넵! 제 판단대로 0으로 그대로 하겠습니다!
그런데 현재 위 코드 블럭의 0은 반복문의 처음 시작 인덱스를 뜻하는데, 상수화할 필요가 있을까요?
| @ParameterizedTest | ||
| @ValueSource(strings = {"가", "#", "a", "a1", "1.1"}) | ||
| @DisplayName("\"꽝\" 이외의 실행 결과가 정수가 아니면 예외를 던진다.") | ||
| void validateRewardNumericExceptNoLuckTest(String reward) { |
There was a problem hiding this comment.
🎯 @DisplayName 사용 시 테스트 메소드 네이밍 질문
@DisplayName으로 테스트 이름을 지정해줄 때,
메소드 네이밍을 @DisplayName으로 지정한 이름의 영어 버전으로 적어줘야 하는지 궁금합니다!
@DisplayName("\"꽝\" 이외의 실행 결과가 정수가 아니면 예외를 던진다.")
void throwsExceptionWhenRunResultIsNotIntegerExceptNoLuck(String runResult) { ... }이런 식으로 영어 버전으로 적었더니 너무 길어지고 가독성이 더 안 좋아지는 것 같아서 커밋한 코드처럼 요약해서 네이밍하게 되었습니다.
@DisplayName으로 테스트 의미를 선언해줬는데,
테스트 메소드 네이밍으로도 의미를 전달해야하는지 궁금합니다!
There was a problem hiding this comment.
둘 중에 하나만 선택해서 적어주시면 됩니다.
DisplayName 어노테이션을 이용해서 테스트명을 적었다면 테스트 메서드명은 아무값으로 적어도 상관없습니다.
| return startIndex.getRawStartIndex(); | ||
| } | ||
|
|
||
| public String getName() { |
There was a problem hiding this comment.
🎯 포장 클래스 getter 사용 시 질문
Player에서 Name과 StartIndex를 포장해서 포장 클래스로 가지고 있습니다.
이때 Player에서 Name을 가져오려고 getter 메소드를 호출 할 때,
포장 클래스인 Name을 반환해야할지,
this.name.getRawName()으로 포장 클래스 안의 String을 반환해야할지 고민됐었습니다.
저는 Player의 getter를 호출하는 LadderGame의 가독성 관점에서
Player.getName().getRawName()보다,
Player.getName()을 호출하는 것이 좋다고 생각해서
Player에서 바로 String을 반환하게 되었습니다.
이렇게 했을 때, Player가 포장되어 있는 Name의 String을 알게 되는데,
- '객체'의 의미로 봤을 때 다른 객체의 getter를 사용하므로 찝찝하다.
Player의 내부 필드로Name을 가지고 있어서 이미 Player가 Name을 알기 때문에 Name의 getter를 사용해도 의미적으로 괜찮다.
이러한 2가지 의견이 생각나는데, 어떻게 생각하시는지 궁금합니다!!
There was a problem hiding this comment.
네 좋은 고민입니다. 그리고 성하가 내린 결론과 저의 생각도 같습니다. Name은 Player의 이름을 위해 만들어진 값객체입니다. 따라서 Player에서 Name을 getter로 호출할때 값객체의 포장을 풀어서 원시값으로 리턴하는게 이상하지 않습니다. 오히려 밖에서 또다시 Name의 getter를 호출해야하는 번거로움을 줄일 수 있습니다.
| this.players = new ArrayList<>(players); | ||
| } | ||
|
|
||
| public Player findByPlayerName(String playerName) { |
There was a problem hiding this comment.
🎯 결과를 보고 싶은 사람의 이름이 참여자에 없을 때 예외 처리하는 곳
Players에서 Player를 돌면서 참여자의 이름과 같은 참여자를 찾는 로직을
findFirst()로 구현했습니다.
이때 반환이 Optional로 돼서 orElseThrow를 써야했었는데 이때 예외 처리를 여기서 하게 되면,
유저에게 자세히 예외 메시지를 전달할 수 없을 것 같더라고요!
저는 에러 메시지에 '결과를 보고 싶은 사람의 이름'이라는 메시지를 넣고 싶은데
Players는 '결과를 보고 싶은 사람의 이름'이 아닌,
'참여자의 이름'만 받고, 알고 있기 때문에
해당 예외 메시지를 Players에서 던지는 건 아니라고 생각했습니다.
그래서 일단 메시지가 없이 orElseThrow로
IllegalArgumentException만 발생시킨 뒤,
Controller에서 try-catch로 '결과를 보고 싶은 사람의 이름' 메시지를 포함해서
예외 메시지를 던졌습니다!
제가 생각하고 구현한 근거가 올바른 근거인지,
혹시 소니가 생각할 때 다른 방법 중에 더 좋은 것이 있을지 궁금합니다!!
There was a problem hiding this comment.
네 그런 이유라면 Players 에서는 NotFoundPlayer와 같은 예외를 던져주고 컨트롤러에서 해당 예외를 받아 성하가 원하는 메시지를 보여주면 될 것 같습니다. 이런 경우는 범용적인 IllegalArgumentException를 사용해서 catch하는것 보다 도메인 예외를 따로 만들어서 해당 예외만 catch하게 하는게 더 좋을 것 같네요 :)
There was a problem hiding this comment.
커스텀 예외를 구현할 생각은 못 했는데
커스텀 예외를 던지면 더 의미가 명확해지겠네요!! 감사합니다!
리팩토링해서 커밋하도록 하겠습니다!
| private final Ladder ladder; | ||
| private final ResultByPlayer resultByPlayer; | ||
|
|
||
| public LadderGame(List<String> names, List<String> results, int height) { |
There was a problem hiding this comment.
🎯 Factory 패턴 사용 고민
실행 결과의 수가 참여자의 수와 다를 때 예외 처리를
InputView나 도메인 객체에서 하는 것이 아니라,
Players와 Results의 사이즈를 비교하는 것이라서
LadderGame의 생성자에서 검증을 하고 있는데
LaddderGame의 생성자 로직이 많아져서
다른 분들은 Factory 패턴을 써서 LadderGame을
Factory에서 생성하고 주도록 코드를 짜신 분도 있더라고요!
Factory 패턴으로 LadderGame을 생성해서 주는 건 어떻게 생각하시는지 궁금합니다!
There was a problem hiding this comment.
네 저는 검증로직 이외에 현재 너무 많은 로직이 LadderGame의 생성자에서 이루어지는 것 같은데요.
- generatePlayers, generateResults, generateResultByPlayer, adderMaker.generate -> 이와 같은 로직이 LadderGame의 생성자에서 이루어질 필요가 있을까요? 외부에서 생성해서 LadderGame에 넣어주는건 어떤가요?
- 그 후에 팩토리 패턴을 고민해봐도 될 것 같습니다 :)
There was a problem hiding this comment.
LadderGame의 생성자에서 수행했던 로직 중 검증 로직을 제외한 로직들을
전부 LadderGameMaker에 위임한 후, LadderGameMaker를 통해 LadderGame을 생성하도록 리팩토링했습니다!
이렇게 하다보니 LadderGame의 생성자 파라미터가 4개나 되더라고요,,,
줄이는 방법이 있을까요??
There was a problem hiding this comment.
네 그렇다면 생성자에 주입받는 객체가 LadderGame의 동작을 위해 모두 필요한 필드인지 생각해볼까요?
예를 들어, Ladder의 경우 LadderGame에서 어떻게 쓰이고 있나요? 현재 코드상에서는 LadderGame 메서드에서 사용되는 곳은 없고 단순 getter로만 사용되고 있네요. 그렇다면 LadderGame이 Ladder를 가지고 있을 필요가 있을까요?
There was a problem hiding this comment.
안녕하세요 성하, 리뷰어 소니 입니다.😊
2단계 잘 구현해주셨습니다. 질문 주신 내용은 답글 남겼으니 확인부탁드릴게요~~
- Map 자료 구조의 네이밍을 어떻게 하시는지 궁금합니다!
네이밍은 참 어렵죠. 그래서 저도 ChatGPT 한테 물어봤습니다. 이렇게 추천을 해줬네요. 어떤게 마음에 드시나요? (저는 개인적으로 by를 쓰지 않는걸 선호합니다. 이건 개인 취향🙃)
- TDD 시 테스트 작성 기준
- 단순한 Getter 테스트도 TDD 시에 테스트 해야하나? -> 저는 필수는 아니라고 생각합니다. getter는 getter만들 위한 테스트가 아니라 다른 테스트에서 자연스레 테스트 되는 경우가 많습니다. 예를 들어 Player 클래스의 isSameName 메서드를 테스트할때 getter도 테스트 되겠네요. 단 생성자 내부에 검증로직이 있다거나 주입한 인자가 의도한대로 할당되었는지 확인을 하기 위해 생성자 테스트에서 getter를 호출해 결과값을 확인할 수는 있을 것 같습니다. 그게 아닌 단순 getter를 위한 테스트는 테스트 우선순위가 낮다고 생각합니다.
- 별도의 로직이 없는 '클래스를 생성한다'라는 의미의 테스트도 TDD 시 작성해야 하는가? -> 마찬가지로 큰 의미가 없다고 생각합니다. 결국 해당 클래스에 있는 메서드를 테스트할때 클래스 생성에 대한 테스트도 간접적으로 되는 것이기 때문에 getter 메서드의 테스트 보다 더 우선순위가 낮다고 생각합니다.
|
|
||
| private LadderGame ladderGame; | ||
|
|
||
| public void start() { | ||
| init(); | ||
| printGameResult(); | ||
| generateLadderGameStep(); | ||
| printGameResultStep(); | ||
| printResultByPlayerStep(); | ||
| } |
There was a problem hiding this comment.
현재는 메서드의 파라미터와 반환값이 없고 함수 호출만 있는 구조네요.
사실 이렇게 필드를 하나 선언해놓고 참조형태로 메서드를 호출하는 구조는 객체지향적인 구조는 아닙니다.
객체지향적인 구조는 객체간 메시지를 주고받는 구조를 말하는데요. generateLadderGame 이라는 메서드를 호출하면 LadderGameStep을 반환받고 그것을 printGameResultStep 메서드에 넘겨줘서 결과를 출력하고 printResultByPlayerStep에 Players 전달하여 값을 출력하는 등의 형태로 변경할 수 있을까요?
There was a problem hiding this comment.
그렇군요!
저는 가독성만을 생각해서 저런 식으로 했었는데 객체 지향적 구조가 아닌 것은 처음 알게 되었습니다!
리팩토링은 LadderGame을 생성하여 반환하고,
반환한 LadderGame의 기능을 사용하여 GameResult을 출력하는 기능에 넘겨주고
ResultByPlayer를 출력하는 기능에 LadderGame을 넘겨주었습니다!
| return startIndex.getRawStartIndex(); | ||
| } | ||
|
|
||
| public String getName() { |
There was a problem hiding this comment.
네 좋은 고민입니다. 그리고 성하가 내린 결론과 저의 생각도 같습니다. Name은 Player의 이름을 위해 만들어진 값객체입니다. 따라서 Player에서 Name을 getter로 호출할때 값객체의 포장을 풀어서 원시값으로 리턴하는게 이상하지 않습니다. 오히려 밖에서 또다시 Name의 getter를 호출해야하는 번거로움을 줄일 수 있습니다.
| this.players = new ArrayList<>(players); | ||
| } | ||
|
|
||
| public Player findByPlayerName(String playerName) { |
There was a problem hiding this comment.
네 그런 이유라면 Players 에서는 NotFoundPlayer와 같은 예외를 던져주고 컨트롤러에서 해당 예외를 받아 성하가 원하는 메시지를 보여주면 될 것 같습니다. 이런 경우는 범용적인 IllegalArgumentException를 사용해서 catch하는것 보다 도메인 예외를 따로 만들어서 해당 예외만 catch하게 하는게 더 좋을 것 같네요 :)
| private final Ladder ladder; | ||
| private final ResultByPlayer resultByPlayer; | ||
|
|
||
| public LadderGame(List<String> names, List<String> results, int height) { |
There was a problem hiding this comment.
네 저는 검증로직 이외에 현재 너무 많은 로직이 LadderGame의 생성자에서 이루어지는 것 같은데요.
- generatePlayers, generateResults, generateResultByPlayer, adderMaker.generate -> 이와 같은 로직이 LadderGame의 생성자에서 이루어질 필요가 있을까요? 외부에서 생성해서 LadderGame에 넣어주는건 어떤가요?
- 그 후에 팩토리 패턴을 고민해봐도 될 것 같습니다 :)
| List<Player> gamePlayers = new ArrayList<>(); | ||
| for (int generateIndex = 0; generateIndex < names.size(); generateIndex++) { | ||
| Name name = new Name(names.get(generateIndex)); | ||
| StartIndex startIndex = new StartIndex(generateIndex); |
There was a problem hiding this comment.
시작 인덱스를 0으로할지 1로 할지는 성하의 판단대로 해주시면 됩니다 ㅎㅎ
다만 0,1과 같은 값을 그대로 사용해서 매직넘버를 만들기 보다는 상수로 선언해 네이밍을 통해 의미를 명확히 해주면 좋을 것 같습니다.
|
질문에 대한 답변 감사드립니다! 저는 개인적으로 Map 자료구조의 네이밍으로 '-s'인 복수형이 들어가게 되면, TDD 테스트 부분도 다음부터는 getter와 생성 테스트는 수행하지 않도록 하겠습니다! 감사합니다! 아직 많이 부족하지만 코멘트 주신 부분들 리팩토링 해보았습니다! |
| @ParameterizedTest | ||
| @ValueSource(strings = {"가", "#", "a", "a1", "1.1"}) | ||
| @DisplayName("\"꽝\" 이외의 실행 결과가 정수가 아니면 예외를 던진다.") | ||
| void validateRewardNumericExceptNoLuckTest(String reward) { |
There was a problem hiding this comment.
둘 중에 하나만 선택해서 적어주시면 됩니다.
DisplayName 어노테이션을 이용해서 테스트명을 적었다면 테스트 메서드명은 아무값으로 적어도 상관없습니다.
| private final Ladder ladder; | ||
| private final ResultByPlayer resultByPlayer; | ||
|
|
||
| public LadderGame(List<String> names, List<String> results, int height) { |
There was a problem hiding this comment.
네 그렇다면 생성자에 주입받는 객체가 LadderGame의 동작을 위해 모두 필요한 필드인지 생각해볼까요?
예를 들어, Ladder의 경우 LadderGame에서 어떻게 쓰이고 있나요? 현재 코드상에서는 LadderGame 메서드에서 사용되는 곳은 없고 단순 getter로만 사용되고 있네요. 그렇다면 LadderGame이 Ladder를 가지고 있을 필요가 있을까요?
안녕하세요 소니!
이번 2단계는 생각보다 어려워서 오래 걸렸던 것 같아요!
진행 중에 궁금했던 질문들을 PR에 남기려고 합니다!
이전처럼 PR에 한번에 남기면 소니가 보기 많이 힘들 것 같아서
추상적인 내용만 PR에 남기고, 제가 구현한 코드와 관련한 내용들은 해당 코드 아래에 Comment로 질문을 남기도록 하겠습니다!
항상 리뷰해주셔서 감사합니다! 😁
질문
🎯 1. Map 자료 구조의 네이밍을 어떻게 하시는지 궁금합니다!
구글링해보니,
Map<Key, Value>에서'Value'By'Key'로 하거나 'Key'To'Value'를 많이 사용한다는 것을 알았는데 'By'를 쓰는 것이 더 이해가 쉬운 것 같다고 생각했습니다.
그래서 '참여자별 결과' Map 네이밍을
ResultByPlayer로 네이밍하게 되었는데,소니는 Map 네이밍을 어떻게 하시는지 궁금합니다!
🎯 2. TDD 시 테스트 작성 기준
이번 미션 요구사항 중에
모든 기능의 단위 테스트가 존재해야 한다라는 요구사항이 있었습니다.이러한 요구사항에 따라, 단순히 Getter 메소드도 테스트하게 되었는데 단순한 Getter 테스트도 TDD 시에 테스트 해야하는 건지 궁금합니다!
또, 결국 맨 처음 TDD 시에는 클래스가 없어서 클래스를 생성하는 것부터 시작하게 되는데,
별도의 로직이 없는 '클래스를 생성한다'라는 의미의 테스트도 TDD 시 작성해야 하는지 궁금합니다.
이렇게 여러 테스트들을 하나하나씩 하다보니 커밋 개수가 너무 많아져서 어떻게 해야할지 고민이 됐었습니다 😂
나머지는 구현 코드 질문들이라서 해당 코드들에 코멘트를 남기도록 하겠습니다! 😃