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

[1단계 - 자동차 경주 구현] 센트(김영우) 미션 제출합니다. #160

Merged
merged 24 commits into from
Feb 12, 2023

Conversation

kyw0716
Copy link
Member

@kyw0716 kyw0716 commented Feb 9, 2023

안녕하세요! 애슐리와 자동차 경주 게임을 페어로 구현한 센트입니다.
첫 코드리뷰 잘 부탁드립니다!!
바쁜시간 쪼개어 리뷰 맡아주셔서 감사합니다😀😀

@kyw0716 kyw0716 changed the title Kyw0716 [1단계 - 자동차 경주 구현] 센트(김영우) 미션 제출합니다. Feb 9, 2023
@sunhpark42 sunhpark42 self-requested a review February 10, 2023 01:09
Copy link

@sunhpark42 sunhpark42 left a comment

Choose a reason for hiding this comment

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

안녕하세요 센트~! 리뷰어 서니입니다:)
미션 구현하시느라 고생많으셨습니다. 👏👏
시작하자마자 미션 진행하시느라 정신 없으셨을 것 같은데 전반적으로 요구사항 및 테스트가 잘 작성되어 있어 좋았습니다. 코드 내에서 더 고민해볼 수 있는 부분이나 컨벤션등에 대한 궁금점을 리뷰로 남겨보았습니다. 확인 부탁드려요~!

.gitignore Outdated Show resolved Hide resolved
__tests__/Car.test.js Show resolved Hide resolved
__tests__/Exception.test.js Outdated Show resolved Hide resolved
src/constants/constants.js Show resolved Hide resolved
src/controller/Controller.js Show resolved Hide resolved
src/model/Car.js Outdated Show resolved Hide resolved
src/utils/Exception.js Outdated Show resolved Hide resolved
src/utils/Exception.js Show resolved Hide resolved
src/view/OutputView.js Outdated Show resolved Hide resolved
__tests__/RandomNumberGenrator.test.js Outdated Show resolved Hide resolved
입력값으로 주어지는 문자열 예시가 잘못되어 해당 내용 수정함.
원래 방식: move 메서드에 랜덤 값을 넘겨주면 해당 값의 크기에 따라 distance를 증가시키는 방식

수정 방식: move 메서드를 호출하기만 하면 distance를 증가시키는 방식 => 따라서 조건을 Car인스턴스를 제어하는 상위 객체에서 판별!
원래는 범위를 따로 입력받지 않고 math.random() 값에 10을 곱해 0 ~ 9까지의 값을 랜덤하게 생성했었는데, 확장성을 고려해 랜덤 숫자의 최소, 최대 값을 입력하면 해당 범위내의 랜덤 값을 생성하도록 수정.
Copy link

@sunhpark42 sunhpark42 left a comment

Choose a reason for hiding this comment

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

안녕하세요 센트~ 반영된 부분 확인해 보았습니다:) 추가로 코멘트 남긴 내용이 있어 확인부탁드려요~ 미션이 내일까지라고 해서 1단계 머지하고 추가 수정사항은 step2에서 반영해주시면 될 것 같습니다. 수고하셨어요~!

@sunhpark42 sunhpark42 merged commit 6442d85 into woowacourse:kyw0716 Feb 12, 2023
@kyw0716 kyw0716 deleted the kyw0716 branch February 13, 2023 04:00
@kyw0716 kyw0716 restored the kyw0716 branch February 13, 2023 04:01
@kyw0716 kyw0716 deleted the kyw0716 branch February 13, 2023 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants