[사다리 게임] 제이 미션 제출합니다. #32
Conversation
Add Position enum Write test code for Line
…osition or consecutive position
Create interface for generator
Natural number does not contain zero
Implements Boolean Generator for production code
Define regular expressions for player names, height
Add Players test
Contains player, ladder
Add Results class for wrapping result object
Checking names and result length is same
The process that confirming random generating ladder is not proper in test code. Though this case is insignificant, in the real world it can be an overhead when there are lots of them.
Check moving boundary is not accepted
Extract duplicate ladder initializing code
Based on consideration that each object's role is proper
Don't need to use raw collection in getting winners, wrap with a class
rises recursion error once get IllegalArgument Exception
Remove String type in hash map, reuse custom classes
|
우와 새벽에 작업하셨군요... ㅠㅠ 힘드셨을 텐데 빠른 피드백 정말 감사드립니다 😀 |
| public class OutputView { | ||
| public static void printLadderStatus(LadderResultDto ladderResultDto) { | ||
| System.out.println(ladderResultDto.getMadeLadderVO().toString()); | ||
| } |
There was a problem hiding this comment.
먼저 ..~과 같이 .을 많이 사용하는 구조는 지양하는 것이 좋습니다.
추가로 제가 toString 부분에 대해 피드백 드린 것은
기존의 MadeLadderVO 가 toString을 오버라이드 하였고, getMadeLadderVO가 MadeLadderVO를 리턴하고 있었기 때문에
getMadeLadderVO().toString() 대신 ladderResultDto.getMadeLadderVO()과 같이 수정 해보라는 의미입니다.
toString을 오버라이드한 객체는 출력시 toString이라고 쓰지 않아도 toString 내부에 구현한 String이 출력됩니다.
오버라이드 하였기 때문에 System.out.println 에서 toString을 제거하여도 출력이 가능하며, 왜 가능한지에 대해 이해하면 좋을 것 같아 남긴 피드백입니다. 조금 번거로우실 수 있겠지만 이 부분은 처음 작성하신 것이 더 좋을 것 같습니다. ㅜㅡㅜ
필요하다면 지금 구현하신 것 처럼 DTO, VO 등의 클래스를 사용하여 작성해도 괜찮습니다. 하지만 저는 개인적으로 사다리 게임을 구현하면서 DTO/VO 구현에 크게 고민해야한다고 생각하지는 않으며, 이러한 클래스들이 없어도 괜찮다고 생각합니다. 오히려 지금은 DTO를 통해 한번에 전달하려는 구조를 만들기 보다, DTO를 사용하지 않고 그냥 Ladder, Winners등을 OutputView에서 사용해도 좋으니, 각 객체가 해야할 역할들과 관련한 테스트 코드 작성을 위한 고민에 더 많은 시간을 사용하면 도움이 될 것 같아요. View 라면 콘솔 출력이 아닌 web 출력이라고 가정하였을때 필요한 값들이 기존의 VO에 있다면, 추가로 작성해야 할 필요가 있을까요?
따라서 저는 데이터 교환을 위한 dto 등의 설계도 로직 구현 못지 않게 중요한 부분이라고 생각합니다. 관련하여 앞으로 진행하게될 web/db 등 관련 미션을 진행하면서 직접 구현해보면 이해가 쉬울 것 같고, 더 자세히 학습하면 좋을 것 같습니다.
| } | ||
|
|
||
| return players; | ||
| } |
There was a problem hiding this comment.
이 부분은
~
List<Player> players;
for {
players.add()
}
return new Players(players);
와 같이 Players에 add/addAll을 구현하는 대신 생성자를 이용하는 것은 어떨까요?
add/addAll등의 메서드가 불필요하다고 생각해요. 또한 이는 언제든 접근하여 List 값을 변경할 수 있는 위험성도 있다고 생각되고요.
Winners 도 마찬가지에요!
추가로 일급 컬렉션에 대해서 https://jojoldu.tistory.com/412 자세히 설명 된 글이 있어요!
참고하면 어떨까요?
There was a problem hiding this comment.
감사합니다!! 값을 변경할 수 있다면 쓰는 이유가 없었군요! 말씀처럼 언제든 접근하여 자료구조의 값을 변경하는 것은 위험한 것 같습니다. 오직 생성자를 통해서만 초기화할 수 있게 변경하였습니다!
| import com.woowacourse.laddergame.util.RandomBooleanGenerator; | ||
|
|
||
| public class LadderGameService { | ||
| public static LadderResultDto play(LadderDto ladderDto) { |
There was a problem hiding this comment.
LadderGameService 클래스를 구현한 것은 👍 잘하셨습니다.
아래 dto 등과 관련하여 피드백에도 남겼지만, 현재 LadderGameService 클래스는 LadderResultDto에 너무 얽매여있어요.
이 이야기는, 만약 LadderGameService가 LadderResultDto가 아닌 다른 Dto를 반환해주어야 한다면요?
재사용이 어렵지 않을까요? 게임을 실행하는 핵심 로직인 GameService 클래스에서 dto 에 대한 부분까지 신경써줘야 하게 될거구요!
따라서 저는 현재 이 클래스가 너무 많은 일을 하고 있다고 생각해요!
There was a problem hiding this comment.
이 부분만 빼고 피드백 반영을 하였습니다!! 고민 중인 상태입니다 ㅠ
제 LaderGameService에서 play를 하면 결과를 돌려주는데 지적해 주신대로 그 결과가 LadderResultDto인 상태입니다. 그렇다면 play가 아닌 getWinnerVO와 getMadeLadderVO 라는 메서드로 분리하여 ladderGameService에서 각각 두개의 메서드를 호출하는 식으로 변경을 해야될지 고민입니다 ㅠㅠ
일단 제가 이해한 바로는 반환할때 아예 dto를 만들어서 반환하는 것이 특정 뷰에 얽매이게 되는 경우라고 피드백을 이해했습니다!
이 클래스를 어떻게 분리할지 혹시 힌트를 얻을 수 있을까요??
There was a problem hiding this comment.
이 부분에 대해서는 사람마다 다르게 생각할 것 같아서 정답은 없는 것 같습니다. 다만 어떻게 하면 더 괜찮은 구조, 깔끔한 구조, 요구사항이 추가되었을때 변경을 최소화할 수 있는 구조등이 될 수 있을지에 대한 고민은 동일하다고 생각합니다.
play는 최종적으로 내가 얻고 싶은 결과인 Winners를 반환해주고, 이를 Dto로 convert 해주는 역할은 분리하도록하면 어떨까요?
또한 말씀하신대로 LadderGameService가 getMadeLadderVO를 가지고 있을 필요가 없잖아요!!
LadderGameService의 역할을 필요한 인풋을 이용해, 사다리를 생성하여 그 결과인 Winners를 return 하는 것이 아닐까요!?
이 부분에 대해 고민중이 라고 하셔서 머지 진행하려다 고민이 완료후 함께 머지할게요!!!
완료되면 알려주세요!
|
|
||
| Ladder ladder = new Ladder(new NaturalNumber(height), new NaturalNumber(countOfPerson)); | ||
| ladder.initLadder(booleanGenerator); | ||
|
|
There was a problem hiding this comment.
initLadder 즉 사다리에 대한 초기화도 Ladder(~) {
initLadder();
}
와 같이 진행되어야해요.
결국 초기화는 Ladder 클래스에 필요한 값들을 넘겨주고 이를 이용해 Ladder 클래스의 private List lines의 값을 생성해주는 것을 의미해요.
그렇다면, private NaturalNumber height, private NaturalNumber countOfPerson 두 변수도 필요없지 않을까요?
There was a problem hiding this comment.
사다리를 초기화할때 내부에서 만들어지는 것으로 바꿨더니 클래스 인스턴스 변수를 줄일 수 있었습니다! 굳이 써도 되지않아도 되는 필드를 줄일 수 있어서 좋은것 같습니다 ㅎㅎ
| Ladder ladder = LadderGenerator.generateLadder(height, countOfPerson, generator); | ||
|
|
||
| System.out.println(ladder.toString()); | ||
| } |
There was a problem hiding this comment.
랜던 generate를 통해 사다리 한줄의 bridge 를 놓을 수 있는지 없는지를 확인하고 있습니다.
지금 처럼 출력을 통해 확인할 수 도 있고, 잘못된 parameter를 이용해 generateLadder 실행시 적절한 예외를 발생시키는지 테스트 할수도 있습니다.
다만, 제대로 생성된 경우 이미 사다리 생성과 관련해 필요한 클래스들을 추출하였고, 각 클래스들의 핵심 로직을
통해 사다리를 생성과 관련한 테스트를 작성 하였습니다.
모든 로직에 대해 테스트 코드를 짜면 좋지만, 이러한 경우 저는 구성하고 있는 다른 핵심 로직을 테스트하므로써 정상적으로 동작함을 확인하는 방향으로 테스트 코드를 작성합니다. 각 객체가 자신이 맡은 역할을 충실히 제대로 수행하면 사다리는 정상적으로 생성되니까요!
| import static org.assertj.core.api.Assertions.assertThatExceptionOfType; | ||
|
|
||
| class LadderDtoTest { | ||
| @Test |
There was a problem hiding this comment.
Dto 클래스에서 "예외"를 처리하는 것이 좋은 방법일까요?
InputView는 입력 즉시 LadderDto를 통해 중복, 길이 등 기능 요구사항에 대해 테스트 하고 있습니다.
물론 모두 조금씩 다르게 생각할 수 있지만 위의 예외 처리는 기능 요구사항의 일부인 핵심 로직이라고 생각합니다.
Dto에서 예외를 처리를 하면 절대 안되는 것은 아니겠지만, 주로 데이터 전송을 위한 목적으로 사용되는 Dto는 변경될 가능성이 많습니다.
따라서 예외 처리에 대해 정해진 답은 없지만, 위의 내용을 토대로 한번 고민해보면 어떨까요?
There was a problem hiding this comment.
저번 과제부터 DTO 클래스에 대한 언급이 있어서 공부를 해보았습니다. 위에 길게 말씀해주신 것처럼 지금 집중해야 할 주제가 더 중요하다고 생각은 되지만 조금 더 공부해보자라는 차원에서 한 번 도입해 보았습니다.
떠도는 정보들을 보며 많이 헷갈렸지만 (vo, dto관련) 교육 때 질문을 통해서 조금은 해소된 상태이긴 합니다. 그래도 아직 완벽하지 않은 상태입니다.
제가 질문을 통해 얻은 내용으로는 일단 내부에서 사용되는 도메인(Player, Result 등등)에서는 예외처리가 필수적이다. 라는 것이었습니다. 그리고 Dto에서도 약간의 중복적인 예외처리도 가능하다고 하여 넣어놓은 상태였습니다. (추가적으로 말씀하시길 dto 없이 도메인이 그대로 사용자쪽으로 노출되도 괜찮다고 하셨습니다. 굳이 만들어본 이유는 학습차원에서 만들었습니다!)그리고 뭔가 조금 더 안정적이라고 판단하였습니다. 말씀 하신것처럼 변경될 가능성이 많다고 했던 부분에서 고민을 해보았습니다.
변경에 대해 Dto 예외처리를 계속 변경해야 하는 경우 생산성이 떨어질 수 있어 굳이 하지 않아도 된다 라는 의미로 받아들여도 될까요?
There was a problem hiding this comment.
과제에 Dto 를 구현하신 것을 통해 관련하여 이미 많은 고민을 하셨을거라고 생각했습니다 👍
중복적인 예외처리도 가능합니다. 굳이 하지 않아도 되는가?에 대해 정확하게 "아니다, 혹은 맞다" 라고 하기는 어렵고 사람마다 다를 것 같아요.
변경 가능성이 높은 Dto에서 예외처리를 하는 것 보다는 도메인의 예외처리가 더 중요하다고 생각합니다. 둘다 처리/도메인 처리 두 가지 중 결국 가장 중요한 것은 도메인 로직에 기능 요구사항과 관련된 예외에 대해서 꼼꼼하게 구현해야한다는 것이 아닐까요???
그리고 중복 코드가 생길 수 있어요. 지금 처럼 같은 일을 하는 코드를 똑같이 두개 작성된 경우 어떤 단점이 있을까요?
예로, dto의 중복을 체크하는 로직과, Players의 중복을 체크하는 동일한 코드가 중간에 요구사항이 변경되어 중복을 확인하는 기준이 변경되었다고 가정하였을때, 두 예외처리 모두를 수정해야 합니다. 도메인의 로직만 수정을 하고 실수로 dto의 로직을 수정하지 않는다면 dto의 예외처리 로직은 의미가 없어지지 않을까요? 이로 인해 기대하지 않았던 결과가 발생할 수도 있을 것 같아요!
또한 지금처럼 단순히 데이터 전달 용도로 작성한 경우에는 그 목적에 맞게 데이터를 전달하기 위한 껍데기로 사용하는 것이 조금 더 좋다고 생각합니다.
Focus on domain's core logic
Field needs to be refactored as final in first collection class. Then once initialized first, there will be no chance to change its own value. Only constructor can initialize first value
This one can be tested indirectly with other cases (Line, Position...)
|
피드백 감사합니다!! ㅎㅎ |
| int no = Integer.parseInt(number); | ||
|
|
||
| if (no < 0) { | ||
| throw new RuntimeException("음수는 입력할 수 없습니다"); |
There was a problem hiding this comment.
Calculator 클래스에도 숫자가 있습니다!
의미를 나타낼 수 있는 경우 상수를 활용하면 어떨까요!?
| // public void add(Player player, Result result) { | ||
| // winners.put(player, result); | ||
| // } | ||
|
|
There was a problem hiding this comment.
사용하지 않는 코드의 경우 주석 보다는 삭제 하는 것이 더 깔끔할 것 같아요!
| import com.woowacourse.laddergame.util.RandomBooleanGenerator; | ||
|
|
||
| public class LadderGameService { | ||
| public static LadderResultDto play(LadderDto ladderDto) { |
There was a problem hiding this comment.
이 부분에 대해서는 사람마다 다르게 생각할 것 같아서 정답은 없는 것 같습니다. 다만 어떻게 하면 더 괜찮은 구조, 깔끔한 구조, 요구사항이 추가되었을때 변경을 최소화할 수 있는 구조등이 될 수 있을지에 대한 고민은 동일하다고 생각합니다.
play는 최종적으로 내가 얻고 싶은 결과인 Winners를 반환해주고, 이를 Dto로 convert 해주는 역할은 분리하도록하면 어떨까요?
또한 말씀하신대로 LadderGameService가 getMadeLadderVO를 가지고 있을 필요가 없잖아요!!
LadderGameService의 역할을 필요한 인풋을 이용해, 사다리를 생성하여 그 결과인 Winners를 return 하는 것이 아닐까요!?
이 부분에 대해 고민중이 라고 하셔서 머지 진행하려다 고민이 완료후 함께 머지할게요!!!
완료되면 알려주세요!
| import static org.assertj.core.api.Assertions.assertThatExceptionOfType; | ||
|
|
||
| class LadderDtoTest { | ||
| @Test |
There was a problem hiding this comment.
과제에 Dto 를 구현하신 것을 통해 관련하여 이미 많은 고민을 하셨을거라고 생각했습니다 👍
중복적인 예외처리도 가능합니다. 굳이 하지 않아도 되는가?에 대해 정확하게 "아니다, 혹은 맞다" 라고 하기는 어렵고 사람마다 다를 것 같아요.
변경 가능성이 높은 Dto에서 예외처리를 하는 것 보다는 도메인의 예외처리가 더 중요하다고 생각합니다. 둘다 처리/도메인 처리 두 가지 중 결국 가장 중요한 것은 도메인 로직에 기능 요구사항과 관련된 예외에 대해서 꼼꼼하게 구현해야한다는 것이 아닐까요???
그리고 중복 코드가 생길 수 있어요. 지금 처럼 같은 일을 하는 코드를 똑같이 두개 작성된 경우 어떤 단점이 있을까요?
예로, dto의 중복을 체크하는 로직과, Players의 중복을 체크하는 동일한 코드가 중간에 요구사항이 변경되어 중복을 확인하는 기준이 변경되었다고 가정하였을때, 두 예외처리 모두를 수정해야 합니다. 도메인의 로직만 수정을 하고 실수로 dto의 로직을 수정하지 않는다면 dto의 예외처리 로직은 의미가 없어지지 않을까요? 이로 인해 기대하지 않았던 결과가 발생할 수도 있을 것 같아요!
또한 지금처럼 단순히 데이터 전달 용도로 작성한 경우에는 그 목적에 맞게 데이터를 전달하기 위한 껍데기로 사용하는 것이 조금 더 좋다고 생각합니다.
|
|
||
| public String getResult(String name) { | ||
| if (name.equals(PLAY_ALL_LADDER_RESERVED_WORD)) { | ||
| return winnerVO.getAllResult(); |
There was a problem hiding this comment.
물론 지금은 예외처리를 꼼꼼하게 잘 하셨기 때문에 괜찮을 것 같지만,
name이 null인 경우가 예상치 못하게 발생할 경우 위의 코드는 NPE를 발생시킬 수 있는 코드입니다!
해결 방법에 대해서 고민해보면 어떨까요?
There was a problem hiding this comment.
일단 null인 경우에 예외를 처리하는 로직을 넣었습니다. 그런데 일일이 모든 경우에 null 체크를 하는 것은 조금 비효율적이라고 생각되어 찾아보았습니다.
- 애초에 프로그래밍 당시 인풋에 대한 validation을 확실히 한다. (저는 인풋을 받을 때 거기서 예외처리를 해서 null이 들어올 경우가 없을것 같은데 이 행위가 저는 인풋에 대한 validation이라고 이해하였습니다!)
- 함수에서 절대 null 반환하지 않기
- Optional의 사용 (Optional을 사용함으로써 코드의 양을 줄이고 가독성을 높이는 장점이 있는데 애초에 null이 들어올 가능성이 있다는 것은 if 문으로 null 체크를 하나 optional을 사용하나 같은 문제라고 생각되었습니다. 그래서 습관적인 null 체크보다는 사전에 null의 가능성이 있는 부분을 논의를하고 그 부분을 Optional을 사용하면 좋을 것 같습니다.)
There was a problem hiding this comment.
혹은 PLAY_ALL_LADDER_RESERVED_WORD.equals(name)과 같이 반대로 비교하고자 하는 대상을 바꾸면 어떨까요?
| private void putBridge(NaturalNumber height, NaturalNumber position) { | ||
| lines.get(height.convertIndex()).putBridge(position); | ||
| } | ||
|
|
There was a problem hiding this comment.
lines.get 하는 대신 Line에 메시지를 보내 Bridge를 놓을 수 있는지 판단하는 것은 어떨까요?
get 하는 대신 제이님이 이미 잘 추출해낸 객체에 메시지를 보내면 좋을 것 같아요!
해당 클래스 전체적으로 적용할 수 있는 부분들은 적용해보면 어떨까요?
또한 아래 isContainsLine 메서드 구현의 경우 lines.contains등과 같이 List가 제공하는 메서드를 사용해도 되지 않을까요?
There was a problem hiding this comment.
get을 한 이유는 어떤 라인에(get) 어떤 위치에 (put bridge) 다리를 놓을지 결정하기 위해서 사용을 했는데 필드가 List lines 라 한 개의 Line을 꺼내기 위해 get을 사용했습니다!
그래도 수정할 부분을 찾다가 get을 하는 부분을 분리시켜 놓았습니다!
isContainsLine(int 높이, Line 라인) 의 의도는 사다리에게 특정 높이에 특정 라인이 있는지 확인하는 의도로 작성하였습니다! 그래서 그냥 사다리에 contains를 하면 라인이 있는 것은 확인이 되지만 어느 높이에 있는지는 테스트로 알 수 없기 때문에 저렇게 하였습니다!
|
|
||
| public String getResult(String name) { | ||
| if (name.equals(PLAY_ALL_LADDER_RESERVED_WORD)) { | ||
| return winnerVO.getAllResult(); |
There was a problem hiding this comment.
혹은 PLAY_ALL_LADDER_RESERVED_WORD.equals(name)과 같이 반대로 비교하고자 하는 대상을 바꾸면 어떨까요?
user input을 받는 부분은 DTO를 정의하여 받았고 안쪽 도메인에서도 검증로직을 넣었습니다.
그리고 primitive type을 최대한 감싸서 해보았고 랜덤 테스트는 true/false를 반환하는 인터페이스로 분리해 의도된 값으로 나오는 generator를 생성해 테스트에서 사용했습니다.
감사합니당!