[자동차 경주] 손민재 미션 제출합니다. #354

MinJaeSon · 2024-10-28T12:13:46Z

No description provided.

j-nary

로직이 깔끔하네요! 함수를 세세하게 분리하여 단일 책임 원칙을 준수하려고 하신 점이 돋보입니당
시간 되시면 제 PR 코드리뷰 도 부탁드릴게용 :)

j-nary · 2024-10-29T09:19:30Z

src/utils/getCarName.js

+  if (name.length > 5) {
+    throw new Error(ERROR.INVALID_CAR_NAME);
+  }
+  if (name.length === 0) {


name이 undefined일 경우엔 length 속성이 없을 것입니다!
프로퍼티를 사용하기 전에 type을 체크하여 안전성을 강화하는 것이 좋아보이는데 민재님 의견도 궁금하네용!

타입체크 ! 안그래도 진님 코드 리뷰 먼저 하고 왔는데 어느 부분인지 확실히 확인했네요ㅎㅎ 안정성을 높이기 위한 좋은 아이디어인 것 같아요. 좋은 피드백 감사합니다 :)

@j-nary
checkNameLength 메서드에 Input 되는 값이 undefined가 될 수 없을 것으로 보이는데(split() 사용으로 인함),
이런 경우에도 type을 체크하는 것이 안정성 측면에서 좋을까요?

불필요한 코드가 가독성을 해쳐 비효율적이라는 생각이 들어서 의견 여쭙습니다! 🤔

j-nary · 2024-10-29T09:21:42Z

src/utils/showRaceState.js

utils 폴더에 car 와 관련된 함수들이 많고, cars 라는 동일한 prop을 받는 것으로 보입니다
이럴 경우 저는 클래스로 프로그래밍을 하는 편인데 유틸함수로 프로그래밍하신 이유가 궁금하네요!

정말 좋은 질문입니다..!!
사실 바람직한건 아니지만 프로그래밍적 이유보다는 1주차 때 클래스형 프로그래밍을 했었는데, 다른 분들 코드를 보면서 어떤 면에서는 코드를 분리하기에 하나의 기능을 하는 함수를 만드는 것이 유리하다고 느꼈어서 조금 작은 단위의 기능들은 유틸함수로 작성해보았어요. 제 나름대로 기능을 구현할 때에 기능 함수로 짤지, 유틸 함수로 분류할지, 혹은 클래스로 만들지 고민을 했다고 생각했는데, 오히려 작은 기능 간에 연관성이 높은 경우 말씀하신 것처럼 클래스로 짜는 것도 너무 좋은 아이디어같습니다.!!! 👍🏻
덕분에 이렇게 또 생각해볼 기회를 가지게 돼서 감사해요 :)

j-nary · 2024-10-29T09:22:15Z

src/utils/showRaceState.js

@@ -0,0 +1,15 @@
+import { Console } from '@woowacourse/mission-utils';
+
+const RACE_PROGRESS_BAR_SYMBOL = '-';


이 아이도 상수폴더에 위치시키는 건 어떨까요?

확장성을 고려해봐도 해당 파일 외에 쓰이는 곳이 없을 것 같아 따로 빼지 않고 해당 파일 내부에 위치시켰습니다..! 혹은 이 경우처럼 프로젝트 전체에서 단독으로 쓰이는 상수가 많다면 의견 주신대로 상수폴더에 모아놓는 것도 좋은 의견 같아요 !!

j-nary · 2024-10-29T09:26:52Z

src/calculateWinner.js

+  const maxIndex = car.reduce((acc, _, index) => {
+    if (maxMovement === carMovements[index]) {
+      acc.push(index);
+    }


reduce 대신 filter와 map 을 사용해보는게 어떨까요?

const maxIndex = carMovements .map((_, index) => index) .filter((index) => carMovements[index] === maxMovement);

배열의 크기가 매우 클 경우, 성능면에서는 reduce가 더 좋지만 해당 과제의 경우엔 둘 간의 큰 성능 차이가 없습니다.
이렇게 성능 최적화가 필요한 경우가 아니라면, 가독성이 높은 filter + map 방식을 권장드리고 싶네요!

오 좋은 방식 같아요 예시 코드까지 주시니 더욱 이해가 잘 되네요! 비슷한 경우가 생긴다면 꼭 피드백 주신대로 반영해보겠습니다ㅎㅎ 팁 감사해요 :)

jin123457

학습 목표인 큰 함수를 작은 함수로 분리하기가 잘 적용된 코드인 것 같아 코드를 보며 많이 배우고 갑니다~
2주차 수고 많으셨습니다!

jin123457 · 2024-10-29T09:32:34Z

.prettierrc

@@ -0,0 +1,10 @@
+{


.gitignore 파일에 추가하셔서 커밋 안하셔도 될 것 같습니다!

팀 프로젝트가 아니다보니 굳이 원격에 올릴 필요성까진 없을지도 모르겠네요..! 의견 감사해요~

jin123457 · 2024-10-29T09:40:06Z

src/utils/getRaceNumber.js

+const checkNumberError = (num) => {
+  const isInvalidNumber = !Number.isInteger(num) || num <= 0;
+
+  switch (true) {


if문으로 작성하시면 조금 더 간결한 코드가 작성 될 수 있을 것 같은데 switch문을 쓰신 이유가 있으실까요??
궁금증에 여쭤봅니다..!🤔

지난 미션 때 제가 if-else문을 너무 자주 사용하기도 했고, 빈도도 빈도지만 nested하게 사용한 부분들이 있어서 관련되어 피드백을 받았었어요..😅 switch문을 쓴건 다른 분 리뷰를 통해서 배운건데 결론부터 이유를 말씀드리자면 가독성이 좀 더 좋게 느껴져서 입니다! 심지어 그 분은 if문 사용 시 else까지 적는 것도 지양한다고 하시더라구요. 가독성에 좋은 방법 같아서 if문 하나로 체크 가능한 부분들은 if문을 사용하였고 이번에는 else문 사용도 최대한 자제해보았습니다! 좋은 질문 감사드려요 :)

jin123457 · 2024-10-29T09:52:54Z

package.json

프로젝트 안에 airbnb 스타일 가이드를 적용하는 것보다 상위 폴더에 적용해서 모든 주차를 한 번에 적용하는 방법이 있습니다..!
커뮤니티에서 재이님이 공유해주신 내용 첨부해드립니다!
상위 폴더에 eslint 적용방법

오 링크까지 첨부해주시다니..! 그러네요ㅋㅋ 반영해서 3, 4주차 때는 또 설정 안해도되겠어요!! 감사합니다 :)

H-sooyeon

수고하셨습니다!

H-sooyeon · 2024-10-30T06:10:52Z

src/calculateWinner.js

+  }, []);
+
+  const winner = maxIndex.map((index) => car[index]).join(', ');
+  Console.print(`최종 우승자 : ${winner}`);


calculateWinner 라는 함수명은 우승자를 찾는 역할 맡고 있다고 생각이 들어요.
하지만 함수 안에서 해당 우승자를 출력하는 역할까지 하고 있으니, 두 역할을 분리하는건 어떨까요?

의견 감사합니다! 출력만 하는 함수는 한 줄밖에 안되어 굳이 따로 분리하지 않았는데, 혹시 H-sooyeon님께서는 이런 경우에 분리를 하는게 좋다고 생각하시나요..?! 의견을 듣고 싶네요!

입력에 대해 input.js로 분리하셨으니 출력에 대해서도 다른 파일로 분리하는게 코드 일관성에 더 좋을 것 같다고 생각해요!
그래서 calculateWinner.js은 우승자를 리턴하고, App.js에서 우승자를 출력 파일로 전달하는 것도 좋을 것 같아요 😄

H-sooyeon · 2024-10-30T06:17:50Z

src/utils/moveCarEachTurn.js

@@ -0,0 +1,11 @@
+import shouldCarMove from './shouldCarMove.js';
+
+const moveCarEachTurn = async (cars, carMovements) => {


함수 안에 비동기로 작동할만한 코드가 보이지 않는데, await 없이 async만 추가하신 이유가 있으신가요?

좋은 지적 감사합니다. 해당 함수 호출 시에 await를 썼는데, 여기선 Promise를 반환하지 않고 있으니 async, await 키워드를 사용할 필요가 없겠네요..! 제가 실수한 부분입니다😓

H-sooyeon · 2024-10-30T06:29:02Z

src/utils/showRaceState.js

+    return `${car} : ${progressBar}`;
+  });
+
+  Console.print(raceState.join('\n') + '\n');


6.3

+ 연산자로 문자열을 연결하기 보단, template string을 사용하시는걸 추천드려요!

Console.print(`${raceState.join('\n')}\n`);

핫 넵 저것도 제가 놓쳤네요 알려주셔서 감사합니다!

chlwlstlf

안녕하세요! 민재님 코드 잘 봤습니다🤗
우테코 6기 코레아 팀의 텐텐이라고 합니다. 우선 저희 서비스 이용해주셔서 감사합니다!!!

코드 깔끔하게 작성해주셔서 리뷰드릴 게 많이 없었습니다! 다른 분들이 JS 문법이나 네이밍 등으로 리뷰를 많이 남겨주셔서 그 외에 생각해볼만 한 것들에 대해 코멘트 달아 놓았습니다. 확인해주세요ㅎㅎㅎ

추가적으로 carMovements 배열을 cars의 길이에 맞게 또 선언을 하여 따로 반복문을 돌며 계산을 해주는 것과 cars가 Car라는 클래스 배열이고 Car의 필드로 carMovements를 가지는 것의 차이점과 장단점을 공부해보시면 좋을 것 같습니다~~

3주차 미션 한창 하실 것 같은데 화이팅입니다!!
다시 한 번 감사드려요🤩

chlwlstlf · 2024-10-31T06:10:21Z

__tests__/ApplicationTest.js

+    { name: "경주 횟수가 음수일 시 테스트", inputs: ["pobi,woni", "-1"] },
+    { name: "경주 횟수가 0일 시 테스트", inputs: ["pobi,woni", "0"] },


만약 횟수를 먼저 입력하고, 자동차 이름을 입력 받는 것으로 로직이 바뀌면 어떻게 될까요~?

테스트 코드는 하나의 프로덕션 코드에 대해 작성하면 좋습니다! 경주 횟수 에러 테스트는 자동차 이름 에러 테스트에 관심이 없기 때문이에요ㅎㅎ

또 에러 케이스에 대해 테스트를 잘 해주셨는데 경곗값들 정상 케이스에 대해서도 해주면 좋습니다😊

생각해보니 그렇네요..! 테스트 코드에서도 서로 영향을 주는 걸 최소화시키는 방향으로 생각해봐야겠어요! 덕분에 배워갑니다 :)

chlwlstlf · 2024-10-31T06:11:04Z

src/constants/errorMsg.js

+const ERROR_PREFIX = '[ERROR]';
+


반복되는 코드를 따로 상수로 빼준 것 아주 좋습니다💯💯

chlwlstlf · 2024-10-31T06:46:46Z

src/utils/shouldCarMove.js

+const shouldCarMove = () => {
+  const randomNum = Random.pickNumberInRange(0, 9);
+
+  return randomNum >= 4;
+};


utils 파일에는 유효성 검사, 파싱 등 여러 곳에서 재사용이 가능한 함수들이 위치합니다.

shouldCarMove에서 랜덤 숫자 생성 부분만 추출한 후 인자로 MIN 값과 MAX 값을 받으면 범위 내의 랜덤 숫자 생성 함수라는 유틸 함수가 탄생하겠죠~?

이렇게 추출된 결괏값을 사용하는 곳은 주요 기능을 담당하는 클래스나 함수가 될 것입니다!

프로젝트를 할 때도 이 함수를 utils로 분리해야 하나? 라는 고민을 마주할 때가 있는데요, 주요 기능이 포함되는 지 여부에 따라 결정하면 좋을 것 같습니다! 비슷하게 showRaceState도 util 함수인지 메소드로 포함해야 하는 지 생각해봐도 좋을 것 같아요!

(물론 지금은 구현 크기가 작아서 이렇게 분리해도 괜찮지만 미리 연습해도 좋을 것 같아서 남겨드렸습니다!)

와 안그래도 작은 기능의 함수를 생성할 때마다 유틸함수로 분리할지 그냥 기능을 담당하는 함수로 할지 계속 고민이 많았는데, 이렇게 설명해주시니 명확하게 이해가 되네요!! 정말 감사합니다😊 이제부터는 이 점 생각하면서 해봐야겠어요ㅎㅎ

MinJaeSon · 2024-10-31T11:05:30Z

추가적으로 carMovements 배열을 cars의 길이에 맞게 또 선언을 하여 따로 반복문을 돌며 계산을 해주는 것과 cars가 Car라는 클래스 배열이고 Car의 필드로 carMovements를 가지는 것의 차이점과 장단점을 공부해보시면 좋을 것 같습니다~~

이렇게 찾아와서 리뷰도 달아주시고 제가 더 감사합니다..ㅎㅎ 부족한 점이 많은데 칭찬도 감사해요
말씀하신 부분에 대해서 생각해보았는데, 객체지향적으로 설계되어 확장성 면에서 훨씬 유리할 것 같다는 생각이 들었습니다! 좋은 아이디어 감사드립니다👍🏻👍🏻

seungineer · 2024-10-31T14:22:19Z

README.md

+
+    - 어떤 자동차라도 이름이 5자를 초과하는 경우
+
+    > 자동차 이름에 특수문자 또는 숫자 또는 공백이 들어가는 경우도 허용하는 것으로 정한다. (이때, 공백이 쉼표 옆에 위치하는 경우는 자동차 이름에 포함시키지 않는다.)


사용자가 오입력 하는 경우는 따로 생각하신 거죠?
보통 홍길동, 길동홍, ... 이런 식으로 많이 작성하니까요!

좋네요 👍👍

seungineer · 2024-10-31T14:23:33Z

README.md

README.md에 구현 완료된 기능의 경우 체크표시를 하여 문서를 관리하면
README.md를 더 생생하게(?) 관리할 수 있을 것 같아요!

seungineer · 2024-10-31T14:28:12Z

src/utils/getCarName.js

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


사용자 안내 메시지의 경우도 constants로 분리 관리하면
가독성 측면에서 더 좋을 것 같습니다!
에러 메시지 처럼요 🙂

seungineer

코멘트의 핑퐁을 굉장히 잘하시는 것 같아요 🙂
1주 차의 피드백도 최대한 반영하려 한 노력도 느껴지구요!

고생 많으셨습니당. 3주 차 미션도 파이팅입니다!!

MinJaeSon added 22 commits October 25, 2024 16:23

docs(README): 구현할 기능 목록 작성

a658da1

feat: 자동차 이름 입력 처리

397bbd9

feat: 경주 횟수 입력 처리

e72cf88

feat: 에러 메시지 상수화

a2e7ce5

chore: 에러 메시지 접두사 수정

9e5a7dd

feat: 자동차의 전진 유무 판별

5de845c

feat: 경주 1회당 자동차 전진 시 이동 처리

3fedc86

feat: 경주 현황 출력 함수 정의

92ed771

feat: 전체 경주 과정 처리 및 출력

4f33191

refactor: 자동차 이름 입력, 경주 횟수 입력 기능 하나의 함수로 묶어 생성

9ed3680

feat: 우승자 계산 후 출력

4a981bc

fix: 자동차 이름 공백 입력 시의 에러 핸들링

c519cc9

fix: 경주 횟수 미입력 시의 에러 핸들링

a311319

docs(README): 에러 케이스 추가

bfd062f

refactor(shouldCarMove): 리턴문 간략화

6bf6d88

refactor(getRaceNumber): 에러 핸들 로직 한곳에 통합

d89f656

test: 테스트 케이스 추가

1f41967

refactor(test): 테스트 코드 파라미터화

39ee389

chore(package): eslint, prettier 종속성 추가

0603cd1

chore(config): eslint, prettier 설정

f8a3189

style: airbnb 코드 스타일 적용

e3bcde9

style: return문 윗 줄 공백 줄로 통일

f030d67

j-nary reviewed Oct 29, 2024

View reviewed changes

jin123457 reviewed Oct 29, 2024

View reviewed changes

H-sooyeon reviewed Oct 30, 2024

View reviewed changes

chlwlstlf reviewed Oct 31, 2024

View reviewed changes

seungineer reviewed Oct 31, 2024

View reviewed changes

		@@ -0,0 +1,15 @@
		import { Console } from '@woowacourse/mission-utils';

		const RACE_PROGRESS_BAR_SYMBOL = '-';

		@@ -0,0 +1,11 @@
		import shouldCarMove from './shouldCarMove.js';

		const moveCarEachTurn = async (cars, carMovements) => {

		{ name: "경주 횟수가 음수일 시 테스트", inputs: ["pobi,woni", "-1"] },
		{ name: "경주 횟수가 0일 시 테스트", inputs: ["pobi,woni", "0"] },


		- 어떤 자동차라도 이름이 5자를 초과하는 경우

		> 자동차 이름에 특수문자 또는 숫자 또는 공백이 들어가는 경우도 허용하는 것으로 정한다. (이때, 공백이 쉼표 옆에 위치하는 경우는 자동차 이름에 포함시키지 않는다.)

[자동차 경주] 손민재 미션 제출합니다. #354

Are you sure you want to change the base?

[자동차 경주] 손민재 미션 제출합니다. #354

Conversation

MinJaeSon commented Oct 28, 2024

j-nary left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MinJaeSon 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

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

jin123457 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

H-sooyeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MinJaeSon Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MinJaeSon Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MinJaeSon Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

chlwlstlf 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MinJaeSon commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seungineer left a comment

Choose a reason for hiding this comment

MinJaeSon Oct 29, 2024 •

edited

Loading

MinJaeSon Oct 31, 2024 •

edited

Loading

MinJaeSon Oct 31, 2024 •

edited

Loading

MinJaeSon Oct 31, 2024 •

edited

Loading

chlwlstlf left a comment •

edited

Loading