-
Notifications
You must be signed in to change notification settings - Fork 252
[번외편 - 처음부터 다시하는 사다리 게임] 로빈(임수빈) 미션 제출합니다. #411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: robinjoon
Are you sure you want to change the base?
Conversation
Gomding
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 로빈!
추가적인 도전 너무 멋지네요 👍
질문에 대한 답변과 함께 몇가지 코멘트 남겼으니 확인해주세요! 🙏
| @@ -0,0 +1,14 @@ | |||
| package domain; | |||
|
|
|||
| record Gift(String name) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Record를 도메인으로 사용하는 것이 적절할까?
Record는 단순히 값을 감싸는 클래스(=DTO)의 불필요한 코드를 없애기 위해 등장한 것으로 이해했습니다. 그런데, 이번에 코드를 작성하면서 도메인인 Point를 Record 로 작성했습니다.
Record 가 DTO 를 위해 추가된 문법이니까 도메인으로 사용하면 오해를 불러올 수 있는 건 아닐까? 라는 생각이 들면서도, 그렇다고 필요 없는 코드를 작성하는 것은 낭비 아닌가? 라는 생각도 들었습니다.
결론: 저는 Record를 도메에서 사용해도 된다고 생각해요~
Record 가 DTO를 위해 추가됐다는것을 저는 찾지못했어요.
많은 블로그에서 그렇게 말하고있는데 공식문서에서 콕 집어서 DTO를 위해 만들었다 라고 명시된 건 없어서요..
(java.lang 패키지에 있는 Record 클래스의 주석을 참고했어요)
혹시 DTO를 위해 만들어졌다고 써있는 공식문서가 있다면 공유부탁드려요! 🙏
(진짜 발견못해서 궁금해서요! 😢)
record는 고정된 값(fixed value)을 가지는 객체를 의미하고 있는것같아요!
대표적으로 DTO, VO 가 있겠네요
+) 오히려 DTO는 equals 나 hashCode가 필수가 아니라고 생각해서 VO에 더 어울리지 않은가 싶어요 ㅎㅎ..
DTO 나 VO 같은 코드를 작성해보면 필요한 코드지만 반복되는 코드(getter, equals, hashCode)가 많은데요
반복적인 작업이라 귀찮고 실수가 발생할 수 있는 부분이죠 ㅎㅎ...
그런 의미에서 로빈의 말대로 필요 없는 코드를 작성하는 것은 낭비 아닌가? 를 좋은 고민이라 생각해요 👍
같은 고민에서 record가 탄생했겠죠~ 😄
저는 필요 없는 코드 보다는반복적으로 발생하는 코드(보일러 플레이트 코드) 에 의한 시간낭비, 실수등을 제거하기 위해라고 생각해요
(오히려 필요없는 코드가 자동으로 생성된다면 record 사용을 더 고민해볼 것 같아요. 필요없는 코드가 생기는 것은 막고싶거든요!)
우리는 Name, Point 같은 값을 담은 객체(VO)를 만드는데 getter, equals, hashCode 같은 코드가 필요한데
다른 개발자는 이 객체가 값 객체인지 뭔지 알기위해 equals, hashCode를 봐야 파악할 수 있어요
그런데 record만 보면 값 객체인지 판단할 수 있으니 코드 가독성에도 도움되겠네요!
+) 보일러 플레이트 코드가 클래스내에 없으니 중요한 메서드들만 남아 그것들만 읽으면 되겠네요 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 DTO를 위해 만들어졌다고 써있는 공식문서가 있다면 공유부탁드려요! 🙏
(진짜 발견못해서 궁금해서요! 😢)
그런데, DTO랑 VO랑 다른 거였군요... 같은 말인 줄 알았어요! 저는 저 문서를 보고 DTO를 위해 만들어진 것이라 생각했는데, VO를 위해 만들어졌다는 말이 더 맞는 것 같네요.
결론적으로 DTO에 Record를 사용하던, 도메인에 사용하던 보일러 플레이트 코드를 줄일 수 있다면 적극적으로 사용하는 것을 고려해야겠네요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞습니다 ㅎㅎ
시간이 되시면 DTO vs VO 도 학습해보면 좋겠어요!
| |-----|-----| | ||
| ``` | ||
| 2. 사다리의 높이는 최소 2, 최대 20이다. | ||
| 3. 두 참여자 사이에는 최소한 한개 이상의 발판이 있어야 한다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이번에 추가하신 요구사항 맞을까요? 😃
도전하는것 너무 좋네요 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이번에 추가하신 요구사항 맞을까요? 😃 도전하는것 너무 좋네요 💯
넵 뭔가 그대로 하는 것 보단, 뭐라도 좀 어렵게 해보는게 좋을 것 같아서 추가해봤습니다!
| () -> new Line(true, true, false)) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessage("|-----|-----| 연결 감지!"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TDD 사이클을 잘 지키고 있는건지 궁금합니다.
문득, 이번 미션의 주제인 TDD 사이클을 잘 지키고 있는건지 궁금해져서 TDD 사이클 단위로 커밋을 해봤습니다.불완전한 상태인 커밋이 존재하는 것은 협업에 있어서 문제가 될 수 있기 때문에 지양하는것이 좋다는 것은 인지하고 있습니다.
정답이 없는 부분이라 생각해요!
요구사항을 검증하는 테스트를 작성 -> 테스트 실패 -> 요구사항을 구현 -> 테스트 성공
위 순서대로 잘 실행했냐고 질문주신거라면
커밋 내역을 봤을때 잘 지켰다고 생각해요 😄
src/main/java/domain/LadderGame.java
Outdated
|
|
||
| public List<String> getRawNames() { | ||
| return names.getRawNames(); | ||
| private static Ladder makeLadder(Supplier<Boolean[]> lineMakeStrategy, int ladderHeight) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
정적 팩토리 메서드
다른 크루들의 PR을 보면, 정적 팩토리 메서드를 적극적으로 활용한 것이 많았습니다. 그래서 우선 저도 한번 도입해 봤습니다.그 결과, 다음과 같은 궁금증이 생겼습니다.
정적 팩토리 메서드가 정말 생성자보다 유의미한 메세지를 전달할 수 있는가?
생성자 자체가 다른 객체나 값을 사용해 객체를 생성한다. 라는 메세지를 충분히 전달한다는 생각이 들었습니다.
저도 생성자가 가진 필드외에 다른 값으로 객체를 생성할 때 정적 팩토리 메서드를 유용하게 사용하고있어요! 😃
정적 팩토리 메서드에서 검증을 하는 것이 좋은가 아니면, 생성자에서 검증을 하는 것이 좋은가?
어차피 정적 팩토리를 통해 객체가 생성된다면, 객체의 생성과 객체의 생성을 위한 매개변수의 검증을 분리하는 관점으로 접근해 더 나은 코드가 될 수 있지 않을까?
새로운 팩토리 메서드가 추가될 때 객체가 생성될 때 필요한 검증이 누락될 가능성이 있다.
저는 객체가 가진 필드에 대해서는 생성자에서 검증을 하는 것이 좋다고 생각해요!
어차피 정적 팩토리 메서드도 생성자를 호출하게 될테니까요 😄
그 외에 정적 팩토리 메서드에서는 필드에 대한 값이 아닌 다른 타입의 값을 인자로 받는 경우도 많은데
다른 타입의 값에대한 검증이 필요하고 이를 생성자의 책임으로 미룰 수 없을 때 팩토리 메서드에서 검증을 진행하고 있어요
새로운 팩토리 메서드가 추가될 때 객체가 생성될 때 필요한 검증이 누락될 가능성이 있다.
이 부분은
다른 팩토리 메서드가 추가될 때 기존에 팩토리 메서드를 주 생성자처럼 호출할 수 있는지
또는 같은 validate 메서드를 호출할 수 있는지 잘 살펴봐야 될것같네요~
| public List<List<Boolean>> getRawLadder() { | ||
| return ladder.getRawLadder(); | ||
| public LadderGameResults start(String operator) { | ||
| if (operator.equals("all")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all 이라는 값은 view에 존재하는 값이 아닐지 의심해보아요!
현재는 Console Application 이지만
Web Application 으로 옮겨간다면 all 이라는 커맨드가 유지될까요? 🤔
| List<Point> points = new ArrayList<>(); | ||
| addFirstPoint(points, canGoRights); | ||
| addRemainPoints(points, canGoRights); | ||
| this.points = Collections.unmodifiableList(points); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
points 를 불변 컬렉션으로 만들어서 변경되지 않는다는걸 표현해주신걸까요? 😮 💯 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
points 를 불변 컬렉션으로 만들어서 변경되지 않는다는걸 표현해주신걸까요? 😮 💯 💯
네 맞습니다. 사다리 타기 게임의 특성 상 맨 앞의 Point 와 나머지 위치의 Point가 고려해야 할 것이 달라서, Point를 생성해 컬렉션에 넣는 메서드를 별도로 만들게 되었고, 자연스럽게 가변 컬렉션을 사용해야 했습니다. 그래서, 모든 Point 를 생성한 직후 불변 컬렉션으로 감싸도록 했습니다!
src/main/java/domain/LadderGame.java
Outdated
|
|
||
| public List<String> getRawNames() { | ||
| return names.getRawNames(); | ||
| private static Ladder makeLadder(Supplier<Boolean[]> lineMakeStrategy, int ladderHeight) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LadderGame 의 makeLadder 메서드는 Ladder의 정적 팩토리 메서드로 보이는데
로빈은 어떻게 생각하시나요? 🤔
그렇다면 Ladder에 위치해도 좋을것같아요 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LadderGame 의 makeLadder 메서드는 Ladder의 정적 팩토리 메서드로 보이는데 로빈은 어떻게 생각하시나요? 🤔
그렇다면 Ladder에 위치해도 좋을것같아요 😄
정적 팩토리 메서드가, 필드가 아닌 값으로 객체를 생성할 때 라는 케이스에 적합한지는 몰랐네요. 말씀대로 해보겠습니다!
|
|
||
| private List<Boolean> generateBridges(int width, Random random) { | ||
| return IntStream.range(0, width) | ||
| private Boolean[] generateBridges(Random random) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
인자로 Random 객체를 받는 이유가 궁금해요!
메서드 내부에서 생성가능해 보여서요 🤔
| } | ||
|
|
||
| public Boolean[] makeLine() { | ||
| return generateBridges(new Random()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makeLine 호출 -> gerateBridges 호출
이런 흐름인데 makeLine은 꼭 필요한 메서드 였을까요 🤔
src/main/java/Main.java
Outdated
| .players(playerNames.toArray(String[]::new)) | ||
| .gifts(giftNames.toArray(String[]::new)) | ||
| .ladderHeight(ladderHeight) | ||
| .ladderMakeStrategy(randomLineMakeStrategy::makeLine) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
randomLineMakeStrategy 객체를 넘기지않고 함수형 인터페이스를 활용한 이유가 궁금해요!
어떤 장점이 있었을까요? 🤔
저는 RandomLineMakeStrategy 를 interface화 하여 넘기는 것이 더 가독성이나 코드 안정성이 좋다고 생각했어요
ladderMakeStrategy 의 인자로 RandomLineMakeStrategy 의 interface 를 들어가보면 어떤 내용이 있는지 파악 가능하고
해당 interface 타입만 들어온다는걸 �강제할 수 있다는 점에서요!
|
가변 배열 관련, RandomLineMakeStrategy 커스텀 인터페이스 정의하는 부분 반영했습니다! 이번에 인터페이스에 봉인 관련 기능들을 사용해 봤는데요, 인터페이스만으로 충분하지 않은 규약을 추상 클래스에서 더 상세하게 강제할 수 있도록 했습니다. |
Gomding
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 로빈!
sealed 를 활용해주신게 인상깊네요 😄 💯
몇 가지 코멘트 남겼으니 의견남겨주시면 좋겠어요!
궁금한 점 있으면 언제든 DM 남겨주세요~!
| boolean thatContainsThis = new HashSet<>(that.ladderGameResults).containsAll(ladderGameResults); | ||
| boolean thisContainsThat = new HashSet<>(ladderGameResults).containsAll(that.ladderGameResults); | ||
| return thatContainsThis && thisContainsThat; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
equals 만 재정의 하신 이유가 궁금해요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
equals 만 재정의 하신 이유가 궁금해요!
hashCode 는 레코드니까, 알아서 잘 만들어진다고 생각했습니다. 그런데 생각해 보니 equals 가 재정의 되었으니, hashCode 의 구현도 변경될 필요가 있을 것 같네요!
| List<Boolean> rawBridges = generateBridges(width, random); | ||
| fixInvalidBridges(width, rawBridges); | ||
| return rawBridges; | ||
| public class RandomLineMakeStrategy { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용하지 않는 클래스라 지워도 되겠군요!
|
|
||
| import java.util.List; | ||
|
|
||
| public sealed interface LineGenerateStrategy permits AbstractLineGenerateStrategy { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sealed 활용 👍 👍 👍
| if (canGoRights.get(index - 1) && !canGoRights.get(index)) { | ||
| points.add(new Point(Direction.LEFT, index)); | ||
| } | ||
| if (!canGoRights.get(index - 1) && !canGoRights.get(index)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if 조건문 각각이 무엇인지 현재는 바로 파악하기가 힘들것같아요 🤔
private 메서드로 추출하여 이름을 부여하면 가독성이 올라가는 효과가 있어요!
|
|
||
| private void validateContinuousRight(int index, List<Boolean> canGoRights) { | ||
| if (canGoRights.get(index - 1) && canGoRights.get(index)) { | ||
| throw new IllegalArgumentException("|-----|-----| 연결 감지!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
view의 형태가 들어간 예외메시지는 아닌지 걱정되는군요!
다른 메시지로 표현할 수도 있곘어요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
view의 형태가 들어간 예외메시지는 아닌지 걱정되는군요! 다른 메시지로 표현할 수도 있곘어요!
예외 메세지를 연속된 다리가 탐지되었습니다! 등의 표현으로 한다면, 이해하기 힘들 것 같아서 최대한 직관적으로 이해할 수 있는 메세지로 만들었습니다. 뷰랑 형태가 똑같지만, 뷰가 변경된다고 예외 메세지가 달라질 것이라 생각하지 않습니다.
| climbResults.forEach(OutputView::print); | ||
| private static LadderGame makeLadderGame(List<String> playerNames, List<String> giftNames, Integer ladderHeight, | ||
| LineGenerateStrategy randomLineMakeStrategy) { | ||
| return LadderGameBuilder.builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LadderGameBuilder 가 외부로 노출되었는데
LadderGame.builder() 로 변경해보면 어떨까요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LadderGameBuilder 가 외부로 노출되었는데 LadderGame.builder() 로 변경해보면 어떨까요!
무슨 말인지 잘 모르겠습니다... 빌더는 외부로 노출되어야 하는 것이 아닌가요???
| endPosition = row.move(endPosition); | ||
| } | ||
| return endPosition; | ||
| public static Ladder of(LineGenerateStrategy lineGenerateStrategy, int ladderHeight, int lineSize) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ladder 의 정적 팩토리 메서드를 클래스 가장 아래에 위치시켜준 이유가 궁금해요!
| validateRightAfterEnd(canGoRights); | ||
| List<Point> points = new ArrayList<>(); | ||
| addFirstPoint(points, canGoRights); | ||
| addRemainPoints(points, canGoRights); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
초기화할 때 어떤 순서로 진행되는지 한 눈에 보여서 좋네요 👍
|
|
||
| private void validateNameBlackList(String name) { | ||
| if (name.equals("all")) { | ||
| throw new IllegalArgumentException("참가자 이름은 all이 될 수 없습니다."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
꼼꼼한 검사 👍
| Assertions.assertThat(length) | ||
| .isEqualTo(5); | ||
| // | | | |-----| | ||
| Line line2 = Line.from(false, false, false, true, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 가독성까지 신경써주셨네요 ❤️
안녕하세요! 네오의 피드백 강의와 다른 크루들의 PR을 보고 학습한 내용을 담아 처음부터 다시 구현을 해봤습니다!
확인해 주셨으면 하는 포인트
아래 질문들에 대해 찰리의 의견을 듣고 싶습니다.
TDD 사이클을 잘 지키고 있는건지 궁금합니다.
문득, 이번 미션의 주제인 TDD 사이클을 잘 지키고 있는건지 궁금해져서 TDD 사이클 단위로 커밋을 해봤습니다.
불완전한 상태인 커밋이 존재하는 것은 협업에 있어서 문제가 될 수 있기 때문에 지양하는것이 좋다는 것은 인지하고 있습니다.
정적 팩토리 메서드
다른 크루들의 PR을 보면, 정적 팩토리 메서드를 적극적으로 활용한 것이 많았습니다. 그래서 우선 저도 한번 도입해 봤습니다.
그 결과, 다음과 같은 궁금증이 생겼습니다.
정적 팩토리 메서드가 정말 생성자보다 유의미한 메세지를 전달할 수 있는가?
생성자 자체가 다른 객체나 값을 사용해 객체를 생성한다. 라는 메세지를 충분히 전달한다는 생각이 들었습니다.정적 팩토리 메서드에서 검증을 하는 것이 좋은가 아니면, 생성자에서 검증을 하는 것이 좋은가?
Record를 도메인으로 사용하는 것이 적절할까?
Record는 단순히 값을 감싸는 클래스(=DTO)의 불필요한 코드를 없애기 위해 등장한 것으로 이해했습니다. 그런데, 이번에 코드를 작성하면서 도메인인 Point를 Record 로 작성했습니다.
Record 가 DTO 를 위해 추가된 문법이니까 도메인으로 사용하면 오해를 불러올 수 있는 건 아닐까? 라는 생각이 들면서도, 그렇다고 필요 없는 코드를 작성하는 것은 낭비 아닌가? 라는 생각도 들었습니다.
View에서 검증한 것을 도메인에서 또 검증해야 하는가? 검증해야 한다면, 검증 코드를 재사용 해야 하는가?
View에서 검증한 것을 도메인에서 또 검증해야 하는가?
검증해야 한다면, 검증 코드를 재사용 해야 하는가? 에 대해선 다음과 같은 생각을 할 수 있을 것 같습니다.
이번에는 저번 구현에서와는 다른 것을 경험해 보고 싶어서 중복해서 검증하고, 각 영역에서의 검증 코드를 따로 작성했습니다.