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

[ 자동차 경주 ] 이진 미션 제출합니다. #455

Open
wants to merge 37 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
410982c
📚 docs(README): 요구사항 분석
j-nary Oct 27, 2024
0bb5903
✨ feat(constant): 메시지 관련 상수 추가
j-nary Oct 27, 2024
bdefa60
✨ feat(input, output): 입력 및 출력 함수 구현
j-nary Oct 27, 2024
e7ccc48
♻️ refactor(input, output): 함수명 readUserInput, printOutput 으로 변경
j-nary Oct 27, 2024
2b0f3d6
✨ feat(input): 사용자에게 자동차 이름 입력받는 기능 구현
j-nary Oct 27, 2024
df9cb07
✨ feat(input): 사용자에게 시도 횟수를 입력받는 기능 구현
j-nary Oct 27, 2024
c07e435
✨ feat(split): 입력받은 값을 쉼표 기준으로 구분하는 기능 구현
j-nary Oct 28, 2024
57ebe3f
✨ feat(random): 범위 내에서 임의의 숫자 추출하는 기능 구현
j-nary Oct 28, 2024
9cbf847
🧪 test(random): `pickRandomNumberInRange` 함수 테스트 추가
j-nary Oct 28, 2024
30a7ac2
✨ feat(simulate): `Simulator` 클래스 및 상태 초기화 기능 구현
j-nary Oct 28, 2024
7bb7b2a
🛠️ chore: 파일명 `random.js`를 `pickRandomNumberInRange.js` 로 변경
j-nary Oct 28, 2024
49f1cac
✨ feat(simulate): 전진 가능한지 여부 확인하는 기능 구현
j-nary Oct 28, 2024
a4411ba
✨ feat(simulate): `simulate` 메서드를 통해 자동차 전진 기능 구현
j-nary Oct 28, 2024
0acfc7b
✨ feat(output): 실행 결과 출력하는 기능 구현
j-nary Oct 28, 2024
e10f48a
🐛 fix(simulate): 클래스 인스턴스 생성 시 `new` 키워드 추가
j-nary Oct 28, 2024
12f8d17
🧪 test(simulate): `Simulator` 클래스 테스트 추가
j-nary Oct 28, 2024
bad4934
♻️ refactor(test): 상태 검증에서 `toStrictEqual` 로 변경
j-nary Oct 28, 2024
759aac4
🐛 fix(simulate, test): `simulate` 메서드에서 상태 반환 형식 수정
j-nary Oct 28, 2024
57b2174
♻️ refactor(simulate): state를 private 필드로 변경
j-nary Oct 28, 2024
bff457a
✨ feat(simulate): 최종 우승자 출력하는 기능 구현
j-nary Oct 28, 2024
e354100
🧪 test(simulate): printWinner 테스트 추가 및 private 필드 적용에 따른 수정
j-nary Oct 28, 2024
0fb75f3
♻️ refactor(constant): `#canMoveForward` 메서드에서 범위 상수 사용
j-nary Oct 28, 2024
d68f883
✨ feat(error): `Validator` 클래스 구현
j-nary Oct 28, 2024
cd6b623
✨ feat(error): 시도 횟수가 양수가 아닐 경우 예외 처리
j-nary Oct 28, 2024
42be5a8
✨ feat(error): 자동차 이름이 중복될 경우 예외 처리
j-nary Oct 28, 2024
cd1516b
✨ feat(error): 자동차 이름이 5자 초과일 경우 예외 처리
j-nary Oct 28, 2024
1dac794
✨ feat(error): 입력값이 없는 경우 예외 처리
j-nary Oct 28, 2024
b09c492
🛠️ chore: 파일명 `random.js` 를 `RandomTest.js` 로 변경
j-nary Oct 28, 2024
4a502f8
🧪 test(error): Application의 예외 처리 테스트 추가
j-nary Oct 28, 2024
8311733
🐛 fix(error): 사용자 입력값에 대한 유효성 검사 추가
j-nary Oct 28, 2024
181c549
♻️ refactor(error): `validate` 메서드에서 빈 값 검증 로직 순서 변경
j-nary Oct 28, 2024
c269b9b
📚 docs(README): 체크리스트 및 폴더구조 확인
j-nary Oct 28, 2024
03d2508
♻️ refactor(simulate): 반복 로직을 `simulate` 메서드로 이동하여 관심사 그룹화
j-nary Oct 28, 2024
46fda47
🛠️ chore: 불필요한 출력 코드 삭제
j-nary Oct 28, 2024
cc5c795
♻️ refactor(simulate): 불필요한 분기 처리 제거
j-nary Oct 28, 2024
616654d
♻️ refactor(simulate): names 배열 초기화 로직을 reduce에서 map으로 변경
j-nary Oct 28, 2024
479e753
♻️ refactor(error): forEach에서 some으로 변경
j-nary Oct 28, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
@@ -1 +1,72 @@
# javascript-racingcar-precourse
## 🚀 요구 사항 분석
### 시작하기

- [x] 요구사항 분석하기 `README`
- [x] constant 작성하기 `constant`

### 입력받기 `input`

- [x] 사용자에게 입력받는 함수 구현하기
- [x] 사용자에게 자동차 이름 입력받기
- [x] 입력받은 값 쉼표(,) 기준으로 구분하기
- [x] 시도할 횟수 입력받기

### 출력하기 `output`

- [x] 출력하는 함수 구현하기
- [x] 실행 결과 출력하기
- [x] 최종 우승자 출력하기

### 게임하기 `game`

- [x] 0~9 무작위값 구하기
- [x] 전진 가능한지 확인하기
- [x] 실행 횟수만큼 실행하기
- [x] 최종 우승자 구하기

### 입력값 예외 처리하기 `error`

- [x] 입력값이 없는 경우
- [x] 이름이 5자 이하가 아닐 경우
- [x] 자동차 이름이 중복될 경우
- [x] 시도 횟수가 양수가 아닐 경우

<br/>

## 🚨 체크리스트
- [x] JavaScript Code Conventions 을 준수하였는가?
- [x] 한 메서드에 오직 한 단계의 들여쓰기만 허용했는가?
- [x] else 예약어를 쓰지 않았는가?
- [x] 모든 원시값과 문자열을 포장했는가?
- [x] 3개 이상의 인스턴스 변수를 가진 클래스를 구현하지 않았는가?
- [x] getter/setter 없이 구현했는가?
- [x] 메소드의 인자 수를 제한했는가?
- [ ] 코드 한 줄에 점(.)을 하나만 허용했는가?
- [x] 메소드가 한가지 일만 담당하도록 구현했는가?
- [x] 클래스를 작게 유지하기 위해 노력했는가?
- [x] 3항 연산자를 사용하지 않았는가?
- [x] AngularJS Commit Conventions 에 맞춰 Commit Message를 작성했는가?
- [x] 이름을 축약하지 않고 의도를 잘 드러냈는가?
- [x] JavaScript API를 적극 활용했는가?
- [x] Jest를 활용해 테스트 코드로 작동을 확인했는가?
- [x] 테스트 코드를 관심사별로 분리했는가?

<br/>

## 📄 폴더구조
```
📦 __tests__
┗━ 📜ApplicationTest.js
Comment on lines +57 to +60

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.

좋게 봐주셔서 감사합니다 :)

📦 src
┣━ 📜App.js
┣━ 📜index.js
┣━ 📂util
┃ ┣━ 📜io.js
┃ ┣━ 📜pickRandomNumberInRange.js
┃ ┣━ 📜printSimulate.js
┃ ┗━ 📜splitByComma.js
┣━ 📜Simulator.js
┣━ 📜Validator.js
┗━ 📜constant.js
```
39 changes: 24 additions & 15 deletions __tests__/ApplicationTest.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import App from "../src/App.js";
import { MissionUtils } from "@woowacourse/mission-utils";
import App from '../src/App.js';
import { MissionUtils } from '@woowacourse/mission-utils';
import { ERROR_MESSAGE } from '../src/constant.js';
import Validator from '../src/Validator.js';

const mockQuestions = (inputs) => {
MissionUtils.Console.readLineAsync = jest.fn();
Expand All @@ -19,18 +21,18 @@ const mockRandoms = (numbers) => {
};

const getLogSpy = () => {
const logSpy = jest.spyOn(MissionUtils.Console, "print");
const logSpy = jest.spyOn(MissionUtils.Console, 'print');
logSpy.mockClear();
return logSpy;
};

describe("자동차 경주", () => {
test("기능 테스트", async () => {
describe('자동차 경주', () => {
test('기능 테스트', async () => {
// given
const MOVING_FORWARD = 4;
const STOP = 3;
const inputs = ["pobi,woni", "1"];
const logs = ["pobi : -", "woni : ", "최종 우승자 : pobi"];
const inputs = ['pobi,woni', '1'];
const logs = ['pobi : -', 'woni : ', '최종 우승자 : pobi'];
const logSpy = getLogSpy();

mockQuestions(inputs);
Expand All @@ -46,15 +48,22 @@ describe("자동차 경주", () => {
});
});

test("예외 테스트", async () => {
// given
const inputs = ["pobi,javaji"];
test.each([
{ inputs: [], expectedError: ERROR_MESSAGE.EMPTY_INPUT },
{ inputs: ['pobi,javaji'], expectedError: ERROR_MESSAGE.NAME_TOO_LONG },
{ inputs: ['pobi,pobi'], expectedError: ERROR_MESSAGE.DUPLICATE_NAME },
{ inputs: ['pobi,java'], expectedError: ERROR_MESSAGE.EMPTY_INPUT },
{
inputs: ['pobi,java', '-1'],
expectedError: ERROR_MESSAGE.INVALID_TRY_COUNT,
},
{
inputs: ['pobi,java', '0'],
expectedError: ERROR_MESSAGE.INVALID_TRY_COUNT,
},
])('예외 테스트: %o', async ({ inputs, expectedError }) => {
mockQuestions(inputs);

// when
const app = new App();

// then
await expect(app.run()).rejects.toThrow("[ERROR]");
await expect(app.run()).rejects.toThrow(expectedError);

Choose a reason for hiding this comment

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

저는 예외 테스트 시 던지는 오류는 커스텀하지 않았는데 여기서도 오류에 맞는 메시지를 띄울 수 있게 설정하신게 좋은 아이디어 같아요! 다만, 어떤 테스트케이스인지 쉽게 알 수가 없어서 .each() 내부에 각각 name을 붙여주시면 더 좋을 것 같네요:)

Copy link
Author

Choose a reason for hiding this comment

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

좋은 피드백 감사합니다! 다음번에는 name 도 활용해봐야겠어요!

Copy link

Choose a reason for hiding this comment

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

아 이렇게 ApplicationTest 등 따로 파일을 만들어서 테스트를 할 수도 있군요..!
저는 몰랐는데 이번 과제부터 이렇게 적용해봐야겠어요! 🔥

});
});
32 changes: 32 additions & 0 deletions __tests__/RandomTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { Random } from '@woowacourse/mission-utils';
import pickRandomNumberInRange from '../src/util/pickRandomNumberInRange';

describe('random function', () => {
beforeEach(() => {
jest
.spyOn(Random, 'pickNumberInRange')
.mockImplementation((startRange, endRange) => {
// 항상 중간값을 반환하도록 모의 처리
return Math.floor((startRange + endRange) / 2);
});
});

afterEach(() => {
jest.restoreAllMocks();
});

test('0-9 사이의 랜덤한 숫자 추출하기', () => {
const startRange = 0;
const endRange = 9;
const result = pickRandomNumberInRange(startRange, endRange);
expect(result).toBeGreaterThanOrEqual(startRange);
expect(result).toBeLessThanOrEqual(endRange);
});

test('mock에서 설정한 중간값 반환되는지 확인하기', () => {
const startRange = 2;
const endRange = 8;
const result = pickRandomNumberInRange(startRange, endRange);
expect(result).toBe(5);
});

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.

어떤 경우에도 일관적인 테스트 결과를 유지하기 위해 spyon 사용해서 pickRandomNumberInRange에서 중간값이 뽑히도록 설정해두었기 때문입니다!

Choose a reason for hiding this comment

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

아 넵 이해하였습니다! 답변 감사합니다 :)

});
98 changes: 98 additions & 0 deletions __tests__/SimulateTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
import Simulator from '../src/Simulator'; // Simulator 클래스가 정의된 경로로 수정하세요.
import { MESSAGE } from '../src/constant';
Comment on lines +1 to +2

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 { printOutput } from '../src/util/io';
import pickRandomNumberInRange from '../src/util/pickRandomNumberInRange';

jest.mock('../src/util/io');
jest.mock('../src/util/pickRandomNumberInRange');

describe('Simulator simulate test', () => {
beforeEach(() => {
jest.clearAllMocks();
});
Comment on lines +10 to +12

Choose a reason for hiding this comment

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

오 모든 모킹을 afterEach가 아니라 beforeEach에서 해주셨네요?
이런 구현방식을 택하신 이유가 있으실까요?

Copy link
Author

Choose a reason for hiding this comment

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

beforeEach에서 jest.clearAllMocks()를 사용하는 이유는 각 테스트가 실행되기 전에 모킹 상태를 초기화하여 테스트 간에 영향을 미치지 않도록 보장하기 위함입니다!
이렇게 하면 각 테스트가 독립적으로 실행되며 이전 테스트의 영향을 받지 않습니다.

반면에 afterEach에서 jest.clearAllMocks()를 사용하는 경우, 현재 테스트가 완료된 후에 모킹이 초기화되기 때문에 다음 테스트의 상태가 이전 테스트의 영향을 받을 가능성이 있습니다.

따라서, 테스트가 시작되기 전에 항상 초기 상태를 유지하려는 의도라면 beforeEach에서 초기화하는 것이 더 적합하다고 생각합니다 :)


test('#canMoveForward가 true일 때 상태 업데이트 확인하기', () => {
const names = ['Car1'];
const simulator = new Simulator(names);
pickRandomNumberInRange.mockReturnValue(5);

simulator.simulate(1);
expect(printOutput).toHaveBeenCalledWith(MESSAGE.EXECUTE_OUTPUT);
expect(printOutput).toHaveBeenCalledWith('Car1 : -');
expect(printOutput).toHaveBeenCalledWith('');
});

test('#canMoveForward가 false일 때 상태 업데이트 확인하기', () => {
const names = ['Car1'];
const simulator = new Simulator(names);
pickRandomNumberInRange.mockReturnValue(3);

simulator.simulate(1);

expect(printOutput).toHaveBeenCalledWith(MESSAGE.EXECUTE_OUTPUT);
expect(printOutput).toHaveBeenCalledWith('Car1 : ');
expect(printOutput).toHaveBeenCalledWith('');
});

test('자동차가 여러 개일 때 상태 업데이트 확인하기', () => {
const names = ['Car1', 'Car2'];
const simulator = new Simulator(names);
pickRandomNumberInRange.mockReturnValueOnce(5).mockReturnValueOnce(3);

simulator.simulate(1);

expect(printOutput).toHaveBeenCalledWith(MESSAGE.EXECUTE_OUTPUT);
expect(printOutput).toHaveBeenCalledWith('Car1 : -');
expect(printOutput).toHaveBeenCalledWith('Car2 : ');
expect(printOutput).toHaveBeenCalledWith('');
});
});
Copy link

@MinJaeSon MinJaeSon 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
Author

Choose a reason for hiding this comment

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

좋은 영향을 드렸다니 너무 뿌듯하네요 :)


describe('Simulator printWinner test', () => {
beforeEach(() => {
jest.clearAllMocks();
});

test('우승자가 한 명일 경우', () => {
const names = ['Car1', 'Car2', 'Car3'];
const simulator = new Simulator(names);

pickRandomNumberInRange.mockReturnValueOnce(5);
pickRandomNumberInRange.mockReturnValue(3);
Comment on lines +60 to +61

Choose a reason for hiding this comment

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

pickRandomNumberInRange.mockReturnValueOnce(5).mockReturnValue(3);

두 개 한 번에 연결하셔도 됩니다 :)

Copy link
Author

Choose a reason for hiding this comment

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

체크리스트에 . (메서드 연쇄) 를 사용하지 않았는가 를 체크하는 부분이 있습니다
메서드 연쇄를 사용하면 코드가 간결해진다는 장점이 있지만, 그만큼 코드 한 줄이 의미하는 복잡성이 증가하게 되고 이는 가독성 저하로 이어집니다
코드가 간결한 것도 중요하지만, 저에게 이번 우테코 프리코스에서 가장 중요하게 생각하는 것은 가독성이었기에 이렇게 작성하였습니다!


simulator.simulate(1);
simulator.printWinner();

expect(printOutput).toHaveBeenCalledWith(`${MESSAGE.FINAL_OUTPUT} : Car1`);
});

test('우승자가 여러 명일 경우', () => {
const names = ['Car1', 'Car2', 'Car3'];
const simulator = new Simulator(names);

pickRandomNumberInRange.mockReturnValueOnce(5);
pickRandomNumberInRange.mockReturnValueOnce(5);
pickRandomNumberInRange.mockReturnValue(3);

simulator.simulate(1);
simulator.printWinner();

expect(printOutput).toHaveBeenCalledWith(
`${MESSAGE.FINAL_OUTPUT} : Car1, Car2`,
);
});

test('아무도 전진하지 않았을 경우', () => {
const names = ['Car1', 'Car2', 'Car3'];
const simulator = new Simulator(names);

pickRandomNumberInRange.mockReturnValue(1);

simulator.simulate();
simulator.printWinner();

expect(printOutput).toHaveBeenCalledWith(
`${MESSAGE.FINAL_OUTPUT} : Car1, Car2, Car3`,
);
});
});
20 changes: 19 additions & 1 deletion src/App.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,23 @@
import { MESSAGE } from './constant';
import Simulator from './Simulator.js';
import { readUserInput } from './util/io.js';
import splitByComma from './util/splitByComma';
import Validator from './Validator';

class App {
async run() {}
async run() {
const input = await readUserInput(MESSAGE.PROMPT_NAME_USER_INPUT);
Validator.validate(input);
const cars = splitByComma(input);
Validator.validate(cars);

Choose a reason for hiding this comment

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

유효성 검사를 위한 메소드를 input 입력 이후 한 번, cars 입력 이후에 또 한 번 해서 두 입력에 대해 각각 호출하고 있는데, 한 번만 호출하도록 로직을 구성하는 것이 좀 더 좋을 것 같아요:)
아예 처음 input에 대한 유효성 검사와 cars에 대한 유효성 검사를 분리하는 것도 좋아보이는데, 클래스를 사용하신 걸로 보아 개인적으로는 Validator.js 내에서 validate() 메소드 자체를 분리시키는 건 어떨까 의견 내봅니다!

Choose a reason for hiding this comment

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

바로 밑에 attempCount에 대한 유효성 검사도 포함해야겠네요

Copy link
Author

Choose a reason for hiding this comment

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

한번에 유효성 검사를 할 수도 있지만, 저는 input에 대해 유효성 검사가 통과되지 못하면 다음단계로 넘어가지 못하도록 분기처리 역할을 만들어주고 싶어서 이렇게 프로그래밍하게 되었습니다!
input 값이 undefined가 아닌 적절한 값이 매핑되었다는 것을 먼저 확인해준 후 splitByComma 와 같은 기능을 수행하고 싶었거든요!
그렇지 않다면, splitByComma 에서 인자로 받은 값의 type을 확인해줘야하는 번거로움이 있는데, 이를 방지하기 위해 매 값마다 validate를 호출하는 형식을 채택했습니다!
이에 대해 어떻게 생각하시는지 의견이 궁금하네요!!

Copy link

@MinJaeSon MinJaeSon Oct 31, 2024

Choose a reason for hiding this comment

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

우선 자세히 생각을 알려주셔서 감사해요😊 어떤 의도로 이렇게 하셨는지 이해했습니다!


const attemptCount = await readUserInput(MESSAGE.PROMPT_COUNT_USER_INPUT);
Validator.validate(+attemptCount);
Comment on lines +14 to +15

Choose a reason for hiding this comment

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

+연산자로 형변환을 하셨군요! 그런데 최근에 보니까 22.3 이런 컨벤션이 있더라구요! +로 형변환하는 것보다 Number()를 하는 것이 조금 더 의도가 명확하다는 것 같아요~ 확인해보시면 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

맞습니다! 노티 감사드립니다 앞으로는 Number() 를 활용해야겠어요


const simulator = new Simulator(cars);
simulator.simulate(attemptCount);
simulator.printWinner();
Comment on lines +17 to +19

Choose a reason for hiding this comment

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

오호.. Simulator라.. 너무 괜찮은 클래스명인데요? 저도 다음에 한 번 써먹어보겠습니다🤤

Copy link
Author

Choose a reason for hiding this comment

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

넹 !! 좋은 영향을 드려 기쁘네요!

}
}

export default App;
38 changes: 38 additions & 0 deletions src/Simulator.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { MESSAGE, RANGE } from './constant';
import { printOutput } from './util/io';
import pickRandomNumberInRange from './util/pickRandomNumberInRange';
import printSimulate from './util/printSimulate';

class Simulator {
#state;
Copy link

Choose a reason for hiding this comment

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

이 #state가 의미하는게 무엇인가요??

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 +7 to +8

Choose a reason for hiding this comment

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

state라는 상태명은 조금 모호한 것 같은데 어떻게 생각하시나요? 처음 봤을 때 무엇인지 알기 힘들다는 느낌을 받았어요😅

Copy link
Author

Choose a reason for hiding this comment

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

자동차의 전진상태를 나타내려고 한 건데 변수명을 너무 추상적으로 지은 것 같네요!
progressState 와 같은 이름을 정할 걸 그랬어요! 알려주셔서 감사합니다 :)

constructor(names) {
printOutput(MESSAGE.EXECUTE_OUTPUT);
this.#state = names.map((name) => ({ name, count: 0 }));
}

#canMoveForward() {
return pickRandomNumberInRange(RANGE.START, RANGE.END) >= RANGE.VALID;
}
Comment on lines +14 to +16

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.

메서드와 프로퍼티를 private로 분리할때 기준이 있으신가요?!

Copy link
Author

Choose a reason for hiding this comment

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

외부에서 사용되지 않을 경우 (내부에서만 사용될 경우) private 으로 처리하여 캡슐화를 강화시켜주었습니다!


simulate(attemptCount) {
for (let i = 0; i < attemptCount; i++) {
this.#state = this.#state.map(({ name, count }) => {
if (this.#canMoveForward()) count++;
printSimulate(name, count);
return { name, count };
Comment on lines +20 to +23

Choose a reason for hiding this comment

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

for문 안에 있는 친구들을 다른 메서드로 분리한다면 depth도 줄이고 가독성도 좋아질 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

인정합니다! 좋은 피드백 감사합니다 :)

});
printOutput('');

Choose a reason for hiding this comment

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

\n 로 개행이라는 것을 명확하게 표현해주면 좋을 것 같아요

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.

근데 아마 \n으로 줄바꿈하면 두 줄이 띄워질 수도 있슴다!
이미 Console.print에서 줄바꿈이 포함되어 있어서용

}
}

printWinner() {
const maxCount = Math.max(...this.#state.map((car) => car.count));
const winner = this.#state
.filter((car) => car.count === maxCount)
.map((car) => car.name);
printOutput(`${MESSAGE.FINAL_OUTPUT} : ${winner.join(', ')}`);
}
}

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.

인정합니다! 좋은 피드백 감사합니다 :)

Choose a reason for hiding this comment

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

우승자를 출력하는 역할인 printWinner에서, 우승자를 결정하는 로직까지 포함하고 있는 것 같아요!
분리하는 것도 좋아보입니다 ㅎㅎ!
만약 그렇게 된다면 printWinner는 단순히 우승자를 print해주는 역할이 될텐데요!
이럴 경우에 과연 Simulator에서 관리할 메서드인지에 대해 고민해보면 좋을 것 같네요.


export default Simulator;
46 changes: 46 additions & 0 deletions src/Validator.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import { ERROR_MESSAGE, MAX_INPUT_LENGTH } from './constant';

class Validator {
#value;
constructor(value) {
this.#value = value;
}
Comment on lines +5 to +7

Choose a reason for hiding this comment

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

각 메서드의 인자로 넣지 않고 constructor로 value를 받으시는 이유가 있으실까요?

Copy link
Author

Choose a reason for hiding this comment

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

클래스 내에서 모든 메서드가 해당 value를 공유하는데, 매번 인자로 넘겨주기보다 클래스의 속성을 활용하고 싶었습니다!


#validateNotEmpty() {
if (this.#value !== 0 && !this.#value) {
throw new Error(ERROR_MESSAGE.EMPTY_INPUT);
}
}
Comment on lines +9 to +13

Choose a reason for hiding this comment

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

프라이빗을 어디에 붙여야할지 기준이 확고하신 것 같네요!
저는 아직 잘 모르겠어서...
진님 처럼 프라이빗 쓰면 되겠다는 생각이 들었습니다.
이렇게 한다면 외부에서 static만 가져다쓰면 되니까!
배워갑니다 ㅎㅎ!

Copy link
Author

Choose a reason for hiding this comment

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

싱글톤 패턴을 활용하였습니다!
관련 아티클: 싱글톤 패턴의 사용 이유와 문제점 참조해보셔도 좋을 것 같아요!


#validatePositiveNumber() {
if (this.#value <= 0) {
throw new Error(ERROR_MESSAGE.INVALID_TRY_COUNT);
}
}

#validateMaxLength() {
if (this.#value.some((value) => value.length > MAX_INPUT_LENGTH)) {
throw new Error(ERROR_MESSAGE.NAME_TOO_LONG);
}
}

#validateNoDuplicates() {
const hasDuplicates = new Set(this.#value).size !== this.#value.length;
if (hasDuplicates) {
throw new Error(ERROR_MESSAGE.DUPLICATE_NAME);
}
}
Copy link

Choose a reason for hiding this comment

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

Set을 사용한 중복 검사 너무 좋네요!

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.

와 MAX_INPUT_LENGH 도 따로 constant로 빼서 상수화하셔서 쓴 부분 너무 좋은 것 같아요
이진님 코드 보면서 상수를 쓰는 방법에 대해 다시 한 번 더 생각해보게 되네요!


static validate(value) {
const validator = new Validator(value);
validator.#validateNotEmpty();
if (typeof validator.#value === 'number') {
validator.#validatePositiveNumber();
} else if (typeof validator.#value === 'object') {

Choose a reason for hiding this comment

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

null도 object로 판별되어 예상치 못한 동작이 될 것 같기도 한데, 이에 대해 어떻게 생각하시나요??

Copy link
Author

Choose a reason for hiding this comment

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

조건문을 들어가기 전에 notEmpty 메서드로 빈값인지 확인하기 때문에 해당 사이드이펙트는 없을 거라고 예상됩니다!

Comment on lines +37 to +39

Choose a reason for hiding this comment

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

해당 조건문에서 else if 가 꼭 필요한 자리는 아닌 것 같아요.

제가 이렇게 생각한 이유는 number이면 object가 아니고, object이면 number가 아니기 때문입니다!

코드를 단편적으로 보아 제가 오해할 수도 있어서 어떤 의도가 있었는지 여쭙습니다! ㅎㅎ

validator.#validateMaxLength();
validator.#validateNoDuplicates();
}

Choose a reason for hiding this comment

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

if문보다 switch문을 사용하면 조금 더 가독성이 좋을 것 같아요! typeof validator.#value가 중복되기도 하구요!

Copy link
Author

Choose a reason for hiding this comment

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

인정합니다! 그 생각까지 못했었네요 좋은 피드백 감사합니다 :)

}
}

export default Validator;
Copy link

Choose a reason for hiding this comment

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

validatior를 class로 구현할 생각을 못했는데 class로 구현하니 가독성도 좋고 코드가 깔끔한거 같네요!!

Copy link
Author

Choose a reason for hiding this comment

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

좋게 봐주셔서 감사합니다 :)

Loading