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

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

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

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

wants to merge 36 commits into from

Conversation

Bori-github
Copy link

@Bori-github Bori-github commented Aug 26, 2023

피드백 반영

  • 코드 리뷰 시 받은 피드백을 최대한 반영해 놓았습니다.
  • 각 피드백은 반영하거나 내용을 확인 했을 경우 Resolve conversation 처리 해두었습니다.
  • Resolve conversation 된 피드백을 다시 확인하고 싶으실 경우 show resolved를 클릭해주세요:)

추가 진행 사항

게임 다시 시작 시 컴퓨터 수 리셋

  • 게임 다시 시작 시 컴퓨터 수가 리셋되지 않아 계속 동일한 컴퓨터의 수로만 게임이 진행되었습니다.
  • 따라서 테스트를 전부 통과하지 못했습니다.
  • 기능을 추가하여 테스트를 전부 통과할 수 있도록 했습니다.
npm test

> javascript-baseball@1.0.0 test
> jest

 PASS  __tests__/StringTest.test.ts
 PASS  __tests__/ApplicationTest.test.ts

Test Suites: 2 passed, 2 total
Tests:       7 passed, 7 total
Snapshots:   0 total
Time:        1.196 s
Ran all test suites.

자바스크립트 테스트 코드를 타입스크립로 전환하기

  • 자바스크립트로 작성된 테스트 코드에 타입스크립트를 적용했습니다.
  • __tests__/StringTest.test.js의 문자열 테스트에서 사용된 toContain/toContainEqual에서 필요로 하는 인자는 1개인데 2개로 적용되어 있어 에러가 발생했습니다.
  • 각 인자를 1개로 변경하였고, arrayContaining를 적용하여 배열의 값을 확인할 수 있는 코드를 추가하였습니다.
  • jest 타입을 사용하기 위해 @jest/globals를 설치했습니다.

tsconfig.json 수정

  • tsconfig.json에서 불필요한 설정 및 주석을 제거했습니다.
  • 테스트 코드를 타입스크립트로 전환하면서 exclude의 __test__ 디렉토리를 제거하고, include를 모든 .ts 파일로 변경했습니다.

Effective TypeScript 사례 반영

  • docs/effective_typescript.md에 내용 작성해두었습니다.

- 전역 스코프를 각 모듈 내 지역스코프로 인식하기 위한 옵션 설정
- 게임이 시작되면 임의의 3자리 수를 생성하여 컴퓨터의 수로 셋팅
- 해당 수를 가져오는 getNumbers 생성
- 타입스크립트 설치로 인해 @types/jest 설치
- jest 실행을 위해 바벨 설치
- 유효성 검사 util 함수 생성
- 유효성 검사 관련 상수 생성
- 에러 메시지 상수 생성
- 사용자 입력값 유효성 검사 실행
- message에 포함되어 있던 관련 상수 이동
- Computer > compareNumbers : 입력한 수와 컴퓨터의 수를 비교하여 볼과 스트라이크 수 반환
- Computer > hintMessage : 볼과 스트라이크 수에 대해 힌트 메시지 생성
- View > printMessage : 메시지 출력
- 게임 다시 시작 및 종료 코드 유효성 검사
- Computer > getNumbers 삭제
@Bori-github Bori-github self-assigned this Aug 26, 2023
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.

리뷰를 많이많이 남겨드리고싶은데, 아쉽게도 제가 드릴건 감탄과 칭찬뿐이네요
이번주 타입스크립트 과제도 수고 많으셨습니다..! 😊

babel.config.js Show resolved Hide resolved
src/constants/game.ts Show resolved Hide resolved
src/model/Computer.ts Outdated Show resolved Hide resolved
src/controller/Controller.ts Outdated Show resolved Hide resolved
src/controller/Controller.ts Show resolved Hide resolved
Copy link
Member

@Ryan-Dia Ryan-Dia left a comment

Choose a reason for hiding this comment

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

코드 잘봤습니다. 한주간 고생하셨습니다.

추가적으로 클래스에서 # prefix 사용해서 외부의 불필요한 접근을 예방하면 더 좋은 것 같습니다.

src/controller/Controller.ts Outdated Show resolved Hide resolved
src/constants/game.ts Show resolved Hide resolved
package.json Show resolved Hide resolved
src/model/Computer.ts Outdated Show resolved Hide resolved
src/model/Computer.ts Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
src/model/Computer.ts Outdated Show resolved Hide resolved
src/controller/Controller.ts Outdated Show resolved Hide resolved
__tests__/ApplicationTest.test.js Outdated Show resolved Hide resolved
src/constants/message.ts Outdated Show resolved Hide resolved
src/constants/message.ts Outdated Show resolved Hide resolved
@Ryan-Dia
Copy link
Member

Ryan-Dia commented Aug 28, 2023

어제 질문하신 Function 타입을 왜사용하지 말야하는지에 대한 답변입니다.

일단 typescript-eslint를 사용하시면 아래의 규칙이 Function 타입을 사용하지 못하게 에러를 발생시킵니다.

ban-types

Function을 타입으로 사용하지 마십시오. Function 타입은 함수와 비슷한 모든 값들을 받아들입니다.
함수를 호출할 때 타입 안전성을 제공하지 않아 버그의 흔한 원인이 될 수 있습니다.
또한 클래스 선언과 같은 것들도 받아들이지만, 이들은 new와 함께 호출되지 않아 실행 중에 오류가 발생할 것입니다.
만약 함수가 특정 인자를 받아들이길 기대한다면, 명시적으로 함수의 형태를 정의해야 합니다.

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