[숫자 야구 게임] 강철원 미션 제출합니다. #4

Ryan-Dia · 2022-11-29T13:59:02Z

미션 제출합니다! ⛄️

에러내용이 깔끔하도록 수정 및 게임이 끝났을 때 안내문 추가

사용자가 서로다른 3자리 숫자를 입력하면 미리 만들어져있던 컴퓨터 숫자와 대조하여 올바른 힌트 출력

3스트라이크가 아닐시 다시 숫자 입력단계로 이동 단 컴퓨터 숫자는 이전과 동일

1을 입력하면 재시작 2를 입력하면 앱을 종료시킨다.:

y00eunji

에러처리가 인상적입니다!

y00eunji · 2022-11-30T08:04:16Z

src/Validator/GameCommand.js

+  #checkNumber() {
+    checkNumber(this.#input);
+  }


굳이굳이굳이찝자면 공통된 부분을 무조건 추리는 것보단 .,,하나만 utils로 빼기위해 utils를 만드는 것보다는 공통된 index에서 하거나 그냥 빼지 않고 각각 유효성 검증하는 코드를 넣어 작성하는 것이 가독성에 좋을 것 같습니다!

네 이 부분은 너무 억지로 조합을 사용한 것 같습니다.
해당 부분은 수정하였습니다 감사합니다!

y00eunji · 2022-11-30T08:05:46Z

src/Model/BaseballModel.js

+      const isStrike =
+        this.#computerNumbers.includes(Number(cur)) && this.#computerNumbers.indexOf(Number(cur)) === index;
+      const isBall = this.#computerNumbers.includes(Number(cur));


굳이굳이굳이찝자면 isStrike, isBall 또한 구현하는 부분이기 때문에 외부로 메서드로 분리하는 것도 함수역할을 덜 수 있을 것 같습니다!

아하 compose method 패턴적용 말씀이군요
그렇게 해도될 것 같습니다.
하지만 메서드로 만들면 함수에 인자가 두 개가 필요하고 isStrike는 두 번 필요하기에 두 번 모두 호출되어야합니다.
그래서 이렇게 설계했던 것 같습니다.

지금은 이렇게 리팩토링 되었습니다.

Suggested change

const isStrike =

this.#computerNumbers.includes(Number(cur)) && this.#computerNumbers.indexOf(Number(cur)) === index;

const isBall = this.#computerNumbers.includes(Number(cur));

compareUserWithComputerNumbers(input) {

this.#forEach(this.#spread(input));

return this.#score;

}

#spread(input) {

return [...input];

}

#forEach(input) {

input.forEach((cur, index) => {

const isStrike = this.#computerNumbers.indexOf(Number(cur)) === index;

const isBall = this.#computerNumbers.includes(Number(cur));

if (isStrike) this.#score.strike += 1;

if (!isStrike && isBall) this.#score.ball += 1;

});

}

y00eunji · 2022-11-30T08:06:19Z

src/Model/BaseballModel.js

+    this.#computerNumbers = RandomNumberGenerator.generateRandomNumber();
+  }
+  compareUserWithComputerNumbers(input) {
+    input.split('').forEach((cur, index) => {


str split vs spread를 비교해보니 spread연산자가 더 빠르다고 합니다!
str split vs spread

관련된 근거를 찾아보려고했지만 자료가 있지 않아서 추가적으로 발견하게 되면 공유해드리겠습니다!!

오! 감사합니다
계속 까먹네요.

y00eunji · 2022-11-30T09:06:12Z

src/Validator/GameNumber.js

+  #input;
+
+  checkGameNumbers(input) {
+    this.#input = input;


Validator에게 input이라는 상태가 필요한 이유가 있을까요?

프로퍼티 메서드를 사용하는 데에선 input을 인자로 사용하기 때문에 의미상 input으로 들어온 값을 검증한다는 느낌입니다.
하지만 input으로 멤버를 변경하면 Validator의 상태를 변경합니다. 그렇다고 유효성 검증 결과만 바뀌지 과정이 변경되는 것이 아닙니다.
이 관점에서 저라면 멤버로 사용하지 않고 인자로 넘겨주는 방식을 사용할 것 같은데 이렇게 멤버로 사용하시는 이유가 따로 있을까요?!

단순히 input이 반복적으로 여러 메서드의 인자로 들어가는 것을 막기위해 그렇게 했는데
아예 근원부터가 잘못된 것 같습니다.
굳이 필요없는 필드를 여러개를 사용해가면서 클래스로 만들이유가 있나 라는 생각을 해보았고 결굴 필요없다는 결론에 이르러 전부 함로 변경해주었고 input도 인자로 사용하도록 변경했습니다.
감사합니다.!

y00eunji · 2022-11-30T09:18:49Z

src/Controller/BaseballController.js

+  #handleError(error) {
+    if (error instanceof ValidationError) {
+      return this.#view.printError(new ReadError('Validation Error', error));
+    }
+    throw error;
+  }


혹시 입력에 관한 에러를 다른 에러로 오버라이딩 하시는건가요? 하나 배워갑니다~!

SWARVY

좋은 코드 공유 감사합니다 !! 많이배워가요 ㅎㅎ

SWARVY · 2022-11-29T23:48:52Z

.gitignore

 node_modules
+.template.txt


EOL 빼먹으셨네요..!

prettier 에 EOL 설정이 있으니 참고해보셔도 좋을거 같아요!!

@turtle601 EOL 설정은 이전부터 이미 되어있습니다.
.gitignore에는 적용이 안되는 것 같은데 이유를 아시나요 ?
.prettierignore 파일을 추가하여 ignore 설정 한 것은 없습니다.
운영체제가 다른 사람과 협업하는 게 아닌 저 혼자 mac os에서 하기에 lf로 설정해 두었습니다.
"endOfLine": "lf",

@Ryan-Dia
음... 사실 .gitignore에 대한 내용은 잘 모르겠습니다 ㅜㅜ

저 같은 경우에는 npm install -D prettier 로 하고 prettierrc 파일을 따로 설정,
대신 package.json과 prettierrc 파일을 upstream에 올리지 않는 방식을 사용하고 있습니다

(추가적으로 endOfLine: auto로 두고 작업하고 있습니다.)

이렇게 할 경우 다른 레포에 적용도 안되서 다른 분들고 협업에도 지장이 없을 거 같다는 제 추측입니다!!

제가 스터디 PR인줄도 모르고 코드 리뷰를 남겼네요 ㅜㅜㅜ 죄송합니다 🙏🙏

SWARVY · 2022-11-29T23:50:13Z

src/App.js

+  #view;
+
+  #model;
+
+  #baseballController;


MVC 디자인패턴에서는 컨트롤러만이 모델과 뷰에 모두 접근 가능한데, 컨트롤러만 app에서 선언해주고 나머지는 컨트롤러에서 선언해주는게 더 좋지 않았을까요?

SWARVY · 2022-11-29T23:50:40Z

src/Constants/Message.js

+const ERROR_MESSAGE = {
+  error: '[ERROR]',
+  length_one: '1자릿수가 아닙니다.',
+  length_three: '3자릿수가 아닙니다.',
+  only_number: '숫자이외의 다른 문자가 존재합니다.',
+  not_zero: '0을 포함시켜면 안됩니다. ',
+  not_duplication: '서로 다른 숫자를 입력하시옵소서',
+  one_or_two: '숫자 1 또는 2만 입력이 가능합니다.',
+};


불변성 유지해주셨으면 더 좋았을 것 같습니다! 물론 저도 깜빡했습니다 ㅎㅎ..

해당부분 스터디 전에 찾아서 몰래 바꾸어 놓았습니다.

앞으로는 잘 체크해야할 것 같습니다.

SWARVY · 2022-11-29T23:51:27Z

src/Controller/BaseballController.js

+const ValidationError = require('../Error/ValidationError');
+const Validator = require('../Validator');
+
+const RESTART = '1';


간단한 상수는 위에 따로 표기하는 방법 좋은거같아요!

D0Dam

철원님 코드에는 항상 새롭게 시도해보려는 코드가 들어가서 보면서 항상 많이 배우는 것 같습니다!
이번에도 코드 잘 봤습니다! 코멘트 남겨두었으니 참고해주세요!

D0Dam · 2022-11-30T00:08:55Z

src/App.js

+    this.#model = new BaseballModel();
+    this.#baseballController = new BaseballController(this.#view, this.#model);
+  }
+  play() {


한 칸 띄워주는 사소한 컨벤션~! 사소하지만 그래도 지켜줍시다~!

D0Dam · 2022-11-30T00:13:01Z

src/RandomNumberGenerator.js

+  generateRandomNumber() {
+    const computer = [];
+    while (computer.length < GENERATOR.pick_number_end) {
+      const number = Random.pickNumberInRange(GENERATOR.start_number, GENERATOR.end_number);
+      if (!computer.includes(number)) computer.push(number);
+    }
+    return computer;
+  },


다음과 같이 집합 Set을 사용하면 includes사용을 없애 시간 복잡도를 낮출 수 있고 if문을 사용하지 않아도 됩니다!

Suggested change

generateRandomNumber() {

const computer = [];

while (computer.length < GENERATOR.pick_number_end) {

const number = Random.pickNumberInRange(GENERATOR.start_number, GENERATOR.end_number);

if (!computer.includes(number)) computer.push(number);

}

return computer;

},

generateRandomNumber() {

const computer = new Set();

while (computer.length < GENERATOR.pick_number_end) {

const { start_number, end_number } = GENERATOR;

const number = Random.pickNumberInRange(start_number, end_number);

computer.add(number);

}

return computer;

},

const RandomNumberGenerator = { generateRandomNumber() { const computer = new Set(); while (computer.size < GENERATOR.pick_number_end) { const number = Random.pickNumberInRange(GENERATOR.start_number, GENERATOR.end_number); computer.add(number); } return computer; }, };

오 감사합니다. 이 경우에는 set이 훨씬 좋군요
해당사항은 반영하였습니다.
set은 고려해보지 않았는데 다음번에는 set도 고려해보겠습니다.

D0Dam · 2022-11-30T00:20:22Z

src/Validator/GameCommand.js

+  #checkNumber() {
+    checkNumber(this.#input);
+  }


이 checkNumber만 왜 외부에서 만든 함수를 가지고 오는 건가요?

상속대신 조합을 사용하였는데
지금 다시 생각해보니 굳이 하나의 함수를 가져오는 데는 필요없을 것 같아 각각 원래 코드로 바꾸었습니다.

D0Dam · 2022-11-30T00:23:05Z

src/Validator/index.js

+  #gameNumber;
+
+  #gameCommand;
+
+  constructor() {
+    this.#gameNumber = new GameNumber();
+    this.#gameCommand = new GameCommand();
+  }


Validator를 클래스화 해보려고 한 것은 좋은 것 같습니다. 하지만 이로인해 외부에서는 사용하지 않고 불필요하게 선언된 필드가 총 4개가 생성되었습니다. 저라면 클래스를 만들더라도 필드를 만들어주지는 않을 것 같습니다.

맞습니다. 다시 곰곰이 생각해보니
굳이 필요없는 필드를 만들면서 까지 클래스를 쓸 필요는 없을 것 같습니다.
따라서 전부 함수로 변경해주었습니다. 감사합니다.

D0Dam · 2022-11-30T00:25:26Z

src/View/OutputView.js

+
+  printGameEnd() {
+    Console.print(OUTPUT_MESSAGE.end);
+    Console.close();


이 Console.close();는 밑의 finishGame()으로 대체해도 좋을 것 같아요!

D0Dam · 2022-11-30T00:27:10Z

src/Model/BaseballModel.js

@@ -0,0 +1,35 @@
+const RandomNumberGenerator = require('../RandomNumberGenerator');


구조 분해 할당 하고 싶어지네요..ㅎㅎ
물론 이대로 사용하셔도 무방합니다..!

D0Dam · 2022-11-30T00:28:50Z

src/Model/BaseballModel.js

+    this.#computerNumbers = RandomNumberGenerator.generateRandomNumber();
+  }
+  compareUserWithComputerNumbers(input) {
+    console.log(this.#computerNumbers);


D0Dam · 2022-11-30T00:31:02Z

src/Controller/BaseballController.js

+  }
+
+  start() {
+    this.#view.pinrtStart();


소소한 오타이기는 하지만.. 수정하면 좋을 것 같아요~!

오호!!
바로 수정했습니다!
감사합니다!

피드백에 따라 클래스를 사용보다 함수로 처리하는 것이 더 맞다고 생각하여 변경

turtle601 · 2022-12-03T16:39:34Z

src/Validator/GameCommand.js

+const { ERROR_MESSAGE } = require('../Constants/Message');
+
+const GameCommand = {
+  checkGameCommand(input) {


메서드 체이닝을 통해서 코드를 더 이쁘게 만들어볼 수 있을 거 같아요!!

혹시 예를 들어 어떤식으로 하는지 알려주실 수 있나요 ?

turtle601 · 2022-12-03T16:41:07Z

src/Validator/GameCommand.js

+  },
+
+  checkNumber(input) {
+    if (/\D/.test(input)) {


REGEX 변수도 상수화하면 유지보수 측면에서 더 좋을거 같아요!!

가독성 향상을 위해 controller구조 변경 문제 조건과 다르게 다른 값을 입력하면 종료되는 것이 아니라 오류 출력후 다시 입력 할 수 있도록 변경

Leemainsw

늦어서 죄송합니다!
코드 잘 봤습니다 : )

Leemainsw · 2022-11-30T09:20:36Z

src/RandomNumberGenerator.js

+    const computer = [];
+    while (computer.length < GENERATOR.pick_number_end) {
+      const number = Random.pickNumberInRange(GENERATOR.start_number, GENERATOR.end_number);
+      if (!computer.includes(number)) computer.push(number);
+    }
+    return computer;


set을 사용하신다면 if (!computer.includes(number))를 확인하지 않아도 될 것 같아요!

Leemainsw · 2022-11-30T09:23:25Z

src/Validator/GameNumber.js

+  #checkNumber() {
+    checkNumber(this.#input);
+  }


GameCommand.js파일에서도 동일하게 checkNumber 함수를 사용하고 있는 것 같아요!
이를 부모 클래스에 선언한뒤 super를 사용하게 하는건 어떨까요?

Leemainsw · 2022-11-30T10:05:36Z

src/Model/BaseballModel.js

+      const isStrike =
+        this.#computerNumbers.includes(Number(cur)) && this.#computerNumbers.indexOf(Number(cur)) === index;


includes는 불필요한 것 같습니다

Suggested change

const isStrike =

this.#computerNumbers.includes(Number(cur)) && this.#computerNumbers.indexOf(Number(cur)) === index;

const isStrike = this.#computerNumbers.indexOf(Number(cur)) === index;

Ryan-Dia added 18 commits November 24, 2022 08:25

init

0afb654

feat: 사용자에게 서로다른 숫자를 입력받는 기능 추가

435457e

refactor(Error): 에러 처리 방식 변경

a732183

에러내용이 깔끔하도록 수정 및 게임이 끝났을 때 안내문 추가

feat : 올바른 숫자를 입력시 알맞은 힌트 제공하는 기능 추가

3d602b3

사용자가 서로다른 3자리 숫자를 입력하면 미리 만들어져있던 컴퓨터 숫자와 대조하여 올바른 힌트 출력

feat : 3스트라이크가 아닐시 다시 숫자입력 단계로 이동

c890b51

3스트라이크가 아닐시 다시 숫자 입력단계로 이동 단 컴퓨터 숫자는 이전과 동일

refacotr(README) : 기능 목록 수정

163f68f

feat : 3스트라이크를 했다면 게임 재시작 여부를 묻는다

39d3a88

feat : 재시작 또는 게임을 종료를 할 수 있는 기능 추가

665fe26

1을 입력하면 재시작 2를 입력하면 앱을 종료시킨다.:

refactor(BaseballController): 오류처리방식 return대신 throw로 변경

1549695

refactor(App): 앱 구동 세팅

6243011

docs(gitignore) :리스트에 .template.txt 추가

9632300

refactor(all) : 문자열 상수화

af584c1

refactor: 숫자 상수화

4882343

docs(README) 디렉토리 구조 추가

4a52e7a

refactor(Validator) input 값 확인 메서드 추가

32210b2

test(Validator): validator 테스트 케이스 추가

e8bba78

refactor(Constants): 객체에 Object.freeze 추가

d43c90e

refactor: 테스트하며 사용했던 console.log 제거

c1ea965

y00eunji reviewed Nov 30, 2022

View reviewed changes

SWARVY reviewed Nov 30, 2022

View reviewed changes

D0Dam reviewed Nov 30, 2022

View reviewed changes

refactor(Validator) : Validator class -> object로 변경

d9a7e8d

피드백에 따라 클래스를 사용보다 함수로 처리하는 것이 더 맞다고 생각하여 변경

turtle601 reviewed Dec 3, 2022

View reviewed changes

refactor(Controller): controller 구조 변경

9a116b3

가독성 향상을 위해 controller구조 변경 문제 조건과 다르게 다른 값을 입력하면 종료되는 것이 아니라 오류 출력후 다시 입력 할 수 있도록 변경

Leemainsw reviewed Dec 6, 2022

View reviewed changes

-      const isStrike =
-        this.#computerNumbers.includes(Number(cur)) && this.#computerNumbers.indexOf(Number(cur)) === index;
-      const isBall = this.#computerNumbers.includes(Number(cur));
+  compareUserWithComputerNumbers(input) {
+    this.#forEach(this.#spread(input));
+    return this.#score;
+  }
+  #spread(input) {
+    return [...input];
+  }
+  #forEach(input) {
+    input.forEach((cur, index) => {
+      const isStrike = this.#computerNumbers.indexOf(Number(cur)) === index;
+      const isBall = this.#computerNumbers.includes(Number(cur));
+      if (isStrike) this.#score.strike += 1;
+      if (!isStrike && isBall) this.#score.ball += 1;
+    });
+  }

		@@ -0,0 +1,35 @@
		const RandomNumberGenerator = require('../RandomNumberGenerator');

		const isStrike =
		this.#computerNumbers.includes(Number(cur)) && this.#computerNumbers.indexOf(Number(cur)) === index;

	const isStrike =
	this.#computerNumbers.includes(Number(cur)) && this.#computerNumbers.indexOf(Number(cur)) === index;
	const isStrike = this.#computerNumbers.indexOf(Number(cur)) === index;

[숫자 야구 게임] 강철원 미션 제출합니다. #4

Are you sure you want to change the base?

[숫자 야구 게임] 강철원 미션 제출합니다. #4

Conversation

Ryan-Dia commented Nov 29, 2022

y00eunji left a comment

Choose a reason for hiding this comment

y00eunji Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y00eunji Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

Ryan-Dia Dec 6, 2022 • 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

SWARVY 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

D0Dam 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

Ryan-Dia Dec 6, 2022 • 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

Ryan-Dia Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leemainsw 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

y00eunji Nov 30, 2022 •

edited

Loading

y00eunji Nov 30, 2022 •

edited

Loading

Ryan-Dia Dec 6, 2022 •

edited

Loading

Ryan-Dia Dec 6, 2022 •

edited

Loading

Ryan-Dia Dec 6, 2022 •

edited

Loading