[1단계 - 자동차 경주 구현] 엽토(김건엽) 미션 제출합니다. #167
Conversation
There was a problem hiding this comment.
안녕하세요 엽토~ 리뷰어 티케입니다. 🧚♀️
사실 푸만능, 엽토, 첵스에게 쓰는 이 전반적인 리뷰 내용은 공통부분이 많습니다. 이해해주셔서 감사합니다
저도 리뷰어가 처음이라, 많이 긴장이 되고 걱정 반 설렘 반이에요! 저야말로 잘부탁드립니다~ 리뷰를 보면서 무조건 적용하거나 받아들이기보다는 늘 의심하면서 비판적으로 받아주세요! 저도 잘 모르는 부분이 있을 수 있고, 엽토가 어떠한 의도에서 개발했는지가 훨씬 중요하니깐요. 많이 들어보셨겠지만, 개발에는 정답이 없습니다 :) 또 개인적으로 이해가 안 가면 질문도 남겨주시고 공개적인 곳이 부담스러우면 DM도 좋아요! 같이 고민해 나가보아요 🧡
첫 미션이 시간이 짧았을 텐데, 여러 가지 병행하면서 이렇게나 빠른 리뷰를 올려주시다니 영광입니다!
리뷰를 하면서 pretter, MVC, 여러가지 자료구조를 사용한 점이 인상깊었습니다 👍👍 예외상황에 대한 구현목록을 써주신것도 좋았고, 그대로 테스트한것까지 완-벽⭐️ 커밋 메세지도 깔끔하네요! 매우 좋아요 :) 상수화도 꼼꼼히 하셨더라구요~ 관련 리뷰는 구체적인 코멘트를 참고해주세요 :)
[전반적인 리뷰]
-
PR 메세지, README
어떤 작업을 했는지 이 프로젝트가 무엇인지에 대한 설명이 README 파일에 담겨있으면 좋았을 것 같아요. 저희 오픈소스나 라이브러리 찾아볼 때 리드미를 통해서 어떤 프로젝트도 어떻게 사용할 수 있는지 파악하는 거랑 같은 맥락이에요. 리뷰미를 꾸며봅시다 💦
또, 현업에서 일하다 보면 어떤 작업을 했는지 PR만 보고 파악하는 건 너무 어려워져서 저희 팀은 PR 템플릿을 통해서 자세하게 어떤 작업을 했는지 명시하는 작업을 하고 있어요. 우테코 미션도 동일하게 리뷰어들이 모든 미션에 대해 인지를 하려고 노력하지만 처음보는 사람들에게 리뷰를 맡긴다는 생각으로 엽토가 어떤 작업을 했고, 어떤 부분을 신경써서 했고, 구조는 어떻게 만들었으며, 어떤 점이 궁금했다, 어떤 점이 어려웠다, 실행했을 때의 이미지 등등 작업 내역과 소감을 담아주시면 좋을 것 같아요. 엽토가 어떤 것에 더 신경썻는지 알면 저도 그것에 맞춰서 리뷰를 줄 수 있을 것 같고, 첫 미션이 어땠는지 소감도 궁금하네요 :)
또 README에 내용이 없는경우 저는 package.json을 보는 편인데요, package.json은 이 프로젝트에 어떤 라이브러리 의존성이 있고, 어떻게 시행되고 등등의 많은 정보를 담고 있기 때문이죠. 그 부분에 대한 수정이 아에 존재하지 않는데 한번 공부를 해보시면 어떨까요? -
노드 패키지의 lock 파일이 2개 있던데.. 어떤걸 사용하시는 걸까요 :) 불필요한 파일은 삭제해주세요
-
클래스
모든 것을 클래스로 만드셨는데 특별한 이유가 있을까요? 또 static만 있는 클래스란 어떤 의미를 가지는 걸까요?
.vscode/settings.json
Outdated
| @@ -0,0 +1,12 @@ | |||
| { | |||
There was a problem hiding this comment.
페어와 setting을 맞추기 위한 것일까요? 그런 의도라면 굳..
There was a problem hiding this comment.
lint 설정을 했는데 잘 되지 않았어요.. 혹시 몰라 제 작업 디렉토리에 settings.json을 만들어보니 됐습니다. gitignore한다는 걸 깜빡한 것 같습니다!
__tests__/Car.test.js
Outdated
| @@ -0,0 +1,16 @@ | |||
| const Car = require('../src/Models/Car'); | |||
|
|
|||
| test('Car Class 테스트', () => { | |||
There was a problem hiding this comment.
Car class의 어떤걸 테스트하신걸까요? 좀 더 친절한 설명이 있으면 좋을 것 같아요
| } | ||
|
|
||
| static print(message) { | ||
| console.log(message); |
There was a problem hiding this comment.
에러 상황에서 정의해둔 에러메세지(영어)가 콘솔에 찍히지 않고 있네용 :)
There was a problem hiding this comment.
Console.print() 메서드는 message를 인자로 받아 출력만 해주는 메서드입니다! 혹시 에러메세지가 콘솔에 찍히지 않는다는 것이 무슨 말씀인지 다시 한번 설명해주실 수 있을까요?
src/Controller/RaceController.js
Outdated
| }); | ||
| } | ||
|
|
||
| handleRace(count) { |
There was a problem hiding this comment.
너무 포괄적인 네이밍같아요. Race를 핸들한다는 건 어떤 의미일까요?
There was a problem hiding this comment.
레이스하고 결과를 보여준다라는 것을 포괄적으로 변수명을 지어줬네요. 한 메서드에 두가지의 행위가 있는 것 같아서 분리를 해주고 싶었는데, 한번 진행 시 한번 출력해주어야하는데 분리하면 어떻게 해야할지 생각나지 않았습니다.
| this.getCarName(); | ||
| } | ||
|
|
||
| getCarName() { |
__tests__/Validator.test.js
Outdated
| ); | ||
|
|
||
| test.each([[' '], ['!'], [' '], ['123'], ['한123']])( | ||
| '자동차 이름이 한글이나 영문이 아닌 경우 예외처리한다.', |
There was a problem hiding this comment.
한글로만 한 이유가 궁금해요. 한글로만 하려면 사용자에게 미리 알려야하지 않을까요?
예를들어
한글로만 이루어진 이름을 입력해주세요. (ex. 갑,을)
There was a problem hiding this comment.
예시에 'pobi,crong,honux'라고 되어있었는데요, 1차원적으로 사람이름이 들어와야한다고 생각을 했어요. 지금 생각해보니 결국 자동차 이름일텐데 제가 만들어버린 필요없는 예외란 생각이 드네요. 예를들어 bmw 520d 처럼 테스트 입력값에서 '한123'은 굳이 에러를 던질 필요가 없을 것 같습니다. 제가 요구사항을 잘못 파악했네요ㅠ
|
엽토! 리뷰중에서 우선적으로 아래 코멘트부터 봐주시고, 나머지는 Step2에서 진행하겠습니다~ #167 (comment) |
|
티케! 우선적으로 봐달라고 하신 코멘트들 중 제가 이해를 하지 못한 것을 제외하고 코드에 반영했습니다! Step2에선 나머지 코멘트와 제가 리팩터링하고 싶은 부분들을 수정하여 PR메시지에 잘 반영해보겠습니다! |
| }; |
There was a problem hiding this comment.
EOL을 수동으로 처리하는 것 외에 자동화할 수 있는 방법은 뭐가 있을지 고민해보면 좋을 것 같아요~
There was a problem hiding this comment.
prettier 설정으로 endOfLine을 'auto'로 해주었습니다. 이것도 수동으로 보아야할까요?
There was a problem hiding this comment.
다음부터는 적용한 부분에 대해서 정리하거나, 아니면 이모지로 엽토만의 표시를 리뷰 코멘트에 남겨주면 리뷰어입장에서 보기 좋을 것 같아요 :)
꼭 이렇게 하라는 건 아니지만, 이런 아주 좋은 예시도 있다~ 정도 봐주시면 좋을 것 같아요. (이모지 혹은 댓글만으로도 충분합니다)
노드 패키지의 lock 파일이 2개 있던데.. 어떤걸 사용하시는 걸까요 :) 불필요한 파일은 삭제해주세요
위 리뷰만 완료되면 Step1은 머지하겠습니다.
적용해주신 부분들 중, 다시 코멘트를 남겼는데 엽토의 리뷰적용 속도가 빨라서 한번 더 request-changes를 보내요!
당연히 Step2에 가서 적용해주셔도 됩니다. 노드패키지 관련 피드백이 마무리 되면 Step2에서 코드리뷰 적용하며 리팩토링을 할지 아니면 Step1에서 한번 더 저랑 핑퐁을 할지 결정해주시면 됩니다~
__tests__/Car.test.js
Outdated
| @@ -1,6 +1,6 @@ | |||
| const Car = require('../src/Models/Car'); | |||
|
|
|||
| test('Car Class 테스트', () => { | |||
| test('Car Class move 메서드 테스트', () => { | |||
docs/REQUIREMENTS.md
Outdated
| @@ -12,6 +12,6 @@ | |||
|
|
|||
| - [x] 시도횟수가 1이상의 숫자가 아닐 경우 예외처리한다. | |||
| - [x] 이름이 다섯자 이하가 아닐 경우 예외처리한다. | |||
| - [x] 이름에 한글이나 영문이 아닐 경우 예외처리한다. | |||
| - [x] 이름에 숫자만 있거나 이름이 문자라면 예외처리한다. | |||
| }); | ||
| } | ||
|
|
||
| handleRaceAndShowResult(count) { |
lock 파일이 두개가 생성되어 yarn.lock을 지우다
이름에 숫자만 있거나 이름이 특수문자라면 예외처리한다로 수정하다
|
코멘트 하나씩 보면서 해결한 것은 리코멘트남기고 잘 이해하지 못했다하는 것은 이해하지 못해서 질문 드린다고 리코멘트를 남기면 되는줄 알았습니다. 처음 코드리뷰를 해봐서 많이 서툴렀던 것 같습니다. 무엇을 반영했는지 정확히 말씀을 드렸어야했는데.. 정말 죄송합니다!!! lock 파일이 두개인 이유는 오늘 공통 피드백 시간에 공원이 미션 처음 만들때 깜빡하고 yarn lock 파일을 올리신 것 같더라구요! 리뷰어 분들께 말씀드리라고 했었습니다! 그래서 yarn.lock을 지우고 반영했습니다! 1단계 피드백 반영 목록
1단계 재리뷰요청 후 반영 목록
Step 2에서 남은 피드백들을 꼼꼼히 확인하여 리팩토링 하도록 하겠습니다. 리뷰요청 두번이나 불편하게 해드린 점 정말 죄송합니다.. 그리고 꼼꼼한 피드백 주셔서 정말 감사합니다! |
80은 defalut값이기도 하면서 요즘은 브라우저가 넓어서 오히려 좁을 수 있다
일관성이 있도록 한글로 변경하다
전~~혀 아니에요! 죄송할 일 아니고, 이렇게 정리를 해두면 당장 낼모레 엽토가 작업을 할때라도 도움이 되어서 알려드린 팁이에요!
자동차 미션은 일주일이라서 빠르게 2단계로 넘어가야하는 부분이 있지만, 피드백 반영이 빨라서 리뷰요청을 한번 더 드린거랍니다~ 죄송할 일이 정말 아니고! 리뷰는 원래 두번정도 주고받아요 :) 피드백 반영 빠르게 잘 해주셔서 감사합니다~ 수고하셨어요! |
|
넵 알겠습니다!! 티케도 수고많으셨어요! 2단계에서 다시 뵙겠습니다 😀 |
안녕하세요 티케😀
장거리 여행 고생 많으셨어요!
첫 미션이라 많이 부족하지만 코드리뷰 잘 부탁드립니다!!!
피드백을 적극적으로 수용하겠습니다!! 많이 알려주시면 정~말 정말 감사하겠습니다🫡