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

숫자 야구 게임 과제 제출 #3

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

숫자 야구 게임 과제 제출 #3

wants to merge 16 commits into from

Conversation

Ryan-Dia
Copy link
Member

No description provided.

@Ryan-Dia Ryan-Dia self-assigned this Aug 26, 2023
Copy link

@Bori-github Bori-github left a comment

Choose a reason for hiding this comment

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

추가적으로

  • npm test 실행 시 테스트가 수행되지 않아요!
    바벨 설정이 필요한데, 링크 참고해서 설정해보세요!
  • any 타입에 최대한 적절한 타입을 적용하여 수정해보시면 좋을 거 같아요!
  • ESLint랑 Prettier 설정까지 하시고 husky도 추가로 적용하셨네요! 대단합니다!
    다만, 각 설정에 대한 커밋과 package.json의 커밋이 분리되어 있는데 분리하지 않고 적용된 이력을 함께 커밋하는 것이 안전해보입니다.
    혹시나 커밋을 수정해야하는 경우를 고려해서 커밋하면 좋을거 같아요.

수고하셨습니다!

@@ -0,0 +1,42 @@
import View from '../View';
import Model from '../Model/index';

Choose a reason for hiding this comment

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

해당 import 문에는 /index 경로가 포함되어 있습니다.
실수로 포함된 것이 아닌가 싶습니다:)

Copy link
Member Author

Choose a reason for hiding this comment

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

찾아주셔서 감사합니다.
이 부분은 import 스타일 통일 때문인가요?

Choose a reason for hiding this comment

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

통일에 대해 생각하기보다는 없어도 동작하기 때문에 불필요한 코드를 줄일 수 있는 부분이라고 생각했습니다:)

runGenerator(this.#run.bind(this));
}

*#run(): Generator<unknown> {

Choose a reason for hiding this comment

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

제너레이터를 사용한 이유가 궁금합니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. 코드를 흐름을 이해하는데 방해가 없습니다.
  2. jest에서 사용가능합니다.
    자세한 내용은 아래 글에 있습니다.
    https://github.com/orgs/woowacourse-precourse/discussions/1606

Choose a reason for hiding this comment

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

아 예전에 관련 내용이 우테코 슬랙에 올라왔던 것을 봤던 것 같네요.
답변 감사합니다!
제너레이터를 실제 적용해본 적이 없어서 어떤 이점을 가지고 적용하셨는지 궁금했습니다.
참고 링크 잘 읽어보겠습니다.
감사합니다:)

const userSelect = yield this.#view.readGameCommand;
if (userSelect === SELECT_FINISH) {
this.#view.finishGame();
return;

Choose a reason for hiding this comment

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

finishGame에서 Console.close를 실행하기 때문에 return이 불필요해 보입니다.
return이 반드시 필요한 상황이라면 왜 그런지 이유를 알 수 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

혹시 return을 빼고 코드를 실행시켜보셨나요?

Choose a reason for hiding this comment

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

제가 코드 이해도가 낮은 상태에서 질문을 드렸네요.
return 문이 없으면 문제가 있네요. 감사합니다:)

Copy link
Member Author

Choose a reason for hiding this comment

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

아뇨아뇨 저는 다른 방법을 알고계신줄 알았습니다.
저도 리팩터링 하는 과정에서 return을 제거하니 에러가 발생해서 다시 return을 넣었을 뿐 더이상 알아보지 않았는데 덕분에 알아보게 되었습니다.

현재 finishGame 메서드는 미션 라이브러리에서 가져온 close 를 사용하고 있습니다.
해당 라이브러리 내부를 보면 readline을 사용하고 있다는 것을 알 수 있습니다.
image

이는 node의 라이브러리 입니다.
공식문서를 잘 보면 rl.close()에 대한 설명이 아래와 같이 나옵니다.

Calling rl.close() does not immediately stop other events (including 'line') from being emitted by the InterfaceConstructor instance.
https://nodejs.org/api/readline.html#rlclose

const userNumbers = [...input];
userNumbers.forEach((number, index) => {
const isNumberInComputerNumbers = this.#computerNumbers.includes(Number(number));
const isStrike = isNumberInComputerNumbers && this.#computerNumbers.indexOf(Number(number)) === index;

Choose a reason for hiding this comment

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

this.#computerNumbers.indexOf(Number(number)) === index 에서 두 인덱스를 비교하는 것 보다
this.#computerNumbers[index] === Number(number)로 접근하면

  • indexOf 연산 없이 비교 할 수 있고
  • 코드가 간결해지면서 이해하기 쉬울 것 같습니다:)

추가로 isNumberInComputerNumbersisStrike에 포함하지 않아도 되지 않을까 생각합니다.
의미적으로는 포함되는 것이 맞다고 생각하지만 인덱스나 숫자가 동일한지 비교하기 때문에 isNumberInComputerNumbers 없이 비교해도 문제가 없을 것 같습니다:)

Copy link
Member Author

Choose a reason for hiding this comment

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

말씀해주신 코드(this.#computerNumbers[index] === Number(number)) 가 더 좋아보입니다.

또한 isNumberInComputerNumbersisStrike에 포함하지 않아도 됩니다.
따라서 ealry return 으로 변경하였습니다.
감사합니다.

compareUserWithComputerNumbers(input: string) {
    const userNumbers = [...input];
    userNumbers.forEach((number, index) => {
      const isNumberInComputerNumbers = this.#computerNumbers.includes(Number(number));
      if (!isNumberInComputerNumbers) {
        return;
      }

      const isStrike = this.#computerNumbers[index] === Number(number);
      if (isStrike) {
        this.#score.strike += 1;
      } else {
        this.#score.ball += 1;
      }
    });
    return this.#score;
  }

return this.#score;
}

isThreeStrikes() {

Choose a reason for hiding this comment

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

isThreeStrikes는 승리 조건이고, 내부에서도 동일한 변수명을 사용하고 있기 때문에 겹치지 않은 변수명을 적용하는 것이 좋을 것 같습니다.
개인적으로는 isWin과 같은 변수명을 추천합니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

일단 내부 변수명을 변경해야 할 것 같습니다. 저도 이 부분이 고민이 많았습니다. 처음에 원래는 isSuccess 였으나 제 개인적인 생각으로는 몇 달 뒤에 이 코드를 다시 보았을 때 '뭐가 더 이해가 빠를까?' 를 생각했을 때 전자였기 때문에 그렇게 작성했던 것 같습니다.

if (this.#model.isThreeStrikes()) {
        this.#view.printSuccess();
        break;
  }
if (this.#model.isSuccess()) {
        this.#view.printSuccess();
        break;
  }

Choose a reason for hiding this comment

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

의견 감사합니다. 이해하기는 정말 좋았어요!

}

const View = {
printStart() {

Choose a reason for hiding this comment

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

여기에 코멘트를 남기긴 하지만 전반적으로 함수의 중첩도가 높다고 생각이 듭니다.
해당 부분을 예시로 들자면 View.printStart는 내부의 OutputView.printStart가 있고, 그 내부에서 실제 동작 코드인 Console.print('숫자 야구 게임을 시작합니다.'); 가 있습니다.
따라서 어떤 로직의 실제 코드를 확인하기 위해 내부를 찾아 들어가는 일이 많다고 느꼈습니다.

개인적으로 추상화에 대해 고민을 하다가 함수의 중첩도가 높아지는 것을 경험 했는데요.
이 부분에 대해 팀원과 의견을 나눴는데 과도한 추상화는 되려 코드의 가독성이나 유지 보수성을 떨어뜨리기도 해서 조심해야한다고 하더라구요.

철원님이 최대한 쪼개서 코드를 작성하려고 노력하는 것 때문에 중첩도가 높아지는 경향도 있지 않나 조심스레 생각합니다. 중첩도는 낮추는 방향성도 같이 고려해보면 더 좋은 코드가 될 것 같습니다:)

Copy link
Member Author

Choose a reason for hiding this comment

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

저는 연습하는 단계라 생각해서 의도적으로 추상화는 많이하고 있기는 합니다.
하지만 이유없이 과도하게 하지는 않습니다. 또한 추상화를 높게하는게 좋다고 생각하는 것은 절대 아닙니다.

실전에서는 정말 코드 스타일이 다양합니다. 실제로 라이브러리 기여할 때나 특정 에러가 발생하였을 때 현재 사용하고 있는 코드가 구현된 파일을 직접 찾아서 보게 될 때가 있는데 어떤 라이브러리는 추상화가 낮게 되어있고 어떤 라이브러리는 높게 되어있어 있습니다. 그래서 어떤 경우에는 높은 추상화가 코드 이해 흐름을 방해할 때도 있지만 어떤 경우에는 이해를 돕는 경우가 있었습니다. 마찬가지로 추상화가 낮게되어 이해가 쉽게 될때도 있고 이해가 더 어려울 때도 있습니다. 즉 정답은 없다고 생각합니다. 해당 팀이나 그룹이 추구하는 스타일이 정답이라고 생각합니다. 따라서 특정 그룹에 참여했을 때 해당 코드 스타일을 받아들일 수 있는 유연성만 있으면 된다고 생각합니다.

이 코드에서 View를 저렇게 한 이유는 나중에 유지보수 할 때 편리하다고 생각했기 때문입니다.

Choose a reason for hiding this comment

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

정답이 있다고 생각한 것은 아닙니다. 그리고 철원님이 이유없이 코드를 작성했다고 생각하지도 않구요.
의도적으로 쪼개어 작성하는 코드를 꾸준히 하시는 것이 대단하다고 생각합니다:)
철원님의 의견이 궁금했고, 중첩도 관련해서도 고려해보시면 훨씬 더 좋은 코드를 작성하실 거 같아서 드렸던 의견입니다.
좋은 의견 감사합니다!

@@ -0,0 +1,26 @@
export const ERROR_MESSAGE = Object.freeze({

Choose a reason for hiding this comment

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

저는 타입스크립트를 사용하기 때문에 Object.freeze 대신 as const를 적용했는데요.
타입스크립트에서 어느 것이 더 적합할 지 또는 두 가지 모두 사용하는 것이 나은지 의견 나눠보면 좋을 것 같네요!

Copy link
Member Author

@Ryan-Dia Ryan-Dia Aug 27, 2023

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.

앗! 철원님 코멘트 완료 상태인지 확인 한 번 해주세요. 철원님 코멘트 안보여요ㅜㅜ

Copy link
Member Author

Choose a reason for hiding this comment

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

❗️ 여기에도 추가해놓겠습니다.

저는 타입스크립트를 사용하기 때문에 Object.freeze 대신 as const를 적용했는데요.
타입스크립트에서 어느 것이 더 적합할 지 또는 두 가지 모두 사용하는 것이 나은지 의견 나눠보면 좋을 것 같네요!

제 코드리뷰에 이렇게 글을 남기셔서 답변을 드립니다.
일단 저는 as const 를 생각지도 못 했습니다.
좋은 방법 알려주셔서 감사합니다.

1. as const

이렇게 하면 변수나 속성을 불변 타입으로 지정하게 되어 불필요한 변경을 방지합니다.
하지만 이는 타입 체크만 수행하면 런타임에서 실제로 객체를 불변하게 만들지는 않습니다.

2. object.freeze()

object.freeze()는 객체의 프로퍼티를 변경하지 못하도록 해주는데 이는 실제 런타임에서 객체의 불변성을 확보해줍니다.

결론

현재 상황에 국한시킨다면 as const로 하는 방법도 알았지만 여전히 object.freeze()를 사용할 것 같습니다.
그 이유는 object.freeze() 를 타입스크립트에서 사용하면 타입스크립트 자체적으로 object.freeze()에 대한 타입선언 파일이 존재합니다.

image

타입 정의를 보시면 object.freeze()의 return 값이 Readonly 입니다.
따라서 object.freeze()를 타입스크립트에서 사용하면 아래와 같이 타입이 추론됩니다.

image

그렇기 때문에 as const를 사용할 때 처럼 컴파일 타임에도 변경을 시도하면 에러표시를 해줍니다.

image

결과적으로 object.freeze()를 타입스크립트에서 사용하면 컴파일 타임과 런타임 모두 불변성을 보장해줍니다.



❗️ 중첩 객체가 존재할 때

ojbect.freeze()를 사용한다면 반드시 알아야할 주의사항이 있습니다.
바로 중첩 객체일 때 freeze가 되지 않는다는 점 입니다.
따라서 지금과 같은 상황이 아니라 중첩 객체를 사용할 때는 이야기가 달라집니다.

먼저 as const를 사용하면 타입 추론을 아래와 같이 합니다.
image

중첩 객체가 없을 때는 object.freeze()as const 가 컴파일 타임에는 동일하지만 중첩 객체가 있다면
object.freeze()를 한 번 더 내부 객체에 사용해주든지 아니면 as constobject.freeze()를 같이 사용해야합니다. 또는 immer같은 외부 라이브러리를 사용하는 것도 방법이 될 수 있습니다.

타입스크립트 5.0에서 Objcet.freeze()의 시그니처를 const 타입으로 바꾸어서
as const를 적용한 것 처럼 하려고 하였으나 Object.freeze 자체가 깊은 복사를 하지 않아서 잘 안된 것 같습니다.
microsoft/TypeScript#52317 (comment)

export const INPUT_MESSAGE = Object.freeze({
new_line: '\n',
game_number: '숫자를 입력해주세요 : ',
game_command: '게임을 새로 시작하려면 1, 종료하려면 2를 입력하세요 \n',

Choose a reason for hiding this comment

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

new_line을 상수로 사용하고 있어서 game_command에서는 \n를 제거해도 좋을 것 같습니다!

success: '3개의 숫자를 모두 맞히셨습니다! 게임 종료',
end: '잘못된 값을 입력하여 게임이 종료됩니다.',
error: (name: string, message: string, cause: string) => `${name} : ${message}\n[CAUSE] : ${cause}`,
noting: '낫싱',

Choose a reason for hiding this comment

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

nothing 오타 있네요. 제가 저번에 추천했던 오타 표시해주는 익스텐션 설치 하셨나요? 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

해당 부분은 익스텐션으로 해결되지 않습니다.
image

Choose a reason for hiding this comment

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

아 그냥 오타인 것을 표시만 해주는 거라서 해결을 해주진 않아요ㅜㅜ
그래도 좋은 도구라고 생각해서 혹시 사용하고 계신지 궁금했습니다!

Copy link
Member Author

@Ryan-Dia Ryan-Dia Aug 27, 2023

Choose a reason for hiding this comment

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

말씀하신 이후로 너무 잘 사용하고 있습니다.
아아 그 해결이 아니라 밑줄로 표시해주지 않는다는 뜻입니다.

이유를 찾아보니까 몇몇의 영어사전과 프로그래밍 언어 사전이 데이터에 있고 거기에 일치하는 단어이면 경고표시를 하지 않는 것 같습니다. streetsidesoftware/vscode-spell-checker#2593

문제되는 단어

noting

npx cspell trace --language-id=cpp nothing 을 입력해보니 해당 단어가 사전에 존재합니다. (별표 표시가 사전에 존재한다는 뜻입니다. 저 별표가 없을 때 Unknown word라고 표시를 해줍니다.)

npx cspell trace --language-id=cpp nothing
사전에 해당 단어 존재 -> 경고 표시 x

image

vallue

npx cspell trace --language-id=cpp vallue
사전에 해당 단어 존재 x -> 경고 표시

image

해결법

이와 비슷한 사례로 calendar가 올바른 스펠링이지만 조금 틀리게 했는데 일부만 경고표시를 하는 것을 볼 수 있습니다.

image

calender이 표기되지 않는 이유는 실제로 존재하는 단어이기 때문입니다.

결론적으로 나의 의도와 다르게 오타가 발생하였는데 실제 있는 단어이고 앞으로 그 단어를 쓸일이 없다면 (ex. noting, calender) spell flag words에 추가하는 방법 밖에는 없습니다.

image

Copy link
Member Author

Choose a reason for hiding this comment

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

추가적으로 npx cspell --no-progress src 하면 src폴더에 있는 모든 오타를 찾아줍니다.
image

import { ERROR_MESSAGE } from '../../constants/Messages';

const GameCommand = {
checkGameCommand(input: unknown) {

Choose a reason for hiding this comment

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

개인적으로 unknown을 적용한 것에 대해 고민해보았을 때, input에 unknown을 적용한다면 문자열이 아닌 모든 것을 입력받을 수 있는 상태인데 그렇다면 보안적으로 문제가 발생할 수 있지 않을까 라는 생각이 들었습니다.
입력값에 대한 특정 타입을 적용해서 해당 타입이 아닌 경우 에러를 발생하고 프로그램을 종료하는 것을 통해 보안성을 높일 수 있지 않을까 생각해보았는데, 이렇게 생각하는 것이 포커스에서 벗어난 생각인지 모르겠네요.
철원님 의견 궁금합니다:)

Copy link
Member Author

@Ryan-Dia Ryan-Dia Aug 26, 2023

Choose a reason for hiding this comment

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

이게 node여서 모든게 string으로 전환이 되어 굳이 unknown을 할 필요는 없습니다.
저는 만약 node가 아니라면 number, string, object 등 어떤 값이 올지 모른다고 가정해서 이렇게 작성하였습니다.

어떤 보안적인 문제가 발생할 수 있는지 구체적으로 알려주실 수 있나요?

Choose a reason for hiding this comment

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

아 저도 node가 아니라 input 창을 가정하며 생각해보았습니다.
악성 스크립트 공격(XSS)의 경우에 사용자 입력창에 스크립트를 적용해서 공격하기도 하기 때문에 unknown으로 문자열 이외의 값들을 입력받을 수 있다면 보안성이 떨어질 수 있지 않을까 생각해봤습니다.
실제로 unknown 타입이면 그럴 가능성이 높은지에 대해서는 잘 알지 못해서 이 생각이 포커스에서 벗어난 것일 수 있겠다 싶었구요.
어떤 값이 올 지 모르기 때문에 입력받을 타입을 정하는 것이 안정성이 높아지는 것이라고 생각하기도 했습니다:)
답변 감사합니다!

Copy link
Member

@SWARVY SWARVY left a comment

Choose a reason for hiding this comment

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

철원님도 한주 정말 수고 많으셨습니다! 이전에 하셨던 숫자야구보다 코드가 더 깔끔해지신 것 같아요!

@@ -0,0 +1,4 @@
#!/usr/bin/env sh
. "$(dirname -- "$0")/_/husky.sh"

Copy link
Member

Choose a reason for hiding this comment

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

오오.. husky라는 툴을 처음 봤는데 하나 배워갑니다..!

runGenerator(this.#run.bind(this));
}

*#run(): Generator<unknown> {
Copy link
Member

Choose a reason for hiding this comment

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

Generator라.. 모던 자바스크립트 딥다이브에서 있다는 것 정도만 알고 넘어갔던 개념이신데 여기에 적용하신거 보니 신기하네요..! 저도 Generator 문법에 대해서 조금 더 공부해봐야 할 것 같습니다 😊

}

#isDuplcation() {
if (typeof this.#inputNumbers === 'number') {
Copy link
Member

Choose a reason for hiding this comment

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

이미 number타입임을 체크하고 중복유무를 체크하는 흐름으로 진행되기때문에 해당 if문은 불필요하다 생각해요!

그리고 사소한 오타가 있네요!
#isDuplcation -> #isDuplication

Choose a reason for hiding this comment

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

isDuplicated 가 문법적으로 더 나아보입니다.


#isNotThreeDigitNumber() {
if (typeof this.#inputNumbers === 'number') {
return this.#inputNumbers.toString().length !== 3;
Copy link
Member

Choose a reason for hiding this comment

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

문제에서 3개로 고정되어있어 사실 그럴일은 없겠으나, 결국 3개인지 체크하는 이유는
숫자를 3개만 뽑기 때문이잖아요? 그렇다면 이미 선언해놓은 상수에서 불러오는게 더 적합하다고 생각해요!

이렇게 수정하면 이후에 숫자를 4개 5개 뽑더라도 따로 수정해 줄 필요가 없으니 말입니당

Suggested change
return this.#inputNumbers.toString().length !== 3;
return this.#inputNumbers.toString().length !== GENERATOR.pick_count;

Comment on lines +11 to +12
checkNumber(input: unknown) {
if (/\D/.test(input as any)) {
Copy link
Member

Choose a reason for hiding this comment

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

들어오는 input이 string으로 고정되어있다고 생각하기때문에, 해당 타입을 unknown에서 string으로 수정한다면
아래에 나오는 as any의 사용도 피할 수 있을 것 같아요.

** 위의 보리님과의 대화도 한번 읽어봤는데, 저 또한 타입을 string으로 고정하는 것이 더 안전하다고 생각해요!

Suggested change
checkNumber(input: unknown) {
if (/\D/.test(input as any)) {
checkNumber(input: string) {
if (/\D/.test(input)) {

@Ryan-Dia Ryan-Dia requested review from SWARVY and Bori-github and removed request for SWARVY and Bori-github August 27, 2023 14:32
error: '[ERROR]',
length_one: '1자릿수가 아닙니다.',
length_three: '3자릿수가 아닙니다.',
only_number: '숫자이외의 다른 문자가 존재합니다.',

Choose a reason for hiding this comment

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

사소하긴 한데요,, 맞춤법이 너무 거슬립니다 ^^,,
그리고 중복적으로 사용되는 숫자들은 선언 후 사용하시면 좋을 것 같아요.

Copy link
Member 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.

숫자 이외의 다른 문자가 존재합니다. 숫자v이외의 가 맞는 문법이라고 하네요.

@@ -0,0 +1,5 @@
import DefaultError from './DefaultError';

class ValidationError extends DefaultError {}

Choose a reason for hiding this comment

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

ValidationError 가 DefaultError를 상속받는 것 외에는 추가적인 기능이 없는데 이렇게 만드신 이유가 있나요?

Copy link
Member Author

@Ryan-Dia Ryan-Dia Sep 2, 2023

Choose a reason for hiding this comment

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

커스텀 에러로 나중에 로직이 복잡해졌을 때 한 눈에 파악할 수 있어서 유용하게 사용할 수 있습니다.
아직 제 코드에서는 적용하지는 않았지만 이전에 현호님과 했던 과제에는 적용되어있으니 한 번 구경하시는 거 추천드립니다.
https://github.com/Gamangjum-lihou/typescript-baseball-refactor/blob/ryan-dia/src/Controller/index.ts

}

#isNotNumber() {
return Number.isNaN(Number(this.#inputNumbers));

Choose a reason for hiding this comment

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

Number.isNaN()과 .isNaN()가 다르군요! 덕분에 알고갑니다~

Choose a reason for hiding this comment

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

맨 앞에 Number 가 추가된 이유가 있나요?
생각해봤는데,
Number('hi') => NaN 가 되니
isNaN(Number('hi')) => true 가 되는데 다른 예외 상황이 있는지 궁금합니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

아래 링크에 자세한 설명이 있습니다.
https://github.com/airbnb/javascript#standard-library--isnan


#haveZero() {
if (typeof this.#inputNumbers === 'number') {
return [...this.#inputNumbers.toString()].includes('0');

Choose a reason for hiding this comment

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

위에서 타입을 Number 로 변환하고, 다시 이 값을 체크하는게 아쉬운 것 같아요.

그리고 typeof this.#inputNumbers === 'number'가 반복되는데
#checkNumber에서 Number 로 변환되지 않으면 무조건 return false 를 하게되네요.
이러면 #checkInput 실행문의 순서가 중요해지는데, 종속성?이 너무 강한 코드가 되서 아쉬운 것 같아요.

Copy link

@chee9835 chee9835 Aug 28, 2023

Choose a reason for hiding this comment

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

아래에서도 this.#inputNumbers.toString() 이 문장이 반복되는데, 왜 #checkNumber 에서 숫자로 변환하신거에요?
그리고 #checkNumber 에서 숫자로 변환되서 012를 입력값으로 받으면,
에러메세지로 '0을 포함시켜면 안됩니다. '가 아닌 '3자릿수가 아닙니다.'가 출력됩니다.

Copy link
Member Author

@Ryan-Dia Ryan-Dia Sep 2, 2023

Choose a reason for hiding this comment

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

역시 고쳐야할 부분이 많죠? 문제인 부분 알려주셔서 감사합니다.

해당 코드의 일부는 만약 node상황이 아니라면 다른 값도 받지 않을까 싶어서 unknwon으로 타입을 지정했던게 상당히 귀찮게 만들어서 그냥 node 상황만 고려해서 string으로 교체했습니다. 이렇게 하면 코드가 더 간단해져서 해당 문제가 해결됩니다.


const GameCommand = {
checkGameCommand(input: unknown) {
this.checkNumber(input);
Copy link

@chee9835 chee9835 Aug 28, 2023

Choose a reason for hiding this comment

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

이 안에서 체크하시는 항목들이 꼭 필요한가라는 느낌이 들어요.
command가 숫자가 아닌 문자열키워드이면 ? 혹은 두자리 이상의 숫자를 받게되면? 이런 생각이 드는 코드에요.
그리고 허용되는 입력값이 1,2가 아닌 다른 값으로 바뀌게 되면 하드코딩 되어 있어서 수정이 어려워보여요.


*#run(): Generator<unknown> {
this.#model.setGame();
while (true) {

Choose a reason for hiding this comment

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

만약 에러 발생 시 에러메세지를 출력 후 다시 입력하게 한다면 Generator 안에서는 어떻게 구현할 수 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

저도 제너레이터로 구현을 안해봐서 잘 모르겠습니다. 만약 구현한다면 아래 코드가 에러가 발생하였을 때 에러메시지 출력 후 다시 입력을 받는데 아래 코드의 일부와 유사할 것 같습니다. 직접 구현해보시는 것을 추천드립니다.
https://github.com/Gamangjum-lihou/typescript-baseball-refactor/blob/ryan-dia/src/Controller/index.ts

es2021: true,
jest: true,
},
extends: ['eslint:recommended', 'prettier', 'hijacking-ts'],
Copy link

@chee9835 chee9835 Aug 28, 2023

Choose a reason for hiding this comment

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

철원님 작성된 import 문을 순서대로 정리해주던데, 이것도 eslint 설정인가요?

Copy link
Member Author

@Ryan-Dia Ryan-Dia Sep 2, 2023

Choose a reason for hiding this comment

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

네 hijacking-js의 rules 폴더를 확인하시면 됩니다.

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.

4 participants