[2단계 - 자동차 경주] 에프이(박철민) 미션 제출합니다. #306
Conversation
There was a problem hiding this comment.
안녕하세요 에프이~
정말 빠르게 step2를 진행해주셨군요 ㅎㅎ 멋집니다 👍 👍 👍
전체적으로 깔끔하게 잘 수정해주시고, 상수도 잘 정리해주셨네요 👍
고민한 것
현재 WinnerService 테스트가 통합 테스트 느낌에 가깝다는 코멘트를 해주셨습니다. 제 의도대로 코드가 동작하는지 잘 확인할 수 있었지만 2단계 요구 사항은 단위 테스트에 초점을 맞춘 느낌이었기 때문에 WinnerService로만 유닛 테스트를 할 수 있을지 고민해보았습니다.
현재 WinnerService 클래스는 자동차 이름 배열과 position 배열이 아닌, Car 객체 배열을 생성자의 인자로 받고 있습니다. 이러한 구조에서 WinnerService 단위 테스트를 하기 위해서는 CarList 객체를 만드는 것이 선행되어야 합니다. CarList 객체 없이 WinnerService를 테스트하려면 CarList에서 position 배열을 추출해서 넘겨줘야 합니다. 하지만 배열을 추출하는 것을 WinnerService로 위임했기 때문에 앞서 리팩토링한 것을 되돌려야 합니다.
따라서 WinnerService를 더 작은 단위로 분리하지 못했습니다. 좋은 방법이 있는지 궁금합니다.
이전 WinnerService의 형태를 생각해보면 오히려 지금 WinnerService가 많은 역할을 하고 있다는 생각은 들지 않는데요. 작은 단위가 얼마나 작은 단위를 생각하고 계신건지 잘 모르겠네요 ㅎㅎ
아직은 미션을 하는 데 있어 git 사용법이 익숙지 않은 것 같습니다😅 step1 merge 이후 원격 저장소를 pull하지 않고 그대로 진행하다가 PR conflict가 발생했고, 이를 찾아서 해결하는데 많은 시간이 걸렸습니다. step1 이후 새로 commit된 내용만 기록에 남는 게 리뷰하기 편하실 것 같은데, 이 부분은 양해 부탁드립니다. 가장 최근 커밋 7개가 step2 입니다!
해결하는 과정에서 git 사용이 좀 더 익숙해졌다는 느낌을 받았습니다.
history를 남겨주신 덕분에 편하게 변경사항을 확인 할 수 있었습니다 감사합니다 에프이 ㅎㅎ 👍 👍👍
git에 조금 더 익숙해졌다는 느낌을 받았다니 좋네요 ㅎㅎ 👍 👍 👍 앞으로도 git을 사용하시다가 이런저런 문제를 겪고 트러블슈팅을 하게 되실텐데 이런 경험을 통해 지금보다 더 성장하실거라고 생각합니다 👍
step1 질문 (모아서 보시는 게 편할거 같아 그냥 들고 왔습니다 ㅎㅎ)
이런 극단적인 상황이 생기지 않는다면 일반적으로는 특정한 컨벤션을 통해 상수임을 드러내고 있기 때문에 freeze()를 꼭 사용할 필요가 있을까 싶어서 드린 리뷰였습니다 ㅎㅎ
이해했습니다! 저는 누군가 실수로, 혹은 악의적으로 상수의 값을 변경하려는 시도를 하는 경우를 생각해봤어요(일종의 예외 상황으로). 그래서 그런 경우라도 값이 바뀌지 않게끔 freeze()를 사용했습니다.
하지만 말씀하신 것처럼 컨벤션을 통해 명확하게 드러냈기 때문에 불필요한 코드를 추가한 것이 될 수도 있을 것 같아요. 이런 경우에는 어느 정도 상황까지 고려해서 코드를 작성하는 것이 좋은가요?
이거는 정말 상황마다 달라지고 어떤 코드인지에 따라서도 달라지기 때문에 정답은 없는 부분인거 같아요. 예를 들어 정적 타이핑 언어를 사용해서 개발한다면 이런 실수들이 코드를 작성하는 단계에서 드러나고 쉽게 수정할 수 있기 때문에 이런 점을 모두 고려할 필요가 없을 수 있는데요. 반면 동적 타이핑 언어를 사용한다면 이런 실수들을 바로 알아채는 게 힘들고 이런 것들이 runtime에서 에러로 발생할 수 있기 때문에 이런 상황이라면 이런 실수를 고려하여 여러 장치들을 추가해야 할수도 있구요
코드를 작성하고 유지보수하는 데 있어 안정성과 개발 효율성 사이의 트레이드오프가 발생할 수 있는 부분이기 때문에 여러 경험을 통해 적절한 균형을 에프이가 찾아나가시면 좋겠네요 ㅎㅎ
이번 미션에서 추가로 드릴만한 피드백은 크게 없어서 이만 merge해도 좋을거 같습니다
첫번째 미션 고생 많으셨습니다 에프이~~ 앞으로도 성장해나가시는 모습 기대하겠습니다 👍
__tests__/Car.test.js
Outdated
| expect(car.getPosition()).toEqual(0); | ||
| }); | ||
|
|
||
| test.each(['pencil', 'woowacourse'])('5자를 초과할 경우 에러를 발생시킨다', carName => { |
There was a problem hiding this comment.
each() 메서드를 사용할 때 각각의 name을 테스트 명에 삽입해줘도 좋을거 같네요 ㅎㅎ 아래 링크를 참고해보시면 좋겠습니다~
https://jestjs.io/docs/api#1-testconcurrenteachtablename-fn-timeout
__tests__/CarList.test.js
Outdated
| describe('Car 테스트', () => { | ||
| test('랜덤값에 따른 전진 여부', () => { |
There was a problem hiding this comment.
랜덤값에 따른 전진 여부라는 이름으로는 어떤 상황에서 어떤 동작을 테스트해서 어떤 결과가 나와야 되는지 알기가 어렵네요 😅
구체적인 상황과 동작, 결과를 알 수 있도록 테스트명을 변경해보셔도 좋을거 같아요
| const getRandomNumberInRange = (from, to) => { | ||
| if (isNaN(from) || isNaN(to)) { | ||
| throw new Error('[ERROR] 범위의 시작과 끝은 숫자여야 합니다.'); | ||
| } | ||
| return Math.floor(Math.random() * (to - from + 1)); | ||
| }; | ||
|
|
||
| export default getRandomNumberInRange; |
| minRandomNumber: 0, | ||
| maxRandomNumber: 9, |
안녕하세요 :)
1단계 merge 이후 2단계 요구 사항과 기타 코멘트를 반영해서 리팩토링 했습니다.
수정한 것
폴더 구조 변경
constants, entities, services를 domain 하위에서 관리하고, 기존의 views도 요구 사항에 맞게 view로 이름 수정했습니다.
constants는 도메인 로직에서 필요한 상수를 정의했기 때문에 관련이 깊다고 판단했고, domain 하위에서 관리하도록 했습니다.
controller는 도메인 로직과 별개로 프로그램의 실행 흐름을 제어하기 때문에 별도로 관리합니다.
요구 사항 중 하나인 'domain은 view를 의존하면 안 된다'는 것은 현재 만족하고 있다고 판단했습니다.
util 함수 분리
상수 대체
고민한 것
단위 테스트
git 사용
p.s. step1에서 궁금한 부분이 있어 추가로 코멘트 남겨서 질문드렸습니다.
감사합니다!