Skip to content

[1단계 - 자동차 경주 구현] 기론(김규철) 미션 제출합니다. #317

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

Merged
merged 46 commits into from
Feb 15, 2022

Conversation

Gyuchool
Copy link

@Gyuchool Gyuchool commented Feb 11, 2022

안녕하세요! 4기 기론입니다!

TDD를 처음으로 접하고 사용하면서 헷갈리는 부분들이 조금 있었습니다. 궁금한 부분에 답변해주시면 감사하겠습니다!!
첫 PR까지 시간이 부족해서 완성도가 아직 많이 부족한 것 같습니다.. 제가 봐도 리펙토링할 부분이 많아 보이는데 빠른 시간안에 리펙토링해서 좋은 변화를 보여드리도록 하겠습니다. 감사합니다!

질문

  1. 랜덤 값에 따라 전진 또는 멈추는 기능을 구현 할 때, 저는 goOrStay라는 매서드 안에서 랜덤 값을 생성해서 동작하는 기능을 구현하고 싶었습니다. (매서드에 매개변수가 많으면 클린 코드가 아니라고 생각했기 때문에 매개변수를 줄이기 위해서 그랬습니다!)
    그런데 테스트 코드를 작성할때, 매서드 안에 랜덤값을 생성하면 어떤 랜덤한 값이 나왔는지 테스트하기 어렵더라구요. 그래서 매개변수를 만들어서 랜덤한 값을 받았었습니다. 그렇다면 실제 랜덤 값을 사용하는 로직을 테스트하려면 매개변수로 뺴내서 하는 방법뿐일까요? 아니면 랜덤값은 잘 나왔다고 믿고 매서드 안에 넣어도 될까요?

  2. TDD를 경험하면서 테스트 코드를 작성할 때, 작은 부분들을 먼저 만들고 나중에 만들어둔 기능들을 전부 애플리케이션으로 합칠 때 어느 부분까지 합쳤는지, 어떻게 합쳐야 할지 생각하면서 헷갈렸던 경험이 있었습니다. 혹시 모든 기능들을 만들고 나중에 합칠때 좀더 유연하게 합칠수있는 팁(?)같은 게 있을까요?
    저는 이번 미션을 진행하면서 생각해봤을 때, 일정 테스트 코드를 작성해서 기능을 완성하면 중간마다 전체 애플리케이션에 통합하면서 잘 돌아가는지 확인하는 방식을 적용하면 어떨까 라고 생각했습니다. 리뷰어님의 의견도 궁금합니다!

  3. validator와 같은 유효성 검사 로직에서, 만약 자연수인지 검증하는 로직이 있으면 딱 검증만 하는 게 좋나요? 아니면 검증을 하고 int로 변환까지 해주는 게 좋나요? 저는 처음엔 딱 검증만 하는 게 한 가지 기능만 하기 때문에 좋다고 생각했는데 이렇게 하면 코드가 길어져서 너무 과하지(?) 않나 생각하게 되어서 질문 드립니다!

감사합니다.

Copy link

@JunHoPark93 JunHoPark93 left a comment

Choose a reason for hiding this comment

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

안녕하세요 기론! 리뷰어 제이입니다.
첫 번째 미션 하시느라 고생많으셨어요 👍
질문 남겨주신부분들은 해당 코드부분에 코멘트 남겨두었어요.
전체적으로 컨벤션과 도메인과 뷰의 분리에 대해서 보시면 될 것 같아요.
궁금하신점 코멘트나 dm남겨주세요~

Comment on lines 27 to 31
public void goOrStay(int randomNumber) {
if (randomNumber >= MOVE_STANDARD) {
position++;
}
}

Choose a reason for hiding this comment

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

기론의 첫번째 질문입니다.

랜덤 값에 따라 전진 또는 멈추는 기능을 구현 할 때, 저는 goOrStay라는 매서드 안에서 랜덤 값을 생성해서 동작하는 기능을 구현하고 싶었습니다. (매서드에 매개변수가 많으면 클린 코드가 아니라고 생각했기 때문에 매개변수를 줄이기 위해서 그랬습니다!)
그런데 테스트 코드를 작성할때, 매서드 안에 랜덤값을 생성하면 어떤 랜덤한 값이 나왔는지 테스트하기 어렵더라구요. 그래서 매개변수를 만들어서 랜덤한 값을 받았었습니다. 그렇다면 실제 랜덤 값을 사용하는 로직을 테스트하려면 매개변수로 뺴내서 하는 방법뿐일까요? 아니면 랜덤값은 잘 나왔다고 믿고 매서드 안에 넣어도 될까요?

랜덤값은 말그대로 랜덤한 값이 나오기 때문에 테스트시마다 실패할 가능성을 가지고있습니다. 테스트하기 어려운구조는 1차적으로 해당 부분을 격리시켜주어야 하는데요, 현재 랜덤값이지만 외부에서 값을 주입받도록 해주셨네요. 잘해주셨어요!
이렇게 되면 RacingCar는 테스트를 잘 할 수 있겠네요.

랜덤 테스트에 대한 고민은 인터페이스를 이용하여 해결할 수 있는데요, 미션을 진행하시면서 자연스럽게 경험하시게 될거에요.
여력이 되시면 서치해보시고, 꼭 코드를 수정해보고싶다! 라고하시면 수정된 코드를 남겨주시면 그때 다시 이야기를 이어가요~

추가로 인자이름은 randomNumber에서 범용적인 이름으로 변경하면 어떨까요?
실제 게임이 진행될때는 random한 숫자가 올거지만 테스트코드상에서는 고정된 숫자가 올거니까요.

Copy link
Author

Choose a reason for hiding this comment

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

좋은 답변 정말 감사합니다! 랜덤한 값 테스트에 대해 고민했었는데 인터페이스를 사용하는 방법도 있었네요..! 한번 공부해보겠습니다!
인자 이름으로는 number라고 사용해도 괜찮을까요?

Choose a reason for hiding this comment

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

넵 일단 random이라는 특정한 단어가 빠진것만으로 더 범용적으로 느껴져요~

private static String getCurrentPosition(RacingCar racingCar){
StringBuilder sb = new StringBuilder();
int position = racingCar.getPosition();
for (int i=0; i<position; i++){

Choose a reason for hiding this comment

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

컨벤션 확인 부탁드려요 :)

Comment on lines 13 to 14
private static final String WINNER_ANNOUNCEMENT = "가 최종 우승했습니다.";
public static void printGameStartMessage(){

Choose a reason for hiding this comment

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

컨벤션 확인 부탁드려요 :)

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

Choose a reason for hiding this comment

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

기론의 두번째 질문입니다. (테스트 관련 이라 그냥 여기다가남겨요~)

TDD를 경험하면서 테스트 코드를 작성할 때, 작은 부분들을 먼저 만들고 나중에 만들어둔 기능들을 전부 애플리케이션으로 합칠 때 어느 부분까지 합쳤는지, 어떻게 합쳐야 할지 생각하면서 헷갈렸던 경험이 있었습니다. 혹시 모든 기능들을 만들고 나중에 합칠때 좀더 유연하게 합칠수있는 팁(?)같은 게 있을까요?
저는 이번 미션을 진행하면서 생각해봤을 때, 일정 테스트 코드를 작성해서 기능을 완성하면 중간마다 전체 애플리케이션에 통합하면서 잘 돌아가는지 확인하는 방식을 적용하면 어떨까 라고 생각했습니다. 리뷰어님의 의견도 궁금합니다!

기능을 조금씩 만들어가면서 기존 로직에 합칠때 어떤식으로 하면 좋을지 대한 내용으로 이해했어요. 그리고 기론은 중간중간 테스트를 계속 돌려가면서 확인하는게 괜찮은 방법일지에 대한 질문이고요. 혹 제가 이해한게 아니라면 다시 말씀 부탁드려요.

넵 저도 좋은 방법이라고 생각해요. 중간에 돌려보지 않으면 나중에 한번에 다 깨질테니 중간중간 깨지는 테스트들을 고쳐나가면서 진행하는게 좋습니다. 근데 사람마다 다른것 같아요. 한번에 다 수정하고 깨지는거 고치는것을 좋아하는 사람도 있고, 중간중간 돌려보면서 확인하는 사람도있고요. 본인에 맞는 방식을 선택하시면 될 것 같네요!

Copy link
Author

Choose a reason for hiding this comment

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

넵! 좋은 답변 감사합니다! 고민했었던 부분이었는데 해결된 것 같습니다!😀

Comment on lines 70 to 77
public static void checkTryCountIsNaturalNumber(String tryCountInput) {
for (int i = 0; i<tryCountInput.length(); ++i) {
isNumber(tryCountInput.charAt(i));
}
if (Integer.parseInt(tryCountInput) == 0) {
throw new IllegalArgumentException(TRY_COUNT_FORMAT_ERROR_MESSAGE);
}
}

Choose a reason for hiding this comment

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

기론의 세번째 질문입니다. 이 부분으로 보여요.

validator와 같은 유효성 검사 로직에서, 만약 자연수인지 검증하는 로직이 있으면 딱 검증만 하는 게 좋나요? 아니면 검증을 하고 int로 변환까지 해주는 게 좋나요? 저는 처음엔 딱 검증만 하는 게 한 가지 기능만 하기 때문에 좋다고 생각했는데 이렇게 하면 코드가 길어져서 너무 과하지(?) 않나 생각하게 되어서 질문 드립니다!

넵 말씀주신것처럼 보통 메서드는 한가지 기능만 담당하게 하는게 좋습니다. 분리해보는게 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

넵 변환하는 매서드와 검증하는 매서드로 분리해보겠습니다!

}

private static void isNumber(char target) {
if (target < '0' || target > '9') {

Choose a reason for hiding this comment

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

의미있는 이름을 가진 상수로 추출해보는건 어떨까요?

Comment on lines 45 to 46
if(racingCar.getName().length() > CAR_NAME_STANDARD_SIZE){
throw new IllegalArgumentException(CAR_NAME_SIZE_MASSAGE);

Choose a reason for hiding this comment

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

자동차 이름에 대한 검증을 잊지 않고 잘 해주셨어요 👍

다만 이름에 대한 판단을 자동차에게 주면 어떨까요?
자동차에게 이름의 길이를 질의하거나, 혹은 생성될때 이름의 길이를 검증하여 적절한 길이가 아니라면 객체 생성을 하지 못하도록요.

Comment on lines 7 to 10
private int position;


private RacingCar(String name) {

Choose a reason for hiding this comment

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

이 클래스도 전체적으로 개행에 관한 컨벤션을 확인해주세요 :)
처음에 익숙치 않더라도 한번 컨벤션에 관해서 신경써서 해보면 다음부터는 훨씬 수월해질거에요~

}

public void join(String inputCarNames) {
Validator.checkHaveLastInputComma(inputCarNames);

Choose a reason for hiding this comment

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

RacingCars에서 checkHaveLastInputComma 라는 메서드를 사용하고있어요.
메서드 이름을 봐서는 뷰에 관련된 검증으로 보이는데요, 뷰단검증은 InputView에서 해주는건 어떨까요?

일차적으로 input값들을 검증해주고 RacingGame라는 컨트롤러에서 RacingCars를 만들어주는 방식으로하면 조금더 mvc구조를 다듬어 볼 수 있을것같아요~

Copy link
Author

Choose a reason for hiding this comment

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

넵! 페어프로그래밍할때는 자동차라는 객체를 만들고 자동차가 정상적인지 검증한다는 생각으로 구현했는데 제이의 말대로 지금 검증 로직들은 전부 인풋이 어떻게 들어오냐에 따른 검증이라서 전부 view에서 처리하도록 수정했습니다!

import java.util.Comparator;
import java.util.List;

public class RacingCars {

Choose a reason for hiding this comment

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

도메인을 잘 만들어주셨어요~
정의된 도메인에 대한 테스트는 꼭 필요한데요. 이 도메인은 테스트를 어떻게 하면 좋을까요? 🤔

@Gyuchool
Copy link
Author

안녕하세요 제이! 코드를 작성하면서 사소한 부분인 것 같지만 제이는 어떻게 하는지 궁금해서 여쭙니다!

숫자 크기를 비교하는 경우 보통 >를 사용하나요? 아니면 >=와 같이 사용하나요? 회사 컨벤션에 따라 다를까요?

@JunHoPark93
Copy link

마지막 질문에 대한 내용입니다.

숫자 크기를 비교하는 경우 보통 >를 사용하나요? 아니면 >=와 같이 사용하나요? 회사 컨벤션에 따라 다를까요?

음 질문의 의도가 예를 들어, 이상으로 표기하는지 초과로 표기하는지 에 대한 질문으로 이해했어요. (아니라면 다시 말씀주세요)
그때그때 마다 다른거같아요. 대신에 메서드로 감추어서 표현하는경우에는 equalOrAfter 와 같이 명확히 경계값이 포함되는지 메서드이름으로 나타냅니다.
요거 답변이 되었을까요?

Copy link

@JunHoPark93 JunHoPark93 left a comment

Choose a reason for hiding this comment

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

안녕하세요 기론!
리뷰 반영하시느라 고생많으셨어요!
오랜기간 고민을 많이 하신거같아요. 👍 충분히 고민하고 반영하시는 모습이 멋져요.
질문하신것은 코멘트로 남겨두었어요. 그럼 이만 머지하겠습니다~

Comment on lines +48 to +54
public static void checkDuplicatedName(String[] racingCarNames) {
List<String> nameList = new ArrayList<>();
for (String racingCarName : racingCarNames) {
validatorDuplicatedName(nameList, racingCarName);
nameList.add(racingCarName);
}
}

Choose a reason for hiding this comment

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

정답은 없지만 고민해볼만한 포인트라 남겨요~
이름중복을 검사하는 책임을 누가가져가는게 좋을까요?
남기는 이유는 RacingCar라는 도메인을 만들어주셨기 때문이에요. Car들을 관리하는 책임이고 중복된 car들이 있는지 검사(관리)하는 역할을 부여하는게 나을지, 아니면 view에서 가져가는게 나을지 고민해보면 좋을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

Car들을 관리하는 RacingCars 도메인을 만들었기 때문에 여기서 관리하는건 어떤지 의문 던져주신 거 맞나요?
RacingCars가 관리 목적이라 중복된 Car들을 확인하는것도 제이 말씀대로 좋을 것 같습니다!
제가 위 코드처럼 Validator 클래스에서 검증 로직을 만들어 View단에서 검증했던 이유는 애초에 중복된 이름이 있으면 객체를 생성하지 않는 게 비용도 덜 들고 좋지 않을까 라고 생각했습니다!
이 부분은 좀 더 고민해보도록 할게요!! 좋은 의문 던져주셔서 감사합니다!

Comment on lines +23 to +27
private static void validatedCarStandard(String[] carNames) {
Validator.checkCarsNameIsEmpty(carNames);
Validator.checkCarsNameSize(carNames);
Validator.checkDuplicatedName(carNames);
Validator.checkCountOfCar(carNames);

Choose a reason for hiding this comment

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

기존에 없던 view단 검증이 많이 추가가 되었네요 👍
추후 미션과 프로젝트를 진행하면서 검증에 대한 일들을 다룰 일이 많을텐데 역할을 누가 가져가는지에 대한 경험을 많이 하시게 될거에요~

@JunHoPark93 JunHoPark93 merged commit 4e7419a into woowacourse:gyuchool Feb 15, 2022
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.

2 participants