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

[1단계 - 자동차 경주] 이든(최승준) 미션 제출합니다. #702

Merged
merged 43 commits into from
Feb 17, 2024

Conversation

PgmJun
Copy link

@PgmJun PgmJun commented Feb 15, 2024

리뷰 포인트

  • 전반적으로 객체지향적인 설계가 잘 이루어졌는 지 알고싶습니다. 부족한 부분이 있다면 매운 맛 리뷰 부탁드리겠습니다!

리팩토링 후 리뷰 포인트

기능 구현 목록

  • 자동차 이름 입력 기능
    • 쉼표를 기준으로 이름 구분
    • ⚠️ 자동차 개수가 1 이하면 예외 발생
    • ⚠️ 자동차 이름의 길이가 1 이상 5 이하가 아니면 예외 발생
    • ⚠️ 자동차 이름이 중복되면 예외 발생
  • 시도 횟수 입력 기능
    • ⚠️ 1 미만 입력 시 예외 발생
    • ⚠️ 입력값이 정수가 아닐 시 예외 발생
  • 자동차 이동 기능
    • 0에서 9 사이에서 random 값을 구한 후 random 값이 4 이상일 경우 전진하고, 3 이하의 값이면 멈춘다
    • 이동을 시도할 때마다 현재 위치 출력
  • 우승자 판별 기능
    • 이동한 거리를 계산하여 우승자 도출
    • "%%%가 최종 우승했습니다." 형태로 출력
      • 2명 이상 일 시 ", "로 이름 구분하여 출력

외부에서 CarRacing을 시작시키는 역할과, CarRacing을 직접적으로 수행하는 역할의 분리 필요성을 느낌
- Cars 생성자 매개변수 변경
- CarAccelerator 객체 생성
  - Car가 Accelerator 를 통해서 moveForward 하여 testable한 코드로 리팩토링
- RandomNumberGenerator util 패키지로 분리
- 가독성 방해되는 매직 넘버 제거
- 위 변경 내용에 적합하도록 테스트 코드 리팩토링
amount는 갯수의 느낌이 강해서 유연한 value 네이밍 사용
- 자동차가 이동하지 않는 경우 테스트
- 엑셀 push 기능 테스트
- 자동차 이름의 길이가 1 이상 5 이하로 주어지면 자동차가 정상적으로 생성된다
- 자동차 이름의 길이가 1 미만 5 초과로 주어지면 자동차가 정상적으로 생성되지 않는다
- 자동차 이름이 null로 주어지면 자동차가 정상적으로 생성되지 않는다
- 자동차 이름이 blank로 주어지면 자동차가 정상적으로 생성되지 않는다
- 입력받은 자동차의 개수가 2대 이상이면 객체 생성에 성공한다
- 입력받은 자동차의 개수가 1대 이하이면 객체 생성에 실패한다
- 시도 횟수가 1 이상이면 객체 생성에 성공한다
- 시도 횟수가 1 미만이면 객체 생성에 실패한다
Copy link

@seovalue seovalue left a comment

Choose a reason for hiding this comment

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

안녕하세요, 이든!
첫번째 리뷰를 맡게 된 조앤이라고 해요ㅎㅎ 잘부탁드려욧
미션 잘 구현해주셨네요! 몇가지 코멘트 남겨뒀는데, 확인해보시고 다시 리뷰 요청 부탁드릴게요~

@@ -0,0 +1,61 @@
## 기능 구현 문서

- [x] 자동차 이름 입력 기능

Choose a reason for hiding this comment

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

👍


private void validateNameLength(String name) {
if (name.length() < 1 || name.length() > 5) {
throw new IllegalArgumentException();

Choose a reason for hiding this comment

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

Screenshot 2024-02-15 at 20 39 33

잘못된 입력이 들어왔을 때 프로그램을 종료하는 것이 아닌 다시 입력을 받을 수 있도록 하는 것은 어떨까요?
그리고 별도의 에러 메시지가 없어서 어떤 이유로 종료되는 건지 파악하기 어려워요.

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.

잘못된 입력이 들어왔을 때 프로그램을 종료하는 것이 아닌 다시 입력을 받을 수 있도록 하는 것은 어떨까요? 그리고 별도의 에러 메시지가 없어서 어떤 이유로 종료되는 건지 파악하기 어려워요.

에러 메시지 지적해주신 부분 정말 동의합니다 ㅠ
저도 어느 부분에서 예외가 발생했는 지, 의도한 방향대로 예외가 발생한 것이 맞는 지 검증하는 것이 애매해서 조앤의 의견 반영해보겠습니다!

다만 재입력을 받도록 하지 않은 이유는 아래와 같았습니다 🥲

미션에는 depth1로 제한한다는 요구사항이 정의되어 있었습니다.

private Cars readCars() {
        while(true) {
            try {
                return createCars(inputView.readCarNames(), new CarAccelerator());
            }catch (IllegalArgumentException exception) {
                continue;
            }
        }
    }

하지만 제가 구상했던 재입력 로직은 다음과 같이 depth가 1을 넘어가는 코드였는데요 !
혹시 이 부분에 대해서 개선할 수 있는 의견주시면 정말 감사하겠습니다:)

Choose a reason for hiding this comment

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

    private Cars readCars() {
        try {
            return createCars(inputView.readCarNames(), new CarAccelerator());
        } catch (Exception exception) {
            outputView.printErrorMessage(exception);
            return readCars();
        }
    }

이런 식으로 한번 해보시겠어요?ㅎㅎ 다만 재귀적으로 호출되어서 문제가 발생할 수도 있어요~

this.accelerator = accelerator;
}

private void validate(String name) {

Choose a reason for hiding this comment

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

Screenshot 2024-02-15 at 20 40 55

,만 여러개 들어오는 것도 invalid 한 입력이라고 생각하는데, 어떠세요?

Copy link
Author

Choose a reason for hiding this comment

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

,만 여러개 들어오는 것도 invalid 한 입력이라고 생각하는데, 어떠세요?

이 부분에 대해서 페어와 정규표현식 을 활용하여 입력값의 형태를 검증해보자는 이야기를 나누었습니다!

그런데 페어 프로그래밍으로 개발을 진행하다보니 생각보다 시간이 많이 필요해서, 먼저 돌아가는 코드 위주로 만들고 하나씩 개선하는 방향으로 개발하다보니 결국 처리하지 못하고 남겨두게 되었었네요 🥲

조앤이 체크해주신 덕분에 다시 한 번 떠올릴 수 있었습니다:) 리팩토링 과정에서 처리해보겠습니다!👍


import util.RandomNumberUtils;

public class CarAccelerator {

Choose a reason for hiding this comment

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

ㅋㅋ재밌는 이름이네용

Copy link
Author

@PgmJun PgmJun Feb 16, 2024

Choose a reason for hiding this comment

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

ㅋㅋ재밌는 이름이네용

감사합니다:) 뭔가 객체지향이다보니 현실에 투영해서 Car의 Accelerator를 밟으면
밟은 power의 값을 return 하는 느낌으로 만들어보고 싶었어요!

}
}

public void tryMove() {

Choose a reason for hiding this comment

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

사소한 궁금증! 그냥 move도 있는데 tryMove라고 해주신 이유가 궁금해요!

Copy link
Author

Choose a reason for hiding this comment

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

사소한 궁금증! 그냥 move도 있는데 tryMove라고 해주신 이유가 궁금해요!

해당 메서드를 호출했을 때 자동차가 100% 이동하는 것이 아니기 때문에 tryMove라는 네이밍을 사용해보았습니다!
이동을 시도했을 때, Acceleratorpush하는 power가 약하다면 움직이지 않으니까요!


public void start() {
Cars cars = createCars(inputView.readCarNames(), new CarAccelerator());
TryCount tryCount = createTryCount(inputView.readTryAmount());

Choose a reason for hiding this comment

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

name은 단순 String으로 받고 count는 TryCount로 감싸주신 이유가 궁금해요~

Copy link
Author

Choose a reason for hiding this comment

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

name은 단순 String으로 받고 count는 TryCount로 감싸주신 이유가 궁금해요~

위에서 말씀드렸다시피, 먼저 돌아가는 코드를 만들고 하나씩 개선해나가는 방향으로 개발해왔습니다.
그 과정에서 개선이 필요한 부분들을 메모하면서 개발했는데요! 페어 프로그래밍이 처음이다보니 살짝 정신이 없어서 체크해두지 못한 부분들이 존재했는데, 조앤이 짚어주신 이 코드가 그 부분 중 하나였습니다 ㅠ

리팩토링 과정에서 객체로 관리하도록 재설계 해보겠습니다:)

@@ -0,0 +1,12 @@
package io.validator;

public class InputValidator {

Choose a reason for hiding this comment

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

해당 클래스의 역할이 애매하다고 느껴져요.
name에 대한 검증은 Car 내부에서, 1보다 작은 수에 대한 검증은 TryCount 내부에서 하는데 intFormat만 여기서 해주는 이유도 궁금하고요.

Copy link
Author

@PgmJun PgmJun Feb 16, 2024

Choose a reason for hiding this comment

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

해당 클래스의 역할이 애매하다고 느껴져요. name에 대한 검증은 Car 내부에서, 1보다 작은 수에 대한 검증은 TryCount 내부에서 하는데 intFormat만 여기서 해주는 이유도 궁금하고요.

IO에 대한 검증과 Domain에 대한 검증을 분리해서 코드를 작성했습니다!
입력 시에 애플리케이션이 요구하는 입력 값의 형태로 입력하였는 지에 대해 검증하는 IO 검증은 InputValidator 에서 처리해주었고
그 값을 통해 Domain 클래스의 생성자를 호출할 때 Domain 객체에서 요구하는 범위로 값이 들어왔는 지 검증하는 로직은 Domain 객체에 배치하였습니다!

말씀해주신 예시로 생각해보면
tryCount를 입력받을 때, tryCount정수 형태로 입력받아야하니 입력값이 정수가 맞는 지에 대한 검증은 IO 객체에서 처리하고
입력받은 정수값이 TryCount가 요구하는 1 미만의 정수는 입력이 불가능하다. 라는 조건은 TryCount 객체에서 검증하였습니다!


라고 생각했는데요..

테스트 코드를 짜다보니 이런 문제가 발생합니다 ⚠️

I/O 로직에서 일부 검증 로직을 수행할 시, 기능에 대한 테스트 코드 작성 시에 I/O가 검증하는 부분에 대한 테스트가 불가능합니다.
때문에I/O는 String만 입력받고, 해당 input값을 처리 및 검증할 다른 방식을 채택할 필요가 있다고 생각이 들었습니다.

때문에

InputValidator 제거 후, 입력 값 검증 및 처리의 책임을 CarRacing으로 이동하는 방향으로 리팩토링 하였습니다.

InputValidator 를 통해 입력값을 InputView에서 검증하게 되면
Input과정에서 검증하는 내용은 Testable 하지 못하게 됩니다.

그래서 InputView는 정말 입력의 책임만을 부여하고
입력값에 대한 검증 및 처리 책임을 CarRacing에 부여하여
Testable한 코드로 리팩토링하였습니다.

개인적으로 생성자에는 클래스의 필드와 같은 자료형의 매개변수를 입력받는 것이 깔끔하고 다양한 상황에서 활용하기에 유연하다고 생각합니다. 이러한 의견을 고수하고자 Input 단에서 값 처리 및 기초 입력값 검증 로직을 수행하도록 구현했었는데요.
리팩토링을 통해 생성자 매개변수에 대한 의견은 지켜내면서,
결론적으로 이전에 테스트 하지 못한 부분을 테스트 할 수 있게 되어 만족스러웠습니다.

하지만 이건 저의 생각이니 해당 로직에 대한 리뷰어님의 의견은 어떠신지도 궁금해요! 답변 부탁드립니다 🙌

Copy link

@seovalue seovalue Feb 17, 2024

Choose a reason for hiding this comment

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

요약하자면 InputValidator를 제거하고 값의 유효성 검증을 각 도메인 객체로 위임했다는 말씀이 맞으시죠? 저는 이 부분에 대해서는 동의해요~ 예를 들어 CarName의 경우 name에 대한 정책은 InputValidator가 아닌 CarName이 관리하는게 맞다고 생각하기 때문이에요.
다만 도메인이 아니라 특정 입력에 대해서는 프로그램 전체의 흐름을 관장하는 CarRacing에서 검증하는 것도 괜찮다고 생각합니다!

개인적으로 생성자에는 클래스의 필드와 같은 자료형의 매개변수를 입력받는 것이 깔끔하고 다양한 상황에서 활용하기에 유연하다고 생각합니다. 이러한 의견을 고수하고자 Input 단에서 값 처리 및 기초 입력값 검증 로직을 수행하도록 구현했었는데요.
이 부분에 대해서는 왜 이렇게 생각하시는 지 궁금해요~ 저는 생성자에서는 객체를 만들 수 있는 데이터가 들어올 수 있는거고 생성자 안에서 가공하여 적절한 객체를 뱉도록 하는 것도 괜찮다고 생각해요.

Copy link
Author

@PgmJun PgmJun Feb 19, 2024

Choose a reason for hiding this comment

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

요약하자면 InputValidator를 제거하고 값의 유효성 검증을 각 도메인 객체로 위임했다는 말씀이 맞으시죠? 저는 이 부분에 대해서는 동의해요~ 예를 들어 CarName의 경우 name에 대한 정책은 InputValidator가 아닌 CarName이 관리하는게 맞다고 생각하기 때문이에요. 다만 도메인이 아니라 특정 입력에 대해서는 프로그램 전체의 흐름을 관장하는 CarRacing에서 검증하는 것도 괜찮다고 생각합니다!

제가 너무 횡설수설 적어서 이해에 도움을 드리지 못한 것 같네요..! 죄송합니다 🥲
제가 리팩토링한 부분도 조앤이 말씀해주신 부분과 동일하게 입력에 대한 검증 책임을 프로그램 전체 흐름을 관장하는 CarRacing에 부여햇다는 것이었습니다:)


public static void validateIntFormat(String text) {
try {
Integer.parseInt(text);

Choose a reason for hiding this comment

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

Integer의 MAX_VALUE까지 입력받을 수 있도록 해 주신 이유가 궁금해요~
1000만 입력한다하더라도 UI가 확인하기 어려워지기도 하고요ㅎㅎ
자체적으로 MAX_VALUE를 지정해보는 것은 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

Integer의 MAX_VALUE까지 입력받을 수 있도록 해 주신 이유가 궁금해요~ 1000만 입력한다하더라도 UI가 확인하기 어려워지기도 하고요ㅎㅎ 자체적으로 MAX_VALUE를 지정해보는 것은 어떨까요?

스크린샷 2024-02-16 18 09 58

조앤이 말씀해주신 부분에 완전 동의합니다!

저도 값이 너무 많으면 UI 확인이 번거로워 질 것 같아서 docs/README.md 에 위와 같이
나중에 추가할 기능으로 자동차 개수, 시도 횟수에 대한 제한을 적어놓았어요!

이것도 먼저 돌아가는 코드를 만들어보자는 목표를 따라가다보니 리뷰 요청 이전까지 미처 처리되지 못했던 부분입니다🥲
리팩토링 과정에서 반영해보겠습니다~!

void createCarsFail(String carNames) {
assertThatThrownBy(() -> {
carRacing.createCars(carNames, accelerator);
}).isInstanceOf(IllegalArgumentException.class);

Choose a reason for hiding this comment

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

전반적으로 발생하는 모든 케이스에 대해서 IllegalArgumentException을 던지고 있기 때문에, 정확히 name으로 인한 실패인지 .isInstanceOf(IllegalArgumentException.class); 이 검증만으로는 확인하기 어렵다고 생각해요.
각 예외 케이스별 메시지나 예외 객체를 따로 두는 이유이기도 하구요ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

전반적으로 발생하는 모든 케이스에 대해서 IllegalArgumentException을 던지고 있기 때문에, 정확히 name으로 인한 실패인지 .isInstanceOf(IllegalArgumentException.class); 이 검증만으로는 확인하기 어렵다고 생각해요. 각 예외 케이스별 메시지나 예외 객체를 따로 두는 이유이기도 하구요ㅎㅎ

동의하는 부분입니다! 어떤 문제로 인해 실패하는 지 검증하기 위한 처리를 리팩토링 과정에서 추가해보겠습니다:)
감사합니다!

@PgmJun
Copy link
Author

PgmJun commented Feb 16, 2024

안녕하세요, 이든! 첫번째 리뷰를 맡게 된 조앤이라고 해요ㅎㅎ 잘부탁드려욧 미션 잘 구현해주셨네요! 몇가지 코멘트 남겨뒀는데, 확인해보시고 다시 리뷰 요청 부탁드릴게요~

세심한 리뷰 감사합니다 조앤! 남겨주신 리뷰들 소중히 읽어보고 답변을 남겨볼게요 🙌
그리고 추가적으로 궁금한 점이 있어서 하나 더 남겨봅니다!

    public void pushAccelerator() {
        moveForward(accelerator.push());
    }

    public void moveForward(int power) {
        if (power >= MIN_MOVABLE_POWER) {
            position++;
        }
    }

Car 객체를 보시면 멤버로 접근제어자가 public인 위 두 가지 메서드가 존재합니다.

pushAccelerator() 메서드를 호출 시, moveForward(accelerator.push()) 가 호출되어 만약 accelerator.push() 의 값이 MIN_MOVABLE_POWER 이상이라면 position 을 1 증가시키는 로직입니다.

그런데 이 코드에 문제가 있는 것 같아요🥲
사실 비즈니스 로직에서 실제로 사용되는 메서드는 pushAccelerator() 뿐인데,

매개변수로 입력된 power의 값이 MIN_MOVABLE_POWER 이상일 때 position이 정상적으로 증가하는 지 테스트하기 위해서
moveForward() 메서드를 public 으로 설정해두어 캡슐화가 깨졌다는 생각이 듭니다..

이 프로그램에서 워낙 중요한 로직이라 테스트를 안하고 넘어갈 수 없는 부분이어서 이런 선택을 하게 되었는데요
혹시 이렇게 테스트를 위해 어느정도는 캡슐화를 느슨하게 만들어도 괜찮은 부분일지 아니면 더 좋은 개선방안이 존재할 지
의견을 남겨주시면 소중히 참고하겠습니다! 감사합니다!🙂

- 2개 미만 시 예외 발생 -> 2개 미만 5개 초과 시 예외 발생
- 1 미만 시 예외 발생 -> 1개 미만 5 초과 시 예외 발생
InputValidator 를 통해 입력값을 InputView에서 검증하게 되면
I/O는 검증하지 않는다는 조건에 의해 Input 단에서 검증하는 내용을 Testable하지 못하게 됨.

때문에 InputView는 정말 입력의 책임만을 부여하고
입력값에 대한 검증 및 처리 책임을 CarRacing에 부여하여
Testable한 코드로 리팩토링.

결론적으로 이전에 테스트 하지 못한 부분을 테스트 할 수 있게 됨.
Copy link

@seovalue seovalue left a comment

Choose a reason for hiding this comment

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

안녕하세요, 이든! 조앤이에요ㅎㅎ
리팩터링 잘 수행해주셨네요~ 남겨주신 질문에 대해 답변도 남겼어요.
우선 지금 남긴 코멘트는 다음 단계 진행하시면서 반영해도 괜찮을 것 같아서, 이 PR은 머지할게요.
추가로 몇가지 코멘트 더 남겼는데, 다음 단계 진행 전 꼭!!!! 확인 부탁드릴게요~

}


public void pushAccelerator() {

Choose a reason for hiding this comment

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

남겨주신 질문(이 엄청 길어서 링크로 대체할게요ㅎㅎ)에 대한 답변 여기서 드려볼게요~ 근데 답변이 아니라 질문이 될 것 같네요ㅎㅎ

어떻게 하면 moveForward를 숨기고 원하는 테스트를 할 수 있을까요? 그리고 왜 public으로 moveForward를 열어주는 방식을 채택하셨나요?

대안을 간단히 생각해보자면, pushAccelaerator에서 power를 매개변수로 받을 수 있을 것 같아요. 다만 이렇게 한다면, 현재 내부적으로 사용하고 있는 accelator.push를 외부에서 주입해줘야한다는 문제가 있는데요, 어떻게 하면 이것을 해결할 수 있을까요?

  • Car는 accelator를 멤버 변수로 왜 가져야 한다고 생각하시나요?
  • Car는 단순히 넘겨받은 수만큼 이동하는 책임만을 갖도록 하는 것은 어떨까요?

Copy link
Author

@PgmJun PgmJun Feb 19, 2024

Choose a reason for hiding this comment

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

어떻게 하면 moveForward를 숨기고 원하는 테스트를 할 수 있을까요? 그리고 왜 public으로 moveForward를 열어주는 방식을 채택하셨나요?
대안을 간단히 생각해보자면, pushAccelaerator에서 power를 매개변수로 받을 수 있을 것 같아요. 다만 이렇게 한다면, 현재 내부적으로 사용하고 있는 accelator.push를 외부에서 주입해줘야한다는 문제가 있는데요, 어떻게 하면 이것을 해결할 수 있을까요?

테스트 코드를 작성할 때 moveForward를 public으로 열어주지 않으면 accelerator.push() 의 값이 난수이기 때문에 테스트가 불가능했습니다🥲
이 문제는 step2에서 전략 패턴을 적용하여 테스트에서 사용하는 전략과 비즈니스 로직 상에서 사용되는 전략을 구분하였습니다!

Copy link
Author

Choose a reason for hiding this comment

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

  • Car는 accelator를 멤버 변수로 왜 가져야 한다고 생각하시나요?

실제로 "엑셀은 자동차에 달려있다" 라는 점이
"Car 객체가 Accelerator 를 가지고 있어야한다는 책임이 있다" 라는 객체지향적인 사고를 이끌어 냈습니다.

Copy link
Author

Choose a reason for hiding this comment

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

  • Car는 단순히 넘겨받은 수만큼 이동하는 책임만을 갖도록 하는 것은 어떨까요?

이러한 방식도 고민을 해보았었는데요, 위에서 말씀드렸듯이 Car 객체가 Accelerator를 가지고 있는 것이 제일 자연스럽다고 느껴졌습니다 🥲 때문에 현재 구조를 유지하면서 이 문제를 개선할 방법을 찾아보았는데요!
전략 패턴 적용을 통해 해당 문제에 해결책을 제시할 수 있었습니다!

- 자동차 이동 기능
- 우승자 판별 기능

- [x] ⚠️ 5 초과 입력 시 예외 발생

Choose a reason for hiding this comment

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

👍

}

private void validateCarNamesInput(String carNames) {
if (!InputView.CAR_NAMES_PATTERN.matcher(carNames).matches()) {

Choose a reason for hiding this comment

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

README에 작성해주신 요구사항을 보면 1이상 5이하의 길이의 이름은 생성할 수 있다고 되어있는데요, 실제로 제대로 동작하지 않아요ㅎㅎ
제출 전 미리 테스트를 해주시면 좋을 것 같아요~
Screenshot 2024-02-17 at 17 08 02

Choose a reason for hiding this comment

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

그리고 pobi, hong 이런 형태로 되어도 안되는데 ,를 기준으로 trim 해줘도 좋을 것 같아요.

Copy link
Author

@PgmJun PgmJun Feb 17, 2024

Choose a reason for hiding this comment

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

README에 작성해주신 요구사항을 보면 1이상 5이하의 길이의 이름은 생성할 수 있다고 되어있는데요, 실제로 제대로 동작하지 않아요ㅎㅎ 제출 전 미리 테스트를 해주시면 좋을 것 같아요~

오류 체크 감사합니다 :) 정규표현식을 {2,5}로 작성해서 문제가 발생했던 것이었습니다 ㅠㅠ 앞으로 잘 체크해보겠습니다!

Copy link
Author

Choose a reason for hiding this comment

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

그리고 pobi, hong 이런 형태로 되어도 안되는데 ,를 기준으로 trim 해줘도 좋을 것 같아요.

이 부분은 요구사항에 구분자가 ","로 표현되어 있어서 trim 처리를 해주지 않고 입력된 값을 전부 이름으로 판단하자라고 생각했던 부분입니다!
이 의견에 대한 조앤의 생각이 궁금해요!

Choose a reason for hiding this comment

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

ㅎㅎ저도 좋습니당!! 굿굿

public static final String CAR_NAME_DOES_NOT_EXIST_ERROR_MESSAGE = "자동차의 이름은 NULL 또는 공백일 수 없습니다.";
public static final String CAR_NAME_LENGTH_ERROR_MESSAGE = String.format("자동차의 이름 %d 이상, %d 미만 이어야 합니다.", MIN_CAR_NAME_LENGTH, MAX_CAR_NAME_LENGTH);

public CarName(String value) {

Choose a reason for hiding this comment

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

👍

@@ -1,3 +1,5 @@
import common.exception.message.ExceptionMessage;

Choose a reason for hiding this comment

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

CarRacingTest에서 name이 한 글자인 경우에 대한 테스트도 있으면 좋을 것 같아요. (1이상 5이하인 경우 length가 1, 5 인 값에 대해 테스트해보면 좋아요.)

Copy link
Author

@PgmJun PgmJun Feb 17, 2024

Choose a reason for hiding this comment

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

CarRacingTest에서 name이 한 글자인 경우에 대한 테스트도 있으면 좋을 것 같아요. (1이상 5이하인 경우 length가 1, 5 인 값에 대해 테스트해보면 좋아요.)

놓친 테스트가 있었군요 ㅠㅠ 체크 감사드립니다! 작성해볼게요!
추가로 carSize에 의한 테스트(자동차가 2대 이상 5대 이하인 경우)에 대한 경계값 테스트도 작성해보았습니다!


private void validateNameLength(String name) {
if (name.length() < 1 || name.length() > 5) {
throw new IllegalArgumentException();

Choose a reason for hiding this comment

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

    private Cars readCars() {
        try {
            return createCars(inputView.readCarNames(), new CarAccelerator());
        } catch (Exception exception) {
            outputView.printErrorMessage(exception);
            return readCars();
        }
    }

이런 식으로 한번 해보시겠어요?ㅎㅎ 다만 재귀적으로 호출되어서 문제가 발생할 수도 있어요~

@seovalue seovalue merged commit 726c26a into woowacourse:pgmjun Feb 17, 2024
@PgmJun PgmJun changed the title [1단계 - 자동차 경주 구현] 이든(최승준) 미션 제출합니다. [1단계 - 자동차 경주] 이든(최승준) 미션 제출합니다. Apr 15, 2024
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