-
Notifications
You must be signed in to change notification settings - Fork 172
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단계- 자동차 경주] 초코(강다빈) 미션 제출합니다. #273
Conversation
- 자동차 : 이름, 이전 단계의 전진 횟수(step), 랜덤 숫자 값을 받아온 후, 전진 여부를 판단해 전진
- 이름, 진행 횟수가 유효하지 않을 때, 다음 게임 단계 진행이 되는 오류 수정 - 오류 원인: catch 문에서 재귀함수를 비동기로 실행하지 않았음
- root/__tests__/utils/index.js -> root/testUtils/index.js로 변경
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 초코! 우테코에서 하는 첫 미션은 즐거우셨나요?
이번 미션 리뷰어를 맡게 된 블링입니다!
페어프로그래밍이 익숙치 않았다고 하셨는데, 경험상 금방 또 적응이 되더라구요! 너무 걱정마세요!
TLDR
전반적으로 리뷰어와 사용자에 대한 배려가 많이 느껴졌습니다.
테스트도 아주 깔끔하고 보기 좋게 구현해주신 것 같아요.
👍 이런 점이 좋았어요.
- README 작성 너무 좋았습니다.
- 비록 터미널이지만 사용자에게 필요한 정보를 주려고 노력한 모습이 보였습니다.
- 디렉토리별로 index 파일에서 reexport 해주는 것도 좋았어요.
- 테스트 명세가 읽기 쉽게 되어있어서 어떤 내용을 어떻게 구현했는지 파악하기 편했습니다.
🤔 이런 점을 더 고민해볼까요?
- MVC 패턴을 처음 사용했다고 하셨는데요, 잘 적용이 된 것 같은지, 그 패턴에 맞게 코드가 잘 분리되었는지 초코의 의견이 궁금합니다!
- 제가 무언가 잘못하고 있는지는 모르겠지만 첨부해주신 예시 실행 사진과 다르게 라운드 정보가 한 번만 나오고 나머지는 쭉 이어서 나오고 있어요! 한 번 확인해주시면 좋을 것 같습니다.
❓ 질문에 대한 답변
자바스크립트 공부... 우테코 첫 주부터 너무 그 고민에 몰두하지 않으셔도 괜찮아요! 이런저런 시도를 많이 해보면서 본인만의 방법을 잘 찾아내는 것이 좋지 않을까요? 사실 저도 공부는 어떻게 하면 좋은지는 아직도 고민입니다 😁
lint와 prettier도 차차 계속 사용하다보면 익숙해지실 거에요!
첫 미션 구현하느라 고생 많으셨습니다!
이런저런 의견 남겼는데 한 번 가볍게 봐주세요!
### 실행 | ||
```bash | ||
node src/index.js | ||
``` | ||
### 테스트 | ||
```bash | ||
npm run test | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
배려 넘치는 README 너무 감사합니다 🥹
name: ' [hint] 자동차 이름은 영문자,한글을 사용해 1자 이상 5자 이하로 해주세요.', | ||
delimiter: ' [hint] 자동차 이름은 쉼표(,)를 기준으로 구분해주세요.', | ||
round: ' [hint] 진행 횟수는 1이상 5이하의 정수만 가능해요.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[hint]가 나와서 표시되는 부분이 사용성이 좋은 것 같아요! 👍
name: ' [hint] 자동차 이름은 영문자,한글을 사용해 1자 이상 5자 이하로 해주세요.', | ||
delimiter: ' [hint] 자동차 이름은 쉼표(,)를 기준으로 구분해주세요.', | ||
round: ' [hint] 진행 횟수는 1이상 5이하의 정수만 가능해요.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상수의 property 역시 SNAKE_CASE로 표기하는 것에 대해서 어떻게 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 javascript 언어에서는 camel case가 더 많이 사용된다고 알 고 있어서 Message.js의 다른 property도 이런식으로 표기했었습니다. snake case가 더 좋은 걸까요?
export const OUTPUT_MESSAGE = Object.freeze({
roundResult: '실행 결과',
winner: '최종 우승자',
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음 이건 개인적인 선호의 영역이자 협업하는 사람들끼리의 컨벤션의 영역일 것 같아요!
만약에 camel이 더 적절하다고 생각하시면 그렇게 유지하도록 하시죠!
src/controllers/Game.js
Outdated
import OutputView from '../views/OutView.js'; | ||
import InputController from './InputController.js'; | ||
import { OUTPUT_MESSAGE } from '../constants/Message.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constants에서 reexport해주고 있는 것으로 봤는데 여기에선 Message.js에서 직접 import 하는걸까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아마 import 과정에서 확인을 못했던 것 같습니다😢 constants/index.js 파일이 의미없어지지 않도록 주의하겠습니다
import InputView from './InputView.js'; | ||
import OutputView from './OutView.js'; | ||
|
||
export { InputView, OutputView }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 깔끔하게 reexport 해주시는 부분들 좋은 시도 같아요!
어떤 계기로 이렇게 작성하게 되셨나요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
처음에는 그냥 페어가 이런식으로 사용해 왔다고 해서 작성했습니다. 지금 생각해 보니 다음과 같은 장점이 있는 것 같습니다.
- 한줄의 코드로 여러 개의 모듈을 가져올 수 있어 코드를 간결하게 유지할 수 있다.
- 코드를 읽는 사람이 코드 구조를 명확하게 파악할 수 있다.
src/controllers/Game.js
Outdated
const message = `${name} : ${Array.from({ length: step }, () => '-').join( | ||
'', | ||
)} `; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
미세꿀팁) 이런 걸 참고해봐도 좋지 않을까요? 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
훨씬 더 간결해지네요!! 내장 메서드를 활용할 줄 모르는 것 같아서 더 공부해보도록 하겠습니다😢
src/models/CarValidator.js
Outdated
import { ERROR_MESSAGE } from '../constants/Message.js'; | ||
|
||
const CarValidator = { | ||
private_confirmType(string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
함수를 이런 식으로 네이밍에서 private 처리해 준 이유는 무엇일까요?
단순히 함수를 export 하지 않고 내부에서만 사용할 수 있도록 구현하는 것과는 어떤 차이가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
자동차에 대한 유효성 검사는 여러 항목에 걸쳐서 해야했습니다. 그래서 페어와 의견을 나눠본 결과 confirm 함수를 통해서 자동차 유효성 검사를 관리하자고 결정했습니다.
export를 하지 않는 방식이 있을 수 있겠지만, private_ 네이밍을 사용하는 것이 코드를 읽는 입장에서 더 직관적일 것이라 판단했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음 일단 익숙하지 않은 방식이긴 합니다.. 불필요하게 함수의 이름이 많이 길어지기도 하구요.
export 한다는 것 자체가 일단 접근이 가능하다는 이야기니까 극단적으로 이야기해서 사용하려면 못할 것은 없을 것 같아요. 함수와 객체를 현재의 형태로 구성해야만 하는 것은 아니라고 생각합니다. 같은 객체 안에 넣었기 때문에 this를 통해서 접근을 해야한다는 점도 어색하게 느껴졌습니다.
다음 단계를 진행하면서 고민해보고 이 방법이 확실한 장점이 있다고 생각하시면 저를 설득해주세요! ㅎㅎ
src/models/CarValidator.js
Outdated
if (length >= 1 && nameArray.length - 1 !== commaCount) { | ||
throw new Error(`${ERROR_MESSAGE} (쉼표 오류)`); | ||
} | ||
}, | ||
|
||
private_confirmDuplicate(nameArray) { | ||
if (nameArray.length >= 2 && new Set(nameArray).size !== nameArray.length) { | ||
throw new Error(`${ERROR_MESSAGE} (이름 중복 오류)`); | ||
} | ||
}, | ||
|
||
private_confirmNumberOfCars(nameArray) { | ||
const { length } = nameArray; | ||
const pass = length >= 1 && length <= 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
검증을 하는 데에 사용하는 1, 5, 2 등의 숫자도 상수로 분리하게 된다면 게임의 규칙을 변경하고 싶을 때 조금 더 유연하게 바꿀 수 있지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 것 같습니다 반영하도록 하겠습니다!
class RandomNumber { | ||
static pickNumber() { | ||
return Math.floor(Math.random() * 10); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
유틸 함수를 class로 묶는 것과 그러지 않는 것 (일반 객체 형식, 아니면 그냥 export)에는 어떤 차이가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class 형식과 일반 객체 형식의 차이는 다음과 같다고 알고 있습니다.
- 클래스의 인스턴스 생성 : class는 ⭕ , 일반 객체는 ❌
- 프로퍼티를 통해 상태 유지 가능 : class는 ⭕ , 일반 객체는 ❌
- 상속을 통해 코드 확장 및 재사용 가능 : class는 ⭕ , 일반 객체는 ❌
따라서, 현재는 랜덤 숫자 생성 메서드는 하나뿐이지만 확장 가능성을 위해 클래스 형식이 맞다고 판단했습니다. 유틸 함수를 class로 묶는 것이 더 효율적인 게 맞을까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확장가능성 때문이라고 해주셨는데, 저는 이 클래스가 앞으로 더 복잡한 요구사항을 마주하더라도 클래스의 장점을 사용한 확장이 어떻게 이루어질지 상상이 잘 안가긴합니다. 과연 다른 형식으로는 풀어낼 수는 없을까 하는 생각도 들고요! 클래스를 사용하지 않으면 앞으로 확장이 어려울 것이라고 보시나요? 한 번 고민해보고, 다른 사람들은 통상적으로 유틸 함수들을 어떤 식으로 관리하는지도 한 번 찾아보면 좋을 것 같네요!!
describe('랜던 숫자에 대한 테스트', () => { | ||
test('0에서 9사이의 정수 중 하나의 숫자를 반환', () => { | ||
const randomNumber = RandomNumber.pickNumber(); | ||
|
||
expect(/^[0-9]$/.test(randomNumber)).toBeTruthy(); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 Random 함수에 대해서 테스트 해야겠다고 생각하신 이유는 무엇인가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
랜덤한 결과를 반환하는 함수는 결과 예측이 불가능합니다. 그래서 조건(0~9 사이의 정수)에 맞게 함수 기능이 동작하는지 검증하기 위해 테스트를 해야겠다고 생각했습니다.!
- 게임에 사용되는 상수 관리 파일(GameRule.js) 생성 - 게임에 사용되는 조건 숫자 위의 파일에서 import 하는 방식으로 수정 - constants 폴더의 index.js 사용하도록 import 문 수정 - 1라운드만 표기되던 오류 해결을 위해 실행결과와 라운드 표기 함수를 분리
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
피드백 반영하시느라고 고생 많으셨습니다!
그런데 잘 적용이 된 것 같은지는 어떻게 확인하면 되는지 잘 모르겠네요,,ㅎ 코드 분리할 때 신경써야 할 부분이나 tip 등이 있다면 알려주시면 감사하겠습니다!!
이 부분은 상당히 어렵긴하죠.. 저도 제가 짠 코드가 잘 분리가 되었는지는 쉽게 파악하기 어려운 것 같아요! 여러가지 단서들은 있긴 합니다. 이 클래스, 매서드가 의존하고 있는 변수와 다른 모듈들은 어떤 것인지, 한 부분의 코드를 바꾸려고 할 때 여러 곳에 영향이 가지는 않는지, 테스트를 하려고 했을 때 코드가 과연 테스터블 한 지 (다른 함수나 모듈을 모킹하지 않아도 충분히 독립적으로 테스트가 가능한지) 등...
개인적으로는 코드의 구조를 그림으로 그려봐도 도움이 되었던 것 같습니다!
1단계 수고 너무 많으셨고 2단계에서 뵙겠습니다!
화이팅하세요!!
느낀점 : 코드 구현 부분
느낀점 : 개인 부분
리뷰어님께 굼긍한 점