[1주차] 숫자 야구 게임 기능 구현 미션 제출입니다.#3
Conversation
Feature/endgame and restart
- 버튼 활성화 부분이 중복되어 함수화로 구현 - 변수명을 명확하게 하기 위해 results를 strikeAndBallCounts로 수정 - 불필요한 else 삭제 - 문자열 연결 대신 템플릿 리터럴 사용
refactor: 코드 컨벤션에 맞게 일부 코드 수정
feature: cypress test 통과
Songhyejeong
left a comment
There was a problem hiding this comment.
안녕하세요 준수님 :)
PR을 일요일에 해주셨는데 제가 늦게 올려서 이제야 리뷰 남기는 점 죄송합니당..
리뷰 남겨보는 건 처음이라서 맞게 한 건지 잘 모르겠네요..
우선 전반적인 코드에 대한 리뷰를 남겨보자면 코드가 전반적으로 너무 깔끔했어요!
그리고 함수의 역할이 명확하게 나눠져서 좋아보여요,,!
제가 구현한 함수와 다르게 대부분의 함수가 정말 하나의 역할만 하니 간결하고 가독성 측면에서 훨씬 좋은거 같아요..!
자바스크립트 처음이라고 하셨는데 준수님 코드 보면서 오히려 많이 배우고 갑니당..
그리고 질문 하나 남겨주셨잖아요.
저도 브라우저 렌더링 과정에 대해서 자세히는 알고 있지 않았어서 이번 기회에 좀 찾아보고 이해본거 토대로 정리해봤어요..!
우선 알아야할 세 가지 특성이에요.
- alert()는 HTML이나 DOM과 직접적인 상호작용을 하지 않는다고 해요.
- alert()가 실행 되면 JS 싱글 스레드 특성 때문에 브라우저 동작을 중지 시킨다고 합니다.
- insertAdjacentHTML()는 HTML을 초기화 시킨다고 합니다.
그래서 이것들 바탕으로 정리해보자면
-
일반적인 렌더링 순서 : HTML 실행 → DOM Tree로 변환→ js 코드 실행 → 렌더링 →레이아웃 그리기
- 정답 값 입력 했을 때 : HTML 실행 → DOM Tree로 변환→ js 코드 실행 이 때 printResult() 함수 호출 → insertAdjacentHTML()로 HTML 초기화 → DOM 변경사항 반영 → js 실행 이 때 checkResult() 함수 호출, alert() 함수 호출 → alert() 창 띄움 → 브라우저 모든 동작 잠시 멈춤 → 렌더링 → 레이아웃 그리기 이 순서 인거 같습니다,,
- alert() 실행 안될 때 : HTML 실행 → DOM Tree로 변환 → js 코드 실행 이 때 printResult() 함수 호출 → insertAdjacentHTML()로 HTML 초기화 → DOM 변경사항 반영 → js 실행 → 렌더링 → 레이아웃 그리기
결론적으로 해결하려면 setTimeout이라는 비동기 함수가 있는데 이 함수를 사용하면 저 순서에 영향을 받지 않고 시간이 지나면 출력될 거 같아요!
setTimeout(function () { alert(result + ' 축하드립니다!'); }, 500);
제가 이해한건 이정도인데 도움이 되셨으면 좋겠네요..
감사합니다:)
- strikeBallCounts 비구조화 할당 적용 및 낫싱 백틱 - 큰따옴표 수정 - 증감연산자 ++ -> += 1 적용
- makeComputerNumbers() 함수의 numbers 변수명을 computerNumbers 로 변경 - while() 반복문의 조건을 validateComputerDifferentNumber()로 변경
Refactor/reflect review
|
우선, 다시 한번 코드 리뷰 감사드립니다! |
wzrabbit
left a comment
There was a problem hiding this comment.
@gogo1414 👋🏻
안녕하세요, 준수님! 저는 그리디의 외부 리뷰어를 맡은 김의천이라고 합니다. 만나뵙게 되어 반가워요 😊
JavaScript는 처음이신 것으로 기억하는데 기능 구현에 코딩 컨벤션에 코드 퀄리티까지 챙겨줘야하는 미션이다 보니 많이 힘드셨을 것 같습니다. 그런데도 기능 분리에 힘써주시고 명확한 함수명과 변수명을 사용하시려고 했던 것이 느껴졌습니다. 솔직히 코드 보고 처음 하는 분의 코드는 아닌 것 같다는 느낌이 들었어요.
테스트의 경우에도 상호 리뷰 이후 요구사항 비교하시면서 제대로 고쳐주신 것 확인했습니다, 수고하셨어요!
준수님의 성장을 위해 코멘트를 몇 개 적어보았으며, 적어주신 질문의 경우에는 답변하는 데 시간이 오래 걸릴 것 같으므로 일단은 나중에 답변드리도록 하겠습니다. 아마 2주차의 미션 수행을 다들 시작하신 것 같은데 오랫동안 발목을 잡을 수는 없으니까요.
그럼, 준수님의 의견 공유를 기다리겠습니다. ⌚
| // 컴퓨터의 서로 다른 3자리 숫자를 서로 다른지 검증 | ||
| validateComputerDifferentNumber(computerNumbers) { | ||
| return new Set(computerNumbers).size === 3; | ||
| } | ||
|
|
||
| // 3개의 숫자가 서로 다른 수인지 검증 | ||
| validateDifferentNumber(userNumbers) { | ||
| if (new Set(userNumbers).size !== 3) { | ||
| alert("잘못된 입력입니다. 중복되지 않는 서로 다른 3개의 숫자를 입력하세요."); | ||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| // 사용자 입력 숫자 길이 검증 | ||
| validateLength(userNumbers) { | ||
| if (userNumbers.length !== 3) { | ||
| alert('잘못된 입력입니다. 3자리 숫자를 입력하세요.'); | ||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| // 사용자 숫자 형식 검증 | ||
| validateNumbers(userNumbers) { | ||
| if (!/^\d{3}$/.test(userNumbers)) { | ||
| alert('잘못된 입력입니다. 숫자만 입력하세요.'); | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
이 부분의 코드는 상당히 깔끔하게 잘 짜인 것 같네요! 왜 그렇게 생각하냐면요, ...
- 메서드 시그니처(함수의 이름, 매개변수들의 이름들로 구성된 조합)이 자세해 이해하기 쉽다고 생각해요.
- 각 함수가 하나의 역할만을 수행하고 있어요. 즉 단일 책임 원칙을 잘 준수하고 있어요. 이렇게 기능을 구성하면 이후 특정 기능을 수정해야 할 때 어느 함수를 수정해야 할 지 명확하고, 수정할 때에도 의존하는 함수가 적거나 없기 때문에 더 안전하게 변경할 수 있어요. 결과적으로 안정성과 유지보수에 기여하는 긍정적인 결과가 될 거에요.
- 함수의 이름과 리턴 타입이 서로 잘 맞물려 있어 예측하기 쉬워요.
validate-라는 네이밍은 주로 무언가를 검증한 후 검증 결과를 반환함을 암시하며,boolean형태의 값을 반환할 것이라고 많은 개발자분들이 예측해요. 그리고 그 예상대로 함수에서는boolean을 반환하네요! 이렇게 하면 함수의 내부 로직을 보지 않고도 함수의 기능을 예측하기 쉬워져요. - 각각의 에러 메시지가 사용자에게 충분한 정보를 제공하고 있어요. 이러한 친절함 하나하나가 모여서 서비스를 사용하며 사용자가 느끼는 UX를 결정하는 요인이 될 겁니다.
앞으로도 구현하실 때 이렇게 구현하시는 것을 유지해 주시면 좋겠어요. 💯
그리고, 개인적인 생각이지만 주석의 경우에는 모든 함수 및 여러 라인에 사용하는 것보다는, 메서드 시그니처를 깔끔하게 작성해 주석을 적지 않더라도 이해할 수 있는 코드를 작성하는 것을 더 우선시하셨으면 좋겠고, 이러한 노력에도 불구하고 함수의 의미를 명확하게 표현하기 어려운 경우 주석을 사용해 주신다면 좋을 것 같다는 생각도 듭니다. 이 때는 주석을 마음껏 자세하게 적어주셔도 좋아요. 과도하게 많은 주석은 오히려 가독성을 떨어뜨리고 혼란을 야기할 수 있다고 생각하는데, 어떻게 생각하시나요?
There was a problem hiding this comment.
💡함수의 의미를 명확하게 표현하기 어려운 경우 주석을 사용
멘토님께서 작성해주신 위의 내용에 저도 매우 공감합니다!
메서드 시그니처로 제가 작성한 코드가 사람들을 충분히 이해시킬 수 있다면, 주석의 사용하지 않아도 된다고 느낍니다!
다만, 코드 중에서 정규식 표현은 한번에 이해하기 어려운 부분이 존재해서 주석을 달아주면 좋지 않을까 하는 생각은 드네요!!
| let strikeAndBallCounts = { | ||
| strike : 0, | ||
| ball : 0 | ||
| }; |
There was a problem hiding this comment.
let과const의 차이는 무엇일까요?- 본 코드에서
let을const로 바꾼다면, 에러가 발생할까요, 아니면 평소대로 올바르게 동작할까요? - 미션에서는
var을 사용하지 말고,let과const를 사용하라고 요구사항에 명시했었어요. 물론 이번에는 요구사항에 적혀있으니 준수하셨지만, 왜 많은 개발자들은var사용을 지양할까요?
There was a problem hiding this comment.
제가 알고 있는 지식을 먼저 말씀드리자면, 가장 큰 차이는 let은 값을 변경시킬 수 있다는 것이고, const의 경우 값의 변경이 불가능한 것으로 알고있어요!
var의 경우엔 범위에 따른 문제 발생이 존재하기 때문에 사용을 지양하고 있는 것으로 알고있어요!
예를 들어, 아래 코드의 경우 let, const에서는 재선언이 불가능해 작동하지 않지만 var는 작동하는 모습으로 나옵니다. 의도를 가지고 했다면 문제가 없겠지만 그렇지 않을 경우에 발생할 문제가 생기기 때문에 개발자의 실수를 유발할 수 있다고 봅니다!
var a = "hey hi";
var a = "hey hello";
console.log(a) // "hey hello" 출력
There was a problem hiding this comment.
var의 경우엔 범위에 따른 문제 발생이 존재
잘 짚어주셨어요. let, const와 달리 var는 함수 스코프를 지니게 되는데, 이는
- 개발자들이 예측하기 힘든 변수의 수명을 지닐 수 있음을 의미하고
- 함수 밖에서 변수가 선언될 경우 이는 전역 변수가 되기에, 더 의도치 않은 변경에 취약해질 수 있음을 의미해요.
그 외에도, var는 재할당 뿐만 아니라 재선언도 가능한데, 이렇게 변경이 너무 자유로우면, 개발자가 의도치 않은 실수를 해도 에러가 발생하지 않아 실수를 알 수 없고, 이로 인해 디버깅이 어려운 상황을 만들기 쉬워질 수 있어요.
이를 이유로 var는 특별하게 납득될 만한 상황이 아니라면 사용을 지양하시는 것을 추천드려요.
1, 2번도 한 번 시도해보세요.
| return; | ||
| } | ||
|
|
||
| let result = this.play(userNumbers, this.computerNumbers); |
There was a problem hiding this comment.
result 변수의 경우 재할당 될 일이 없으니, let 대신 const로 변수를 선언해 주셔도 좋을 것 같습니다!
둘 다 잘 작동할 텐데 왜 굳이 재할당을 하지 못하도록 제한해두는 것일까요? 이 이유에 대해서도 생각해 보시면 좋을 것 같아요.
There was a problem hiding this comment.
브라우저 상에서 의도치 않은 변경 가능성이 존재해서 const로 두는게 아닐까하는 생각이 드는데.. 정답은 아니겠죠??
Refactor/reflect review
답변이전에 못했던 질문에 대한 답변을 남겨드리며, 저도 정확히는 이해하지 못했고 사실임을 뒷받침해줄 수 있는 문서들이 전무하다 보니 일부 설명은 추측성임을 알려드립니다. 핵심만 요약
아래의 코드를 실행해 보세요document.body.innerHTML = 'innerHTML';
console.log(document.body.innerHTML) // 'innerHTML'
alert('Alert!');이 코드를 실행하면, 그러나, 세부 설명왜 이러한 현상이 발생하는지, 제가 알고 있는 지식을 가능한 한 동원해 설명해 보겠습니다. 여기서부터는 지루한 개념 설명이 포함됩니다. 1️⃣ JavaScript는 싱글 스레드 방식으로 작동한다
2️⃣ WebAPI의 도움으로 비동기 작업을 수행할 수 있다여기까지만 보면 웹 브라우저에서는 하나의 작업을 수행하는 동안, 다른 작업은 수행할 수 없어야 하는데, 애니메이션이 재생되면서도 사용자는 다른 작업을 할 수 있고, 대용량의 파일을 다운받으면서도 다른 작업을 할 수 있는 점은 설명이 되지 않을 겁니다. 허나 런타임 환경에서는 JavaScript는 WebAPI의 도움을 받아 여러 작업들을 수행할 수 있게 됩니다.
3️⃣ 이벤트 루프가 비동기 작업들의 실행을 담당합니다여기에서는 이러한 비동기 작업들 ( 콜 스택위에서 설명드렸습니다. 실행해야 하는 코드를 콜 스택에 넣고, 콜 스택에서 함수를 빼내면서 해당 함수를 실행하게 됩니다. 더 요약하면 콜 스택에는 실행될 예정인 작업들이 들어갔다 나오는 겁니다. 마이크로 태스크 큐 / 매크로 태스크 큐WebAPI가 비동기 작업들의 수행을 지시해 비동기 작업이 수행된 후 작업이 완료되면, 종류에 따라 마이크로 태스크 큐 또는 매크로 태스크 큐에 작업이 들어가 대기하게 됩니다. 이벤트 루프자바스크립트 엔진의 구성 요소로, 마이크로 태스크 큐와 매크로 태스크 큐 등 여러 큐들을 확인하면서 처리 준비가 된 비동기 작업들을 수행하는 역할을 합니다. 즉 이벤트 루프는 콜 스택과 큐를 확인하면서 비동기 작업 이후 실행되어야 할 함수들을 관리해 콜 스택으로 밀어넣어주는 역할을 수행하는 등, 전반적인 관리 역할을 하는 것이죠. 이벤트 루프는 크게 아래의 우선순위대로 수행됩니다.
이러한 방식으로, 동기적인 코드, 그리고 비동기적인 코드 모두 콜 스택에서 하나씩 최종 실행되므로 싱글 스레드의 특성이 유지되는 것입니다. 그리고 우선순위도 지니고 있죠. 4️⃣ innerHTML과 alert를 사용하는 예제에서 innerHTML보다 alert이 먼저 반영되는 이유여기서부터는 저도 100% 제대로 이해하고 있지 않습니다. 틀린 설명이 있을 수 있음은 감안해 주셨으면 합니다. document.body.innerHTML = 'innerHTML';
alert('Alert!');지금까지의 원리를 바탕으로 왜
어쨌든 이렇게 5️⃣
|
wzrabbit
left a comment
There was a problem hiding this comment.
@gogo1414 👋🏻
리뷰 반영 확인했습니다! 변수, 주석 등등 잘 관리해 주시면서 코드가 한층 더 깔끔해진 것 같군요. 사소해 보일 수 있지만 예측하기 쉽고, 이해하기 쉽도록 만드는 코드의 핵심이자 비교적 반영하기 좋은 방법이라고 생각해요.
적어주신 질문에 대해서는 완전하지는 못하지만 답변을 달아보았습니다. 어려운 개념들이 상당히 많이 들어갔는데 동기적인 코드인데 순서가 다르게 실행될 수 있는건가? 라는 혼란에 대해서는 너무 크게 생각하지 않으셨으면 좋겠습니다. 코드 자체는 동기적이라면 순서대로 실행되는 것이 맞으니 계속해서 순차적인 코드를 작성해 주시면 됩니다.
다만 JavaScript라는 녀석은 혼자 작동하는 것이 아닌 브라우저의 Web API 등 여러 도구들과 함께 협동하면서 작동하는 지라 이런 상황이 발생할 수 있구나 정도만 생각하고 넘어가주시면 좋을 것이라고 생각합니다. 너무 오래 발목이 묶이지는 않으셨으면 하는 마음입니다.
다음 미션은 이제 작성하신 코드를 MVC 패턴을 준수하는 코드로 리팩터링하는 작업인가요? 프론트엔드에서 MVC라니... 어쩌면 의아해하실 수도 있겠군요.
다만 한 번 디자인 패턴을 적용해 보시면서, 이러한 디자인 패턴을 왜 사용하는 것인가를 생각해 보시면 성장에 도움이 많이 될 수 있을 것 같습니다. MVC의 원칙에 너무 집중하기보다는 부딪혀보시면서 목적이 과연 어떨 지를 고민해 보시면 좋을 것이라는 뜻입니다
이번 미션은 여기까지~ 다음 미션도 화이팅이에요! 👋🏻
| // 볼, 스트라이크르 문자열로 변경 | ||
| extractResult(strikeAndBallCounts) { | ||
| if(strikeAndBallCounts.ball === 0 && strikeAndBallCounts.strike === 0) | ||
| extractResult({ ball, strike }) { | ||
| if(ball === 0 && strike === 0) | ||
| return "낫싱"; | ||
| if(strikeAndBallCounts.ball !== 0 && strikeAndBallCounts.strike === 0) | ||
| return `${strikeAndBallCounts.ball}볼`; | ||
| if(strikeAndBallCounts.ball === 0 && strikeAndBallCounts.strike !== 0) | ||
| return `${strikeAndBallCounts.strike}스트라이크`; | ||
| return `${strikeAndBallCounts.ball}볼 ${strikeAndBallCounts.strike}스트라이크`; | ||
| if(ball !== 0 && strike === 0) | ||
| return `${ball}볼`; | ||
| if(ball === 0 && strike !== 0) | ||
| return `${strike}스트라이크`; | ||
| return `${ball}볼 ${strike}스트라이크`; |
There was a problem hiding this comment.
구조 분해 할당을 잘 적용해 주셨군요 👍🏻
여담으로 이렇게 오브젝트를 구조분해 할당하는 경우에는 오브젝트의 일부분만을 필요할 때 꺼내서 사용할 수 있는 이점도 있습니다. 범용성이 높은 함수를 짜실 때 유용하게 사용하실 수 있으며 React를 접하신 이후에는 저 구조 분해 할당은 토할 정도로 많이 접하실 겁니다 😄
그 때 코드를 보실 때에는 "뭐야 저 문법은;;" 이라고 당황하지 마시고 "그렇지, 저게 바로 구조 분해 할당이지. 저렇게 쓰일 수 있구나" 로 느끼시길 바라겠습니다
| // [1-9] 1부터 9의 숫자, {3} 3자리 수를 의미 | ||
| if (!/^[1-9]{3}$/.test(userNumbers)) { | ||
| alert('잘못된 입력입니다. 1부터 9까지의 숫자만 입력하세요.'); |
There was a problem hiding this comment.
요구사항 확인 후 정규 표현식을 다시 잘 수정해 주셨고, 설명이 필요한 곳에만 주석을 사용해 주셨군요, 좋습니다
여기에 더해 저는 두 개의 의견을 더 남겨보겠습니다
- 코드의 각 요소를 설명할 수도 있지만, 이 코드가 전체적으로 하는 일을 나타내 보는 것은 어떨까요? 예를 들면
// `userNumbers`가 세 자리 수이고, 1부터 9까지의 숫자로만 이루어져 있는지 검증
가 될 수 있을 것 같습니다.
- 상수 형태로 선언해 두어 정규 표현식을 저장해 두고 사용하는 방법도 있습니다. 이 방법도 필요에 따라 고려해 보시면 좋을 것 같습니다! 아래의 코드는 예시입니다.
const THREE_DIGIT_NON_ZERO_REGEX = /^[1-9]{3}$/;
if (!THREE_DIGIT_NON_ZERO_REGEX.test(userNumbers)) {
alert('잘못된 입력입니다. 1부터 9까지의 숫자만 입력하세요.');
}|
|
||
| const output = document.querySelector("#result"); | ||
| output.innerHTML = ''; | ||
| output.textContent = ''; |
|
|
||
| // 결과 확인 | ||
| checkResult(result) { | ||
| isPlayerWinner(result) { |
There was a problem hiding this comment.
좋습니다 👍🏻 이제 명확하게 플레이어가 이긴 상황인지를 판별하는 함수임이 예상이 되고 결과도 boolean으로 줄 것이 예상되는 것 같습니다
is-, check-, should-, validate- 등의 접두사를 적절히 사용해주시면 함수 내부의 구조를 보지 않고도 어떠한 형태로 값을 반환할 지 예측하기 좋다고 생각해요
| } | ||
|
|
||
| let result = this.play(userNumbers, this.computerNumbers); | ||
| const result = this.play(userNumbers, this.computerNumbers); |



안녕하세요, 리뷰어님!
저는 자바스크립트가 처음이라 조금씩 찾아보면서 하다보니 생각보다 오래 걸리더군요.. 2~3일을 몇시간에 걸쳐서 완성했답니다.
우선, 코드 컨벤션 내용을 최대한 지켜보려고 했는데 내용이 너무 길고 한번에 외우는게 불가능해보여서 제대로 지켜졌는지 확인이 불가능하네요.. (참고 부탁드립니다!)
제가 실행하다보면서 약간 이해가 안되는 부분(?)이 몇가지 생겼는데 만약 알고 계신 내용이라면 알려주세요!!😊
저는 자바스크립트를 싱글스레드로 알고 있어서 처음에는 왜 그런지 이해를 못했어요. 그래서 조금 찾아보았는데 브라우저(?)에서 실행되는 뭐라뭐라 작성되어 있는데 사실 잘 이해가 안가더군요..
알고 계신다면 알려주시길 부탁드립니다!🙏🙏🙏
추가) cypress test를 까먹고 있었어요..! 실행해보니 test 1개만 통과하고 있어서 insertAdjacentHTML -> innerHTML로 변경했습니다.