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

[step3] 자동차 경주 리뷰 부탁드립니다 #1994

Merged
merged 12 commits into from Mar 14, 2021

Conversation

loop-study
Copy link

안녕하세요 박현철입니다

이번에 step3 자동차 경주 진행하면서 재미가 붙어 추가적인 기능 요구사항을 몇개 생각했다가(레이싱카 주행 중 사고, 타이어교체 피트스톱 등)
간단한 걸로 추가해서 진행했습니다.

진행하면서 의문사항이 생겨 의견 여쭤드립니다.

  1. 처음에 테스트로 진행 중일 때는 랜덤값 사용하는 여러개의 메소드(ex:formulaOneMove) 경우에 인자로 직접 넣어줬지만.
    구현 후에는 랜덤값을 인자로 안 넣고 메소드 내부에서 랜덤을 호출하는 형식으로 변경했습니다.
    이럴 경우 처음에 진행한 테스트코드가 없어지는데 없어지는 것이 맞는지 모르겠습니다.
    테스트에서 테스트 값을 안 넣어주는 경우엔 내부에서 생성되는 랜덤값으로 어떻게 테스트 해야하는지 의문스럽고,
    메소드 외부에서 랜덤값을 생성해서 넣어주자니, 구현에서는 불필요한 행동처럼 느껴지네요.
    현재는 테스트용 메소드를 놔둔 상태입니다. 어떻게 해야할까요.

  2. 이번에 진행하면서 System.in, System.out 관련 UI 로직은 단위테스트 안해도 된다해서 안했지만 jUnit 에서 Scanner 사용해봤습니다.
    무한루프문제가 생겨서 지웠지만. 사용자가 잘못된 값을 입력했을 경우엔 어떻게 단위테스트에서 예외처리를 해야할까요?
    (임시로 구현에서 잘못된 값 입력 시 재입력하게 만들었습니다.)

  3. 힌트중에 InputView, ResultView 있어서 MVC 방식으로 접근을 했습니다.
    InputView 경우엔 사용자가 입력하는 화면으로 입력된 값을, Controller 역할을 하는 Main 메소드에서 처리 후
    Service인 GrandPix 를 호출하면, 입력된 선수만큼 Model 인 FormulaOne 을 생성하고 완주까지 턴을 진행하는데,
    사용자가에게 레이스 진행상황을 출력하는 ResultView 를 Service 에서 실행했습니다. 하지만 View 는 사용자에게 넘겨주는거라
    Main 에서 처리해야할까 고민해봤는데, 그럴 경우 Main 에서 완주까지 반복문을 Main 에서 만들어야된다는 생각에 Service 에서 실행했습니다.
    올바르게 접근했는지 궁금합니다

  4. 역할과 책임을 생각을 했지만 제가 미흡하여 올바르게 책임과 역할을 나눴는지 궁금합니다

피드백 부탁드리겠습니다
감사합니다!

2021. 03. 09
자동차 경주를 제출합니다.
테스트코드 -> 구현으로 작업했으며,
기능 요구사항을 추가하여 진행했습니다.
 - 참가 최소 2 ~ 24명 제한
 - 완주 목표 5 ~ 55 제한
 - 레이싱카가 random 최대값 9 나올 시 한칸 더 이동
 - 레이싱카 이동마다 일정확률로 타이어교체 피트스톱 추가 (패널티로 다음 이동 시 요구조건 1 증가)

Resolves : loop-study
See also : changgunyee
2021. 03. 10
ResultView 내부에 레이싱 진행 로직이 있어 잘못된 역할이라 생각되어, 해당 로직을 GrandPrix 로 옮겼습니다.
ResultView 는 순수 출력용 메소드만 남겼습니다.

Resolves : loop-study
See also : changgunyee
Copy link

@changgunyee changgunyee left a comment

Choose a reason for hiding this comment

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

안녕하세요. 현철님 😄

열심히 미션 수행하셨군요!
본격적인 자동차 경주 미션 잘 구현해주셨어요 👍

1,3번은 코드 상에 답변 드린 것 같습니다
2번은 제가 제대로 이해를 한 것인지 모르겠지만 사용자가 잘못된 값을 입력한다면 에러를 던지도록 코드를 짜고 테스트 코드 상에서 해당 인풋이 들어왔을 때 예외가 제대로 던져지는지 확인하면 될 것 같아요!

몇가지 고민해볼 부분이 있어 피드백 남기고 Request Changes 드렸습니다.

객체의 역할과 책임에 대해 생각해보세요.
이 메소드가 여기 위치하는게 맞는 것인가? 에 대해 생각해보시고 객체에 메시지를 보내는 형태로 변경해보세요.

인터페이스 기반으로 전략을 바꿔가며 테스트
테스트 하기 쉬운 부분과 어려운 부분을 나누고 적용해보세요.

비지니스 로직과 UI로직은 분리해주세요.

위의 피드백 중점으로 고민해보시고 궁금한점이나, 어려운점 있으면 슬랙을 통하여 DM 주세요. 함께 고민해보아요.🔥
리팩토링 해보시고 다시 PR 부탁드립니다~ 화이팅 입니다 🔥

src/main/java/racingcar/GrandPrix.java Outdated Show resolved Hide resolved

// 서비스 로직,
public class GrandPrix {
private ResultView resultView = new ResultView();

Choose a reason for hiding this comment

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

GrandPrix 에서 일어나야 하는 일에 대해서 생각해보세요.

  • InputView에서 Car 갯수, Round 갯수를 입력 받는다.
  • GrandPrix가 자동차 경기를 시작한다.
  • 경기 결과를 가져와 ResultView에 전달한다.
  • ResultView 가 경기 결과를 출력한다.

Record와 같은 객체를 만들어 ResultView에 전달해도 되겠군요.
간결하고 직관적으로 리팩토링 해보시면 됩니다.

@@ -0,0 +1,82 @@
package racingcar;

public class FormulaOne {

Choose a reason for hiding this comment

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

인터페이스 기반으로 전략을 바꿔가면서 테스트 할 수 있게 리팩토링 해보는건 어떨까요?

테스트 할 수 있는 부분과 테스트 할 수 없는 부분을 나누고 외부에서 조작 가능한 형태로 바꾸는 방법을 취하는 것입니다.
자동차 입장에서 랜덤하게 움직이던지, 속도를 높여 2칸씩 움직이던지 어떠한 방식으로 움직이던지 상관없이
그냥 내가 움직일 수 있는지 여부만 알면 됩니다.

자동차의 움직임을 결정하는 부분을 외부로 분리함으로써 테스트 할 수 없는 부분에 대한 테스트가 가능합니다. 😄

전략패턴 : https://lee1535.tistory.com/93
Test Stub : https://beomseok95.tistory.com/294

Copy link
Author

Choose a reason for hiding this comment

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

학습 후 진행하겠습니다!

src/main/java/racingcar/GrandPrix.java Outdated Show resolved Hide resolved
public FormulaOne(int lastLab) {
this.lastLab = lastLab;
this.labCount = 0;
this.pitstop = false;

Choose a reason for hiding this comment

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

pitstop에 대해서는 요구사항이 따로 없었는데 재미로 넣으신걸까요? 😄

Copy link
Author

Choose a reason for hiding this comment

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

한바퀴 돌 때 마다 일정확률로 타이어교체하러 피트스톱 들어간다는 식으로 추가해봤습니다.
사고 확률도 넣을까했다가 단순히 진행상황을 - 표시 외에도 피트스톱 표현, 사고발생 표현이 들어가면
제 머리속이 꼬일거 같아서 아직 추가는 안했습니다.
다음 우승자 단계에서 넣어볼 생각입니다.


private int labCount; // 현재 진행 랩
private int lastLab; // 목표 랩
private boolean pitstop; // 타이어교체 피트스톱 여부

Choose a reason for hiding this comment

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

레이싱 라운드(랩) 만을 담당하는 객체를 만들면 자동차 객체에서 랩을 세도 되지 않을 것 같아요.

FormulaOne이 자동차를 의미하는 것 같습니다.
단 랩을 세고 이동도 하느라 역할이 많아 보이는 군요.
역할에 맡게 좀 더 객체를 나누어 보는 것은 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

좀 더 분류에 맞게 정리하고 나누겠습니다

src/main/java/racingcar/InputView.java Outdated Show resolved Hide resolved
src/main/java/racingcar/README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,15 @@
package racingcar;

public class ResultView {

Choose a reason for hiding this comment

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

ResultView에서 경기 결과를 string으로 변환하여 출력해보는 것은 어떨까요?
UI로직은 UI에서만 담당하고 도메인 객체들은 비즈니스 로직만 담당하는 것이죠. 😄

Copy link
Author

Choose a reason for hiding this comment

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

최대한 노력하겠습니다!

src/test/java/racingcar/RacingCarTest.java Outdated Show resolved Hide resolved
2021. 03. 11
1) 2뎁스 이상을 1뎁스 이내로 변경
2) 자동차가 움직였는지 boolean 반환하도록 변경
3) 통제불능인 랜덤값을 외부에서 주입하는 방식으로 변경
4) 레이싱 라운드 객체(Circuit) 추가
5) ResultView 에서 String 으로 진행상황 출력하게 변경
6) 초기 작업했던 테스트코드를 현재 테스트코드와 비교하기 위해 커밋합니다.

Resolves : loop-study
See also : changgunyee
@loop-study
Copy link
Author

안녕하세요. 박현철입니다.
피드백 반영하여 수정했습니다.

  1. 전략 패턴을 사용하면서 입력된 수 만큼 자동차가 생성되어 move 를 진행합니다.
    random 값에 따라 사용되는 MoveStrategy 3가지인데 매번 자동차가 움직일때마다 인스턴스를 생성하는건 불필요한 행동같고,
    Move 전략마다 공유되어도 된다고 생각되어 static 클래스로 사용했습니다.

  2. 레이싱 라운드를 객체로 분리했습니다.

  3. 2뎁스 이상을 1뎁스로 수정했습니다.

  4. 결과 출력에서 print("-") 방식을 String 방식으로 변경했습니다.

  5. 통제불능 관련된 랜덤값을 외부에서 넣도록 변경했습니다.

  6. 처음 테스트코드를 추가했습니다, 현재의 테스트코드와 비교하기 위한 용도입니다.

잘못된 점이 있다면 피드백 부탁드리겠습니다!
감사합니다!

2021. 03. 11
누락된 실행결과 추가했습니다.

Resolves : loop-study
See also : changgunyee
2021. 03. 11
전략패턴 MoveStrategy 테스트 추가했습니다.

Resolves : loop-study
See also : changgunyee
2021. 03. 11
잘못된 변수명 수정했습니다.

Resolves : loop-study
See also : changgunyee
Copy link

@changgunyee changgunyee left a comment

Choose a reason for hiding this comment

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

안녕하세요 석희님 😄
이번에도 열심히 작업을 해주셨군요!

적어주신 구현 기능 목록이 요구사항과는 많이 다른 것 같습니다.

요구사항을 잘 반영하는 것도 하나의 역량이라고 생각합니다.

물론 구현하고 싶은 방향이 있으시겠지만 포비님과 논의 결과 미션에서의 요구사항을 잘 구현해내는 것도 중요하다는 결론이 났습니다.

한 리뷰 늦게 말씀드려 죄송하네요 🙏

그럼 미션의 요구사항에 맞게 다시 구현하여 리뷰 신청 부탁드리겠습니다 😄

감사합니다!

2021. 03. 12
기본 요구사항으로 다시 했습니다!

Resolves : loop-study
See also : changgunyee
2021. 03. 12
기본 요구사항으로 다시 했습니다!

Resolves : loop-study
See also : changgunyee
2021. 03. 12
조건에 사용되던 하드코딩 값을 상수로 변경

Resolves : loop-study
See also : changgunyee
@loop-study
Copy link
Author

안녕하세요 박현철입니다. (석희님 아닙니당 😭😭😭😭)
기존 요구사항으로 재작업했습니다!
이전은 제가 너무 복잡하게 요구사항 추가하고 하면서 머리아팠는데
이전과는 다르게 많이 깔끔해지고 단순해졌습니다!
머리도 더 맑아진 느낌이네요🙈

알려주신 전략패턴을 사용했습니다. 랜덤값에 따라 MoveOne, MoveStop 를 세팅하여 move 를 실행하는데
생성된 자동차 수 만큼 매번 Move 객체를 생성하여 작업하기엔 비효율적인거 같아서 final static 로 진행했습니다.

미흡한 점이나 보완사항(공부거리도 좋습니당) 피드백 부탁드리겠습니다!
감사합니당🎶

Copy link

@changgunyee changgunyee left a comment

Choose a reason for hiding this comment

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

안녕하세요. 현철님(아.... 석희님과 착각했군요 죄송합니다. 😢)

열심히 미션을 수행해주셨네요. 🔥
피드백을 많이 반영해주신 것 같아요. 👍

앞으로 진행하실 미션에서 계속해서 고려해야할 내용이기 때문에
좀 더 고민해보시면 좋을것 같아 피드백과 함께 Request changes 드렸습니다. 😄

UI와 도메인 객체들의 책임을 생각하고 분리 부탁드리겠습니다.
굳이 매 턴마다 라운드를 출력할 필요는 없습니다.(분리가 더 어려워지기 때문이기도 하구요.)

** 전략 패턴 **
전략 패턴의 구현에 아쉬운 점이 보입니다.
자동차 객체에 전략을 결정하는 부분은 위치가 이상해보입니다.
오히려 자동차들의 이동 전략을 결정하는 곳은 GrandPrix가 적당해 보입니다.

클래스들의 역할을 명확히하여 다른 곳에 있어야 할 코드가 들어있지는 않은지 확인해보세요.

위의 피드백 중점으로 고민해보시고 궁금한점이나, 어려운점 있으면 슬랙을 통하여 DM 주세요.
함께 고민해보아요 🔥 _🔥

리팩토링 해보시고 PR 부탁드립니다~ 화이팅 입니다 🚗 🚗 🚗

}

public void start(int playerCount, int turnCount) {
List<Car> cars = createCar(playerCount);

Choose a reason for hiding this comment

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

차를 생성하는 것은 의미상 생성자에서 해주는 것이 좋아보입니다!
또한 cars를 일급 컬렉션으로 만들어 컬렉션에 대한 로직을 한곳에 모아보는 것은 어떨까요?

import java.util.List;

public class GrandPrix {
private ResultView resultView;

Choose a reason for hiding this comment

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

GrandPrix 에서 일어나야 하는 일에 대해서 생각해보세요.

  • InputView에서 Car 갯수, Round 갯수를 입력 받는다.
  • GrandPrix 자동차 경기를 시작한다.
  • 경기 결과를 가져와 OutputView에 전달한다.
  • ResultView 가 경기 결과를 출력한다.

최대한 간결하고 직관적으로 리팩토링 해보시면 됩니다.

resultView.printGameStart();

for(int i = 0; i < turnCount; i++) {
turn(cars);

Choose a reason for hiding this comment

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

turn은 함수명이 애매해보입니다.

Copy link
Author

Choose a reason for hiding this comment

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

변수명짓기 좋은 팁이 있을까요...?

@@ -0,0 +1,13 @@
package new_racingcar;

public class MoveStopStrategy implements MoveStrategy {

Choose a reason for hiding this comment

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

Stop을 위한 Strategy는 불필요해보이네요.
그냥 이동을 안하면 되기 때문이죠!

Copy link
Author

Choose a reason for hiding this comment

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

jjvpx.jpg

//then
assertThat(result).isFalse();
}
}

Choose a reason for hiding this comment

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

테스트는 파일별로 나눠주시는 게 관리면에서나 가독성면에서 좋답니다.

Copy link
Author

Choose a reason for hiding this comment

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

객체별로 나누겠습니당

this.moveStrategy = moveStrategy;
}

private MoveStrategy getMoveType(int randomValue) {

Choose a reason for hiding this comment

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

자동차내에서 MoveStrategy를 결정하는 것은 이상해 보이는 군요.

자동차는 자신이 이동할 수 있는지 없는지 외부에서 주입된 MoveStrategy에게 맡기는 형식으로 하면 좋을 것 같네요.

public int getDistance() {
return this.distance;
};
}

Choose a reason for hiding this comment

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

Record를 객체화 해주셨군요.


resultView.printGameStart();

for(int i = 0; i < turnCount; i++) {

Choose a reason for hiding this comment

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

라운드를 관리하는 객체가 추가되어도 괜찮아 보이는군요.

int randomValue = RandomUtil.getValue();

//then
}

Choose a reason for hiding this comment

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

요 테스트는 조금 이상해보이는군요~
에러가 발생하지 않는다는 의미의 테스트라면 큰 의미는 없어보입니다.

https://stackoverflow.com/questions/17731234/how-to-test-that-no-exception-is-thrown

return cars;
}

public void turn(List<Car> cars) {

Choose a reason for hiding this comment

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

이 함수도 테스트를 할 수 있으면 좋을 텐데 말이죠.

MoveStrategy를 누가 가져야할 지 생각해보세요.
자동차가 이동할 rule의 역할을 함으로 GrandPrix가 가지고 있어야할 것 같네요.

Copy link
Author

Choose a reason for hiding this comment

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

아직 역할과 책임 개념이 덜 잡힌거 같네요
내일 오브젝트 책으로 개념 좀 잡고 진행하겠습니다

2021. 03. 13
1) Car 일급컬렉션 추가
2) 참가선수, 진행횟수 생성자로 생성처리
3) 라운드별 결과 출력이 아닌 종료 후 결과 일괄 출력으로 변경

Resolves : loop-study
See also : changgunyee
2021. 03. 13

Resolves : loop-study
See also : changgunyee
@loop-study
Copy link
Author

loop-study commented Mar 13, 2021

안녕하세요 박현철입니다.
잘못된 데이터중심의 사고방식이 남아
리뷰어님을 고생시켜 죄송합니다.😭

오브젝트를 읽고 책임과 역할에 대한 부족함을 채웠습니다.📖

의문사항이 생겨 의견 부탁드립니다.

  1. 현재 Move 전략패턴에서 무의미한 MoveStop 을 제거했습니다.
    제거 후엔 MoveOne 하나만 존재하는데 전략패턴을 사용해야할지, 지워야할지 의문스럽습니다.
    사용한다면 이동 요구사항 변경에 대비하는 뜻으로 이해하면 될까요?
    필요없다면 단순히 Car 객체에 isMove(int randomValue) 메서드로 변경해야하는게 맞지 않을까 싶습니다.

  2. 일급 컬렉션을 사용해봤습니다. 처음 사용해서 많이 부족합니다.
    보완할 점 있는지 확인 부탁드리겠습니다.

  3. 각 Round 결과를 출력하기위해 Collection 을 Wrapping 한 Cars 를 반환하여 저장합니다,
    Round 를 이용해서 결과를 출력하려고보면 Cars 를 통해 List Car 를 가져옵니다. 중간흐름인 Cars 를 빼고 List Car 로 변경할지 아니면 그대로 사용해야할지 고민스럽습니다.
    public class Round {
    private final Cars cars; -> private final List< Car > cars;
    ...
    }

  4. 변수명 짓기 좋은 방법은 뭐가있을까요...😂

피드백 부탁드리겠습니다.

2021. 03. 14
1) GrandPrix에서 경주 결과를 반환하도록 변경
2) 결과출력을 RacingMain 에서 처리하도록 수정

Resolves : loop-study
See also : changgunyee
Copy link

@changgunyee changgunyee left a comment

Choose a reason for hiding this comment

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

안녕하세요. 현철님! 😄

주말동안 피드백 반영하시느라 고생많으셨습니다.
열심히하신만큼 코드가 좋아지는 것을 확인할 수 있더군요 🔥

질문을 몇가지 주셨는데 답변을 드리자면

전략 패턴을 사용해주세요
이동 요구사항 변경에 대비하는 것은 아닙니다.
현재 RandomUtils를 이용하여 randomValue를 파라미터로 받고 있습니다.
이 코드가 Cars에 들어가 있는 것이 역할에 맞지 않는 것 같습니다.
이를 MoveStrategy에 아예 맡겨보세요.
그리고 테스트를 할 때는 람다로 임의의 MoveStrategy를 만들어 내는 것이지요 😄

일급 컬렉션
잘 사용하고 계신것 같아요!

Round클래스
Cars를 사용하는 것이 좋다고 생각합니다.
일급 컬렉션을 쓰다보면 값을 꺼낼 때 몇 번씩 가져오는 과정이 생기는 것은 어쩔 수 없습니다.(그렇지만서도 불편한 것은 사실.....)

변수명
사실 변수명은 사람들마다 짓는 법이 다릅니다.
제가 생각하기에 연습하기에 좋은 방법은

  • 줄여쓰지 않는다.
  • 도메인 지식이 없는 사람도 이해가 가능한가?
  • 너무 길어진다면 새로운 용어를 도입하고 주석을 달자
  • 함수는 동사, 클래스는 명사

등등...
네이밍은 다른 사람들의 코드를 자주 보다보면 실력이 늘기도 한답니다~ 😄

첫 리뷰 때 미션의 구현사항을 바로 지켜달라고 말씀을 못드린 덕에 조금 늦어진 것도 있는 것 같습니다.
하지만 지금 코드 퀄리티와 구조가 좋아 다음 단계부터는 수월할 것으로 예상됩니다.

이번 단계 수행하시느라 고생많으셨습니다!
이만 머지하겠습니다. (물론 추가적으로 드린 피드백 반영부탁드립니다.)
나머지 미션도 파이팅하세요! 👋

src/main/java/new_racingcar/Cars.java Show resolved Hide resolved
src/main/java/new_racingcar/GrandPrix.java Show resolved Hide resolved
src/main/java/new_racingcar/Cars.java Show resolved Hide resolved
src/main/java/new_racingcar/RacingMain.java Show resolved Hide resolved
src/main/java/new_racingcar/Cars.java Show resolved Hide resolved
src/main/java/new_racingcar/MoveStrategy.java Show resolved Hide resolved
src/main/java/new_racingcar/InputView.java Show resolved Hide resolved
List<Car> cars = new ArrayList<>();

for(int i = 0; i < playerCount; i++) {
cars.add(new Car(MoveOneStrategy.INSTANCE));

Choose a reason for hiding this comment

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

MoveOneStrategy를 GrandPrix에서 가지고 있는 것은 어떨까요?
자동차 이동 규칙은 게임 관리자가 가지고 있는게 맞다고 생각하는데 어떠신가요? 😄
그렇게 되면 Cars.run과 같은 함수도 테스트가 가능해질 것 같은데 말이죠.

@changgunyee changgunyee merged commit 2610239 into next-step:loop-study Mar 14, 2021
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.

None yet

2 participants