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

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

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

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

wants to merge 49 commits into from

Conversation

chee9835
Copy link

No description provided.

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.

추가적으로

  • 개인적으로 희수님 커밋 이력이 보기 좋고 메시지도 간결해서 이해하기 쉬웠습니다. 배워갑니다:)
  • 테스트 케이스 추가한 것과 기능 내용 정리한 것 좋네요.
  • 타입스크립트 설치하지 않고, tsconfig 적용한 것으로도 동작하나요? 현호님도 이렇게 적용하신 것 같은데 잘 몰라서 질문으로 남겨요.

수고하셨습니다!

@@ -0,0 +1,33 @@
const ERROR = '[ERROR]';

const JUDGE = 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를 적용했는데요.
타입스크립트에서 어느 것이 더 적합할 지 또는 두 가지 모두 사용하는 것이 나은지 의견 나눠보면 좋을 것 같네요!
⇒ 철원님 코드 리뷰에 남겼던 내용과 동일합니다:)

}
}

resetComputer() {

Choose a reason for hiding this comment

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

다른 분들 코드 보다가 제 코드 다시 테스트 해봤는데, 게임을 다시 시작해도 컴퓨터의 수가 변경되지 않더라구요.
희수님 코드 보고 수정해보겠습니다:)

}

#handleUserInput(input: string) {
try {

Choose a reason for hiding this comment

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

try catch 문이 인상깊네요:)

@@ -0,0 +1,15 @@
{

Choose a reason for hiding this comment

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

tsconfig.json 설정 아직도 많이 어려운데, 제가 적용해보려고 했던 설정들이 포함되어 있네요.
스터디 시간에 짧게 질문하려고 합니다. 일단 질문 예고(?) 코멘트 남깁니다!

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
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.

한 주 정말 수고 많으셨습니다! 특히 MVP 패턴으로 푸셔서 해당 내용에 대해서도 학습할 수 있었던 것 같아요

오늘 스터디때 뵙겠습니다 정말 수고많으셨어요!!

@@ -1,2 +1,3 @@
dist
node_modules
.DS_Store
Copy link
Member

Choose a reason for hiding this comment

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

후후 실수시겠지만 EOL 처리를 빼먹으셨네요!

Copy link
Author

Choose a reason for hiding this comment

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

prettier 설정 하겠습니다 👍

Comment on lines 1 to 3
const NUMBER_LENGTH = 3;
const START = 1;
const END = 9;
Copy link
Member

Choose a reason for hiding this comment

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

해당 코드는 객체로 관리해주심이 더 좋을거같아요!!

Suggested change
const NUMBER_LENGTH = 3;
const START = 1;
const END = 9;
const INPUT_OPTION = {
length: 3,
start: 1,
end: 9,
}

@@ -0,0 +1,33 @@
const ERROR = '[ERROR]';

Copy link
Member

Choose a reason for hiding this comment

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

해당 내용들을 Message 파일로 하나에 퉁 치기에는 다른 성격의 상수들이 있는 것 같아요

가령 ERROR / JUDGE .. 이런것들은 출력에 대한 Message라고 볼 수 있겠지만
Command의 retry와 quit같은 내용들은 input 판별을 위한 상수라고 볼 수 있잖아요?

따라서 단순 Message로 이 다른 성격의 상수들을 하나로 묶기에는 어려움이 있는 것 같아요!
다른 파일로 분리하시거나, 이 모두를 아우르는 파일 이름으로 명명하시는게 좋을 것 같아요

import { Console } from '@woowacourse/mission-utils';
import { COMMAND } from '../constants/Message';

class BaseballGamePresenter {
Copy link
Member

Choose a reason for hiding this comment

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

MVC 디자인 패턴이 아닌 MVP 디자인 패턴을 채용하셨네요!

Copy link
Author

@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.

MVC, MVP, MVVM 이란? 이 글에 있는 mvc, mvp 사진을 봤는데요, p가 더 끌리더구요.

presenter에서 input이 올바르지 않으면 다시 받기도 하고,
view랑 model이 아예 안 이어지게 presenter가 중간다리 역할을 하는 것 같아서 mvp로 선택했습니다.

readUserNumber(this.#handleUserInput.bind(this));
}

#handleUserInput(input: string) {
Copy link
Member

Choose a reason for hiding this comment

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

저는 개인적으로 view단에서 인풋이 들어올 때 pure한 값을 넘겨줘야 한다고 생각하는데, presenter에서 try-catch를 하시더라구요! 이에 대한 희수님의 의견이 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

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

presenter 의 역할을 게임진행으로 봐서, 유저 답이 이상한지도 판단해야된다고 생각했습니다.
그리고 오류메세지 출력 후 재입력 받게 하려면 callback 함수 내에서 validation을 수행해야 된다고 생각해서! 그렇게 코드를 작성했는데요. 더 좋은 방법이 생기면 리팩토링해서 커밋하겠습니다. :)

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 사용해서 외부의 불필요한 접근을 예방한 점 좋았습니다.

Comment on lines +1 to +2
# 😄 기능 요구 사항

Copy link
Member

Choose a reason for hiding this comment

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

오 유일하게 하셨군요.
이 파일 덕분에 코드 이해하기가 쉬웠습니다.

Comment on lines 12 to 17
setComputer() {
while (this.#computer.length < NUMBER_LENGTH) {
const randomNum = Random.pickNumberInRange(START, END);
if (!this.#computer.includes(randomNum)) this.#computer.push(randomNum);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

예전에 저도 이렇게해서 피드백받았는데 그 내용 공유해드립니다.
피드백에서 수정해야할 점은 computer.length -> computer.size 입니다.
image

Comment on lines 25 to 26
let ball = 0,
strike = 0;
Copy link
Member

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#variables--one-const

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