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

[자동차 경주] 박건홍 미션 제출합니다. #737

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

parkgggg
Copy link

@parkgggg parkgggg commented Nov 1, 2023

No description provided.

Copy link

@Parkhanyoung Parkhanyoung left a comment

Choose a reason for hiding this comment

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

이번 한 주도 과제 완성하시느라 고생 많으셨어요!
전반적으로 잘 작성해주셔서 이해하는 데에는 큰 무리가 없었습니다.
하지만 만약 로직을 여러 메소드로 더 나눠주셨다면 가독성이 더 좋아지지 않을까 하는 생각은 들었던 것 같아요!
이번 주차에 로직들을 잘게 나눠보고, 가능하면 다른 파일로 분리해보는 연습을 해보시면 도움이 될 것 같습니다!

다음주도 같이 화이팅해봐요 💪🏻

class App {
async play() {}

nameValidation(input) {

Choose a reason for hiding this comment

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

Suggested change
nameValidation(input) {
validateCarNames(input) {

일부 케이스를 제외하고는 보통 메소드나 함수명은 동사형으로 짓는 것 같은데, 동사형 이름은 어떠신가요?

Comment on lines +8 to +13
if (input.includes(' ')) throw new Error("[ERROR] 공백을 제외하고 입력해주세요.");
if (input.includes(',,')) throw new Error("[ERROR] 쉼표가 연달아 입력되었습니다.");
if (inputArray.length <= 1) throw new Error("[ERROR] 2개 이상의 자동차가 있어야 시작할 수 있습니다.");
inputArray.forEach((item) => {
if (item.length > 5) throw new Error("[ERROR] 5자 이하로 입력해주세요.");
if (names.has(item)) throw new Error("[ERROR] 중복된 이름이 있습니다.");

Choose a reason for hiding this comment

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

이 검증 로직들을 또 별도의 메소드로 쪼개볼 수 있을 것 같은데, 더 많은 메소드로 쪼개보는 건 어떠신가요?

Choose a reason for hiding this comment

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

잘게 나누면 로직이 일정 구간 별로 묶이면서 읽는 입장에서 이해하기가 수월하더라구요


async play() {
try {
const stuffs = await this.playerInput();

Choose a reason for hiding this comment

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

혹시 stuffs라는 변수명을 지으신 이유가 있으신가요~? 변수명은 최대한 명확한 게 좋다고 들어서 stuffs와 같이 불분명한 이름 대신 확실하게 의미가 드러나는 단어로 지어보시면 가독성에 도움이 될 것 같습니다!

또, 자바스크립트에서 배열로부터 값을 꺼내는 아래와 같은 문법도 있으니 참고해보시면 좋을 것 같아요!

Suggested change
const stuffs = await this.playerInput();
const [names, rounds] = await this.playerInput();

let i = rounds;
while(i != 0) {
names.forEach((value, key, map) => {
const num = MissionUtils.Random.pickNumberInRange(0, 9);

Choose a reason for hiding this comment

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

이 랜덤 수를 뽑는 로직을 별도의 함수로 분리하면 어떨까요?

names.forEach((value, key, map) => {
const num = MissionUtils.Random.pickNumberInRange(0, 9);
if (num >= 4) map.set(key, value+"-");
MissionUtils.Console.print(`${key} : ${map.get(key)}`);

Choose a reason for hiding this comment

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

결과를 출력하는 이 부분도 분리해볼 수 있을 것 같아요!

}

diceLogic(names, rounds) {
let i = rounds;

Choose a reason for hiding this comment

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

round 수와 같은 정보는 App 클래스의 프로퍼티로 다루는 것도 괜찮을 것 같다는 생각이 들었습니다!

@parkgggg
Copy link
Author

parkgggg commented Nov 3, 2023 via email

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