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

[자동차 경주] 임현우 미션 제출합니다. #434

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

Conversation

gusn9719
Copy link

과제 요구 사항에 따라 구현한 기능:

  • 자동차 이름 입력받기: 쉼표로 구분된 여러 자동차 이름을 입력받아 각 자동차 객체를 생성하는 기능을 구현했습니다.
  • 랜덤한 전진 여부 결정: Random.pickNumberInRange() 메서드를 사용하여 각 자동차의 전진 여부를 결정하고, 시도 횟수만큼 반복하여 각 라운드의 결과를 출력합니다.
  • 우승자 결정: 모든 시도가 완료된 후 가장 멀리 이동한 자동차를 우승자로 선정하고, 여러 명일 경우 쉼표로 구분해 출력했습니다.
  • 입력 유효성 검사: 자동차 이름과 시도 횟수에 대해 유효성 검사를 실시하여, 잘못된 입력 시 [ERROR] 메시지를 출력하고 프로그램이 종료되도록 구현했습니다.
  • 라운드별 결과 출력: 각 시도 후 자동차의 상태를 명확하게 출력하여 사용자가 진행 상황을 쉽게 파악할 수 있도록 구현했습니다.
  • 예외 상황 테스트: 입력 값의 잘못된 형식에 대한 예외 처리 테스트를 추가하여 안정성을 검증했습니다.
  • 다양한 입력 테스트 케이스 추가: 숫자, 특수문자, 한글 등 다양한 형태의 자동차 이름 입력에 대한 테스트 케이스를 추가하여 유연성을 높였습니다.

- 프로젝트의 기본 구조 정의
- 구현할 기능 목록 및 리팩토링 계획을 README에 정리
- 초기 실행 예시 포함
- 자동차 이름을 입력받고 분리하는 기능 추가
- 자동차 이름에 대한 유효성 검사 추가 (이름 개수, 중복, 길이 제한)
- 시도 횟수를 입력받고 유효성 검사 수행 (양의 정수 제한)
- Car 클래스 생성: 자동차의 이름(name)과 위치(position)를 저장하고 관리
- move() 메서드 추가: 자동차가 전진할 때 위치를 업데이트하는 기능
- moveCars() 메서드 추가: 시도 횟수만큼 각 자동차의 전진 여부 결정 및 이동
- shouldMove() 메서드 추가: 랜덤 숫자로 자동차의 전진 여부 판단 (4 이상일 경우 전진)
- printRoundStatus() 메서드 추가: 각 시도 후 각 자동차의 상태 출력
- `printWinners()` 추가: 최대 이동 거리를 기준으로 승자를 선정하고 출력
  - 자동차들의 위치를 비교해 가장 멀리 이동한 자동차들을 추려 승자를 결정
  - 복수의 우승자가 있을 경우, 이름을 쉼표로 구분하여 출력
- 자동차 경주 게임에서 사용되는 에러 메시지를 객체로 상수화하여 관리
- 코드의 가독성과 유지보수성을 높이기 위해 에러 메시지를 일관되게 사용
- 수정이 필요할 경우 단일 지점에서 변경 가능하도록 개선
- Console.print 기능을 printUtils.js 파일로 분리하여 관리
- 각 기능별로 printMessage, printRoundStatus, printWinners 함수로 모듈화
- 코드의 가독성을 높이고 유지보수를 용이하게 개선
- 출력 기능을 분리하여 코드의 재사용성을 높이고, 유지보수 시 수정 지점을 명확히 함
- Console.readLineAsync 기능을 getUserInputUtils.js 파일로 분리하여 관리
- getUserInput 함수를 만들어 중복된 입력 처리를 간소화
- 자동차 이름과 시도 횟수 입력을 각각 userInputCarNames, userInputTryCount 함수로 분리
- 코드의 가독성을 높이고 유지보수를 용이하게 개선
- `removeWhitespace`와 `splitCarNames` 함수로 각각의 작업을 처리하도록 기능 분리
- `splitAndRemoveWhitespace` 함수로 두 기능을 결합하여 자동차 이름을 효율적으로 처리
- `carNameUtils.js` 파일에서 모듈화하여 가독성과 유지보수성을 개선
- 에러 메시지를 구조분해 할당 방식으로 호출
- 자동차 이름 관련 유효성 검사: 최소 2개 이상, 이름 길이 1~5자, 중복 방지
- 시도 횟수 유효성 검사 추가: 양의 정수일 경우만 허용
- 유효성 검사를 세부 함수로 나누어 가독성과 유지보수성 향상
- Car 클래스를 별도의 모듈로 분리하여 관리
- 자동차 이름을 이용해 자동차 객체 리스트를 생성하는 로직을 모듈화
- 자동차를 객체로 변환하는 기능을 분리함으로써 코드의 재사용성 및 유지보수성을 높임
- 랜덤 숫자가 4 이상일 경우 자동차가 전진하는 조건을 moveOnFourOrOver 함수로 분리
- 각 자동차의 이동 로직을 moveCars 함수로 모듈화하여 코드의 가독성과 유지보수성 향상
- 이동 조건을 별도의 함수로 만들어 재사용성을 높임
- 자동차 중 가장 높은 위치 값을 반환하는 getMaxPosition 함수 추가
- 최고 위치에 있는 자동차를 필터링하는 filterCarsByMaxPosition 함수 추가
- 자동차 객체에서 이름만 추출하는 extractCarNames 함수 추가
- 우승자 추출 과정을 작은 단위의 함수로 분리하여 가독성과 유지보수성을 높임
- userInputTryCount로 함수명을 일관성 있게 변경
- 기존 네이밍의 불일치를 해결하여 코드 가독성 향상
- 게임 시작 시 실행 결과를 출력하는 기능을 함수로 모듈화
- App.js에서 직관적인 함수 호출로 코드의 가독성을 높이기 위해 추가
- App.js에서 각 기능을 별도의 모듈로 분리하여 불러오는 방식으로 리팩토링
- 코드의 가독성을 높이고, App.js를 단순히 기능의 흐름을 보여주는 형태로 변환
- 주요 기능: 자동차 이름 입력, 유효성 검사, 자동차 객체 생성, 이동 및 결과 출력
- 자동차 이름 길이 유효성 검사에서 최소 길이(1자 이상) 조건을 추가
- 기존에 누락된 최소 이름 길이 조건을 반영하여 에러 발생 로직 수정
- 다양한 자동차 이름 케이스에 대한 기능 테스트 추가
- 각 라운드의 출력 결과를 검증하는 테스트 케이스 추가
- 예외 상황 검증을 위한 입력 값 테스트 케이스 작성
- 테스트 데이터 상수화 및 `testCasesData.js` 파일로 분리
@KMKang01
Copy link

코드 잘 보았습니다! 저는 생각하지 못한 부분이었는데, 로직을 모듈화하신 것을 보고 감탄했네요 ㅎㅎ..
JS 컨벤션이나 커밋 컨벤션도 잘 지켜서 코딩하신 것 같습니다.
저는 아직 Jest 사용법에 대해서 제대로 습득하지 못해서 이번 주차에도 Jest 사용은 못 해보았는데, 현우 님의 커밋을 보고 이번 주차에는 꼭 테스트 코드도 작성을 해봐야겠다는 도전의식이 생기는 것 같습니다.
2주차 과제 고생하셨습니다.!

@gusn9719
Copy link
Author

코드 잘 보았습니다! 저는 생각하지 못한 부분이었는데, 로직을 모듈화하신 것을 보고 감탄했네요 ㅎㅎ.. JS 컨벤션이나 커밋 컨벤션도 잘 지켜서 코딩하신 것 같습니다. 저는 아직 Jest 사용법에 대해서 제대로 습득하지 못해서 이번 주차에도 Jest 사용은 못 해보았는데, 현우 님의 커밋을 보고 이번 주차에는 꼭 테스트 코드도 작성을 해봐야겠다는 도전의식이 생기는 것 같습니다. 2주차 과제 고생하셨습니다.!

리뷰 감사합니다.
저도 Jest가 정확히 뭔지 몰라 원래 제공한 파일을 복사 붙여 넣기 한 다음에 제가 생각한 케이스들만 상수화한 것이 다에요...

Copy link

@Db0111 Db0111 left a comment

Choose a reason for hiding this comment

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

안녕하세요 ! 에러메시지 부분을 객체로 한번에 export 하신 부분 인상깊었습니다 ㅎㅎ 그 부분은 저도 다음주차 과제 진행할때 참고할 부분인 것 같아요! 코드리뷰 남겨두었으니 한번 봐주세요 !


const VARIOUS_NAME_TEST_CASES = [
{
description: '한글, 숫자, 특수문자 통과 테스트',
Copy link

Choose a reason for hiding this comment

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

한글 및 영어 여부를 테스트 케이스로 분리하신 이유가 있으실까요? 한글이든 영어든 둘다 결국 string 값이고 5자의 글자 제한이 들어간건 동일해서 굳이 분리하신 이유가 따로 있을지 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

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

테스트 케이스여서 넣었었습니다. 사실 저는 이런 테스트 케이스 작성은 처음이라 이런 걸 넣어도 작동을 한다~ 보여주는 의미인가 싶어서 넣었죠....

logs: [
'pobi : -',
'woni : ',
'', // 첫 라운드 후 빈 줄 출력
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.

감사합니다. 저는 저렇게 됐을 때 입력 값이 pobi, woni, '' 로 착각할 수 있지 않을까? 라는 우려가 있었습니다.
그런데 말씀대로 input에서 이미 pobi, woni로 적어 놨으니 불필요했겠군요.

class Car {
constructor(name) {
this.name = name;
this.position = 0;
Copy link

Choose a reason for hiding this comment

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

외부에서 클래스 내의 변수에 접근할 수 없도록 private 변수로 활용해보시는 것은 어떨까요? ( 이 부분은 저도 받았던 피드백이라 공유드리고 싶어서 말씀드려요 !)

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다. 이번주 미션은 그렇게 해봐야겠어요!

}
};

const validateCarNameLength = (carNames) => {
Copy link

Choose a reason for hiding this comment

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

input이 빈 문자열일 경우와 5글자 이상일 경우의 에러처리를 분리해서 하시는 걸 권장드립니다! some에 달린 조건을 봤을 때 한눈에 들어오지는 않는 느낌이에요 !

}
};

const validateCarNames = (carNames) => {
Copy link

Choose a reason for hiding this comment

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

validate 함수들을 굳이 한번 더 감싸지 않고 validateCarNames 함수 안에서 if문으로만 분기 처리를 해도 괜찮을 것 같습니다 !

Copy link
Author

Choose a reason for hiding this comment

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

이 함수만 읽고 이게 어떤 일을 할지 보여주는 느낌으로 작성을 했었는데
그냥 validateCarNames에서 if문 3개를 작성하는 게 더 좋아 보인다는 말씀인가요??

Copy link

Choose a reason for hiding this comment

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

넹 이미 if 문으로도 충분히 어떤 걸 검증하는지 확인 가능하니 개인적으로는 하나의 validate 함수 안에 if문을 두는게 나을 것 같아 제안드렸습니다!

};

const validateTryCount = (tryCount) => {
if (isNaN(tryCount) || tryCount <= 0 || !Number.isInteger(tryCount)) {
Copy link

Choose a reason for hiding this comment

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

이것도 에러 처리를 세부적으로 1) 음수인 경우 2) 0인 경우 3) 시도횟수가 문자열일 경우 등등 이렇게 분리하는게 좋을 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다. 1주차 공통 피드백인 하나의 함수에서 최대한 적은 일을 시켜라...
망각했던 것 같아요

Choose a reason for hiding this comment

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

역할을 분리한다면 vaildation파일도 carName 유효성 검사를 처리하는 파일과 Trycount 횟수를 유효성 검사하는 파일로 나누어서 사용해도 좋을 것 같습니다 :)

Copy link

@yoouyeon yoouyeon left a comment

Choose a reason for hiding this comment

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

2주차 미션 정말 수고 많으셨습니다! 🍀
리팩토링 계획까지 꼼꼼하게 readme에 적어두신 부분이 인상적이었네요! 코드에서도 함수 하나가 최대한 작은 일을 할 수 있도록 잘 분리해주셔서 흐름을 읽기 수월했던 것 같습니다 ㅎㅎ
코멘트 몇가지 남겼는데 확인해봐주시면 감사하겠습니다! 3주차 미션도 화이팅입니다! 💪


describe('자동차 경주', () => {
describe('기능 테스트', () => {
test.each(VARIOUS_NAME_TEST_CASES)(

Choose a reason for hiding this comment

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

개인적인 의견이고 저도 아직 정답이 뭔지 모르겠지만,, 테스트 코드 안에서 쓰이는 테스트 케이스들을 변수화해서 테스트코드 밖에서 관리하게 된다면 실제 쓰이는 데이터들을 확인하기 위해서 읽던 파일을 이동해야 하니 가독성에 좋지 않다는 생각이 들기도 합니다...! 재사용되는 것이 아니라면 test.each에 바로 작성해주는 것도 좋을 것 같아요!!

Comment on lines +44 to +46
} catch (error) {
throw error;
}

Choose a reason for hiding this comment

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

error 를 catch해서 다시 throw 해 주는 특별한 이유가 있나요?? 여기서 catch 해주지 않아도 동일한 결과가 나올 것 같아서요!

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 +12
const splitAndRemoveWhitespace = (input) => {
const splitNames = splitCarNames(input);
return removeWhitespace(splitNames);
};

Choose a reason for hiding this comment

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

함수의 이름을 함수 내부에서 하는 동작 자체로 짓게 되면 나중에 함수 내부 동작이 달라졌을 때 이름까지 변경해줘야 하는 상황이 발생할 수도 있을 것 같습니다. (이름 양쪽의 공백을 허용하는 식으로 정책이 바뀐다면 함수 이름에서 AndRemoveWhiteSpace를 제거해줘야 하는 상황을 예로 들 수 있겠네요!)
따라서 함수 내부에서 발생하는 모든 내용을 이름에 표현하기 보다는 makeNameArray (이것도 별로 좋은 이름 같진 않지만..) 이런 식으로 궁극적으로 어떤 목적으로 호출하게 되는 함수인지 정도만 이름에 표현해주면 좋을 것 같습니다!

Copy link
Author

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.

관점의 차이일수도 있지만 저는 결과를 산출해내는 이 파일 내의 유틸함수들도 결국에는 gameLogic에 포함될 수 있을 것 같습니다..! 게임 관련된 연산들이 gameLogic.js 파일과 이쪽에 분산되어 있는 느낌이라 차를 움직이고, 결과를 내는 기능을 아예 합쳐서 모듈화해도 좋을 것 같아요!

Copy link
Author

@gusn9719 gusn9719 Oct 29, 2024

Choose a reason for hiding this comment

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

어렵군요. 감사합니다..! 많은 생각을 하면서 제 코드들을 다시 읽어봐야겠습니다.

Copy link

@Songhyejeong Songhyejeong left a comment

Choose a reason for hiding this comment

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

전반적으로 함수의 책임을 덜어내려고 노력하신 게 잘 드러나는 코드였습니다!! 테스트 예시도 많아서 많이 배우고 갑니다! 2주차 미션 수고많으셨습니다 👍 남은 주차도 파이팅하세요!


const userInputCarNames = async () => {
return await getUserInput(
'경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)\n'

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.

에러 메세지는 사용하는 곳이 여러 곳이었고, 테스트 케이스에서 해당하는 에러에 맞게 출력을 하는 등 쓰임이 많지만 이거는 한 번 사용하고 사용을 안 해서 그랬었습니다.
다음에는 한 번 사용하는 것도 상수화를 고려해야겠군요.

} = ERROR_MESSAGE;

const validateCarNameListLength = (carNames) => {
if (carNames.length < 2) {

Choose a reason for hiding this comment

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

자동차 갯수를 확인하는 방법도 있었군요 👍
이 변수만 봤을 때 자동차의 길이가 1일 때인 느낌이 들어서 변수명을 carNameList 이런식으로 바꾸는 건 어떠신가요?


const moveOnFourOrOver = () => {
const randomNumber = Random.pickNumberInRange(0, 9);
return randomNumber >= 4;

Choose a reason for hiding this comment

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

조건을 리턴하는 함수로 만들 수도 있군요! 배워갑니다 👍


const printRoundStatus = (cars) => {
for (const car of cars) {
printMessage(`$${car.name} : ${'-'.repeat(car.position)}`);

Choose a reason for hiding this comment

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

여기서 $$이 두 번 삽입되면 어떤식으로 출력이 되나요? ${car.name} : ${'-'.repeat(car.position)} 와 어떤 차이가 있는지 궁금합니다..!

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

@mun-kyeong mun-kyeong left a comment

Choose a reason for hiding this comment

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

2주차도 수고하셨습니다~
저도 아직 많이 배우는 중이라 리뷰를 하면서 깨달아가는 점들이 많은 것 같네요
특히 test을 data로 나누어 적용한 점이 놀랐습니다! 좋은 아이디어인 것 같네요

나중에 시간되실때 PR 한번 부탁드립니다! 수고하셨어요

const EXCEPTION_TEST_CASES = [
{
inputs: ['pobi'],
expectedError: INVALID_CAR_NAMES_LENGTH,

Choose a reason for hiding this comment

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

개인적인 취향이 될 수도 있지만, 자칫 INVALID_CAR_NAMES_LENGTHINVALID_CAR_NAME_LENGTH를 사용하다가 헷갈릴 수 있을 것 같다는 생각이 들어요.

INVALID_CAR_LIST_LENGTH과 같은 방식으로 사용하는건 어떤가 싶습니다!

});

describe('자동차 경주 예외 테스트', () => {
test.each(EXCEPTION_TEST_CASES)(

Choose a reason for hiding this comment

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

이런식으로 한번에 예외처리도 가능했군요!!
예외가 더 추가될 때에도 testCasesData.js파일에만 추가하면 되니 관리가 훨씬 편해질 것 같습니다!!

Comment on lines +78 to +80
inputs: ['pobi,pobi'],
expectedError: DUPLICATE_CAR_NAMES,
},

Choose a reason for hiding this comment

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

빈 입력값이 들어왔을때에 대한 예외처리가 빠져있는 것 같아요!
추가로 문자열에 공백이 존재할 경우에도 입력을 허용할 것인지, 아니면 공백이 불가능하게 에러를 출력할 것인지에 대한 고민을 해봐도 좋을 것 같아요.

참고로 저는 공백인 경우엔 hi hi 는 구분이 힘들 것 같아 공백을 허용하지 않았습니다! 본인만의 기준이 있다면 공백을 허용하는것도 괜찮을 것 같아요!!

Choose a reason for hiding this comment

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

지금보니 공백인 경우 removeWhitespace를 통해 공백제거는 해주고 계셨네요!!
좋습니다 :)

Comment on lines +90 to +93
inputs: ['pobi,woni ', 'a'],
expectedError: INVALID_TRY_COUNT,
},
];

Choose a reason for hiding this comment

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

여기도 마찬가지로 입력값이 공백과 빈 값이 들어왔을때에 대한 예외처리가 빠져있는 것 같습니다.
조금 더 세분화하자면 저는 소수인 경우도 예외처리를 해두었는데 이부분은 개인의 취향에 맞게 작성하면 될 것 같아요. :)

};

const validateTryCount = (tryCount) => {
if (isNaN(tryCount) || tryCount <= 0 || !Number.isInteger(tryCount)) {

Choose a reason for hiding this comment

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

역할을 분리한다면 vaildation파일도 carName 유효성 검사를 처리하는 파일과 Trycount 횟수를 유효성 검사하는 파일로 나누어서 사용해도 좋을 것 같습니다 :)

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