[yechoi] javascript-baseball-precourse

[yechoi42]

No description provided.

[hochan222]

class를 잘 활용하신점이 좋았습니다! commit을 조금 더 기능별로 주기를 자주 나누면 좋을것같습니다! 첫날인데 정말 고생 많이하셨습니다!!

[hochan222]

yarn-lock 과 package-lock이 동시에 존재하는데

npm 명령으로는 package-lock이
yarn 명령으로는 yarn-lock이 생성됩니다.

불필요한 중복을 피하기 위해 나중에 명령어 쓸때 두 패키지 관리자중 하나를 선택해서 쓰는것도 나쁘지 않을것같아요!

[hochan222]

Suggested change

	randomNumGenerator() {
	let first = Math.floor(Math.random() * 9 + 1);
	let second = Math.floor(Math.random() * 9 + 1);
	while (first == second) {
	second = Math.floor(Math.random() * 9 + 1);
	}
	let third = Math.floor(Math.random() * 9 + 1);
	while (third == first || third == second) {
	third = Math.floor(Math.random() * 9 + 1);
	}
	return (
	third.toString() + second.toString() + first.toString()
	);
	}
	randomNumGenerator() {
	let first = Math.floor(Math.random() * 9 + 1);
	let second = Math.floor(Math.random() * 9 + 1);
	while (first == second) {
	second = Math.floor(Math.random() * 9 + 1);
	}
	let third = Math.floor(Math.random() * 9 + 1);
	while (third == first || third == second) {
	third = Math.floor(Math.random() * 9 + 1);
	}
	return (
	third.toString() + second.toString() + first.toString()
	);
	}

randomNumGenerator()인데 반환값이 number가 아니고 string이여서 나중에 혼동의 소지가 있을것같아요!

Math.floor(Math.random() * 9 + 1)의 불필요한 중복이 많은것같아서 함수로 따로 뺴는것도 좋아보입니다.

[hochan222]

Suggested change

	while (first == second) {
	while (first === second) {

자바스크립트에서 == 는 값을 비교하고, ===는 타입을 기반으로 비교하기 때문에 엄격한 비교인 ===를 쓰는게 조금 더 좋아보여서 리뷰했습니다.

[hochan222]

Suggested change

	third.toString() + second.toString() + first.toString()
	third.toString() + second.toString() + first.toString()

아래와 같은 로직으로 짜면 조금 더 간단해질 수 있을것같은데 .toString()과 array의 비용은 따로 찾아봐야할것같습니다.

let arr = [];
arr.push(Math.floor(Math.random() * 9 + 1));
...

arr.join('');

[hochan222]

Suggested change

	else if (isNaN(userInputNumbers) == true) {
	else if (Math.isNaN(userInputNumbers) == true) {

isNaN() 함수는 어떤 값이 NaN인지 판별합니다. isNaN 함수는 몇몇 일반적이지 않은 규칙을 가지고 있으므로, ECMAScript 2015에서 추가한 Number.isNaN()으로 바꾸는 편이 좋을 수도 있습니다.

• https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/isNaN

[hochan222]

Suggested change

	computerNumArray.forEach(function(item, index) {
	computerNumArray.forEach(function(item, index) {

혹시 map도있는데 forEach을 쓴 이유가 있으신가요?

[yechoi42]

@hochan222 그것은 몰랐기 때문입니다...!! 하하..

[hochan222]

저도 잘은 모르지만 둘다 똑같은 역할을 하는데 forEach는 원본 array를 변화시키고 map은 새 배열을 반환합니다. 효율은 forEach가 빠른걸로 알고있습니다!

[hochan222]

Suggested change

	let result = document.getElementById('result');
	let $result = document.getElementById('result');

저같은 경우에 직접 DOM 관련해서 할당하는 변수는 $를 붙여줘서 구분해주었는데 저도 좋은지는 잘 모르겠어서 이 부분에 대해서는 어떻게 생각하시는지 궁금합니다.

[yechoi42]

@hochan222 아직은 환경변수 같이 보여서 저는 잘 읽히지가 않는데, 차차 다른 코드들을 참고하면서 생각을 정리해볼게요 !

[hochan222]

Suggested change

	if (resultText == '🎉 정답을 맞추셨습니다! 🎉') {
	console.log("haha")
	let newDiv = document.createElement('div');
	newDiv.innerHTML = '게임을 새로 시작하시겠습니까?';
	newDiv.setAttribute('id', 'newDiv');
	let newBtn = document.createElement('button');
	newBtn.innerHTML = '게임 재시작';
	newBtn.setAttribute('id', 'newBtn')
	newDiv.append(newBtn);
	document.body.appendChild(newDiv);
	newBtn.addEventListener('click', this.restartGame);
	}
	}
	if (resultText == '🎉 정답을 맞추셨습니다! 🎉') {
	console.log("haha")
	let newDiv = document.createElement('div');
	let newBtn = document.createElement('button');
	newDiv.setAttribute('id', 'newDiv');
	newBtn.setAttribute('id', 'newBtn')
	newBtn.innerHTML = '게임 재시작';
	newDiv.innerHTML = '게임을 새로 시작하시겠습니까?';
	newDiv.append(newBtn);
	document.body.appendChild(newDiv);
	newBtn.addEventListener('click', this.restartGame);
	}
	}

뭔가 이런것도 조금 더 짧아질 여지가 있는것 같아서 리뷰했습니다.

[jwon42]

다양한 함수를 사용한 것 같아서 좋고 문법적으로 배운 점(forEach 있어서 좋았음~!

[jwon42]

console.log 부분은 보안이슈가 있을 수도 있으니 완성 버전에는 삭제하는게 더 좋을 것 같음

[jwon42]

한 함수 내부에서 변수 선언과 함수 작동부를 구분하면 가독성에 더 좋을 것 같음~!

[jwon42]

덕분에 사용법을 배웠음~!