[2단계 - 자동차 경주 리팩터링] 저문(유정훈) 미션 제출합니다. #587
Merged
Conversation
This file contains 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
iphwade
requested changes
Feb 12, 2023
There was a problem hiding this comment.
step2 잘 구현해주셨습니다-!
step2에서 지켜야할 요구사항은 기존에도 잘 구현해주셔서 잘해주셨네요-!
저문께서 작성하신 리팩토링에 근거에 대해서 말씀주실 수 있으실까요?
- getter, equals, hashcode 등의 위치
- 불변 객체 사용 이유
- Position, Name을 클래스화한 이유
또, step1 리뷰에서 전달드렸던 내용들도 적용해주시면 감사하겠습니다-!
질의응답
-
movingTrial을 컨트롤러에서 받아서 도메인으로 넘겨서 처리를 하고 싶습니다.
RacingGame 객체가 라운드 회수를 가지고 있도록 구현해보고 메세지를 던져보도록 구현하는 것은 어떨까요?
큰 틀에서의 코드 변화는 없겠지만 그래도 라운드 회수를 카운팅하는 로직이 controller에 있지 않게 구현할 수 있어보입니다. -
position의 경우 증가할 때마다 객체가 생성된다면, 단순히 1을 더해주는 로직에 비해 효율적이지 않다고 생각합니다.
이 내용은 불변객체가 왜 필요한지에 대해 학습해보시면 좋아보입니다-! :)
| } | ||
|
|
||
| public Position increase() { | ||
| return new Position(this.position + MOVEMENT); |
There was a problem hiding this comment.
Position을 불변하게 변경하셨군요! 어떤 이유로 선택하셨나요~?
iphwade
approved these changes
Feb 13, 2023
Comment on lines
-22
to
26
| public void race(boolean isMovable) { | ||
| public void race(NumberGenerator numberGenerator) { | ||
| for (Car car : cars) { | ||
| car.move(isMovable); | ||
| car.move(numberGenerator); | ||
| } | ||
| } |
There was a problem hiding this comment.
NumberGenerator를 구현하여 모든 자동차가 이동했는지를 테스트할 수 있어보입니다.
There was a problem hiding this comment.
아래 getCars() 메서드보다 검증 메서드 setUp 메서드가 더 중요해보입니다-!
| @@ -28,8 +28,4 @@ public RacingResult produceRacingResult() { | |||
|
|
|||
| return new RacingResult(history); | |||
| } | |||
|
|
|||
| private boolean isMovable() { | |||
| return (numberGenerator.generate() >= MOVABLE_CONDITION); | |||
| @@ -17,7 +17,7 @@ public RacingGame(CarGroup group, RandomNumberGenerator randomNumberGenerator) { | |||
|
|
|||
| //TODO: 테스트 | |||
| public void race() { | |||
| carGroup.race(isMovable()); | |||
| carGroup.race(numberGenerator); | |||
| } | |||
|
|
|||
| public RacingResult produceRacingResult() { | |||
|
|
||
| @Override | ||
| public int generate(){ | ||
| return (int)(Math.random() * 10); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
안녕하세요 코일! 저문입니다 :)
2단계 요구사항에 대해서 리팩터링 해보았습니다.
MVC구조의 역할에 집중하면서 구조에 대한 변경을 중점적으로 해보았고,
원시 값을 포장하는 것도 추가적으로 진행해보았습니다.
2단계를 진행하던 도중 궁금한 점이 생겨서 질문을 남깁니다!
먼저, 전체적인 로직에 관한 내용입니다.
총 시도횟수를 movingTrial로 받고 있는데, 이 로직이 도메인에서 처리되도록 변경하고 싶습니다.
하지만 출력과 연관되어 있는 바람에 controller에서 처리하는 방법을 선택했었습니다.
movingTrial을 컨트롤러에서 받아서 도메인으로 넘겨서 처리를 하고 싶습니다.
어떤 방법을 사용하면 효율적으로 도메인으로 넘겨서 로직을 처리할 수 있을지 궁금합니다.
두번째, 값 객체로 리팩터링을 진행하던 도중 궁금한 점이 생겼습니다.
Car객체 같은 경우에는 name과 position을 가지는데 name은 변하지 않지만,
position은 증가로직이 필요하게 됩니다.
여기서 position을 값 객체로 사용하게 되면 값이 증가될 때마다 new를 통해 새롭게 객체를 생성해야합니다.
제가 생각했을 때 값을 포장하는 것도 중요하지만, 메모리를 효율적으로 사용하는 것도 중요하다고 생각합니다.
position의 경우 증가할 때마다 객체가 생성된다면, 단순히 1을 더해주는 로직에 비해 효율적이지 않다고 생각합니다.
이런 상황에서도 값 객체를 사용하는 것이 더 큰 이점이 있을까요?
2단계 리뷰도 잘 부탁드립니다!
감사합니다 :)