[1단계 - 자동차 경주 미션] 밧드(감우영) 미션 제출합니다. #81
Conversation
|
추가로 이번 미션을 진행하면서 궁금했던 것들 질문드립니다.
|
There was a problem hiding this comment.
안녕하세요 밧드님!
코드 리뷰어로 배정된 Vallista 입니다 👍
요번 코드리뷰가 2일정도 늦어졌는데,
앞으로 코드리뷰는 매일 오전 7시에 일괄적으로 진행하겠습니다!
오전에 코드리뷰를 드리면, 익일 7시 전까지 PR 혹은 피드백을 주시면 됩니다!
첫 코드리뷰어로 밧드님의 코드를 보게되어 영광입니다!
앞으로 잘 부탁드려요!
1단계에서 중점적으로 보는 포인트
1단계에서 중점적으로 보는 포인트는 다음과 같습니다.
- eslint, prettier 등 설정을 왜 하셨는지, 하셨다면 그 의도가 무엇인지
- e2e 등의 테스트를 의도가 이해되는 단위로 작성되었는지
- html을 semantic 하게 작성하셨는지
- javascript 문법을 적절하게 사용하셨는지
- 재사용될 수 있는 영역에 대해 로직을 잘 분리하셨는지
- css 문법을 적절히 사용하셨는지
총 6가지 관점에서 보았습니다.
그렇기에 전체적인 구조 관점에선 코멘터리를 남기진 않았고,
해당 리뷰의 가장 마지막에 구조적으로 어떻게 개선하면 좋겠는지 말씀드릴 예정이에요!
피드백을 받아보시고, 가용할만 하다 싶으면 2단계 미션에서 적용해주시면 좋겠습니다!
(그런데 구조를 잘 짜셨다면 피드백이 없을수도 있습니당)
칭찬해요
와.. MVC도 잘 적용하셨고, 1단계 코드임에도 제가 아쉬웠던 적이 없는 코드는 처음인 것 같네요. 깔끔하게 잘 짜셨습니다! 현재 규모로 적절하게 배합하신 것 같고, 여기서 더 기능이 늘어난다면 코드를 분리해서 더 확장성 있게 작성해야할 것 같지만, 지금 당장은 괜찮은 코드인 것 같습니다!
피드백
사실.. 없어요 ㅋㅋㅋ e2e 만 보완하시면 좋을 것 같아요.
일단 리뷰 코맨트로 17개를 적어두었고, 17개에 대해서 확인해주시면 감사하겠습니다!
(억지로 짜내느라 힘들..)
질문 답변
Q1
MVC 패턴을 사용해볼려고 노력했습니다. 최대한 목적에 맞게 분리하면서도 이렇게 하는게 맞는지 의문이 들어서, 현재 패턴에 맞게 잘되어있는지 궁금합니다.
패턴에 맞게 잘 되어있는 것 같습니다!
코드의 의도인, Controller에서 View와 Model에서 메소드를 꺼내와 제어하는 형태로 잘 표현해주셨습니다!
조금 추가되면 좋을 부분은 Models 인데요! Models에서 여러 모델을 파사드 패턴 (지금부터 벌써 패턴을 논하기엔 1단계지만..) 으로 응집해주는 역할을 할 수 있는데요!
그에따라 Car를 관리하는 CarManager 등으로 배열을 갖고있고, CarManager와 같은 친구들이 Models에 퍼사드로 응집되어, 레이어 단위를 촘촘히 설계해서 코드의 사이드 이펙트를 줄이고 테스트 코드를 짤 수 있게 용이하게 하는 구조를 설계하는게 좋을 것 같습니다!
Controller <- Models <- CarManager <- Car
__________________<- LineManager <- Line
__________________<- Board
위와 같은 구조로 하게 되면 레이어가 4단계로 이루어집니다. 그래서 목적을 좀 더 세밀하게 분리할 수 있는 그런 장점이 있네요!
Q2
MVC 패턴에서, DOM 요소를 모두 View에 넣고자 하여서, DOM에서 이벤트 부착을 하였고, 이때 콜백함수를 인자로 받고, 이를 Controller에서 실행하는 방식으로 하였습니다. DOM은 View에 존재해야하지만, 데이터 로직 변경은 Controller에서 Model로 넘겨주기 위하여 위와 같이 설계했습니다. 이벤트 부착 곳을 어디로하면 좋을 지 확인해주시면 감사하겠습니다.
넘 잘 하셨습니다! 자바스크립트는 함수가 일급객체이기 때문에 이러한 테크닉이 가능하죠. 이러한 패턴을 헐리웃 원칙 이라고 하는데, 이렇게 되면 고수준과 저수준에서 서로간 커뮤니케이션을 하지 않게 (의존성 부패) 막을 수 있고, 이벤트를 단방향으로 전달시킬 수 있습니다 :) 아주 잘 하셨어요!
다만, 타입스크립트와 같이 함수단위로 parameter가 어떻게 전달되는지는 모를 수 있기 때문에 jsdoc 기준으로 주석을 달아주는게 좋지 않을까 싶네요~~!
Q3
for(let i=0; i<2; i++)이런 식으로 최대한 안쓰도록 해봐라 말을 들었던 기억에 있어서, 실제로 for문을 많이 안쓰는지 궁금합니다.
그리고 안쓰게 된다면 Array.fill(숫자).forEach를 생각하는 데, for문과 같은 역할을 해야한다면 어떻게 쓰는지 궁금합니다.
for를 최대한 쓰지말라고 하는 이유는 overflow 때문에 그렇습니다!
그래서 es6에선 forEach, map 등의 array 상에 존재하는 여러 고차함수가 존재해요!
고차함수로 진행하면 array의 최대 길이까지 무조껀 보장되므로 안전하죠!
밧드님이 사용하신 for 부분은 count 까지 도는데, array가 count이상으로 실행될 수 있는 위험을 가지긴 했어요..
그래서 array.length를 사용하는게 안전한데, count가 그 이하일수 있으니 array.length를 벗어나도
어플리케이션이 깨지지 않도록 제공하는 로직 보완이 필요하겠습니다!
Q4
버튼을 클릭했을 때 콜백함수 내부에 실행됬으면 하는 메소드를 넣었는데, 만약에 실행시켜야될 메소드가 많아지면 계속해서 메소드를 분리해서 묶어주는 것이 맞는지 궁금합니다.
메소드가 많아졌을때 메소드 별 관심사의 분리를 해주기 위해 여러 메소드로 묶어줄 수 있겠지만, 너무 커져서 메소드가 많아지면 메소드를 타고 들어가서 봐야될 포인트가 많아지겠죠.. 그럴땐 수퍼클래스를 하나 만들고, 슈수퍼 클래스에서 submit이라는 액션을 만든 후, 수퍼클래스를 상속받는 서브클래스를 만듭니다. 서브 클래스는 해당 관심사에 따른, Submit 액션들이 존재하겠죠. 그 후 다형성을 이용해 수퍼클래스의 submit 액션을 for 문으로 등록된 서브 클래스의 액션을 구동시킵니다. 뭐 이런 형태로.. (이 부분은 리펙터링에도 나오는 이야기입니다.) 추후 이런 상황이 있을때 피드백을 리뷰어분들이 잘 해주실 것 같아요! 마주쳤을때 여쭤보시면 좋을 것 같습니다.
| "parser": "@babel/eslint-parser", | ||
| "extends": [ | ||
| "airbnb-base", | ||
| "plugin:jsx-a11y/recommended", |
There was a problem hiding this comment.
air bnb의 lint 스타일 가이드를 적용하는데 있어서, 의존성 패키지 목록에 있었기 때문에 설치를 했습니다.
| }, | ||
| "parser": "@babel/eslint-parser", | ||
| "extends": [ | ||
| "airbnb-base", |
There was a problem hiding this comment.
airbnb-base로 하면 어떤 이점이 있기 때문에 사용하시는 걸까요?
There was a problem hiding this comment.
처음 eslint를 설치하면서 블로그를 많이 참조했고, rule을 하나씩 찾으면서 넣기보다 많이 알려져있는 convention을 따라서 해보자 라는 생각을 해서 airbnb 가이드를 선택했습니다.
.prettierrc.json
Outdated
| "printWidth": 100, | ||
| "semi": true, | ||
| "singleQuote": true, | ||
| "trailingComma": "all", |
There was a problem hiding this comment.
참고한 글에 있는거 그대로 사용했고, es5가 있는지 이제 알게 됐습니다!
cypress/integration/racing.spec.js
Outdated
| beforeEach(() => { | ||
| cy.visit('index.html'); | ||
| }); | ||
| it('자동차 이름이 공백인 경우 alert를 띄운다', () => { |
There was a problem hiding this comment.
왜 공백인 경우 alert을 띄워야 하나용!?
테스트의 의도가 무엇인지 명확하게 알 수가 없네요 ㅠㅠ
공백인 경우 에러이기 때문에, 행동을 제한하는 목적이라면
"자동차의 이름이 공백이거나 최대 글자 수(5자)를 넘어가는 경우 에러 알럿을 출력해 사용자에게 실패 메시지를 전달한다" (너무긴가..) 정도로 명확하게 해주는게 좋을 것 같습니다!
어차피 목적은 "에러 알럿 출력" 이므로, 하단의 6자 이상인 경우와 동일한 테스트 로직을 갖게되므로 함께 테스트 할 수 있을 것 같아요.
There was a problem hiding this comment.
자동차 이름이 공백으로 입력된 경우와 이름이 5자를 넘는 경우 두 가지를 한 번의 입력으로 테스트할 수 없을 것 같다고 생각해서 나눴습니다.
자동차 이름의 길이를 검증하는 것이 같은 맥락이기 때문에 하나의 테스트 함수에서 진행해보라는 것으로 이해했고, 수정 해봤습니다
cypress/integration/racing.spec.js
Outdated
| }); | ||
| }); | ||
|
|
||
| context('화면표시 테스트', () => { |
There was a problem hiding this comment.
e2e 테스트는 밧드님이 해주신 것처럼 여러 관점으로 해석해서 사용할 수 있지만 불변하는 하나의 중요한 점은 "협업 혹은 새로 프로젝트에 들어온 사람이 한 눈에 이 프로젝트가 어떻게 사용자 프로세스로써 진행되고, 사용자가 어느 목적 (지금 프로젝트는 자동차가 골인할 때까지) 으로써 전체 맥락을 볼 수 있어야한다." 라고 생각해요.
지금 상황에선 단순히 어떤 행동으로 나누신 것 같은데, 조금 더 맥락있게 순서대로 배치하고 어떤 테스트를 왜 테스트하는 것인지 그러한 관점에서 작성해주시면 더 좋을 것 같아요!
There was a problem hiding this comment.
"UI테스트", "화면표시 테스트"에서
- "입력된 자동차 이름과 시도할 횟수가 조건에 맞지 않을 때 alert가 나타나는지 테스트"
- "입력이 완료된 후, 레이싱 결과와 우승자, 다시 시작 버튼이 올바르게 나타는지 테스트"로 수정해봤고,
- 추가로 "다시 시작하기 버튼을 클릭했을 때 초기 화면의 상태로 리셋되는 테스트" 분리해봤습니다.
index.css
Outdated
| } | ||
|
|
||
| .input-section__button { | ||
| width: 65.68px; |
There was a problem hiding this comment.
figma에 나와있는 데로 적용했었는데, 67로 수정해봤습니다.
index.css
Outdated
| height: 36px; | ||
| border-radius: 4px; | ||
| background-color: var(--color-primary); | ||
| color: white; |
There was a problem hiding this comment.
이왕 하는거 white 같은 애들도 변수로 만드는게 좋지 않을까요?
언제 바뀔지 모르니까요!
There was a problem hiding this comment.
넵 현재 css 파일에서 white를 버튼 타이틀에만 사용이 되어서 --color-button-text로 선언해봤습니다.
index.html
Outdated
| <body> | ||
| <div id="app"> | ||
| <h1 id="title">🏎️ 자동차 경주 게임🏁</h1> | ||
| <div class="input-section"> |
There was a problem hiding this comment.
div로만 쓰지말고 section, article을 적절히 섞어서 semantic한 웹을 구성해보는건 어떨까요?
- section, article을 쓰지 않은 이유가 있나요?
- section, article은 각각 언제 쓰이는 친구일까요?
There was a problem hiding this comment.
1.평소에 많이 사용해본 경험이 없어 생각하지 못했습니다!
2.
-
section은 서로 관련성을 가지는 element를 묶어줌으로써 가독성을 높이는 데에 사용하는 것으로 봤습니다.
현재 템플릿에서 이름과 횟수를 입력받는 부분 또는 counter container과 각 자동차의 레이스 결과를 보여주는 step-section을 section으로 분리해줄 수 있다고 생각했습니다. -
article은 독립적으로 존재할 수 있는 요소에서 사용하는 것으로 봤습니다. 저는 이 태그을 많은 내용의 글이 들어가는 곳에 사용했던 것으로 기억하는데, 현재 템플릿에서는 어디에 사용되어야 적당할 지 정확히 모르겠습니다.
There was a problem hiding this comment.
넵 지금 이해하신게 적절한 것 같고, 앞으로 계속 써보시면서 왜 좋고 언제쓰면 더 좋은지 경험해보시면 좋을 것 같습니다!
src/car.js
Outdated
| this.step = NUMBER.INITIAL_STEP; | ||
| } | ||
|
|
||
| generateRandomNumber() { |
There was a problem hiding this comment.
class 내부에서만 쓰이는 private method는 #으로 접근지정자를 사용해주는게 어떨까요?
https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Classes/Private_class_fields
There was a problem hiding this comment.
넵. private 접근 레벨로 지정해서 외부에서 사용되지 않게 해서
추후의 클래스 내부의 메소드가 다른 곳에서 사용 되고있을 때 내부 구현을 변경하는 데에 자유로울 수 있는 점,
캡슐화에 있어서 클래스 내부의 데이터 처리를 요청 받을 때 외부에서 사용되었으면 하는 메소드와 구별할 수 있다는 점 등 이런 장점을 봤는데 의도가 맞을까요? 아니면 조금 호들갑일까요?
src/controller.js
Outdated
| this.triggerEvent(); | ||
| } | ||
|
|
||
| triggerEvent() { |
There was a problem hiding this comment.
triggerEvent 보다 initEventHandler 뭐 요런 뉘앙스가 더 낫지 않을까요?
trigger는 방아쇠를 당기는. 뭐 그런 뜻인데 그럼 이벤트 내에 있는 무언가의 로직이 동작해야한다 이런 느낌이 강해서, 하지만 메소드의 로직을 보면 세팅해주는 코드밖에 없네요!
There was a problem hiding this comment.
메소드 이름을 view의 이벤트 추가 메소드와 controller의 콜백 메소드를 설정 시켜준다는 의미로 setEventHandler라고 지어봤습니다.
|
퍼사드 패턴을 적용해볼려고 했습니다. carManager에서 car 인스턴스 리스트를 조작하는 내용이 들어가는 것이 맞는지 모르겠습니다. 아직 응집할게 별로 없어서인지 그런건지.. 사이에 불필요한 층이 하나 생긴것 같다는 느낌을 받았습니다! |
자동차 경주 미션 1단계를 진행하면서 느낀점
온보딩 미션 때와는 달리 페어 프로그래밍에 조금 익숙해진 것 같고, 확실히 혼자 할 때보다 컴파일하기 전에 사전에 버그를 발견하면서 "왜 안되지"하는 고민을 안할 수 있었습니다. 결과적으로도 프리코스에서 작성했던 것보다 훨씬 깔끔한 코드를 작성했다는 느낌을 강하게 받았습니다.
이번에는 html, css, js를 다 만든 뒤에 테스트를 진행했고, 어제까지는 손으로 충분히 테스트를 진행해도 되지않을까 라고 생각했습니다. 하지만 오늘 리팩토링을 하면서 이때까지 많은 코드를 작성했고 혹시나 놓친 부분이 있을 것 같다는 걱정을 덜하게 되었고, 이게 수업에서 말했던 자신감을 가질 수 있는 방법이라는 것을 느낄 수 있었습니다.