[자동차 경주] 정해성 미션 제출합니다. #2

junghaesung79 · 2024-10-27T06:03:13Z

2주 차 자동차 경주 게임을 구현했습니다.

이렇게 구현했어요!

사용자에게서 입력받고 터미널에 출력하는 부분을 UserInterface 클래스를 만들어서 담당하게 했습니다.
UserInterface에서 Validator 클래스를 사용해서 사용자의 입력을 검증했습니다.
RacingGame, Cars, Car 클래스를 만들어서 게임과 관련된 로직들을 처리했습니다.
에러 메시지와 입출력 안내 문구를 상수화하였습니다.

이 부분을 리뷰해주세요!

Validator는 인스턴스를 생성하지 않지만 내부적으로 로직이 많이 있고, 기능들 사이에 연관성이 있어서 강하게 묶어주는 느낌으로 클래스로 만들었습니다.
MyUtils는 기능들이 더 추가될 수도 있지만, 그 기능들 사이에 공통점이 옅고 지금은 그 기능들이 별로 없어서 단순히 네임스페이스를 지정하는 느낌으로 객체 리터럴을 사용했습니다.
로직 진행 과정이 영어 문장을 한 단어씩 이해하는 것처럼 읽히도록 RacingGame, Cars, Car 처럼 클래스 분리를 하였습니다. 주요 로직에서 가독성이 떨어지는 부분이 있으면 언제든지 리뷰 남겨주세요!

- 요구사항 - 구현 단계 - 알고리즘 흐름도 - 특수 상황 정리

- eslint - prettier - config airbnb

- Car 클래스 구현 - 입력을 변환하여 필드에 할당 - Car 인스턴스 생성 - 시도 구현

- try -> attempt

- MyUtils => GameUtils

manNomi

폴더 구조와 코드가 너무 깔끔해서 너무 읽기 좋았습니다.
특히 객체란 무엇인지 정확히 배우고 가는것 같습니다
좋은 코드 감사합니다

manNomi · 2024-10-28T15:15:06Z

__tests__/ApplicationTest.js

-    await expect(app.run()).rejects.toThrow("[ERROR]");
+    await expect(app.run()).rejects.toThrow('[ERROR]');
+  });
+};


테스트 코드가 상세해서 무조건 예외처리 만점이네요!!
테스트 코드를 파일 분리 안하신 이유가 있을까요 ?

처음에는 분리하려고 했었다가 겹치는 코드가 좀 있는 것 같아서 다시 붙였습니다😅

manNomi · 2024-10-28T15:15:53Z

src/App.js

 class App {
-  async run() {}
+  async run() {
+    const nameString = await UserInterface.processStringInput(UI_MESSAGES.NAME_STRING_INPUT);


상수를 모두 분리하신 것이 인상적이네요
이것 같이 한번만 사용하는 상수를 분리하신 이유가 있을까요 ?

경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분) 이런 문자열이 직접 들어가는 것보다 상수명을 통해서 무슨 글인지 알 수 있게 하는 것이 조금 더 읽기 편하지 않을까 생각했습니다!

UserInterface 로 묶은 부분이 인상적입니다!

manNomi · 2024-10-28T15:16:45Z

src/models/Car.js

@@ -0,0 +1,31 @@
+import { GameUtils, UserInterface } from '../utils/index.js';
+
+class Car {


자동차 객체로 만든거 너무 좋은 방식이네요
객체로 만드셔서 좋았던 장점이 있을까요 ?

클래스를 나누면서 관심사를 분리하기 편했던 것 같아요. 함수나 변수를 분리하는 기준이 되기도 하고, 이렇게 분리되어 있으니 한 번에 많은 생각을 하지 않아도 되어서 좋았어요!

manNomi · 2024-10-28T15:17:40Z

src/models/Cars.js

+import { UserInterface } from '../utils/index.js';
+import { UI_MESSAGES } from '../constants/uiMessages.js';
+
+class Cars {


car 와 cars 가 의미가 너무 비슷한데 방법이 없을려나요 ..
충분히 좋은 이름이지만 더더욱 좋은게 있을지 고민이 되네요

혹시 어떤 부분에서 헷갈리는 부분이 있으셨나요..? 안에 있는 메소들도 자동차 한 대에서 가질 메소드, 여러 대일 때에 가능한 메소드.. 이런 식으로 분리했었는데요.😥

아 혹시 이름이 비슷하다는 말씀이신가요오..

이름이 비슷해서 혼동될 수 있다는 말인 것 같습니다
cars와 car이 조금 헷갈릴 수 있다면 carList라는 클래스명은 어떤가요?
근데 이부분도 개인 취향이 좀 탈수도 있는게 이번에 스터디 진행하면서 한 분께선 함수 및 클래스 이름에 List, Array같은 이름이 들어가는걸 선호하지 않는 분도 계시더라구요

CarList 좋은 것 같아요! 감사합니다~

manNomi · 2024-10-28T15:19:45Z

src/models/index.js

@@ -0,0 +1,3 @@
+export { default as RacingGame } from './RacingGame.js';


파일 관리를 위해서 index 를 두어서 import 하는 방식 너무 좋네요 참고하겠습니다

1주 차에서 다른 분들을 리뷰하다가 얻은 지식입니다! 배럴 패턴이라고 해요!

manNomi · 2024-10-28T15:24:23Z

src/utils/Validator.js

@@ -0,0 +1,41 @@
+import { ERROR_MESSAGES } from '../constants/errorMessages';
+
+class Validator {


static으로 이루어져있어서 객체 생성 안해도 되는 장점이 있겠네요 !!
근데 그렇다면 굳이 생성할필요가 없다면 class로 만드신 이유가 있을까요 ??

이 부분을 저도 고민했었는데요. 아직 객체 리터럴로 할 지 클래스로 할 지 기준이 명확하게 서지 않은 것 같아요😥 관리하고 있는 상태가 없어서 객체 리터럴도 좋았을 것 같아요...

MinSungJe

안녕하세요, 2주차 코드 너무 잘봤습니다.☺️

저도 객체 리터럴과 클래스 선언의 경에 대해 고민을 가지고 있었던 참인데, 해성님만의 기준을 공유해주셔서 너무 감사합니다. 들어가는 함수들의 연관성 여부에 따라 분리를 하셨군요.

전체적으로 로직이 잘 읽히도록 고민을 하셔서인지 전체적으로 구조가 잘 읽히고 깔끔한 코드였습니다. 경주에 필요한 요소를 Models에, 그걸 움직이는 함수모음을 utils에 보관하는, 의도가 보이는 분리라고 생각이 드네요, 많이 배워갑니다🥲

다만 models의 Car와 Cars가 너무 비슷한 점이 걸립니다. Car, CarList? Driver, Cars? 명확한 이름이 떠오르진 않지만 보관한 폴더 경로도 똑같아서 얼핏보면 헷갈릴거 같습니다.

전체적인 코드의 리뷰를 남깁니다. 앞서 언급했듯 배워갈 점이 많은 코드였습니다.🙂
2주차 고생 많으셨고, 3주차도 화이팅입니다!

MinSungJe · 2024-10-28T15:19:28Z

README.md

+- 이름이 한 개인 경우
+- 같은 이름이 있는 경우
+- 최대 시도 횟수를 초과한 값을 입력한 경우
+  - 15자리수: 원시 자료형 Number가 안전하게 표현할 수 있는 수(16자리) - 1


저번 코드랑 똑같은 이유로 Number의 최댓값을 지정해두셨네요👍

네ㅎ 그런데 이번에는 주관적으로 더 낮은 숫자를 할까도 생각했었어요. 실제로 저 숫자를 입력하면 터미널이 타라라라락 되어서 조금 더 사실적인 숫자도 좋았을 것 같아요🫠

주도적으로 문제 정의한 게 꼼꼼하고 좋아보이네요 !! 👍

MinSungJe · 2024-10-28T15:21:16Z

__tests__/ApplicationTest.js

+    });
+  });
+
+  describe('예외 테스트', () => {


긴 코드의 반복되는 부분을 묶어서 한 줄로 테스트 케이스를 추가하는 구문이 인상깊습니다...🙂

아직 jest가 익숙하지 않아서 급조한 코드라... 칭찬받으면🫠🫠
3주 차에는 조금 더 jest 적으로 중복을 줄여볼게요..!

MinSungJe · 2024-10-28T15:23:21Z

src/App.js

 class App {
-  async run() {}
+  async run() {
+    const nameString = await UserInterface.processStringInput(UI_MESSAGES.NAME_STRING_INPUT);


저는 Console.readLineAsync()로 입력을 받고 밑에 이를 처리하는 코드를 바로 parseInput 이런식으로 적었던 거 같은데, 이를 UserInterface로 한 줄로 묶은 셈이네요! 코드가 더 깔끔해진거 같습니다.

맞아요! 저도 입력이 두 개인 점에서
입력1 - 입력2 - 검증1 - 검증2
or
입력1 - 검증1 - 입력2 - 검증2
어느 게 좋을까? 이런 고민을 하다가 이렇게 구현하게 되었어요😅

오 글쿤요. 저도 두 방법 중에 고민하다가 개인취향으로 전자를 택했는데, 해성님은 후자를 택하신 부분에서 코드의 다양성을 경험할 수 있어서 좋았습니다!

MinSungJe · 2024-10-28T15:29:10Z

src/constants/uiMessages.js

@@ -0,0 +1,6 @@
+export const UI_MESSAGES = Object.freeze({


export가 하나밖에 없다면 default를 사용해보시는 건 어떨까요?🙂 (10.6)

맞아요 이런 컨벤션이 있었죠! 하지만 제 개인적인 생각으로는 여전히 named export를 사용하고 싶어요🫠 우선 상수라는 성격이 특수하고 named export이면 어퍼 스네이크 케이스를 사용하더라도 헷갈리지 않고 import 할 수 있어요! 저는 함수나 객체 리터럴, 클래스와 같은 모듈에서 이 10.6 컨벤션을 적용하려고 해요!

친절한 답변 감사합니다!😊 헷갈리지 않고 import 할 수 있다는 이야기는 불러온 어퍼 케이스가 상수인지 객체인지 헷갈리는 상황에서 import { 상수명 }으로 둘 다 가져올 수 있으니 좋다는 의도로 이해했는데 맞을까요?

오오 헷갈리게 말했군요! 제가 생각했던 상황은 어퍼 스네이크가 자주 사용하는 형식이 아니라서 눈에 안 익숙할 것이고 UIMESSAGES나 MESSAGES 처럼 잘못 적었는데 못 알아채는 거였어요😅 제가 named export를 선호하는데 그 이유가 휴먼 에러를 방지할 수 있어서예요!

MinSungJe · 2024-10-28T15:31:51Z

src/models/Cars.js

+import { UserInterface } from '../utils/index.js';
+import { UI_MESSAGES } from '../constants/uiMessages.js';
+
+class Cars {


Car클래스의 집단을 나타내는 클래스를 선언하는 방식이 있었군요. 저는 Car배열을 하나 만들어서 이리저리 다뤘는데, 배워갑니다.
cars의 내용을 상태로 담을 수 있으므로 static으로 다루지 않고 관리하신 기법이 돋보입니다.👍

저도 처음에는 RacingGame에 cars 배열 상태를 갖는 방식으로 구현했었어요! 하지만 그 두 클래스에서 메소드가 점점 늘어나서 분리를 생각했죠. Cars를 처음 만들었을 때는 Cars의 성격이 Car과도 비슷하고 RacingGame하고도 비슷해서 고민을 했던 기억이 나네요😁

MinSungJe · 2024-10-28T15:35:45Z

src/utils/Validator.js

+import { ERROR_MESSAGES } from '../constants/errorMessages';
+
+class Validator {
+  static rules = {


조건을 함수로 모두 뭉쳐서 Validator랑 같은 파일에 둔 게 가독성 측면에서 너무 좋네요.

여기의 2번을 보고 처음 이런 방식을 알았어요! js 클린코드에 대한 블로그인데 꼭 한 번 읽어보시는 것을 추천해요~ 핵심들만 있어서 금방금방 잘 읽혀요!

"또한 내부 로직을 안 보고도 함수명을 통해 어느 정도 예측할 수 있는 장점이 있다."

MinSungJe · 2024-10-28T15:46:14Z

src/models/index.js

@@ -0,0 +1,3 @@
+export { default as RacingGame } from './RacingGame.js';


이것은 무슨 역할을 하는 코드인가요?🤔 혹시 10.3의 코드와 비슷한 예시인가요?

오호... 이런 컨벤션이 있었군요. 다음 주차부터는 import와 export를 분리해야겠어요👍
이 코드는 배럴 패턴이라고 해서 default export를 모아서 named export로 re-export하는 코드예요! 이렇게 하면 예를 들어 세 줄로 import할 것들을 한 줄로 import 할 수 있고, 모듈 이름을 import 하면서 오타가 날 걱정도 안 해도 돼요!😄

답변 감사합니다! index.js로 import를 편하게 만들어주는거네요. 저도 3주차때부터 활용해봐야겠습니다.😊

junghaesung79 · 2024-10-28T17:38:45Z

저도 객체 리터럴과 클래스 선언의 경에 대해 고민을 가지고 있었던 참인데, 해성님만의 기준을 공유해주셔서 너무 감사합니다. 들어가는 함수들의 연관성 여부에 따라 분리를 하셨군요.

사실은 아직 정확한 판단 기준이 안 선 것 같아요..😥 지금 보면 Validator나 UserInterface 클래스의 경우, 모든 값이 static이고, 따로 관리하는 상태도 없어서 객체 리터럴을 했을 걸 생각하기도 해요.🫣

junghaesung79 · 2024-10-28T17:38:56Z

지금 시간 02:38... 내일 일어나서 민제님 것도 리뷰해드릴게요!!!🫠

MinSungJe · 2024-10-29T03:19:19Z

감사합니다! #100 입니다.

j-nary

Car Cars RacingGame 으로 나눈 부분이 정말 인상적이었습니다!
코드리뷰하면서도 많이 배우고 가네용
시간되시면 제 PR 코드리뷰 도 부탁드립니다 :)

j-nary · 2024-10-29T09:53:32Z

README.md

+- 이름이 한 개인 경우
+- 같은 이름이 있는 경우
+- 최대 시도 횟수를 초과한 값을 입력한 경우
+  - 15자리수: 원시 자료형 Number가 안전하게 표현할 수 있는 수(16자리) - 1


주도적으로 문제 정의한 게 꼼꼼하고 좋아보이네요 !! 👍

j-nary · 2024-10-29T09:55:40Z

__tests__/ApplicationTest.js

-  test("예외 테스트", async () => {
-    // given
-    const inputs = ["pobi,javaji"];
+const testExeptionTamplate = (query, inputs) => {


test.each 가 아닌 별도의 함수로 분리하신 특별한 이유가 있으신지 궁금합니다!

그건! 아직 제가 jest를 못 쓰기 때문입니다!😅

학습하기만 하고 바로 적용하기는 조금 어렵더라구요ㅜ 그래서 pr에 포함시키지 않았어요.

j-nary · 2024-10-29T09:59:51Z

src/App.js

 class App {
-  async run() {}
+  async run() {
+    const nameString = await UserInterface.processStringInput(UI_MESSAGES.NAME_STRING_INPUT);


UserInterface 로 묶은 부분이 인상적입니다!

j-nary · 2024-10-29T10:02:46Z

src/models/Car.js

+class Car {
+  constructor(name) {
+    this.name = name;
+    this.score = 0;


score 같은 경우는 외부에서 값을 변경하지 못하도록 private 필드로 지정해주는 것도 좋은 방법인 것 같아요! 해성님은 어떻게 생각하시는지 궁금합니다 :)

js로 클래스를 사용하는 것이 익숙치 않아서 private 필드로 설정하는 방법이나 필요에 대해 깊이 생각하지 않았었어요! private하게 가져가는 것은 정말 좋은 것 같아요!

j-nary · 2024-10-29T10:04:00Z

src/models/Cars.js

+
+  race(counts) {
+    UserInterface.print(UI_MESSAGES.EXECUTION_RESULT);
+    for (let i = 0; i < counts; i += 1) {


비교적 많이 쓰이는 i++ 이 아닌 i += 1 을 사용하신 특별한 이유가 있으신지 궁금합니다!

어디선가 봤었던 것 같은데 js에서 ++을 사용하지 말라고 했었던 것 같아요...ㅎㅎ 한 번 근거를 다시 찾아봐야겠어요.

13.6 이런 컨벤션이 있었군요.😲

"eslint 문서에 따르면, 단항 증감 구문은 자동으로 세미콜론을 삽입하며, 어플리케이션에서 값을 증감할 때 오류를 일으킬 수 있습니다."

덕분에 저도 배워가네요! 감사합니다 :)

j-nary · 2024-10-29T10:08:02Z

src/models/RacingGame.js

+
+  getHighestScore() {
+    const scores = this.cars.getScores();
+    return scores.reduce((acc, cur) => Math.max(acc, cur), 0);


return Math.max(...scores); 와 같이 스프레드 연산자를 사용하는 건 어떨까요?
reduce를 쓰는 방식보다 더 직관적이고 가독성이 높은 방식인 것 같습니다!

오오오오! 배열로 max()를 하는 게 가능했었군요!! 이걸 이제야 알다니.. c++ 버릇으로 깊게 생각 안 하고 두 개 요소를 비교한 것 같아요ㅋㅋ😅 감사합니다~

오잉 그러게요 배열로 max()를 쓸 수 있다는걸 배워갑니다..!

wnsxk2

폴더 구조가 너무 깔끔하고 조건들을 객체로 만드니 가독성이 좋은 것 같습니다!!
해성님 코드를 보고 많이 배우고 갑니다~!

wnsxk2 · 2024-10-29T14:58:21Z

src/utils/Validator.js

@@ -0,0 +1,41 @@
+import { ERROR_MESSAGES } from '../constants/errorMessages';
+
+class Validator {


검증 부분을 클래스로 작성한 부분이 재사용성을 생각했을 때 더 좋은 코드 같네요 ㅎㅎ 하나 배우고 갑니다!!

wnsxk2 · 2024-10-29T14:59:34Z

src/models/Cars.js

+
+  race(counts) {
+    UserInterface.print(UI_MESSAGES.EXECUTION_RESULT);
+    for (let i = 0; i < counts; i += 1) {


처음에는 do ~ while 문을 사용했다가 for문으로 변경한 이유는 가독성 때문인가요??

커밋 기록도 확인해주셨군요! 섬세하신 걸요~~

제가 while (state--) { ... } 이 구문을 좋아해서 사용해보려고 하다가 count의 타이밍이 조금 이상하게 꼬여서 do while로 하게 되고, 가독성이 떨어지는 느낌을 받아 결과적으로 보편적인 for 문으로 변경하게 되었습니다!

xxziiko

안녕하세요! 클래스 관심사 분리가 잘 되어 있는 것 같아서 인사이트 얻고 갑니다!
코드리뷰가 처음이라 조금 서툰데, 좋은 경험하고 갑니다 😊
시간 되시면 저도 리뷰 부탁드릴게요! #266

xxziiko · 2024-10-29T15:31:21Z

src/utils/Validator.js

+  static validateName(nameString) {
+    const names = nameString.split(',');
+
+    if (this.rules.isShorterThanTwo(names)) throw new Error(ERROR_MESSAGES.name.MIN_NAMES);
+    if (this.rules.hasDuplication(names)) throw new Error(ERROR_MESSAGES.name.DUPLICATEED);
+
+    names.forEach((name) => {
+      if (this.rules.isEmpty(name)) throw new Error(ERROR_MESSAGES.name.EMPTY);
+      if (this.rules.hasSpacing(name)) throw new Error(ERROR_MESSAGES.name.HAS_SPACING);
+      if (this.rules.isLongerThanFive(name)) throw new Error(ERROR_MESSAGES.name.MAX_LENGTH);
+    });
+
+    return names;
+  }
+
+  static validateAttempt(attempts) {
+    if (this.rules.isNotNaturalNumber(attempts)) throw new Error(ERROR_MESSAGES.count.NOT_NATURAL_NUMBER);
+    if (this.rules.isNotNumber(attempts)) throw new Error(ERROR_MESSAGES.count.NOT_NUMBER);
+    if (this.rules.isLessThanOne(attempts)) throw new Error(ERROR_MESSAGES.count.MIN_ATTEMPTS);
+    if (this.rules.exceedsMaxDigits(attempts)) throw new Error(ERROR_MESSAGES.count.MAX_LENGTH);
+
+    return attempts;
+  }


확실히 가독성이 좋네요!
좋은 인사이트 얻고갑니다!

xxziiko · 2024-10-29T15:37:07Z

src/models/Car.js

+  printScore() {
+    UserInterface.print(`${this.name} : ${'-'.repeat(this.score)}`);
+  }


저도 놓친 부분인데, -를 상수로 관리하는게 유지보수를 고려했을 때 더 적절하다고 생각되더라구요!
최대한 매직넘버는 피하는게 좋을 것 같아요! 😊

맞아요. 다음부터는 매직넘버들을 설정 상수로써 한 파일에서 관리하려고 합니다!

xxziiko · 2024-10-29T15:40:31Z

src/constants/errorMessages.js

+    MIN_ATTEMPTS: '[ERROR] 1 이상의 시도 횟수를 입력해주세요.',
+    MAX_LENGTH: '[ERROR] 15자리수 이내의 시도 횟수를 입력해주세요.',
+  },
+});


저는 [EEROR] 를 중복으로 작성하는게 싫어서 util 함수를 만들었는데, 이부분도 생각해보시면 좋을 것 같아용!

좋은 방법인 것 같아요! throw new Error('[ERROR] 까지 해서 묶으면 편하게 사용할 수 있을 것 같네요~

오 에러메시지를 그룹화 해서 작성하신 부분이 인상깊습니다!
안그래도 에러메시지를 하나의 ERROR_MESSAGES 안에서 작성하려니 조금 부산스러운 느낌이 있었는데 그룹핑을 통해 조금 더 직관적이고 깔끔하게 구현하셨네요.
배우고 갑니다 👍👍

추가로, 그룹핑 네이밍에도 해당되는 것인진 잘 모르겠지만,, Airbnb 네이밍 컨벤션 23.10 에 보시면 top level에만 upperCase를 작성한다고 나와있습니다!
그룹핑 안의 properties에서도 upperCase를 적용하는게 좋을까요? 아니면 카멜케이스처럼 소문자로 작성하는게 좋을까요
이부분은 조금 헷갈리긴 하네요..

어머 이런 컨벤션이 있었군요! 그렇다면 ERROR_MESSAGE.name.minNames 이런 식으로 적을 수 있을 것 같네요! 감사합니다~

jeheecheon

짧은 코드도 메소드로 잘 분리되어 있어서 코드 흐름 파악하기 정말 좋았습니다.
저는 이번 2주차 때 단일 메소드로 처리하는 것이 나을지, 조건별로 나누어 명확한 형태로 표현할지 저울질을 많이하다 결국 전자를 선택하고 해결했는데 해성님 방식이 오히려 더 나아보이기도 하네요!

jeheecheon · 2024-10-29T20:25:35Z

src/constants/errorMessages.js

Object.freeze는 전달받은 객체의 멤버변수만 동결시키기 때문에 NAME과 COUNT에도 Object.freeze를 사용해주어야 할 것 같아요.

그렇군요! 다음부터는 중첩된 상수 객체를 만들 때, 깊은 동결 함수를 만들어서 사용해야겠어요! 감사합니다~

jeheecheon · 2024-10-29T20:33:19Z

src/models/Cars.js

+
+  race(counts) {
+    UserInterface.print(UI_MESSAGES.EXECUTION_RESULT);
+    for (let i = 0; i < counts; i += 1) {


한 눈에 읽히는 코드라 너무 매력적입니다. 그런데 매직넘버 i가 살짝 아쉬웠어요!!..
++
i+=1 는 잊고 있었는데... 덕분에 리마인드 되었습니다!

아하! 제희님 코드처럼 i가 아닌 의미있는 이름을 사용하면 더 좋을 것 같아요! 만약 중괄호 블록 안에서 i를 사용해야 할 일이 있으면 확실히 헷갈릴 수도 있을 것 같아요😅 감사합니다~
+
i += 1표현을 쓴 이유는 13.6이런 식으로 js에서 증가 연산자를 반기지 않는다고 해서 그랬어요! 확인해보시면 좋을 것 같아요~

jeheecheon · 2024-10-29T20:49:15Z

src/utils/Validator.js

+import { ERROR_MESSAGES } from '../constants/errorMessages';
+
+class Validator {
+  static rules = {


각각 별도의 static 메소드로 두셨을 수도 있는데 rules라는 객체안으로 묶어주셔서 읽는데 너무 편했습니다!

그런데 가독성과는 별개로, 파편화되어 늘어난 api를 미래의 동료 개발자들이 모두 학습하고 적용해주어야 하는데, 모두가 적재적소에 꺼내어 사용하기는 힘들지 않을까라는 생각이 들었습니다.

...개인적으로 2주 차 과제하며 오래 고민하게된 문제인데 해성님은 어떻게 생각하시나요?

음.. 지금 보니 rules도 name과 number로 쪼갰었으면 더 좋았을 것 같네요😅 확실히 규칙이 더 늘어나면 혼란이 생길 수도 있을 것 같아요🫨 하지만 그렇게 늘어날 수록 더더욱 조건문을 분리한 것은 포기할 수 없을 것 같아요. 코드로 해결할 방법이 당장 떠오르지는 않는데, 기획에서 명세한 규칙을 주석으로 자세하게 적는 건 어떨까라는 생각이 들어요.

/** * 사용자 입력값 검증을 위한 규칙들을 정의합니다. * * 이름 검증 규칙: * - 최소 2명 이상의 이름이 입력되어야 함 * - 중복된 이름은 허용되지 않음 * - 각 이름은 비어있지 않아야 함 * - 각 이름에 공백이 포함되면 안됨 * - 각 이름은 5자 이하여야 함 */ static nameRules = { isShorterThanTwo: (str) => str.length < 2, hasDuplication: (str) => new Set(str).size !== str.length, isEmpty: (str) => str === '', hasSpacing: (str) => str.includes(' '), isLongerThanFive: (str) => str.length > 5, };

어떤 것 같으신가요?

jeheecheon · 2024-10-29T20:53:36Z

src/utils/UserInterface.js

+  static printNewline() {
+    Console.print('');
+  }


위에 정의한 print 함수를 사용해서 다음과 같이
print('')로 개행시켜도 될법한데 따로 메소드 정의하고 분리되어 있어서 코드 읽는데 편했습니다.

Console 모듈을 완전히 분리하고 싶다는 욕심이 있었습니다!ㅎㅎ

UserInterface.js의 함수에만 static이 붙은 이유가 있을까요?
지정한 Console 모듈만을 사용해야했어서 그런건지 궁금합니다! 의도하셨던 바가 있으실까요?

인스턴스를 생성하지 않으려고 한 것이 의도였습니다! 하지만 지금 다시보니 사용자가 interface 모듈을 사용하는 의미에서 인스턴스를 생성했어도 좋았을 것 같아요. 아니면 모든 메소드가 static하니까 차라리 객체 리터럴로 만들었어도 괜찮을 것 같아요😅

mun-kyeong

2주차도 수고하셨습니다~!
class를 사용해서 코드가 굉장히 깔끔하게 잘 읽혔던 것 같아요.
특히 index를 사용한 배럴 패턴과 여러 리뷰어들을 통해 airbnb 컨벤션을 다시금 확인하게 되는 시간이였네요 😊😊

많은 고민을 거쳐 작성한 코드라는게 느껴졌던 것 같네요.
나중에 시간날때 첨부해주신 클린코드 블로그를 한번 읽어봐야겠어요. 많은 꿀팁들을 배워갑니다!!
시간나실때 PR 리뷰 한번 부탁드릴께요!!
넘 수고하셨습니다 :)

mun-kyeong · 2024-10-30T09:24:16Z

src/constants/errorMessages.js

+    MIN_ATTEMPTS: '[ERROR] 1 이상의 시도 횟수를 입력해주세요.',
+    MAX_LENGTH: '[ERROR] 15자리수 이내의 시도 횟수를 입력해주세요.',
+  },
+});


오 에러메시지를 그룹화 해서 작성하신 부분이 인상깊습니다!
안그래도 에러메시지를 하나의 ERROR_MESSAGES 안에서 작성하려니 조금 부산스러운 느낌이 있었는데 그룹핑을 통해 조금 더 직관적이고 깔끔하게 구현하셨네요.
배우고 갑니다 👍👍

추가로, 그룹핑 네이밍에도 해당되는 것인진 잘 모르겠지만,, Airbnb 네이밍 컨벤션 23.10 에 보시면 top level에만 upperCase를 작성한다고 나와있습니다!
그룹핑 안의 properties에서도 upperCase를 적용하는게 좋을까요? 아니면 카멜케이스처럼 소문자로 작성하는게 좋을까요
이부분은 조금 헷갈리긴 하네요..

mun-kyeong · 2024-10-30T09:31:34Z

src/models/Cars.js

+import { UserInterface } from '../utils/index.js';
+import { UI_MESSAGES } from '../constants/uiMessages.js';
+
+class Cars {


이름이 비슷해서 혼동될 수 있다는 말인 것 같습니다
cars와 car이 조금 헷갈릴 수 있다면 carList라는 클래스명은 어떤가요?
근데 이부분도 개인 취향이 좀 탈수도 있는게 이번에 스터디 진행하면서 한 분께선 함수 및 클래스 이름에 List, Array같은 이름이 들어가는걸 선호하지 않는 분도 계시더라구요

mun-kyeong · 2024-10-30T09:32:53Z

src/models/RacingGame.js

+
+  getHighestScore() {
+    const scores = this.cars.getScores();
+    return scores.reduce((acc, cur) => Math.max(acc, cur), 0);


오잉 그러게요 배열로 max()를 쓸 수 있다는걸 배워갑니다..!

mun-kyeong · 2024-10-30T09:35:12Z

src/utils/UserInterface.js

+  static printNewline() {
+    Console.print('');
+  }


UserInterface.js의 함수에만 static이 붙은 이유가 있을까요?
지정한 Console 모듈만을 사용해야했어서 그런건지 궁금합니다! 의도하셨던 바가 있으실까요?

rubberchin · 2024-10-31T01:13:46Z

src/utils/Validator.js

+  static validateName(nameString) {
+    const names = nameString.split(',');
+
+    if (this.rules.isShorterThanTwo(names)) throw new Error(ERROR_MESSAGES.name.MIN_NAMES);
+    if (this.rules.hasDuplication(names)) throw new Error(ERROR_MESSAGES.name.DUPLICATEED);
+
+    names.forEach((name) => {
+      if (this.rules.isEmpty(name)) throw new Error(ERROR_MESSAGES.name.EMPTY);
+      if (this.rules.hasSpacing(name)) throw new Error(ERROR_MESSAGES.name.HAS_SPACING);
+      if (this.rules.isLongerThanFive(name)) throw new Error(ERROR_MESSAGES.name.MAX_LENGTH);
+    });
+
+    return names;
+  }
+
+  static validateAttempt(attempts) {
+    if (this.rules.isNotNaturalNumber(attempts)) throw new Error(ERROR_MESSAGES.count.NOT_NATURAL_NUMBER);
+    if (this.rules.isNotNumber(attempts)) throw new Error(ERROR_MESSAGES.count.NOT_NUMBER);
+    if (this.rules.isLessThanOne(attempts)) throw new Error(ERROR_MESSAGES.count.MIN_ATTEMPTS);
+    if (this.rules.exceedsMaxDigits(attempts)) throw new Error(ERROR_MESSAGES.count.MAX_LENGTH);
+
+    return attempts;
+  }


NameValidator랑 AttemptValidator를 작은 모듈로 분리하고
그걸 Validator 모듈에서 import하는 방식으로 변경하면
특정 모듈이 필요로 하는 검증 로직만 모아줄 수 있어 가독성이 좋아질 것 같아요!
코드 확장성도 커지구요 😊

좋은 생각인 것 같아요! 비슷한 구조의 클래스가 만들어지는 게 조금 걸리긴 하지만, 확실하게 관심사가 분리될 것 같아요!

salmonco

고생많으셨습니다!

다른 분들 해주신 코드 리뷰 보면서도 배울 수 있었네요ㅎㅎ

3주차도 화이팅입니다:)

salmonco · 2024-11-01T18:30:30Z

src/App.js

 class App {
-  async run() {}
+  async run() {
+    const nameString = await UserInterface.processStringInput(UI_MESSAGES.NAME_STRING_INPUT);


오 글쿤요. 저도 두 방법 중에 고민하다가 개인취향으로 전자를 택했는데, 해성님은 후자를 택하신 부분에서 코드의 다양성을 경험할 수 있어서 좋았습니다!

salmonco · 2024-11-01T18:39:04Z

src/constants/errorMessages.js

+export const ERROR_MESSAGES = Object.freeze({
+  name: {
+    MIN_NAMES: '[ERROR] 두 명 이상의 이름을 입력해주세요.',
+    DUPLICATEED: '[ERROR] 중복되지 않는 이름을 입력해주세요.',
+    EMPTY: '[ERROR] 이름을 입력해주세요.',
+    HAS_SPACING: '[ERROR] 공백이 없는 이름을 입력해주세요.',
+    MAX_LENGTH: '[ERROR] 5자 이내의 이름을 입력해주세요.',
+  },
+  count: {
+    NOT_NATURAL_NUMBER: '[ERROR] 시도 횟수에 자연수를 입력해주세요.',
+    NOT_NUMBER: '[ERROR] 시도 횟수에 숫자만 입력해주세요.',
+    MIN_ATTEMPTS: '[ERROR] 1 이상의 시도 횟수를 입력해주세요.',
+    MAX_LENGTH: '[ERROR] 15자리수 이내의 시도 횟수를 입력해주세요.',
+  },
+});


errorMessages랑 uiMessages 관련해서,

저는 같은 메시지라는 점에서 여러 파일로 분산되어 있는 것보다 한 파일에 놓는 게 관리가 편할 것 같다는 이유로 error message 객체 하나랑 io message 객체 하나 해서 두 개를 한 파일에 export했었어요. 혹은 message 객체 하나에 프로퍼티로 error랑 io를 둘 수도 있을 것 같습니당. 혹은 message 폴더 내에서 두 파일로 분리해도 되겠네요. (방법은 많은데 뭘 택해야 될지가 고민..)

한 파일에 error message랑 io message 각각 상수로 분리하기

한 파일에 message 객체 하나 두고 프로퍼티로 error랑 io 분리하기

messages 폴더에 error랑 io 메시지를 파일로 분리하기

해성님께선 error랑 io 메시지를 각각 파일로 분리한 이유가 궁금해요! 혹시 해성만의 기준이 있다면 알고싶습니다ㅎㅎ

salmonco · 2024-11-01T18:40:52Z

src/constants/errorMessages.js

+export const ERROR_MESSAGES = Object.freeze({
+  name: {
+    MIN_NAMES: '[ERROR] 두 명 이상의 이름을 입력해주세요.',
+    DUPLICATEED: '[ERROR] 중복되지 않는 이름을 입력해주세요.',
+    EMPTY: '[ERROR] 이름을 입력해주세요.',
+    HAS_SPACING: '[ERROR] 공백이 없는 이름을 입력해주세요.',
+    MAX_LENGTH: '[ERROR] 5자 이내의 이름을 입력해주세요.',
+  },
+  count: {
+    NOT_NATURAL_NUMBER: '[ERROR] 시도 횟수에 자연수를 입력해주세요.',
+    NOT_NUMBER: '[ERROR] 시도 횟수에 숫자만 입력해주세요.',
+    MIN_ATTEMPTS: '[ERROR] 1 이상의 시도 횟수를 입력해주세요.',
+    MAX_LENGTH: '[ERROR] 15자리수 이내의 시도 횟수를 입력해주세요.',
+  },
+});


메시지 유형을 name과 count로 구분한 것 좋네요!

salmonco · 2024-11-01T18:45:20Z

src/models/Car.js

+  canMoveForward(number) {
+    return number >= 4;
+  }


네이밍 관련해서, 저는 isCarMove로 했는데 can~~이란 표현이 더 다가오는 느낌이었어요. 👍

salmonco · 2024-11-01T18:45:45Z

src/models/Car.js

+  moveForward() {
+    this.score += 1;
+  }
+


score에서 1 더하는 부분을 moveForward 함수로 나타낸 것이 가독성이 좋았어요.

salmonco · 2024-11-01T18:48:27Z

src/models/Cars.js

+    }
+  }
+
+  attemptMoveAllCars() {


Car이랑 Cars를 분리할 생각은 못 했었는데, 분리하면 관리가 용이할 것 같아서 좋았습니다.

Car에서 차마 처리하지 못할 로직들이 생겨남에 따라 분리하게 됐어요! RacingGame와 Car에 들어가기에 어색한 로직들을 넣었어요!

salmonco · 2024-11-01T18:49:54Z

src/models/RacingGame.js

+    const highestScore = this.getHighestScore();
+    const winners = this.cars.getNamesByScore(highestScore);
+    const winnerNameString = winners.join(', ');
+
+    return winnerNameString;
+  }
+
+  getHighestScore() {
+    const scores = this.cars.getScores();
+    return scores.reduce((acc, cur) => Math.max(acc, cur), 0);


어떤 함수에는 return 앞에 개행이 있고 어떤 함수에는 return 앞에 개행이 없어서, 혹시 해성님만의 개행하는 기준이 있으신지 궁금합니다:)

아직까지는 개인적인 기준이긴 한데 return 문 전에 오는 줄들에 블록이 없고 한 두 줄 뿐이라면 붙여서 적는 것 같아요😅 이것에 관해서 다른 사람들의 코드도 보면서 깊게 생각해봐야겠어요!

salmonco · 2024-11-01T18:53:58Z

src/utils/GameUtils.js

+  getRandomSingleDigit: () => {
+    return Random.pickNumberInRange(0, 9);
+  },


getRandomSingleDigit를 유틸로 뺀 게 좋았어요~
저는 getRandomNumber라 네이밍했는데, getRandomSingleDigit이 더 이해하기 쉬웠어서 좋았습니다.

감사합니다! 딱 0~9까지라서 single digit이라는 키워드를 썼는데, 하지만 만약 기획이 달라질 수도 있다고 생각하면 조금 더 일반화해도 좋을 것 같다는 생각이 들기도 하더라구요😅 아니면 중간 함수를 만들어서 사용하거나!

salmonco · 2024-11-01T19:04:42Z

src/utils/GameUtils.js

+  getRandomSingleDigit: () => {
+    return Random.pickNumberInRange(0, 9);
+  },


메서드를 화살표 함수로 정의하신 이유가 궁금합니다:)

모던 자바스크립트 딥 다이브 책 보다가 메서드를 화살표 함수로 정의하는 것은 피해야 한다고 나와있어서요. 대신 메서드 축약 표현을 사용할 것을 권장하네요. 그 이유는 객체의 프로퍼티에 할당한 화살표 함수 내부의 this는 메서드를 호출한 객체를 가리키지 않고, 상위 스코프인 전역의 this가 가리키는 전역 객체를 가리키기 때문이라고 합니다.

// Bad const person = { name: 'Lee', sayHi: () => console.log(`Hi ${this.name}`) }; person.sayHi(); // Hi

// Good const person = { name: 'Lee', sayHi() { console.log(`Hi ${this.name}`); } }; person.sayHi(); // Hi Lee

제가 이해하기로는 객체에서의 메서드는 인스턴스를 생성할 목적을 가지고 정의하기 때문에 이런 권장사항이 나온 것 같았어요. 그래서 저는 아예 GameUtils를 메서드를 가진 객체로 정의하기보다, getRandomSingleDigit 등의 함수를 별도 도메인의 폴더에서 파일로 정의하고 import 해오는 방식은 어떨까 싶습니다:)

즉, 아래 세 가지 방법이 있을 수 있는데,

GameUtils를 객체로 정의해서 메서드에 getRandomSingleDigit를 넣어서 GameUtils.~~로 가져오는 방식

GameUtils를 클래스로 정의해서 안에 static 메서드로 가져오는 방식

getRandomSingleDigit 등의 함수를 별도 도메인의 폴더에서 파일로 정의하고 가져오는 방식

GameUtils의 경우 지역변수를 this로 공유할 메서드가 없고 인스턴스를 갖지 않는다는 점에서, new 키워드로 인스턴스 생성이 가능한 1,2번 방법은 목적에 맞지 않는다고 생각했어요. 그래서 전 3번 방법이 나아보이는데, 해성님은 어떻게 생각하시는지 궁금합니다!

모던 자바스크립트 딥 다이브 책 보다가 메서드를 화살표 함수로 정의하는 것은 피해야 한다고 나와있어서요. 대신 메서드 축약 표현을 사용할 것을 권장하네요. 그 이유는 객체의 프로퍼티에 할당한 화살표 함수 내부의 this는 메서드를 호출한 객체를 가리키지 않고, 상위 스코프인 전역의 this가 가리키는 전역 객체를 가리키기 때문이라고 합니다.

좋은 지식 감사합니다!😄 앞으로 참고해서 코딩할게요!

지금 제 방식은 1번이 되는 건가요? 객체 리터럴 방식과 클래스 방식 그리고 일반 함수 선언 방식 등 모듈을 어떻게 만드나 하는 고민은 항상 있었던 것 같아요😥 지금 3주 차가 끝나는 시점에서 저는 지역변수를 this로 공유할 메서드가 없고 인스턴스를 갖지 않는다는 이런 상황에 저는 일반함수로 선언하고 사용하려고 해요! 그렇게 단일 함수들을 각각 import해서 사용할 때도 있고, 만약 그 개수가 많아진다거나 아주 긴밀한 관계를 갖고 있다면 `import * as ~~Utils from './' 이런 식으로 네임 스페이스를 달아서 사용하기도 해요!
아직은 조금 더 고민이 필요할 것 같아요😅

junghaesung79 added 24 commits October 22, 2024 18:13

docs: README.md 작성

efea519

- 요구사항 - 구현 단계 - 알고리즘 흐름도 - 특수 상황 정리

chore: 개발 환경 의존성 추가

821d648

- eslint - prettier - config airbnb

feat: 기본 입출력 구현

c26baea

feat: RacingGame 클래스 부분 구현

0cbe34e

- Car 클래스 구현 - 입력을 변환하여 필드에 할당 - Car 인스턴스 생성 - 시도 구현

test: 전체 테스트 코드 추가

7ded72e

feat: 우승자 결과 구현

08f4ae8

feat: Validator 클래스 추가

a0ca7ac

test: 예외 테스트 추가

8bc0ede

feat: 에러 메시지 상수 분리

a7a6db6

refactor: Validator 클래스 스태틱화 및 키워드 변경

a037681

- try -> attempt

refactor: Cars 클래스 분리

62643b3

refactor: 클래스 import 방식 통일

13c3f7f

feat: 유틸 함수 모듈 분리

8947900

feat: Interface 클래스 추가 및 스크립트 상수화

8e3d8e4

refactor: UserInterface로 클래스명 변경 및 MissionUtils 의존성 캡슐화

61d12a5

refactor: 클래스 내 메소드 순서 번경

c2bee7e

refactor: 유틸 클래스 이름 변경

fce2258

- MyUtils => GameUtils

refactor: 파일 이름 변경

5bdcfa2

chore: 개발 의존성 삭제

031917b

refactor: Validator 규칙 분리

a8c2c7e

refactor: Validator 내부 사용 메소드 변경

35066af

test: 테스트 코드 중복 제거

cc98f8d

refactor: 폴더 구조 수정

591d4c0

refactor: 상수에 Object.freeze 적용

00bc76a

manNomi reviewed Oct 28, 2024

View reviewed changes

junghaesung79 mentioned this pull request Oct 28, 2024

자동차 경주 박세영 미션 제출합니다 #1

Open

6 tasks

MinSungJe reviewed Oct 28, 2024

View reviewed changes

j-nary reviewed Oct 29, 2024

View reviewed changes

wnsxk2 reviewed Oct 29, 2024

View reviewed changes

xxziiko reviewed Oct 29, 2024

View reviewed changes

jeheecheon reviewed Oct 29, 2024

View reviewed changes

mun-kyeong reviewed Oct 30, 2024

View reviewed changes

rubberchin reviewed Oct 31, 2024

View reviewed changes

salmonco reviewed Nov 1, 2024

View reviewed changes

		@@ -0,0 +1,31 @@
		import { GameUtils, UserInterface } from '../utils/index.js';

		class Car {

		@@ -0,0 +1,3 @@
		export { default as RacingGame } from './RacingGame.js';

		@@ -0,0 +1,41 @@
		import { ERROR_MESSAGES } from '../constants/errorMessages';

		class Validator {

[자동차 경주] 정해성 미션 제출합니다. #2

Are you sure you want to change the base?

[자동차 경주] 정해성 미션 제출합니다. #2

Conversation

junghaesung79 commented Oct 27, 2024

이렇게 구현했어요!

이 부분을 리뷰해주세요!

manNomi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MinSungJe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junghaesung79 Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junghaesung79 commented Oct 28, 2024

junghaesung79 commented Oct 28, 2024 • edited Loading

MinSungJe commented Oct 29, 2024

j-nary left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junghaesung79 Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junghaesung79 Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wnsxk2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxziiko left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxziiko Oct 29, 2024 • edited Loading

junghaesung79 Oct 28, 2024 •

edited

Loading

junghaesung79 commented Oct 28, 2024 •

edited

Loading

junghaesung79 Oct 29, 2024 •

edited

Loading

junghaesung79 Oct 29, 2024 •

edited

Loading

xxziiko left a comment •

edited

Loading

xxziiko Oct 29, 2024 •

edited

Loading