[클린코드 8기 김민석] 자동차 경주 미션 STEP 3 #360
Conversation
There was a problem hiding this comment.
안녕하세요 민석님!
🔥 열정적으로 미션에 임해주셔서 저한테도 너무 힘이 되고 동기부여가 됩니다. 감사드려요 🙏🏻
저번에 해 주신 고민점에 관해 답을 드리지 못했군요ㅠ 제가, 코멘트로 따로 남겨두었으니 코드 리뷰 확인하시면서 살펴주시길 바랄게요!
또한, 이번 고민점에 대해서도 코멘트 간략히 남겨두겠습니다~
이번 피드백도 파이팅이에요 💪🏻💪🏻💪🏻
- view와 model을 분리하고 싶다고 하셨는데요. 어떤 부분에서 고민점이 있으신지 보다 자세히 설명해주시면 같이 고민해 볼 수 있을 것 같습니다🙂.
- race.test.js에 별도로 코멘트 드렸습니다. '단위 테스트'의 본질에 대해서 생각해 보신다면, 좀 더 이해가 수월하실 거예요! 테스트라고 해서 무조건 파일별로 테스트 코드를 작성해야 하는 것은 아니니까요!
__tests__/model/car.test.js
Outdated
| @@ -0,0 +1,52 @@ | |||
| import {Car} from '../../src/model/car'; | |||
There was a problem hiding this comment.
이전에 질문 남겨주신 부분 이곳에 옮겨 드릴게용
tests 파일로 분리하여 테스트 작성할 때 import {Car} from '../src/car';를 작성 하는 것이 번거롭네요..!🤣 js 개발할 때는 이를 모두 인지하며 하나요? 아니면 더 편한 방법이 있나요? java로 개발할 때는 같은 패키지로 선언된 경우 바로 대응되는 클래스 접근이 쉬웠어서 js의 경험이 어색하네용 ㅎㅎ
자바스크립트는 Java와는 다르게 엄격한 타입 검사를 거치지 않아서인지, import할 때 vscode나 다른 IDE를 통해 자동 완성 기능을 이용하기가 번거로우실 수 있습니다🥲. 요즘 추세는 자바스크립트의 유연성을 보완하면서도 타입의 일관성을 유지할 수 있는 TypeScript를 많이 이용하는데, TypeScript를 사용하신다면 이러한 점이 자연스럽게 보완될 거예요!
__tests__/model/car.test.js
Outdated
|
|
||
| test('생성자로 설정할 수 있다.', () => { | ||
| // given | ||
| let carName = "둥둥이"; |
There was a problem hiding this comment.
자바스크립트에서는 한 번의 할당 이후에 더 이상 값 변경이 필요하지 않다면, let보다는 const 키워드를 우선적으로 사용하고 있어요. 왜냐하면, const는 단 한 번의 할당만 가능하기 때문에 이후에 재할당될 여지를 두고 코드를 읽을 필요가 없기 때문이에요. 이 부분을 고려해서 let을 const로 대체할 수 있는 부분이 있다면, 개선해 보시는 건 어떨까요?
__tests__/model/car.test.js
Outdated
| expect(car.name).toEqual(carName); | ||
| }) | ||
|
|
||
| test.each([ |
__tests__/model/car.test.js
Outdated
| ])('길이는 1이상 5이하여야 한다.', (name, expected) => { | ||
| // when, then | ||
| if (expected) { | ||
| expect(() => {new Car(name)}).not.toThrow(); | ||
| } else { | ||
| expect(() => {new Car(name)}).toThrow(new Error('자동차 이름은 1글자 이상, 5글자 이하여야 합니다.')); | ||
| } | ||
| }) | ||
| }) |
There was a problem hiding this comment.
each를 사용하신 의도는 좋았지만, 이 경우에는 명시적으로 정상적으로 통과해야 하는 경우(1~5글자)와, 예외가 발생해야 하는 경우(1~5글자가 아닌 경우)의 테스트 케이스가 분리되어야 하지 않을까요? 🤔
__tests__/model/car.test.js
Outdated
| describe('이름을 상태로 가진다.', () => { | ||
|
|
||
| test('생성자로 설정할 수 있다.', () => { |
There was a problem hiding this comment.
Grouping하시는 의도는 이해했는데, test 케이스 분류가 어색하게 느껴집니다. describe에서는 "이름에 대한 테스트"를 하고 계시지만, 아래의 test 케이스에서는 자동차 이름이 아니라 Car 생성자를 만드는 테스트를 하고 있지 않나요?
아마 의도하신 바는, '자동차 > 이름 > 이름을 생성자로 설정할 수 있다'는 체계를 구축하려고 하신 것 같은데, 지금 describe의 '이름을 상태로 가진다'라는 그룹 이름이 이하 test의 명세와 조금 이질적이라는 느낌을 주고 있는 것 같아요.
따라서, 아래의 테스트 구조가 좀 더 일관성이 있을 것 같아요!
describe('자동차', () => {
describe('이름' /*📌*/, () => {
test('생성자로 설정할 수 있다.', () => { ... })
test('길이는 1이상 5이하여야 한다.', () => { ... })
// ...
})
})There was a problem hiding this comment.
클래스의 경우 Race.js로 파일명을 변경하심이 좀 더 타당할 것 같아요~!
There was a problem hiding this comment.
play가 명사로도, 동사로도 쓰이지만, 지금 상태에서는 조금 모호하다는 느낌이 드는데요. 좀 더 파일의 이름을 구분짓기 위한 이름이 있을까요? 제 생각에는 main.js가 play 함수를 머금고 있어도 지금 상황에서는 괜찮아 보입니다. 혹시 따로 play로 빼신 이유가 있을까요?
| #position = 0; | ||
|
|
||
| constructor(name) { | ||
| if (name.length < 1 || name.length > 5) { |
There was a problem hiding this comment.
지금은 도메인이 간단해서 자동차의 이름의 길이가 1~5인지 유효성을 검사하는 내용이라고 쉽게 눈치챌 수 있는데요.
하지만, 프로그램 구동 과정에서 필요한 상수라면 따로 정의해 두는 편이 명확하지 않을까요? 예를 들어 1은 무슨 숫자를 의미하는 것인지, 5는 무슨 숫자를 의미하는 것인지를요!
| constructor(name) { | ||
| if (name.length < 1 || name.length > 5) { | ||
| // todo : ModelError 정의 | ||
| throw new Error('자동차 이름은 1글자 이상, 5글자 이하여야 합니다.'); |
There was a problem hiding this comment.
마찬가지로 예외 메시지도 상수로 정의해 둔다면, 관리하시기 편할 거예요!
| import {Car} from '../../src/model/car'; | ||
| import { Race } from '../../src/model/race'; | ||
|
|
||
| describe('경주', () => { |
There was a problem hiding this comment.
'경주'라는 요소를 테스트하기 전에 먼저 책임을 명확히 해야 할 것 같아요. 이미, 자동차에서 테스트하고 있는 내용을 똑같이 테스트할 필요가 있을까요? 즉, “경주(Race)”라는 객체의 요구사항을 검증해야 하는 자리에서, 사실상 자동차(Car)의 내부 동작을 직접적으로 테스트하고 있다는 점을 말씀드리고 싶었어요. 단위 테스트의 의미를 조금 더 살펴주시면 이해가 되실 거예요 🙂
2 participants
고민점
궁금한 것