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

[조수연] 자동차 경주 미션 Step1 #5

Merged
merged 21 commits into from
Jun 26, 2024

Conversation

suyeon1218
Copy link

미션을 수행하며 어려웠던(고민한 점)

구조 (우측이 현재 구조)

image

1. 처음 생각한 구조(좌측)

  • 처음엔 Main 클래스가 CarRacing 클래스의 중재자 역할을 하는 구조를 생각했습니다.
  • Main 클래스가 Car 인스턴스가 담긴 배열을 this.cars 로 관리하며 필요할 때마다 Racing 클래스를 호출하여 경기 진행을 Main 에서 처리하는 로직이었습니다.
  • 하지만 승자를 판별할 때 결국은 Racing 클래스에 Car 인스턴스들을 모두 넘겨줘야 하고, Main 에서 경기 진행을 하게 될 시 Racing 클래스의 쓸모가 딱히 없을 것 같아(...) 후자로 바꾸었습니다.

2. 현재 구조(우측)

  • Main 클래스

    • 후에 여러 게임을 진행하고 싶을 때를 대비해 라운드를 배열형태로 초기화할 수 있도록 했습니다. rounds 를 순회하며 새로운 게임을 만들고, 각 번호만큼 라운드를 실행합니다.
      const main = new Main([GAME.ROUND]);
    • Racing 클래스에 라운드와 자동차 이름을 전달해 초기화합니다.
    • Racing 클래스의 초기화가 이뤄진뒤 racing.play 를 호출하여 게임을 진행시키고, 승자를 반환받아 출력합니다.
  • Racing 클래스

    • Car 클래스를 호출합니다.
    • Main 으로부터 진행할 라운드와 자동차 이름을 받습니다. 자동차 이름이 올바르면 Car 인스턴스 배열을 생성해서 경기에서 사용할 자동차들을 관리합니다.
    • play 메서드를 외부에서 호출해서 실행합니다.
  • Car 클래스

    • 이름과 위치를 기억합니다.
    • move 를 통해 원하는 만큼 이동할 수 있습니다.

보완할 부분

  1. 글을 쓰다보니 생각이 났는데 같은 Racing 이나 Car 를 재사용하기 위해 init 등의 메서드가 있어도 괜찮을 것 같습니다.
  2. 도메인 로직과 비즈니스 로직을 완전히 이해하지 못해서 다른 분들의 PR 리뷰를 보면서 차근차근 고치겠습니다.

리뷰 받고 싶은 부분

  1. 컨벤션이 적절한지
  2. 폴더 구조가 적절한지
  3. 코드에서 보이는 안 좋은 습관이 있는지... ㅠㅠ

이것 외의 더 많은 논의를 나눠도 좋습니다 👍

궁금한 점

사소한 질문도 다 써놔서 이전 PR 리뷰에 이미 작성해놓은 게 있다면 찾아가며 후에 수정하겠습니다...!! 단순히 제가 이런 고민을 했구나~ 정도로 봐주셔도 됩니다!!

콘솔의 입출력에 대해

  1. 콘솔 input 과 output 을 따로 관리하는 게 좋은 선택일까요?

    input 과 output 을 알려주는 문자열을 클래스 내부에 작성하게 되면 직관적으로 어떤 역할을 하는지 알 수 있을 것 같아서 좋아보이기도 했지만 코드의 가독성 측면에서 정리하고자 하는 습관이 있어 우선은 분리해놓은 상태입니다. 그런데 이렇게 하니 InputView 와 OutputView 가 뭔지 모르는 사람 입장에선 헷갈릴 우려가 있을 것도 같아요.

  2. inputView 에서 후처리를 해주는 게 좋을까요, 클래스에서 해주는 게 좋을까요?

    이름을 , 로 구분지어 문자열로 받다보니, split 메서드를 필수로 사용해야 했습니다. 그런데 이 코드를 inputView 에서 사용할지, main 클래스에서 사용할지 고민했어요. 그러다 inputView 는 readLine 을 사용하는 거니까 모두 문자열로 받아오는 게 헷갈리지 않을까라는 생각을 했는데 다른 분들의 의견이 궁금합니다.

테스트 케이스에 대해

  1. 테스트를 진행할 모델이나 유틸함수가 여러개 있는 경우 test 폴더 내부에서 파일 관리를 보통 어떻게 해주나요?
  2. 클래스 메서드를 테스트 하기위해 인스턴스를 test 콜백함수 내부에서 생성하거나 describe 내부에서 전역으로 생성했는데 함수 내부나 클래스 내부의 메서드는 이렇게 테스트하는 게 맞을까요?
  3. 특정 유틸함수가 문자열인지, 숫자형인지 등을 판별해야 한다면 일일이 each 로 값을 넣어주는 방법보단 상수로 분리해놓고 거기서 가져다 쓰는 것이 편리할 것 같은데 실제론 어떻게 반복을 줄이나요?

유효성 검사인 validation 파일에 대해

  1. 규모가 커지면 수많은 validation 을 하나의 객체 안에서 관리하기엔 무리가 있어 보이는데 이 경우엔 validation 을 어떻게 관리하나요?
  2. validation 함수의 네이밍 컨벤션이 존재하나요? (있다면 isValidatedCar 와 같은 형태가 좋은지 isString 과 같은 형태가 좋은지도 궁금합니다...!)

클래스 정의에 대해

  1. 정적 프로퍼티의 경우 특정 인스턴스에 종속되지 않는 상수 값 등을 저장할 때 쓰는 것으로 이해했는데 이게 맞을까요?
  2. 프라이빗 프로퍼티의 경우 외부에서 직접 접근을 방지할 목적에서 사용했지만 외부에서 접근이 필요한 경우도 있을 것 같아 set~, get~ 메서드를 만들었습니다. 이 경우 get, 혹은 set 시 유효성 검사를 추가로 등록해줄 수 있다는 점에서 일반적인 접근과 다르다고 생각하는데 프라이빗 프로퍼티는 되도록 set 이나 get 을 사용하지 않는 게 좋을까요?

@suyeon1218 suyeon1218 requested a review from JunilHwang June 24, 2024 04:51
@suyeon1218 suyeon1218 self-assigned this Jun 24, 2024
Copy link

@seeyoujeong seeyoujeong left a comment

Choose a reason for hiding this comment

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

코드 잘 읽었어요! 테스트 코드 짜느라 고민을 많이한 흔적이 보입니다 ㅎㅎ


inputView 에서 후처리를 해주는 게 좋을까요, 클래스에서 해주는 게 좋을까요?

저는 지금 클래스에서 해줬지만... 구분자가 뭘지 도메인 클래스에서는 모른다고 생각하고 후처리해서 배열로 넘겨주는게 좋을 것 같다는 생각이 떠올랐네요... 저도 수정을..ㅎ

Comment on lines +5 to +12
const car = new Car('meca');
const position = 4;

test('setName 에 올바른 문자열을 할당하면 해당 문자열을 반환한다.', () => {
const nextName = car.setName('Dva');

expect(nextName).toBe('Dva');
}),

Choose a reason for hiding this comment

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

재사용성을 위해서 Car 클래스의 인스턴스를 테스트들 상위에 생성해주셨는데 제가 읽고 있는 책에서 아래와 같은 단점이 있다고 해요!

  • 테스트 간 결합도가 높아진다.
  • 테스트 가독성이 떨어진다.

그래서 각 테스트마다 필요한 코드를 팩토리 메서드를 활용해서 중복을 줄이면 좋다고 합니다.

function creatCarWithName() {
  return new Car('meca');
}

팩토리 메서드는 지금 테스트 파일에 선언하면 되구요!

Copy link
Author

Choose a reason for hiding this comment

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

헉... 테스트 코드 책 저도 읽어야 할까봐요 ㅋㅋㅋ ㅠㅠㅠㅠ 알려주셔서 감사합니다! 기록해둬야겠어요!

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.

원래는 세미콜론으로 하다가 인터넷에선 또 쉼표로 구분을 하는 경우를 봐서 describe 내부의 test 를 객체처럼 취급하기 위해 그런건가? 라는 생각을 했는데 흔치는 않은 것 같네요... 테스트간 공백도 추가하겠습니다!

Comment on lines +12 to +14
expect(() => {
validation.isValidateRounds(rounds);
});

Choose a reason for hiding this comment

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

toThrow()가 빠진듯합니다!

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 +13 to +20
setRounds(rounds) {
if (!validation.isValidateRounds(rounds)) {
return [];
}

this.#rounds = rounds;
return this.#rounds;
}

Choose a reason for hiding this comment

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

setter로 만들어도 좋을듯합니다!

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 +19 to +21
getName() {
return this.#name;
}

Choose a reason for hiding this comment

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

이건 getter로!

Comment on lines +18 to +20
getCars() {
return this.#cars;
}

Choose a reason for hiding this comment

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

this.#cars가 배열이라 외부에서 수정하면 영향을 받을 것 같네요!

Comment on lines +4 to +13
isValidateRounds: (rounds) => {
if (Array.isArray(rounds) === false) {
throw new Error('rounds 는 배열이어야 합니다.');
}
if (rounds.some((round) => typeof round !== 'number')) {
throw new Error('rounds 는 숫자 배열이어야 합니다.');
}

return true;
},

Choose a reason for hiding this comment

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

뭔가 is를 접두사로 가진 메서드는 실패시 false를 반환해줄거라 기대하게 되는데 이 메서드는 그렇지않아서 검증 실패하면 false를 반환하게 수정하고 에러를 던지는건 따로 분리해도 좋을듯합니다!

Copy link
Contributor

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.

안 그래도 false 를 반환해야 할지 아니면 에러를 바로 반환할지 고민했는데 1. is 를 사용하지 않거나 2. 따로 에러처리를 하는 방법을 생각해보겠습니다!

Copy link

@jgjgill jgjgill left a comment

Choose a reason for hiding this comment

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

안녕하세요 수연님..!
이렇게 리뷰로 인사드리게 되네요!

콘솔의 입출력에 대해

저는 좋다고 생각했어요..!
제가 생각했을 때 현재 코드에서 Input은 API 요청, model은 커스텀훅, Output은 컴포넌트로도 볼 수 있을 것 같아요.
그래서 저는 따로 분리하는게 맞지 않을까 싶었네요.

테스트 케이스에 대해

으음 테스트에서만 쓰는 유틸함수들이 뭐가 있을까요? 해당 함수들이 왜 서비스 코드에는 쓰이지 않는지 궁금해지네요..! 🤔

미션 수고 많으셨습니다..! 🙇‍♂️

name: ['hello'],
},
{
name: '',
Copy link

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.

그렇겠네요...! 수정하겠습니다!

{ rounds: null },
{ rounds: ['12', '23'] },
])(
'Main 클래스 초기화 시 전달하는 ($rounds) 는 문자열 배열이 아니면 에러를 반환한다.',
Copy link

Choose a reason for hiding this comment

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

문자열 배열이 아닌 숫자 배열이지 않을까 싶었어요..!

const carSet = new Set(['jjanggu', 'hodu']);
const racing = new Racing(5, ['jjanggu', 'hodu']);

test('setRound 에 올바른 값을 할당하면 round 가 해당 값으로 바뀐다.', () => {
Copy link

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.

좀 더 명시적으로 작성하도록 해야겠네요...!!

}),
test('dice 메서드는 0~9 사이의 값을 반환한다.', () => {
const diceResult = [...new Array(10)].fill(Racing.dice());
const isCollectNum = diceResult.every((dice) => dice <= 9 && dice >= 0);
Copy link

Choose a reason for hiding this comment

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

사소한데 0 <= dice && dice <= 9 가 더 직관적일 것 같아요..!

Copy link
Author

Choose a reason for hiding this comment

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

중요한 걸 지적해주신 것 같아요!! 감사합니다!

},
{ round: 50 },
])(
'라운드($round)가 1 이상 50 이하의 숫자면 에러를 발생하지 않는다.',
Copy link

Choose a reason for hiding this comment

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

오오 라운드가 50까지 정해진 이유가 있을까요?? 🧐

Copy link
Author

Choose a reason for hiding this comment

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

요구사항엔 5까지라고 했지만 후에 라운드를 자유롭게 변경하기 위해 & 그래도 범위 제한이 필요할 것 같아 임의로 지정해주었습니다!

Copy link

Choose a reason for hiding this comment

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

저 개인적으로는 index 파일을 보면 export를 모아두는게 형태들이 먼저 떠오르더라구요..!

Copy link
Author

Choose a reason for hiding this comment

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

저도 평소엔 인덱싱 용도로만 사용하는데 폴더 내에 종종 파일이 하나밖에 필요하지 않은 경우 index.js 에 바로 선언을 하게 되더라구요...! 종길님 말을 듣고 나니 해당 패턴이 오히려 가독성을 해치진 않는지 고민해보게 되네요...!

}

setName(name) {
if (!validation.isValidateCarName(name)) return '';
Copy link

Choose a reason for hiding this comment

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

중괄호를 포함시켜도 괜찮을 것 같아요..! 😆
curly

Suggested change
if (!validation.isValidateCarName(name)) return '';
if (!validation.isValidateCarName(name)) {
return '';
}

Copy link
Author

Choose a reason for hiding this comment

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

린트 세팅을 좀 더 꼼꼼히 해야겠습니다 ㅠㅠ!!

});
}

static dice() {
Copy link

Choose a reason for hiding this comment

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

Racing이라는 모델과 dice가 같이 결합될 필요가 있을까 라는 생각을 했어요..!
제가 느꼈을 때 dice는 유틸 함수 느낌??

Copy link
Author

Choose a reason for hiding this comment

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

저도 dice 가 꼭 Race 에 있을 필요는 없다고 생각하는데 아직은 다른 곳에서 쓰이지 않다보니 그대로 뒀어요...! 그래도 결국엔 나중을 생각해서라도 util 로 빼는 게 맞는 것 같네요!

Copy link

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.

OMG 🤯 중요한 요구사항을 제가 빼먹은 것 같아요 ㅠㅠ!! 수정하도록 하겠습니다!!

Copy link
Contributor

@JunilHwang JunilHwang left a comment

Choose a reason for hiding this comment

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

안녕하세요 수연님!
수연님 리뷰는 처음인 것 같네요 ㅋㅋㅋ


폴더 구조가 적절한지

지금 유틸의 validation이 domain과 상호 의존하고 있어요. 이런 부분은 개선하면 좋을 것 같아요!

콘솔의 입출력에 대해

저는 "콘솔 입출력"이 리액트로 따지면 "컴포넌트" 라고 생각합니다.
도메인은 리액트로 따지면 "hook"에 해당하는거죠 ㅎㅎ

위와 같은 관점에서 생각하고 판단해보면 어떨까요?

테스트를 진행할 모델이나 유틸함수가 여러개 있는 경우 test 폴더 내부에서 파일 관리를 보통 어떻게 해주나요?

저는 그냥 test 폴더 하위에 유틸 함수나 목업 데이터를 관리하는 파일을 만들어서 관리한답니다!

클래스 메서드를 테스트 하기위해 인스턴스를 test 콜백함수 내부에서 생성하거나 describe 내부에서 전역으로 생성했는데 함수 내부나 클래스 내부의 메서드는 이렇게 테스트하는 게 맞을까요?

이렇게 해주셔도 무방합니다!
반대로, 이렇게 하면 안 되는 이유는 뭘까에 대해 고민해보셔도 좋아요 ㅎㅎ
항상 정답이 있다기보단 선택이 있다고 생각합니다.

특정 유틸함수가 문자열인지, 숫자형인지 등을 판별해야 한다면 일일이 each 로 값을 넣어주는 방법보단 상수로 분리해놓고 거기서 가져다 쓰는 것이 편리할 것 같은데 실제론 어떻게 반복을 줄이나요?

개인적으로 상수로 검증하는건 지양하는 편인데, 그 이유는 "스펙이 변경되었을 때" 테스트에서 사이드 이펙트를 감지하기가 어렵기 때문입니다.
상수만 바뀌면 테스트가 통과되는데, 그게 테스트를 작성하는 목적에 적합한걸까요?

규모가 커지면 수많은 validation 을 하나의 객체 안에서 관리하기엔 무리가 있어 보이는데 이 경우엔 validation 을 어떻게 관리하나요?

자기 자신에 대한 검증은 자기 자신이 책임져야겠죠!?
그리고 validation도 도메인에 대한 검증이 있고, 도메인 외에 대한 검증이 있을 것 같아요.
그래서 이건 이름을 지어주는 것도 무척 중요하다고 생각합니다.
자세한건 상세 리뷰를 참고해주세요!

validation 함수의 네이밍 컨벤션이 존재하나요? (있다면 isValidatedCar 와 같은 형태가 좋은지 isString 과 같은 형태가 좋은지도 궁금합니다...!)

저는 보통 isXXX -> true/false 를 반환, validateXXX -> 오류 발생

이런식으로 사용하는 편이랍니다. 수연님은 두 가지를 섞어주신 것 같네요..!

정적 프로퍼티의 경우 특정 인스턴스에 종속되지 않는 상수 값 등을 저장할 때 쓰는 것으로 이해했는데 이게 맞을까요?

static을 말씀하시는거겠죠!?
꼭 상수만 저장한다기보단... 뭐랄까... class instance에 포함시키기엔 적절하지 않지만 class의 일부 역할을 수행하는 것들을 모아놓는다고 생각하고 있어요.

Object의 method를 떠올려보세요!

프라이빗 프로퍼티의 경우 외부에서 직접 접근을 방지할 목적에서 사용했지만 외부에서 접근이 필요한 경우도 있을 것 같아 set~, get~ 메서드를 만들었습니다. 이 경우 get, 혹은 set 시 유효성 검사를 추가로 등록해줄 수 있다는 점에서 일반적인 접근과 다르다고 생각하는데 프라이빗 프로퍼티는 되도록 set 이나 get 을 사용하지 않는 게 좋을까요?

참고로 get/set 이라는 이름을 사용하는게 바람직한 패턴은 아니랍니다 ㅎㅎ
단순히 데이터 구조를 표현하는거면 몰라도 도메인 객체의 경우 이미 주어진 어떤 로직이 있기 때문에, 특히 set은 불필요한 경우가 많아요.

set을 사용할바에는 인스턴스를 새로 만드는게 나은거죠. 그래야 인스턴스가 어떤식으로 사용될지 예측하기가 더 쉬워요!

}

setName(name) {
if (!validation.isValidateCarName(name)) return '';
Copy link
Contributor

Choose a reason for hiding this comment

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

CarName에 대한 validation인데, 다른 파일의 내용으로 분리할 필요가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

util 폴더엔 있기엔 특정 도메인에 의존한다는 생각을 해서 이 방법이 맞는지 고민이 되었는데 다른 분들 코드를 보니 static 으로 유효성 검사를 해주더라구요...! 저도 해당 방법으로 바꿔보겠습니다!

Comment on lines +7 to +8
constructor(name) {
this.#name = this.setName(name);
Copy link
Contributor

Choose a reason for hiding this comment

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

어차피 setName에서 name을 변경해주게 되어 있어서

아예 setName 내부에서 this.#name 을 변경하는 로직을 제거한 다음이 이름이 getValidatedName 처럼 표현되어야 자연스럽지 않을까 싶어요!

Copy link
Author

Choose a reason for hiding this comment

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

사용하는 쪽에서 어떻게 쓸지 몰라서 setName 에서도 name 을 변경하게 해주고 return 값으로도 변경할 수 있도록 했는데 확실히 그쪽이 더 자연스럽겠네용...!!

}

setName(name) {
if (!validation.isValidateCarName(name)) return '';
Copy link
Contributor

Choose a reason for hiding this comment

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

그리고 여기서 빈 값을 리턴하는건 이 메소드의 일관성이 조금 깨지는 부분 같아요!
아예 오류를 발생시킨다거나, 혹은 아무일도 일어나지 않는다거나 하는게 좋지 않을까요!?

Comment on lines +23 to +28
setPosition(position) {
if (!validation.isValidatePosition(position)) return this.#position;

this.#position = position;
return this.#position;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

앞의 리뷰를 참고해주세요!

Comment on lines +22 to +29
setRound(round) {
if (!validation.isValidateRound(round)) {
return 0;
}

this.round = round;
return this.round;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

round가 private이 아닌 public을 참조하고 있네요!?
원하는 형태로 값이 제어가 되진 않을 것 같아요~

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 +4 to +13
isValidateRounds: (rounds) => {
if (Array.isArray(rounds) === false) {
throw new Error('rounds 는 배열이어야 합니다.');
}
if (rounds.some((round) => typeof round !== 'number')) {
throw new Error('rounds 는 숫자 배열이어야 합니다.');
}

return true;
},
Copy link
Contributor

Choose a reason for hiding this comment

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

유틸 함수로 만들고 싶다면, 여기서 도메인과 의존적인 용어는 전부 제거하는게 좋을 것 같아요.
가령, isValidateRounds 가 아니라 isValidateNumberArray 처럼 표현되어야겠죠?

Comment on lines +14 to +19
isValidateRacing: (cars, round) => {
return (
cars.every((car) => car instanceof Car) &&
validation.isValidateRound(round)
);
},
Copy link
Contributor

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.

아닌 것 같습니다... ㅠㅠㅠㅠㅠ!! Car 의 static 메서드로 만들어야겠어요!

Comment on lines +20 to +76
isValidateRound: (round) => {
if (isNaN(round) === true || typeof round !== 'number') {
throw new Error('라운드는 숫자형태여야 합니다.');
}
if (round < 1 || round > 50) {
throw new Error('라운드 횟수는 1회 이상 50회 이하여야 합니다.');
}

return true;
},
isValidateCars: (cars) => {
if (Array.isArray(cars) === false) {
throw new Error('cars 는 배열이어야 합니다');
}
if (cars.some((car) => typeof car !== 'string')) {
throw new Error('cars 는 문자열로 이뤄져야 합니다');
}

cars = cars.map((car) => car.trim());
const carSet = new Set(cars);

if (carSet.size !== cars.length) {
throw new Error('중복되는 이름이 있습니다.');
}
if (carSet.size < 2 || carSet.size > 10) {
throw new Error('자동차는 2개 이상 10개 이하여야 합니다.');
}

return true;
},
isValidatePosition: (position) => {
if (isNaN(position) || typeof position !== 'number') {
throw new Error('포지션은 숫자여야 합니다');
}
if (position < 0) {
throw new Error('거리는 최소 0부터 시작해야 합니다.');
}

return true;
},
isValidateCarName: (name) => {
if (typeof name !== 'string') {
throw new Error('자동차의 이름은 문자열 형태여야 합니다.');
}
name = name.trim();

if (name.length === 0) {
throw new Error(
'자동차의 이름은 공백을 제외한 한 글자 이상이어야 합니다.'
);
}
if (name.length < 1 || name.length > 10) {
throw new Error('자동차 이름은 한 글자 이상 열글자 이하여야 합니다.');
}

return true;
},
Copy link
Contributor

Choose a reason for hiding this comment

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

나머지 내용도 참고해주세요!

Comment on lines +22 to +33
async play() {
const result = this.#rounds.map(async (round) => {
const cars = await InputView.getCarName();
const racing = new Racing(round, cars);
const winners = racing.startRacing(cars);

OutputView.printWinners(winners);
return winners;
});

return result;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

플레이가 반환하는 값이... 뭔가 요상해보이네요 ㅋㅋ
Promise.all 로 감싸줘야할 것 같은 느낌!?

Copy link
Author

Choose a reason for hiding this comment

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

헉 그러게요...!!! Promise 배열이라고 생각 못했습니당...! 수정할게요!

const car = new Car('meca');
const position = 4;

test('setName 에 올바른 문자열을 할당하면 해당 문자열을 반환한다.', () => {
Copy link
Contributor

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.

더 명시적으로 테스트 코드 작성하도록 해볼게요!!

@suyeon1218 suyeon1218 changed the base branch from main to suyeon1218 June 25, 2024 06:42
@JunilHwang JunilHwang merged commit 2405a0d into fe-clean-code-study:suyeon1218 Jun 26, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants