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

[자동차 경주] 문성희 미션 제출합니다. #404

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

Conversation

seong-hui
Copy link

@seong-hui seong-hui commented Oct 28, 2024

javascript-racingcar-precourse

  • 1주차 코드 리뷰에서 다른 분들의 코드를 보며, 저와 다른 플로우 차트로 작성된 코드를 읽는 데 시간이 걸리는 것을 느꼈습니다.
    리뷰어분들이 제 코드를 더 쉽게 이해할 수 있도록 이번에는 플로우 차트를 함께 만들어 보았습니다!
  • 코드 내 주석은 최소화하고 함수명을 최대한 직관적으로 작성하고자 했습니다.
    이해가 어려운 부분이나 주석이 필요하다고 느껴지는 부분이 있다면 말씀해 주시면 감사하겠습니다.
  • 함수형 프로그래밍과 객체지향 프로그래밍 사이에서 고민하였고 유효성 검사는 함수형으로, 게임 로직은 객체지향 방식으로 구현하려 노력했습니다. 아직 객체지향에 미숙한 부분이 많으니 보완할 점이 있다면 리뷰 부탁드립니다.
  • 테스트 코드는 각 기능별로 검증할 수 있도록 구성하였습니다. 추가로 필요하다고 생각되는 테스트가 있다면 자유롭게 의견 남겨주세요.

자동차 경주 플로우 차트

자동차경주_플로우차트

 

기능 구현 목록

  1. 자동차 경주 안내 문구 출력
  2. 경주할 자동차 이름 입력
    • 사용자의 입력 받기
  3. 입력 유효성 검사
    • 알파벳 대소문자와 “,”이외에 다른 문자가 들어온 경우 ERROR
  4. 자동차 이름 유효성 검사
    • 구분자 ","를 기준으로 분리된 이름 배열에 저장
    • 분리된 이름들 중에 1자 이상 5자 이하가 아닌 경우 ERROR
    • 중복되는 이름이 있는 경우 ERROR (대소문자는 다른 이름으로 취급)
    • 자동차 개수 제한을 넘어가는 경우 ERROR
  5. 시도할 횟수 입력
    • 사용자의 입력 받기
  6. 횟수 유효성 검사
    • 숫자외에 다른 문자가 들어온 경우 ERROR
    • 0이하 또는 너무 큰 수일 경우 ERROR
  7. 자동차 이름 저장
    • 이름과 전진 횟수를 가지는 Car 클래스 생성
  8. 자동차 경주 시작
    • 자동차 경주 게임 로직을 담을 클래스 생성
  9. 이동 횟수동안 반복하며 자동차 경주 반복
    • 난수 생성 함수
    • 4이상의 값일 경우 움직이는 함수(전진 횟수에 1더하기)
    • 중간 결과 출력 함수
  10. 최종 우승자 출력 - [x] 전진 횟수가 가장 큰 수 구하는 함수 - [x] 우승 자동차를 필터링하는 함수 - [x] 자동차의 이름을 가져오는 함수 - [x] 최종 우승자 출력

 

예외 처리

  • 자동차 이름 입력 유효성 검사
    • 알파벳과 숫자로만 구성되고 이름들은 쉼표(,)로 구분되지 않은 경우
    • 빈 입력값인 경우
  • 자동차 이름 유효성 검사
    • 자동차 이름의 길이가 1이상 5이하가 아닌 경우
    • 자동차 이름이 중복된 경우
    • 자동차 개수가 1대 이상 100대 이하가 아닌 경우
  • 시도 횟수 유효성 검사
    • 빈 입력값인 경우
    • 숫자가 아닌 값이 입력된 경우
    • 시도 횟수가 1이상 1000이하가 아닌 경우

 

실행 결과 예시

----------[자동차 경주 게임 안내]----------
게임 설명: 자동차 이름과 시도 횟수를 입력하고 가장 멀리 전진한 자동차가 우승하는 게임입니다!

- 자동차 이름은 쉼표(,)로 구분하여 알파벳과 숫자로 입력 가능합니다.
- 이름은 1~5자 이내, 최대 100대까지 입력 가능합니다.
- 자동차의 이름은 중복이 불가합니다.
- 시도 횟수는 1~1000 사이의 숫자로 입력 가능합니다.
- 매 시도마다 0부터 9 사이의 무작위 값이 나오며 4 이상이면 1칸 전진합니다.
- 가장 멀리 전진한 자동차가 최종 우승합니다.
------------------------------------------

경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)
moon,seong,hui
시도할 횟수는 몇 회인가요?
5

실행 결과
moon : -
seong : -
hui :

moon : --
seong : --
hui :

moon : ---
seong : ---
hui :

moon : ---
seong : ----
hui :

moon : ----
seong : -----
hui :

최종 우승자 : seong

- every를 사용하여 배열의 모든 이름이 조건에 충족하는지 확인하고 조건에 맞지 않는 요소를 만날
즉시 순회를 중단하고 false를 반환하여 이후의 요소들의 불필요한 검사를 막았다.
 유효성을 검사하고 유효하지 않으면 바로 throw된다는 점에서 is~ 라는 함수명 보다는
 check~ 라는 이름의 함수명이 더욱 직관적이라 생각하여 변경
Set은 중복을 허용하지 않는 자료구조이기 때문에 배열을 Set으로 변환하
중복된 값이 자동으로 제거된다. 변환된 Set의 크기와 기존 배열의 크기를 비교하면
쉽게 중복 여부를 확인할 수 있기에 Set 자료구조로 변환하고 크기를 비교한다.
… 입력받는 함수로 변

기존의 getRachingCarsName()함수는 오직 자동차 이름을 받기 위한 문구를 출력
매개변수로 알림 문구를 받아 출력하고 입력을 받는 함수를 만든다면
하나의 함수로 서로 다른 입력 문구를 출력할 수 있어서 해당 함수로 변경
각각의 자동차가 가진 name, moveCount를 관리하고 조작하기 위해서 생성
외부에서 조회가 가능하도록 getName()과 getMoveCount() 메서드 구현
Car클래스에서 자동차가 전진하는 동작을 처리하기 위해서 move 메서드 생성
게임에서 가장 멀리 전진한 자동차를 결정하기 위해서 최대 전진 횟수 계산이 필요하다.
maxMoveCount와 같은 전진횟수를 가지는 자동차 객체만 필터링하여  새로운 배열 반환
기존의 splitByComma 파일명을 stringUtils로 변경하면서 문자열 처리하는 함수들을 함께 관리
함수들과 초기값을 받아서 순차적으로 모든 함수에 입력을 전달하며 결과를 반환하기 위해 파이프라인 함수 생성
이름 입력이 게임 요구사항에 충족하는지 순차적으로 진행하기 위해서 구현
seong-hui and others added 19 commits October 27, 2024 17:25
- 4이상일 때 전진하는 테스트
- 4미만일 때 정지하는 테스트
- 랜덤값에 따라서 moveCount가 예상대로 변하는지 확인
- 각 자동차의 전진 횟수가 예상된 전진 횟수와 일치하는지 확인을 통해 예상대로 게임이 진행되는지 검사
- 최대 전진 횟수를 가진 자동차의 이름 목록이 예상된 우승자와 일치하는지 검사
-  자동차 개수가 100개를 넘어간다면 출력에서 제대로된 결과를 확인하기 힘들것으로 판단하여 1000개에서 100개로 조정
입력 문자열에 tirm()을 사용해 공백만 있는 입력도 NULL_INPUT오류로 처리하도록 수정
Copy link

@wo-o29 wo-o29 left a comment

Choose a reason for hiding this comment

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

성희님의 커밋 수와 플로우 차트를 그리신 부분을 보고 뜨거운 열정을 느낄 수 있었던 것 같네요🔥🔥🔥

전체적으로 코드를 잘 나눠주셔서 흐름이나 맥락을 파악하기 좋았던 것 같아요!

근데 함수를 작성하실 때 화살표 함수보다는 명명된 함수 표현식으로 작성하신 것 같은데 그 이유가 있으신가요??

많이 배워가고 제 코드를 반성하게 되는 것 같네요🙇‍♂️

수고하셨습니다👍👏💪🙃

Copy link

Choose a reason for hiding this comment

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

파일 명을 조금 더 자세하게 작성해서 이 상수들이 어떤 관심사를 가지고 있는지 명시적으로 드러내도 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

파일명을 더 자세하게 쓰면 훨씬 좋겠네요! 좋은 의견 감사합니다

},
};

const ERROR_MESSAGE = {
Copy link

Choose a reason for hiding this comment

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

상수 객체를 Object.freeze()로 동결하고 객체 내부에 여러 속성들이 있기 때문에 네이밍으로 복수인 것을 명시해도 좋을 것 같아요!

Suggested change
const ERROR_MESSAGE = {
const ERROR_MESSAGES = Object.freeze({

Copy link
Author

Choose a reason for hiding this comment

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

Object.freeze()로 동결하면 객체의 불변성을 유지할 수 있다는 측면에서 확실히 좋은 방법인 것 같습니다!! 덕분에 Object.freeze()에 대해서 찾아보게 되었는데 Object.freeze()는 얕은 동결을 수행하기 때문에 객체 내에 중첩된 객체까지 동결하려면 중첩 객체들도 각각 Object.freeze()로 동결해야 한다는 새로운 사실도 알게 되었습니다.🤓

위의 PRINT_MESSAGES의 경우 중첩된 객체이기 때문에

Object.freeze(PRINT_MESSAGES.INPUT);
Object.freeze(PRINT_MESSAGES.OUTPUT);
Object.freeze(PRINT_MESSAGES);

이와 같이 freeze를 해야한다는 새로운 사실도 알게되는 리뷰였습니다. 좋은 의견 남겨주셔서 감사합니다!!


const MAX_RANDOM_NUM = 9;

const MAX_CAR_COUNT = 100;
Copy link

Choose a reason for hiding this comment

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

개인적으로는 게임 룰이라는 관심사로 뭉칠 수 있을 것 같아서 GAME_RULES라는 객체로 묶어도 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

상수들을 객체로 묶을 생각을 따로 하지 못했는데 우혁님께서 상수들을 객체로 묶으신 것을 보고 저도 이런 방식이 더 좋겠다고 생각했었습니다! 좋은 의견 감사합니다

Copy link

Choose a reason for hiding this comment

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

상수를 객체로 그룹화시킬 때 Object.freeze() 사용하는 것을 추천드립니다!

import Car from '../car/Car.js';

const createCars = function createCarsFunc(carNamesArray) {
return carNamesArray.map((name) => new Car(name));
Copy link

Choose a reason for hiding this comment

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

new Car() 를 거쳐서 { name, moveCount } 객체로 바뀌는 것 같은데 사실 Car라는 이름 만으로 이 동작을 유추할 수 없을 것 같아서 조금 더 내부 구현을 유추할 수 있는 이름으로 수정해보는건 어떨까요??

Copy link
Author

Choose a reason for hiding this comment

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

createCars 함수에서는 carNamesArray에 있는 각 이름으로 자동차 객체를 생성하는 과정이 맞습니다. 말씀하신 대로 클래스명이 Car 하나만으로 내부 구현을 모두 유추하기는 어려울 수 있겠지만 자동차를 나타내는 객체가 이름(name)과 주행거리(moveCount)를 가지고 있다는 점에서 자동차(Car)라는 개념을 직관적으로 표현하는 데 적합하다고 생각했습니다!!

자동차라는 개념 자체에 이름과 주행거리가 어울리는 속성이라고 판단하여 Car 클래스를 사용하게 되었습니다. 혹시 다른 개선 방안이 있다면 피드백 주시면 감사하겠습니다!

Copy link

Choose a reason for hiding this comment

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

carInfo, carData와 같이 자동차의 어떤 정보가 있다는 걸 명시해줘도 좋을 것 같아요👍

const inputForRacingCars = await getUserInput(PRINT_MESSAGES.INPUT.CAR_NAME);
validateRacingCarInput(inputForRacingCars);

const carNamesArray = splitByComma(inputForRacingCars);
Copy link

Choose a reason for hiding this comment

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

Suggested change
const carNamesArray = splitByComma(inputForRacingCars);
const carNamesArray = inputForRacingCars.split(",");

저도 이런 추상화를 했었는데 최종적으로 과한 추상화라고 결론을 짓게 되었는데 성희님은 어떻게 생각하시나요??

Copy link
Author

Choose a reason for hiding this comment

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

의견 주셔서 감사합니다! 1주차 공통리뷰에 있던 동영상에도 비슷한 고민이 있었던 것으로 기억합니다. splitByComma 함수는 단순한 .split(",") 연산을 감싸는 형태이지만 제가 추상화가 필요한 작업이라고 생각한 이유는 splitByComma라는 이름을 통해 충분히 코드의 의도와 목적을 전달할 수 있으며 추가적으로 trim이 필요하다거나 하는 확장성 측면에서도 내부를 수정하면 되어 유지보수 측면에서도 유리하다고 생각했습니다! 이러한 이유로 저는 splitByComma가 과한 추상화보다는 유의미한 명확성을 더하는 추상화라고 생각했습니다. 추가 의견이 있으시면 알려주시면 감사하겠습니다! 😊

Copy link

@wo-o29 wo-o29 Oct 29, 2024

Choose a reason for hiding this comment

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

맞아요 추상화를 통해 확장에 유연하고, splitByComma와 같은 함수 이름을 통해 어떤 동작을 하는지 명확하게 나타낼 수 있는 점은 좋은 것 같아요!

하지만 말씀해주신 trim()을 하는 로직이 추가된다면 splitByComma라는 이름만으로는 trim()을 한다라는 동작을 유추할 수 없기 때문에 변경에 유연한 추상화는 아니였다는 생각이 드는 것 같아요🤔

@@ -0,0 +1,8 @@
import { MissionUtils } from '@woowacourse/mission-utils';
Copy link

Choose a reason for hiding this comment

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

Suggested change
import { MissionUtils } from '@woowacourse/mission-utils';
import { Random } from '@woowacourse/mission-utils';

다른 곳은 이렇게 Random 객체를 직접 import 하는 경우도 있는데 여기는 MissionUtils를 import하는 것 같네요!

성희님만의 기준이 따로 있으신가요??

Copy link
Author

Choose a reason for hiding this comment

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

결국 MissionUtils 에 있는 랜덤을 사용하는 것이니 MissionUtils, Random 둘 중 아무거나 import해도 큰 차이가 없다고 생각했습니다!

Copy link

@wo-o29 wo-o29 Oct 29, 2024

Choose a reason for hiding this comment

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

넵 맞습니다! 두 import 방식 모두 같은 동작을 하지만 코드의 일관성 측면에서 하나로 통일하는게 좋다고 생각했는데 어느 곳은 Random을 직접 가져오고 여기는 MissionUtils을 가져오셔서 특별한 이유가 있었는지 여쭤봤던 것 같습니다🙃

Comment on lines +3 to +6
const getUserInput = async function getUserInputFunc(promptMessage) {
const userInput = await Console.readLineAsync(`${promptMessage}\n`);
return userInput;
};
Copy link

Choose a reason for hiding this comment

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

저는 개인적으로 템플릿 리터럴 문법을 사용할 때 중간에 어떤 문자열이 추가되는 경우에 사용하는데 희성님은 어떤 기준으로 사용하시나요??

  • 제가 템플릿 리터럴 문법을 사용하는 기준
const a = "안녕";
const b = "홍길동";
console.log(`${a} ${b}`); // 중간에 공백이 있어서 템플릿 리터럴 문법 사용
console.log(`${a}-${b}`); // 중간에 다른 문자열이 있어서 템플릿 리터럴 문법 사용
console.log(a + b); // + 연산자로만 해결할 수 있다면 + 연산자 사용
Suggested change
const getUserInput = async function getUserInputFunc(promptMessage) {
const userInput = await Console.readLineAsync(`${promptMessage}\n`);
return userInput;
};
const getUserInput = function getUserInputFunc(promptMessage) {
return Console.readLineAsync(promptMessage + "\n");
};

그리고 별도의 로직이 없기 때문에 이렇게 async/await 키워드를 생략하고 바로 return해도 문제 없을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

airbnb js 컨벤션에서 문자열을 생성하는 경우 문자열 연결 대신 템플릿 문자열을 사용하라는 내용이 있습니다. 따라서 저는 값을 문자열을 만들어야 하는 상황에서 모두 템플릿 문자열을 사용하고 있습니다! 해당 내용 링크 함께 첨부해 드리겠습니다
https://github.com/airbnb/javascript?tab=readme-ov-file#es6-template-literals

Copy link
Author

Choose a reason for hiding this comment

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

getUserInput에서 userInput을 반환한 이유는 반환된 값이 파이프라인을 통해 유효성 검사 등의 후속 처리를 받기 위함입니다! pipeline 함수는 함수 배열을 순차적으로 실행하며 각 함수의 결과를 다음 함수의 입력으로 전달하는 구조이기 때문에 userInput을 반환하여 다음 유효성 검사 단계를 수행할 수 있도록 했습니다.

Copy link

Choose a reason for hiding this comment

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

airbnb js 컨벤션에서 문자열을 생성하는 경우 문자열 연결 대신 템플릿 문자열을 사용하라는 내용이 있습니다. 따라서 저는 값을 문자열을 만들어야 하는 상황에서 모두 템플릿 문자열을 사용하고 있습니다! 해당 내용 링크 함께 첨부해 드리겠습니다 https://github.com/airbnb/javascript?tab=readme-ov-file#es6-template-literals

요 내용은 제 PR에서 다른 분께서도 같은 리뷰를 해주셔서 제가 답변을 달았는데 그 링크를 첨부해놓겠습니다🙃
#54 (comment)

Copy link

Choose a reason for hiding this comment

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

getUserInput에서 userInput을 반환한 이유는 반환된 값이 파이프라인을 통해 유효성 검사 등의 후속 처리를 받기 위함입니다! pipeline 함수는 함수 배열을 순차적으로 실행하며 각 함수의 결과를 다음 함수의 입력으로 전달하는 구조이기 때문에 userInput을 반환하여 다음 유효성 검사 단계를 수행할 수 있도록 했습니다.

이 부분은 getUserInput 함수 내부에 추가적인 로직이 없기 때문에 async/await 키워드를 사용하지 않더라도 똑같은 동작을 해서 외부에서 사용할 때만 await 키워드를 사용해주면 되기 때문에 getUserInput 함수에서는 async/await 키워드가 없어도 될 것 같다는 리뷰였습니다:)

Comment on lines +12 to +14
const printNewLine = function printNewLineFunc() {
Console.print('');
};
Copy link

Choose a reason for hiding this comment

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

이거는 살짝 줄바꿈(개행)을 하려고 끼워넣은 느낌이 드는 것 같은데 필요한 곳에서 \n(개행 문자)를 작성해서 줄바꿈 한다는 것을 명시하는 건 어떨까요??


const printPrevNewLine = function printPrevNewLineFunc(string) {
Console.print(`\n${string}`);
};
Copy link

Choose a reason for hiding this comment

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

이것도 개인적으로는 사용하는 곳에서 \n(개행 문자)를 사용하는 게 더 의도를 명확하게 표현할 수 있을 것 같다는 생각이 드는 것 같네요🤔

Comment on lines +5 to +7
const joinByComma = function joinByCommaFunc(inputString) {
return inputString.join(', ');
};
Copy link

@wo-o29 wo-o29 Oct 29, 2024

Choose a reason for hiding this comment

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

Suggested change
const joinByComma = function joinByCommaFunc(inputString) {
return inputString.join(', ');
};
// 사용처에서 패턴을 주입하기 때문에 어떤 결과를 받을 수 있는지 유추할 수 있고 범용적으로 사용할 수 있음
const joinByPattern = function joinByCommaFunc(array, pattern) {
return array.join(pattern);
};

위에도 리뷰 남기긴했지만 저도 이렇게 추상화를 했던 적이 있었고 과한 추상화라는 결론을 하게 되었는데 만약에 이렇게 사용하더라도 pattern을 인자로 받아서 범용적으로 사용하는건 어떨까요??

joinByComma라는 함수 이름은 쉼표(,)를 기준으로 문자열로 합칠 것 같은데 내부적으로는 , 이렇게 공백도 포함되어 있기 때문에 함수의 동작을 유추하기는 어려울 것 같다는 문제가 있을 것 같아요🤔

그리고 인자로 inputString을 받는데 string 타입을 다시 join하는게 살짝 어색하게 느껴지는 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

확실히 패턴을 받아서 join을 하는 것이 더 좋은 코드 같아 보이네요 !! 좋은 의견 감사합니다 👍

Copy link

@bunju20 bunju20 left a comment

Choose a reason for hiding this comment

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

안녕하세요! 전반적으로 노력하신게 보인 코드였고 특히 플로우차트와, 잘 분리된 테스트 코드들이 가장 인상깊었어요! 덕분에 오오...하면서 즐겁게 리뷰 진행했습니다. 이번주차 너무너무 고생 많으셨습니다.

시간되시면 제 코드도 리뷰 부탁드립니다!!
#161


## 자동차 경주 플로우 차트

<img src="https://github.com/user-attachments/assets/5568c9d8-d2e9-43e7-87ac-f6903c78b330" alt="자동차경주_플로우차트" width="300"/>
Copy link

Choose a reason for hiding this comment

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

세상에 이렇게 플로우 차트로 보기 쉽고 이해하기 좋게 세팅해주셔서 리뷰하는 입장에서도 너무 좋고, 프로그램 이해하기 쉬울것같아요. 저도 배워야겠네요...! 우와 수고하셨습니다.

Copy link
Author

Choose a reason for hiding this comment

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

좋은 말씀 감사합니다!!😊 이해하시는데 조금이라도 도움이 되었기를 바랍니다

Copy link

Choose a reason for hiding this comment

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

테스트 코드를 되게 체계적으로 정리하셨네요...! 여러 파일로 테스트를 분리해서 더 직관적인 테스팅이 가능할것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

테스트 코드를 처음 작성해 보는거라서 최대한 기능별로 테스트를 작성하는 방법을 익히고자 노력했습니다!

import { GUIDE_MESSAGE } from '../constants/messages.js';

const displayGameGuide = function displayInputGuideFunc() {
Console.print(GUIDE_MESSAGE.HEADER);
Copy link

Choose a reason for hiding this comment

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

음... 기능 요구사항에 기재되지 않은 내용은 스스로 판단해서 구현한다라는 말이 있긴 했지만 입출력 예시랑 정확히 일치하지 않는 출력이라 괜찮을지 모르겠다는 생각이 들어요...! 혹시 이렇게 진행해도 된다는 안내를 받으셨을까요??

Copy link
Author

Choose a reason for hiding this comment

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

과제에 제시되어있는 실행 결과 예시는 말그대로 예시라고 생각했습니다. 입력을 받아야 하고 정해진 출력(결과 출력, 에러 출력의 경우)을 뱉어야 하지만 입력을 받기전에 출력에 대해서는 따로 정해진 양식이 없어서 이처럼 구현을 하였습니다! 혹시 추가 의견이 있으시면 알려주시면 감사하겠습니다

return this.name;
}

getMoveCount() {
Copy link

Choose a reason for hiding this comment

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

Car 클래스에서 private 필드를 사용하지 않아서 캡슐화가 불완전한 상태인데, getter메소드가 있어요! js 코드 컨벤션을 준수해야한다는 부분에 있어서 이부분 확인하시면 좋을것같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

오호 정말 그러네요! 확실히 name과 moveCount를 private필드를 사용하고 getter함수를 정의하는 것이 캡슐화를 강화할 수 있겠네요! 신경쓰지 못한 부분인데 열심히 봐주셔서 감사합니다. 덕분에 private필드에 대해서 다시 생각해 볼 수 있었습니다!!

@seong-hui
Copy link
Author

성희님의 커밋 수와 플로우 차트를 그리신 부분을 보고 뜨거운 열정을 느낄 수 있었던 것 같네요🔥🔥🔥

전체적으로 코드를 잘 나눠주셔서 흐름이나 맥락을 파악하기 좋았던 것 같아요!

근데 함수를 작성하실 때 화살표 함수보다는 명명된 함수 표현식으로 작성하신 것 같은데 그 이유가 있으신가요??

많이 배워가고 제 코드를 반성하게 되는 것 같네요🙇‍♂️

수고하셨습니다👍👏💪🙃

제 코드를 꼼꼼하게 읽어주시고 코드 리뷰를 남겨주셔서 다시 한 번 감사드립니다!!
함수 작성시에 화살표 함수가 아닌 기명함수 표현식을 사용한 이유는 airbnb js컨벤션에서 기명함수 표현식을 사용하라는 내용이 있었기 때문입니다! 화살표 함수의 사용은 기명 콜백을 전달하는 아래와 같은 경우에 사용하라고 나와있습니다! 해당 내용이 나와 있는 부분 함께 링크해두겠습니다. 추가적으로 기명 함수 표현식은 디버깅할 때 함수 이름이 오류 메시지에 나타나 문제의 위치를 쉽게 파악할 수 있고 재귀 호출도 가능합니다!

[1, 2, 3].map((x) => {
  const y = x + 1;
  return x * y;
});

https://github.com/airbnb/javascript?tab=readme-ov-file#functions--declarations

다시 한 번 좋은 리뷰 남겨주셔서 감사드립니다😊

@wo-o29
Copy link

wo-o29 commented Oct 29, 2024

제 코드를 꼼꼼하게 읽어주시고 코드 리뷰를 남겨주셔서 다시 한 번 감사드립니다!! 함수 작성시에 화살표 함수가 아닌 기명함수 표현식을 사용한 이유는 airbnb js컨벤션에서 기명함수 표현식을 사용하라는 내용이 있었기 때문입니다! 화살표 함수의 사용은 기명 콜백을 전달하는 아래와 같은 경우에 사용하라고 나와있습니다! 해당 내용이 나와 있는 부분 함께 링크해두겠습니다. 추가적으로 기명 함수 표현식은 디버깅할 때 함수 이름이 오류 메시지에 나타나 문제의 위치를 쉽게 파악할 수 있고 재귀 호출도 가능합니다!

[1, 2, 3].map((x) => {
  const y = x + 1;
  return x * y;
});

https://github.com/airbnb/javascript?tab=readme-ov-file#functions--declarations

Airbnb팀에서 해당 의견에 대한 근거를 이렇게 제시하고 있는데

1. 함수 선언 대신 함수 표현식을 사용해라
2. 익명 함수 표현식 보다는 명명된 함수 표현식을 선호해라
3. 함수에 명시적인 이름을 부여하라. 이는 디버깅과 오류 추적에 도움을 준다.
4. 함수 호이스팅을 피하고 코드의 가독성과 유지보수성을 향상 시킬 수 있다.

이 근거 모두 화살표 함수에도 해당되는 내용이기 때문에 명명된 함수 표현식, 화살표 함수 둘 다 사용할 수 있다고 이해했는데 성희님은 어떻게 이해하셨나요??

추가적으로 저는 명명된 함수 표현식보다 화살표 함수가 가독성이 더 좋다고 생각해서 화살표 함수를 사용하고 있습니다🙃

@seong-hui
Copy link
Author

제 코드를 꼼꼼하게 읽어주시고 코드 리뷰를 남겨주셔서 다시 한 번 감사드립니다!! 함수 작성시에 화살표 함수가 아닌 기명함수 표현식을 사용한 이유는 airbnb js컨벤션에서 기명함수 표현식을 사용하라는 내용이 있었기 때문입니다! 화살표 함수의 사용은 기명 콜백을 전달하는 아래와 같은 경우에 사용하라고 나와있습니다! 해당 내용이 나와 있는 부분 함께 링크해두겠습니다. 추가적으로 기명 함수 표현식은 디버깅할 때 함수 이름이 오류 메시지에 나타나 문제의 위치를 쉽게 파악할 수 있고 재귀 호출도 가능합니다!

[1, 2, 3].map((x) => {
  const y = x + 1;
  return x * y;
});

https://github.com/airbnb/javascript?tab=readme-ov-file#functions--declarations

Airbnb팀에서 해당 의견에 대한 근거를 이렇게 제시하고 있는데

1. 함수 선언 대신 함수 표현식을 사용해라
2. 익명 함수 표현식 보다는 명명된 함수 표현식을 선호해라
3. 함수에 명시적인 이름을 부여하라. 이는 디버깅과 오류 추적에 도움을 준다.
4. 함수 호이스팅을 피하고 코드의 가독성과 유지보수성을 향상 시킬 수 있다.

이 근거 모두 화살표 함수에도 해당되는 내용이기 때문에 명명된 함수 표현식, 화살표 함수 둘 다 사용할 수 있다고 이해했는데 성희님은 어떻게 이해하셨나요??

추가적으로 저는 명명된 함수 표현식보다 화살표 함수가 가독성이 더 좋다고 생각해서 화살표 함수를 사용하고 있습니다🙃

Airbnb팀에서 제시해 준 4가지 근거에 화살표 함수는 1번 4번만 해당되는 내용입니다! 화살표함수는 이름을 붙일 수 없어서 명명된 함수 표현식의 사용이 불가하고 이로 인해 스택 트레이스에 함수이름이 표시 되지 않습니다. 따라서 airbnb의 가이드라인은 디버깅과 오류 추적의 용이성을 높이기 위해 기명 함수 표현식을 추천하는 것으로 이해했습니다!!

저도 가독성 측면에서는 화살표 함수가 좋다고 생각하지만 디버깅 편의성과 코드의 유지보수성까지 고려했을 때 이러한 컨벤션이 생긴 이유가 있다고 생각합니다. 🙂

Copy link

@rtttr1 rtttr1 left a comment

Choose a reason for hiding this comment

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

깔끔한 코드 잘 보고갑니다!! 3주차도 화이팅해봐요~~!!

(description, input, expectError) => {
expect(() => validateTryCount(input)).toThrow(expectError);
},
);
Copy link

Choose a reason for hiding this comment

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

테스트 코드 정말 꼼꼼하게 구현하셨네요.. 정성 대박입니다..!!!

class App {
async run() {}
async run() {
displayGameGuide();
Copy link

Choose a reason for hiding this comment

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

game 가이드 보여주는거 너무 센스있네요!!

const tryCount = await getValidatedTryCount();
const cars = createCars(carNamesArray);
const racingGame = new RacingGame(cars, tryCount);
racingGame.play();
Copy link

Choose a reason for hiding this comment

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

저는 프로세스를 run 메서드 내부에 작성했는데 이렇게 따로 빼주니까 코드가 훨씬 가독성이 좋아지네요! 배워갑니다!

@@ -0,0 +1,20 @@
const getMovePosition = function getMovePositionFunc(car) {
return '-'.repeat(car.getMoveCount());
Copy link

Choose a reason for hiding this comment

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

요 '-' 도 상수화 해도 좋을것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

신경쓰지 못한 부분인데 좋은 의견 감사합니다!!

@wo-o29
Copy link

wo-o29 commented Oct 29, 2024

Airbnb팀에서 제시해 준 4가지 근거에 화살표 함수는 1번 4번만 해당되는 내용입니다! 화살표함수는 이름을 붙일 수 없어서 명명된 함수 표현식의 사용이 불가하고 이로 인해 스택 트레이스에 함수이름이 표시 되지 않습니다. 따라서 airbnb의 가이드라인은 디버깅과 오류 추적의 용이성을 높이기 위해 기명 함수 표현식을 추천하는 것으로 이해했습니다!!

저도 가독성 측면에서는 화살표 함수가 좋다고 생각하지만 디버깅 편의성과 코드의 유지보수성까지 고려했을 때 이러한 컨벤션이 생긴 이유가 있다고 생각합니다. 🙂

airbnb/javascript#794 (comment)
airbnb/javascript#794 (comment)
저도 이분들의 의견에 동감하는 것 같네요 요즘 브라우저들은 화살표 함수를 지원하고, 최신 개발자 도구들은 화살표 함수의 디버깅을 잘 지원하기 때문에 스택 트레이스에서 함수 이름이 필요한 경우, 변수에 할당된 화살표 함수의 이름을 사용하기 때문에 큰 문제가 없을 것 같다고 생각합니다🙃

실제로 저 디스커션 내용도 2020년도이고 eslint-config-airbnb 릴리즈 태그도 2021년 12월에 머무르고 있어서 현 시점에는 함수 이름 추론이 안되는 IE는 공식적으로 지원이 중단되었기 때문에 화살표 함수를 사용해도 괜찮을 것 같다는 생각이 드네요:)

스크린샷 2024-10-30 오전 12 04 27
https://techaffinity.com/blog/arrow-functions-in-javascript/

Copy link

@m2na7 m2na7 left a comment

Choose a reason for hiding this comment

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

안녕하세요 성희님 !

리뷰어들을 위한 플로우 차트와, 꼼꼼한 기능 목록 작성, 사용자를 위한 게임 설명 작성까지...
세심함과 디테일, 열정이 느껴집니다 .. ! 👍👍👍

특히, 잘 정돈된 테스트 코드들이 인상깊습니다. 리뷰를 통해 많이 배워갑니다~
2주차 미션 고생많으셨어요 :]

Comment on lines +9 to +17
const getValidatedCarNames = async function getValidatedCarNamesFunc() {
const inputForRacingCars = await getUserInput(PRINT_MESSAGES.INPUT.CAR_NAME);
validateRacingCarInput(inputForRacingCars);

const carNamesArray = splitByComma(inputForRacingCars);
validateCarNames(carNamesArray);

return carNamesArray;
};
Copy link

Choose a reason for hiding this comment

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

getValidatedCarNames 함수를 입력값을 받으면서, 유효성 검사를 동시에 진행하는 방식으로 구현하셨네요.
저도 미션을 진행하면서 이 부분에 대해 고민이 많았는데, 저는 함수의 책임을 보다 명확하게 하기위해 입력과 유효성 검사를 분리하는 방법으로 구현했어요.
이와 관련해서 의견이 궁금합니다 !

Copy link
Author

Choose a reason for hiding this comment

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

저도 이 부분에 대해서 고민을 많이 하였습니다. 기능별로 함수를 나누긴 했지만 결국 나눈 함수들을 불러오는 곳이 존재해야 하기 때문에 이러한 코드가 발생하였습니다 😢 함수들을 불러오는 함수는 어쩔 수 없이 하나 이상이 책임이 불러온다고 생각을 하였는데 이 부분에 대해서도 더 고민을 해봐야겠네요. 좋은 질문 해주셔서 감사합니다!!

Comment on lines +9 to +10
const getValidatedCarNames = async function getValidatedCarNamesFunc() {
const inputForRacingCars = await getUserInput(PRINT_MESSAGES.INPUT.CAR_NAME);
Copy link

Choose a reason for hiding this comment

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

getValidatedCarNames 함수의 인자로 message를 넘기는 방식으로 구현하면 어떨까요 ?
PRINT_MESSAGES.INPUT.CAR_NAME 에 의존하지 않고, 모듈간의 결합도를 낮출 수 있다고 생각해요.

Suggested change
const getValidatedCarNames = async function getValidatedCarNamesFunc() {
const inputForRacingCars = await getUserInput(PRINT_MESSAGES.INPUT.CAR_NAME);
const getValidatedCarNames = async function getValidatedCarNamesFunc(message) {
const inputForRacingCars = await getUserInput(message);

Comment on lines +22 to +26
move() {
if (getRandomNum() >= MIN_MOVABLE_NUM) {
this.moveCount += MOVE_INCREMENT;
}
}
Copy link

Choose a reason for hiding this comment

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

move() 메서드가 랜덤 숫자를 비교하고, moveCount를 증가시키는 두 가지 작업을 수행하고 있어요.
해당 로직을 분리하고, private 메서드로 구현하면 클래스의 캡슐화와 메서드의 SRP를 지킬 수 있다고 생각해요 !

Copy link
Author

Choose a reason for hiding this comment

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

확실히 그렇네요!! 민하님 말씀처럼 해당 로직은 분리되는 것에 동의합니다. 좋은 의견 제시해 주셔서 감사합니다. 덕분에 배웠습니다!!

@seong-hui
Copy link
Author

Airbnb팀에서 제시해 준 4가지 근거에 화살표 함수는 1번 4번만 해당되는 내용입니다! 화살표함수는 이름을 붙일 수 없어서 명명된 함수 표현식의 사용이 불가하고 이로 인해 스택 트레이스에 함수이름이 표시 되지 않습니다. 따라서 airbnb의 가이드라인은 디버깅과 오류 추적의 용이성을 높이기 위해 기명 함수 표현식을 추천하는 것으로 이해했습니다!!
저도 가독성 측면에서는 화살표 함수가 좋다고 생각하지만 디버깅 편의성과 코드의 유지보수성까지 고려했을 때 이러한 컨벤션이 생긴 이유가 있다고 생각합니다. 🙂

airbnb/javascript#794 (comment) airbnb/javascript#794 (comment) 저도 이분들의 의견에 동감하는 것 같네요 요즘 브라우저들은 화살표 함수를 지원하고, 최신 개발자 도구들은 화살표 함수의 디버깅을 잘 지원하기 때문에 스택 트레이스에서 함수 이름이 필요한 경우, 변수에 할당된 화살표 함수의 이름을 사용하기 때문에 큰 문제가 없을 것 같다고 생각합니다🙃

실제로 저 디스커션 내용도 2020년도이고 eslint-config-airbnb 릴리즈 태그도 2021년 12월에 머무르고 있어서 현 시점에는 함수 이름 추론이 안되는 IE는 공식적으로 지원이 중단되었기 때문에 화살표 함수를 사용해도 괜찮을 것 같다는 생각이 드네요:)

스크린샷 2024-10-30 오전 12 04 27 https://techaffinity.com/blog/arrow-functions-in-javascript/

덕분에 이 내용을 더 깊이 생각해볼 수 있는 계기가 되었어요! 😊 그리고 현재 시점에서 함수 이름 추론을 지원하지 않는 IE가 공식적으로 지원 종료되었다는 사실도 우혁님 덕분에 새롭게 알게 되었습니다. 함께 고민하고 깊이 있는 이야기 나눠주셔서 정말 감사드립니다! 👍👍

Copy link

@j-nary j-nary left a comment

Choose a reason for hiding this comment

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

플로우 차트부터 테스트 코드, 각각의 기능을 관심사별로 분리한 부분들까지 섬세하고 꼼꼼한 코드보고 많이 배워갑니다!!
시간 괜찮으시면 제 PR 코드리뷰 도 부탁드립니다!


const MAX_RANDOM_NUM = 9;

const MAX_CAR_COUNT = 100;
Copy link

Choose a reason for hiding this comment

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

상수를 객체로 그룹화시킬 때 Object.freeze() 사용하는 것을 추천드립니다!

Comment on lines +7 to +9
getMovePosition,
getMaxMoveCount,
filterWinningCars,
Copy link

Choose a reason for hiding this comment

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

해당 함수들도 RacingGame 이라는 클래스와 관심사를 같이할 수 있지 않을까요?
특정 기능을 클래스의 메서드와, util 함수로 분리하는 성희님 만의 기준이 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

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

RacingGame 클래스에서만 사용하는 함수에 한해서 racingGameUtils에 분리하였습니다. 클래스에서 중요한 동작은 메서드로 분리하고 동작을 수행하기 위한 작은 일들을 utils로 분리하였습니다!

Comment on lines +6 to +9
return cars.reduce(
(maxCount, car) => Math.max(maxCount, car.getMoveCount()),
0,
);
Copy link

Choose a reason for hiding this comment

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

return Math.max(...cars.map((car) => car.getMoveCount())); 이런 식으로 더 간결하고 가독성있는 고차함수 사용이 가능할 것 같은데 어떻게 생각하시나용~?

Copy link
Author

Choose a reason for hiding this comment

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

확실히 map을 사용하는 방법이 더욱 가독성이 좋네요! 저는 새로운 배열을 반환할 필요가 없는 상황에서는 reduce를 하는 것이 옳다고 생각해서 reduce를 사용해서 배열을 순회하는 방식을 선택했습니다. map은 전체 배열을 순회하여 새로운 배열을 만들게 되기에 가독성과 메모리의 측면에서 상황을 잘 고려해서 선택하면 더욱 좋을 것 같습니다. 좋은 의견 내주셔서 감사합니다!

Comment on lines +11 to +13
const pipeline = function pipelineFunc(functions, input) {
return functions.reduce((result, fn) => fn(result), input);
};
Copy link

Choose a reason for hiding this comment

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

해당 함수를 정의한 의도가 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

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

여러 함수들을 순차적으로 실행하기위해서 pipeline함수를 정의하였습니다.

const validateCarNames = function validateCarNamesFunc(input) {
  return pipeline(
    [checkValidNameLength, checkDuplicateNames, checkCarCountLimit],
    input,
  );
};

해당 코드와 같이 유효성 유효성 검사를 checkValidNameLength, checkDuplicateNames, checkCarCountLimit 3개의 함수를 수행할 때 파이프 라인을 통해 순차적으로 실행하고 최종결과를 반환하도록 하였습니다.

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.

6 participants