Skip to content
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

[민형] 레이싱 게임 #17

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

[민형] 레이싱 게임 #17

wants to merge 7 commits into from

Conversation

Gobukgol
Copy link

@Gobukgol Gobukgol commented May 2, 2020

No description provided.

2. RacingGameResult 제거, 각 이동 시도 후 결과 출력하도록 변경
3. InputValueDto 추가, 정수형 인자값 validate
1. Service, Controller 붙어있는 클래스명 변경(웹 계층 -> 도메인)
2. 각 이동 결과를 가지고있는 RacingResult.java 추가
3. 각 RacingResult.java 를 가지고있는 GameResult.java 추가
1. 각 시도 결과를 가지고 있는 PhaseResult 추가
   - 각 PhaseResult는 자동차의 이름을 Key, 이동 결과를 Value로 하는 Map을 가지고 있음
   - PhaseResult는 현재 선두의 이름을 List의 형태로 반환하는 메소드를 가지고있음
2. PhaseResult들을 가지고있는 총 게임 결과 GameResult 추가
   - 시도 번호로 각 시도 결과를 가져올 수 있음
   - 게임 우승자는 마지막 PhaseResult의 선두의 이름을 가져옴
3. ParticipateCars 의 주석은 어떤게 더 보기좋은지 궁금해서 주석 처리하였음
@pci2676 pci2676 requested a review from chldbtjd2272 May 2, 2020 03:41

import racing.domain.common.NumberGenerator;

public class RacingPhase {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 객체의 의도는 뭐지?
필요없어보이는 객체같은데



public class PhaseResult {
private final Map<String, Integer> raceResults;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PhaseResult를 만들어 매 레이싱의 경기결과를 분리해 순위를 찾을수 있는 로직을 넣어놓는건 굉장히 좋은거같아.
한가지 아쉬운건 원소값만 담은 맵을 사용한 점이 아쉬워, Map이든 List든 자기 거리를 알 수 있는 자동차 객체를 넣어놓으면 더 표현하기도 쉽고 getCurrentLeads()함수가 저렇게 복잡하지 않았을거 같아

CarResult 라는 객체로 결과의 리스트를 들고있었으면 CarResult에 Comparable을 구현하면 순위별로 정렬하기도 훨씬쉬워질거고,
의미도 더 명확해질거같아

.collect(Collectors.toList());
}

public Map<String, Integer> tryMoveWithName(NumberGenerator numberGenerator) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PhaseResult를 리턴하는게 더 좋아 보여

@@ -0,0 +1,21 @@
package racing.domain.common;

public class FixedNumberGenerator implements NumberGenerator {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트용 스텁객체는 테스트 패키지로 옮기는게 좋아

그리고 generateNumber를 호출할때마다 바뀌는 환경보단 FixedNumberGenerator를 만들때 어떤식으로 움직일지 미리 주입시켜 놓고 동작하는게 훨씬 직관적일거같아.

FixedNumberGenerator {
public FixedNumberGenerator(List moveCondition){
this.moveCondition = moveCondition;
}
}


public class RacingGame {
private final RacingPhase racingPhase;
private final InputValueDto inputValueDto;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지금 보면 도메인이 dto를 의존하는 형태니 dto를 도메인객체가 멤버변수로 들고있는건 너무 별로야

ParticipateCars participateCars = InputValueDto.getParticipateCars();
int numberOfAttempts = InputValueDto.getNumberOfAttempts();

new RacingGame(participateCars,numberOfAttempts) 형태가 더 좋아보여, 아니면 게임 정보를 담는 객체를 도메인 레이어에 하나 만들고 RacingGame이 들고있어도 나쁘지않을거같고

cars = createCars(namesOfCars);
}

public List<Integer> tryMove(NumberGenerator numberGenerator) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지금 이름하나 추가되었다고 tryMoveWithName 함수가 하나 생겼는데 자동차 하나하나에 대한 결과 객체를 만들었다면 함수가 추가되지않고 자동차 단일객체에 이름만 추가 할 수 있었을거야,

PhaseResult 를 리턴하되 PhaseResult안에 들고있는 자동차에 대한 결과들도 객체로 분리해서 만들어봐

Gobukgol added 2 commits May 14, 2020 20:45
1. 각 자동차가 이동 후 결과를 가지고있는 VO CarResult 구현
2. InputValueDto 대신 게임 정보를 가지고있는 GameInformation 로 변경
1. 패키지 구조 변경
CarResult target = (CarResult) o;

int compare = Integer.compare(target.distance, this.distance);
//
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주석 지우기 >.0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integer.compare 문 바로 return 하기 >.0

cars = createCars(namesOfCars);
}

public List<CarResult> moveCars(NumberGenerator numberGenerator) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 좋다 b


public PhaseResult findRacingResultByPhase(int phase) {
try {
return phaseResults.get(phase - 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이건 취향인데, 이 find를 쓰는 곳이 outputView인데, outputView에서는 저 phase -1 때문에 i 를 1부터 시작하더라고 0부터 시작하지 않은 이유가 있어?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음 배열이나 List의 시작 index가 0인건 프로그래머는 알고있지만 게임을 하는 사용자 입장에서는 0이 시작이라고 생각하지않고 1이라고 생각한다고 느껴서 일단은 그렇게 했어!



public class GameResult {
private final List<PhaseResult> phaseResults;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이게 결국 이력관리를 하고있다는거에서 놀람 굿

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants