[2단계 - 사다리 생성] 제이미(임정수) 미션 제출합니다. #201
Conversation
2단계 미션에 대한 요구사항 반영
map -> ladder
map -> ladder
유틸 클래스 생성 방지를 위해
- 사용하지 않는 패키지 제거 - 가독성 및 디버깅을 위한 개행 추가
docs/README.md
Outdated
| - 참가자 | ||
| - [x] 사람은 10명까지 ( 테스트 원할한 범위 ) | ||
| - [x] 사람은 2명 이상이다. | ||
| - [x] 사람이름은 ',' 로 구분한다. | ||
| - [x] 중복된 이름을 가질 수 없다. | ||
|
|
||
|
|
||
| - 결과들 |
There was a problem hiding this comment.
- README가 중복된 부분이 있는 것 같습니다. 확인부탁드려요!
- docs에 README를 따로 넣으신 이유가 있을까요?
There was a problem hiding this comment.
README 중복이라는 것은 README 파일이 두 개라는 의미가 맞을까요?
docs에 넣은 이유는 크게 없었습니다...!
기존 있는 README를 건드리지 않으려 한 것인데, 질문을 보니 오히려 함께 있는 것이 다른 사람들로 하여금 제 프로젝트를 확인하는 데 용이하겠다는 생각이 드네요.
이러한 이유로 현재 합쳐두었습니다!
There was a problem hiding this comment.
README 중복이라는 것은 README 파일이 두 개라는 의미가 맞을까요?
아뇨, cf546ca 커밋 기준으로
사진과 같이 작성되어있어서 말씀드린 부분이었습니다.
기존 있는 README를 건드리지 않으려 한 것인데, 질문을 보니 오히려 함께 있는 것이 다른 사람들로 하여금 제 프로젝트를 확인하는 데 용이하겠다는 생각이 드네요.
이러한 이유로 현재 합쳐두었습니다!
👍
|
|
||
| public class LadderGameController { | ||
|
|
||
| private static final String GET_RESULT_ALL = "all"; |
There was a problem hiding this comment.
GET은 붙이지 않아도 되지 않을까요?
| private static final String GET_RESULT_ALL = "all"; | |
| private static final String RESULT_ALL = "all"; |
src/main/java/domain/Ladder.java
Outdated
| public int getResultIndex(int index) { | ||
| int resultIndex = index; | ||
| for (Line line : lines) { | ||
| resultIndex = getNextIndex(index, line); |
There was a problem hiding this comment.
도메인의 중요한 부분이 잘못된 것 같습니다. getNextIndex의 인자로 index를 받고 있어서 매번 같은 인자로 받게 됩니다.
이 부분 때문에 Ladder의 lines 필드를 아래와 같이 넣고 테스트해보면 결과값이 틀리게 나옵니다.
// a,b,c,d
// x,100,200,x
// 4
// 정답 : a : 200, b : x, c : 100, d : x
lines.add(new Line(List.of(true, false, false)));
lines.add(new Line(List.of(false, false, true)));
lines.add(new Line(List.of(false, false, true)));
lines.add(new Line(List.of(false, true, false)));
아래처럼 수정이 필요하지 않을까요?
| resultIndex = getNextIndex(index, line); | |
| resultIndex = getNextIndex(resultIndex, line); |
There was a problem hiding this comment.
정말 바보 같은 실수네요... 🥲
해당 부분은 수정해 두었습니다.
테스트 때 잡히지 않는 것을 보니 테스트도 부실했었다는 생각이 듭니다...
이를 해결하기 위해 TestGenerator를 만들어 좀 더 자세한 테스트가 가능하도록 수정했습니다!
There was a problem hiding this comment.
테스트 때 잡히지 않는 것을 보니 테스트도 부실했었다는 생각이 듭니다...
개발자로서 좋은 태도를 가지셨군요 👍 테스트 작성 시, 복잡한 도메인 혹은 비즈니스적으로 중요한 도메인은 가급적 테스트에 많은 노력을 기울이시면 좋을 것 같습니다. 추후 이상적인 테스트는 어떤 것일까 고민하게 될텐데요. (시간적 여유가 있다면 단위테스트 라는 책 추천드립니다.)
src/main/java/domain/Ladder.java
Outdated
| return lines; | ||
| } | ||
|
|
||
| public int getResultIndex(int index) { |
There was a problem hiding this comment.
단순히 index라는 변수명이 아니라 좀 더 명확하게 어떤 인덱스인지 말해주면 좋지 않을까요?
There was a problem hiding this comment.
의견을 보고 participnatIndex로 했지만, Ladder에서는 참여자 인덱스인 것은 알 필요가 없다고 판단해 startIndex로 결정했습니다.
해당 변수명은 괜찮을까요?
There was a problem hiding this comment.
저는 participantIndex로 하는 게 좋지 않을까?라는 생각이었는데요. 말씀대로 Ladder 도메인에서 참여자 인덱스는 굳이 알 필요 없는 게 더 나을 것 같습니다. 👍
| if (line.canMoveLeft(index)) { | ||
| return index - 1; | ||
| } | ||
| if (line.canMoveRight(index)) { | ||
| return index + 1; | ||
| } | ||
|
|
||
| return index; |
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| public class GameResult { |
| return people.get(index); | ||
| } | ||
|
|
||
| public Person getByName(String name) { |
There was a problem hiding this comment.
네 맞습니다..!
도메인 분리 과정에서 누락된 것으로 보입니다.
수정해 두었습니다!
docs/README.md
Outdated
| | 맵 | Map | 맵 정보 지칭 | | ||
| | 사다리 | Ladder | 사다리 지칭 | | ||
| | 라인 | Line | 사다리의 한줄을 지칭 | | ||
| | 한글명 | 영문명 | 설명 | |
There was a problem hiding this comment.
도메인을 OutputView에서 출력할 때 필요한 형태로 변환해 주는 기능에 대한 역할 부여를 어디에 해야 할지 고민인 상태입니다.
처음에는 도메인 자체에서 값을 반환할 때 변환해 전달해 주었습니다.
하지만, 해당 기능은 단지 출력해 주기 위한 기능이라는 생각이 들어 도메인에서는 값을 처리 없이 반환하도록 수정했습니다.
그리고 게임 전체를 관리하는 도메인을 생성해 해당 도메인에서 모든 값 변환을 수행해 주었습니다.
그런데 제가 역할을 제대로 부여했는지 잘 모르겠습니다.
넵, 잘해주셨어요! Controller가 말 그대로 뷰와 모델의 중간에서 컨트롤하는 역할이라고 보시면 됩니다. (링크)
또한, 1단계 리뷰 중 “OutputView가 도메인을 모른다면 훨씬 유연해질 수 있을 것 같은데요." 라는 의견을 주셨는데,
제가 수행한 방식이 생각한 방식이 맞는지 궁금합니다.
네 맞습니다! 👍 OutputView가 도메인을 아예 모르는 상태가 되면 도메인을 의존하지 않고 확장가능하다는 이야기를 하고싶었습니다.
기존 제 코드에서는 Height과 Weight을 하나의 도메인에서 사용했었습니다.
하지만, 이번에 코드가 변경되며 Height과 Weight을 사용은 하지만, 굳이 저장까지는 필요 없어진 상태입니다.
일단 검증 로직을 위해 각각의 도메인을 남겨 두었는데, 저장까지 필요가 없으니 validator 클래스를 생성해 검증만 수행하는 클래스로 변경해 주는 게 좋을지 궁금합니다.
현재도 충분히 객체지향에 맞는 코드라고 생각합니다. 굳이 Validator 클래스로 빼지 않아도 될 것 같아요. (빼도 좋고 안 빼도 좋습니다)
보고 싶은 사람을 입력할 때 사람의 이름 혹은 ‘all’이라는 문자열을 입력하게 됩니다.
현재 저는 한 사람에 대한 결과를 원하는지 전체 결과를 원하는지에 대한 판단을 컨트롤러에서 수행하고 있습니다.
그런데 과연 해당 판단을 수행하는 것이 컨트롤러의 역할이 맞는지에 대한 의문이 들고 있습니다.
해당 판단에 대해선 어느 위치에서 수행하는 것이 이상적일지 궁금합니다.
Controller의 역할이 커지는 것 같지만 현재 미션에서는 Controller에서 해도 괜찮다고 생각합니다. 다른 입력을 통해 결과물을 달리 준다는 역할을 분리한 하나의 클래스에서 진행하게 된다면 DispatcherServlet가 하는 역할과 비슷할 것 같은데요. DispatcherServlet처럼 다른 클래스로 또 나눌 수도 있을 것 같지만 현재 미션에서 적용하기보단 추후 웹 미션에서 적용해보면 좋을 것 같습니다.
도메인 중 GameResult는 List 형태의 값을 저장하고 있습니다.
해당 도메인에 대한 테스트로 객체를 제대로 생성했다는 테스트 및 값을 제대로 가져오는지에 대한 수행을 하려고 합니다.
그런데 해당 도메인에서 getter 수행 시 List 형태로 바로 반환하도록 했습니다.
그런데 값을 제대로 가져오는지 확인하기 위해선 Person 객체로 검사해야 하는데 new Person()을 수행하더라도 이는 다른 객체로 인식되기에 검증이 어려운 상태입니다.
이러한 경우 어떻게 검증할 수 있는지 궁금합니다.
GameResult는 Map 형태로 저장하고 있는 것 같은데요. GameResult가 맞을까요? 다른 객체로 인식되는 부분은 아래 답변처럼 equals와 hashcode를 재정의하면 해결되는 문제같습니다. 현재 상황에서는 재정의해도 된다고 생각되고요.
또한, Results에 대한 테스트를 수행할 때 역시 값이 제대로 저장되는지 확인하려고 합니다.
이 역시 값이 아닌 객체를 반환하는 메서드이기에 아래와 같이 검증을 수행했습니다.
그런데 이와 같이 Result 도메인에 대한 메서드를 수행하면서 테스트를 해도 무관한 것인지 궁금합니다.
Result 자체도 하나의 value로서 특정될 수 있는 것 같은데요. equals와 hashcode를 재정의하면될 것 같습니다.
| gameResult = new GameResult(ladderResult); | ||
| } | ||
|
|
||
| // @DisplayName("요청 값으로 all을 전달한 경우 전체 결과를 반환한다.") |
There was a problem hiding this comment.
- Person이 Key이기 때문에 Person으로 감싸주셔야합니다.
- Result에도 equals, hashcode 오버라이딩이 필요해보입니다.
- 굳이 매번 gameResult.getAllResult()할 필요없이 변수로 빼도 좋을 것 같습니다.
There was a problem hiding this comment.
말씀해 주신 대로 Result에 equals와 hashcode를 오버라이딩해 해결할 수 있었습니다.
그런데, 도메인에 존재하는 메서드가 비즈니스 로직이 아닌 오로지 테스트만을 위한 것도 괜찮은지 궁금합니다.
전체 comment에 질문을 남기긴 했지만 관련 내용이 있어 한 번 더 여쭤봅니다.
There was a problem hiding this comment.
도메인에 존재하는 메서드가 비즈니스 로직이 아닌 오로지 테스트만을 위한 것도 괜찮은지 궁금합니다.
도메인에 존재하는 메서드가 비즈니스 로직이 아닌 오로지 테스트만을 위한 것도 괜찮을까? 라는 질문에만 답변하자면 이런 의문을 떠올리면 좋을 것 같습니다. 프로덕션 코드와 테스트 코드 중 어느 것이 더 중요할까? 이 의문을 가지면 당연하게도 실제로 돌아가는 코드는 프로덕션 코드이기 때문에 테스트만을 위한 코드는 도메인에 존재하지 않는 게 좋습니다.
이와 별개로 조금 더 생각해봐야할 부분은 현재 Height 클래스와 Result클래스의 경우, 값 그 자체이기 때문에 VO의 성격이 강한데요. VO(값 객체)라면 equals와 hashCode를 오버라이딩해줘야 합니다. 그래서 Result 클래스의 경우(VO의 경우), 클래스 안에 존재하는 equals와 hashCode가 테스트만을 위한 코드라고 볼 수 있을까? 라는 의문도 가지면 좋을 것 같네요
의존성 주입을 생성자 주입으로 수정함
원하는 랜덤 테스트를 위한 클래스 추가
10이하는 가능한데, 9까지만 입력 가능하도록 제한되어 있었음 이를 수정함
- ArrayList - LinkedList
|
빙봉 2단계 PR 리뷰도 감사합니다! 주요 변경 사항주요 변경사항에 대해 간단하게 정리해 보았습니다. 1. 예외 처리를 위한 함수형 인터페이스 생성재귀 함수를 통해 재입력을 수행한 기존 방식은 콜스택이 쌓이다 보면, StackOverFlow를 발생시킬 수 있기에 리스크가 크다고 생각했습니다. 2. 좀 더 정확한 랜덤 테스트를 위한
|
| private String reformatStatus(boolean status) { | ||
| if (LineStatus.getStatus(status) == LineStatus.CONNECTED) { | ||
| return CONNECTED_LINE; | ||
| } | ||
| return DISCONNECTED_LINE; | ||
| } |
There was a problem hiding this comment.
LineStatus라는 enum 클래스를 생성한 것의 목적 중 하나가 의미를 보다 명확하게 전달하기 위함이라고 생각했습니다.
기존의 true, false와 같은 boolean 값이 아닌 상수명으로 사용할 수 있기 때문입니다.
위와 같은 이유로 OutputView에서 값을 비교할 때도 enum 클래스를 사용해보았는데 괜찮은지 궁금합니다.
There was a problem hiding this comment.
네, 명확하기 전달하기 위해서 enum 클래스 작성하신 점 좋습니다. 오버엔지니어링이라고 생각들지 않네요 👍
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class TestGenerator implements BooleanGenerator{ |
| boolean current = order.get(0); | ||
| order.remove(0); |
There was a problem hiding this comment.
그냥 든 생각인데 get하고 remove를 할 거라면 queue의 poll을 사용했어도 됐을 것 같네요?
| radderGameController.play(inputView, outputView, randomBooleanGenerator); | ||
|
|
||
| LadderGameController ladderGameController = | ||
| new LadderGameController(inputView, outputView, randomBooleanGenerator); |
There was a problem hiding this comment.
BooleanGenerator를 Application에서부터 주입시켜 주는 것이 좋은가
사실 이 말을 듣고 많은 생각이 떠올랐는데요. 역할부터 시작해서 많은 의문이 들긴했습니다ㅎㅎ
- BooleanGenerator를 주입시킨다는 생각을 하기 전에, 애초에 BooleanGenerator의 역할은 무엇일까요?
- Boolean value를 만드는 거 아닐까요?
- RandomBooleanGenerator는 랜덤한 Boolean Value를 만드는 역할인데 왜 인터페이스를 구현해줬어야할까요?
- 단순히 랜덤한 Boolean Value를 만드는 역할이라면 어떤 인터페이스를 구현하는 클래스이기보단 어디서든 사용할 수 있는 유틸 클래스가 더 맞지 않을까요?
- 유틸 클래스라면 따로 생성자로 주입해주지 않고도 어디서든 사용가능하지 않을까요?
- 현재 단순히 테스트의 용이함을 위해서 BooleanGenerator 인터페이스를 구현한 건 아닐까요?
라는 생각이 들었네요ㅎㅎ 다시 돌아와서,
그런데, 사용은 Ladder 생성 시에만 사용되는 것을 확인할 수 있습니다.
컨트롤러 테스트를 위해 BooleanGenerator를 컨트롤러에서부터 주입을 시켰는데 괜찮은지 빙봉의 의견이 궁금합니다.
라는 물음에 답해본다면 BooleanGenerator의 역할을 먼저 생각해볼 필요가 있으며, 인터페이스를 그대로 사용하고자 한다면 BooleanGenerator는 어디에 있어야하는 게 맞을지 고민해보면 좋을 것 같습니다. (본질적으론 아마 이걸 물어보신 거겠죠?)
현재는 Controller 안에 있는데요. BooleanGenerator가 gernerate()하는 곳을 살펴보면 Line 클래스 안 뿐입니다. 결론적으로 Line의 상태를 생성한다고 볼 수 있으므로 LineFactory이란 클래스를 따로 생성하고 이 안에 BooleanGenerator가 있어야된다고 생각이 드네요. (LineFactory를 생성할 때 new RandomBooleanGenerator() 주입하는 형태, Model과 View를 연결짓는 Controller 역할과는 상관없으며 Ladder 도메인과도 연관이 없음)
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class TestGenerator implements BooleanGenerator{ |
There was a problem hiding this comment.
랜덤 값인 만큼 테스트를 하기에 어려움이 존재했습니다.
그래서 기존에는 모든 랜덤 값을 무조건 true로 반환하거나 false로 반환하는 두 가지 경우에 대한 테스트만 진행했습니다.
- TestGenerator라면 test 패키지에 있어도 되지 않았을까요?
- BooleanGenerator가 인터페이스니 이를 구현한TrueGenerator와 FalseGenerator를 만들어서 테스트에 사용했어도 됐을 것 같네요 (테스트 패키지에)
|
|
||
| public interface Task { | ||
|
|
||
| static <T> T retryUntilSuccess(Supplier<T> supplier, OutputView outputView) { |
There was a problem hiding this comment.
try-catch 문을 통한 예외처리에 대해 빙봉이 참고용으로 보내준 리뷰를 통해 새로운 예외 처리 방식에 대해 배울 수 있었습니다.
예외가 발생되지 않을 때까지 반복을 통해 수행하도록 하는 함수형 인터페이스를 생성해 주었습니다.
👍 Task 라는 클래스 이름보다는 좀 더 적절한 이름이 있을 것 같네요ㅎㅎ
| gameResult = new GameResult(ladderResult); | ||
| } | ||
|
|
||
| @DisplayName("요청 값으로 all을 전달한 경우 전체 결과를 반환한다.") |
There was a problem hiding this comment.
지난번 리뷰 요청 때 질문드린 내용에서 equals에 대해 도메인에서 재정의하면 될 것 같다는 답변을 받았습니다.
그래서 이번 리뷰 적용에서 이를 적용해 Result에 equals와 hashcode를 재정의해 테스트를 진행했습니다.
그런데, 해당 메서드는 테스트 이외에서는 사용하지 않는 것을 확인할 수 있습니다.
이러한 경우에도 equals 메서드를 재정의하는 것이 맞는지 궁금합니다.
이쪽에 코멘트 답변 달았습니다 🙇 (링크)
| @@ -11,6 +13,7 @@ public class Person { | |||
|
|
|||
| public Person(String name) { | |||
| validateName(name); | |||
| name = name.trim(); | |||
There was a problem hiding this comment.
Java 11 이라면 strip()이라는 메서드를 사용해도 좋을 것 같습니다. 더 많은 공백 문자를 제거해줍니다.
| private String reformatStatus(boolean status) { | ||
| if (LineStatus.getStatus(status) == LineStatus.CONNECTED) { | ||
| return CONNECTED_LINE; | ||
| } | ||
| return DISCONNECTED_LINE; | ||
| } |
There was a problem hiding this comment.
- LineStatus에 대해 get하지 않고 메시지를 물어보는 형태로 작성해도 좋을 것 같습니다. LineStatus.isConnected(status)
- 더불어
CONNECTED_LINE이것도 LineStatus 도메인에 특정된 값이라고 생각해서 LineStatus.display 이런 식으로 필드를 하나 추가해줘도 좋을 것 같네요.
안녕하세요 빙봉!
1단계 리뷰 정말 감사합니다.
덕분에 여러 고민을 해볼 수 있었습니다.
이번 리뷰도 잘 부탁드립니다!
아래는 2단계를 진행하며 고민하게 된 사항들입니다.
또한, 아직 제가 만든 구조에 대해 계속해 의문이 드는 상태입니다.
추천하는 방향에 대한 의견이 있다면 알려주시면 감사하겠습니다!
좀 더 생각해봐야 할 문제
View에서 필요한 형태로 반환하기
도메인을 OutputView에서 출력할 때 필요한 형태로 변환해 주는 기능에 대한 역할 부여를 어디에 해야 할지 고민인 상태입니다.
처음에는 도메인 자체에서 값을 반환할 때 변환해 전달해 주었습니다.
하지만, 해당 기능은 단지 출력해 주기 위한 기능이라는 생각이 들어 도메인에서는 값을 처리 없이 반환하도록 수정했습니다.
그리고 게임 전체를 관리하는 도메인을 생성해 해당 도메인에서 모든 값 변환을 수행해 주었습니다.
그런데 제가 역할을 제대로 부여했는지 잘 모르겠습니다.
또한, 1단계 리뷰 중
“OutputView가 도메인을 모른다면 훨씬 유연해질 수 있을 것 같은데요."라는 의견을 주셨는데,제가 수행한 방식이 생각한 방식이 맞는지 궁금합니다.
저장하지 않고 검증만을 위한 객체
기존 제 코드에서는 Height과 Weight을 하나의 도메인에서 사용했었습니다.
하지만, 이번에 코드가 변경되며 Height과 Weight을 사용은 하지만, 굳이 저장까지는 필요 없어진 상태입니다.
일단 검증 로직을 위해 각각의 도메인을 남겨 두었는데, 저장까지 필요가 없으니 validator 클래스를 생성해 검증만 수행하는 클래스로 변경해 주는 게 좋을지 궁금합니다.
보고 싶은 사람에 대한 입력 판단의 위치
보고 싶은 사람을 입력할 때 사람의 이름 혹은 ‘all’이라는 문자열을 입력하게 됩니다.
현재 저는 한 사람에 대한 결과를 원하는지 전체 결과를 원하는지에 대한 판단을 컨트롤러에서 수행하고 있습니다.
그런데 과연 해당 판단을 수행하는 것이 컨트롤러의 역할이 맞는지에 대한 의문이 들고 있습니다.
해당 판단에 대해선 어느 위치에서 수행하는 것이 이상적일지 궁금합니다.
객체를 저장하는 도메인에 대한 테스트
도메인 중
GameResult는 List 형태의 값을 저장하고 있습니다.해당 도메인에 대한 테스트로 객체를 제대로 생성했다는 테스트 및 값을 제대로 가져오는지에 대한 수행을 하려고 합니다.
그런데 해당 도메인에서 getter 수행 시 List 형태로 바로 반환하도록 했습니다.
그런데 값을 제대로 가져오는지 확인하기 위해선 Person 객체로 검사해야 하는데 new Person()을 수행하더라도 이는 다른 객체로 인식되기에 검증이 어려운 상태입니다.
이러한 경우 어떻게 검증할 수 있는지 궁금합니다.
또한,
Results에 대한 테스트를 수행할 때 역시 값이 제대로 저장되는지 확인하려고 합니다.이 역시 값이 아닌 객체를 반환하는 메서드이기에 아래와 같이 검증을 수행했습니다.
그런데 이와 같이 Result 도메인에 대한 메서드를 수행하면서 테스트를 해도 무관한 것인지 궁금합니다.
Assertions.*assertThat*(results.getResult().get(0).getResult()).isEqualTo("a");아래는 제가 아직 추가하지 못한 기능 혹은 좀 더 고민하고 싶어 적용하지 못한 사항입니다.
이번 pr 리뷰 적용과 함께 추가적으로 적용할 수 있도록 하겠습니다.
혹시 이와 함께 고민해 보면 좋은 점들이 있다면 알려주시면 함께 고민해 보도록 하겠습니다!
아직 추가하지 못하거나 고민 중인 것들