Conversation
seonghun127
reviewed
Nov 17, 2021
seonghun127
left a comment
seonghun127
left a comment
There was a problem hiding this comment.
구현하느라 고생많았습니다~
view와 도메인간의 의존관계를 완전히 끊었고 MVC 패턴을 적절하게 구현했다고 생각합니다.
몇가지 코멘트 남겼으니 확인부탁드려요!
추가로 하나 더 고민해보면 좋을 것이
저번 베이스볼 코드랑 전체적인 포맷은 매우 비슷해서 다른 구조로도 한번 생각해보면 좋을 것 같습니다.
베이스볼과 지금 구현 포맷을 보면 아래와 같은데
- 게임 -> 라운드 -> 도메인 로직 (요구사항에 맞춰 객체 생성 및 메세지 상호작용)
여유가 된다면 다른 구조도 한번 생각해보는 것을 추천드립니다.
| return generateRandom(); | ||
| } | ||
|
|
||
| protected boolean generateRandom() { |
|
|
||
| public class RandomMovementStrategyStrategy implements MovementStrategy { | ||
|
|
||
| private final Random random = new Random(); |
There was a problem hiding this comment.
Random 객체가 딱한번 초기화돼서 사용되는 걸로 보이는데 상수로 바꾸는건 어떨까요?
| testImplementation "org.junit.jupiter:junit-jupiter:5.7.2" | ||
| testImplementation "org.assertj:assertj-core:3.19.0" | ||
| testImplementation 'org.junit.jupiter:junit-jupiter:5.8.1' | ||
| testImplementation 'org.assertj:assertj-core:3.21.0' |
| .stream() | ||
| .map(Car::new) | ||
| .collect(Collectors.toUnmodifiableList()); | ||
|
|
| @CsvSource(value = {"name,name,true", "name,other,false"}) | ||
| @DisplayName("이름이 같으면 같은 해시값을 반환하고, 다르면 다른 해시값을 반환한다") | ||
| void hashCode(String value, String otherValue, boolean expected) { | ||
| //given |
There was a problem hiding this comment.
IDE로 자동화할 수 있습니다.
| Round round = initialRound; | ||
|
|
||
| while (round.isOver()) { | ||
| round = round.play(movementStrategy); |
There was a problem hiding this comment.
프로그램 전반적으로 객체의 불변성을 계속 유지했는데, 여기도 Round 객체를 불변으로 가져가면 어떨까요?
| public class GameController { | ||
| private final InputView inputView; | ||
| private final OutputView outputView; | ||
| private final MovementStrategy movementStrategy; |
There was a problem hiding this comment.
GameController가 MovementStrategy 필드를 가져야할까요?
결국 round 객체에 바이패스될텐데 그렇다면 구체적인 구현체를 직접 round에 직접 전달해도 되지 않을까 싶어요.
어떻게 생각하시나요?
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
No description provided.