[2단계 - 자동차 경주 구현] 블링(김요욱) 미션 제출합니다. by uk960214 · Pull Request #110 · woowacourse/javascript-racingcar

uk960214 · 2022-02-18T08:58:56Z

안녕하세요! 좋은 한 주 보내셨나요?
2단계를 진행하면서 동시에 1단계에서 미흡했던 부분들이 곳곳에서 발견돼서 함께 수정해서 PR 보냅니다.

이번 단계에서 진행한 내용을 간단하게 요약하면 아래와 같습니다.

1단계에 대한 수정
- innerHTML로 사용자가 입력한 값을 그대로 삽입하는 부분 수정 (자동차 이름)
  1. “<div>”를 이름으로 넣었을 때 html을 망가뜨릴 수 있음
  2. innerHTML을 최대한 지양하고, 직접 HTML 요소를 만들어서 append 하는 방식으로 변경
- 자동차를 관리하는 carManager 클래스 추가로 생성
  1. 이전의 자동차 객체에서 이름에 대해서 검증을 하는 방식으로는 잘못된 이름에 도달할 때까지는 객체를 만들고, 그 id 값을 set에 추가한 상태로 유지
  2. carManager 클래스에서 전체 이름이 다 유효한 지 우선 검사한 이후 통과한 경우에만 객체를 만들기 시작
  3. 추가적인 역할 분담 구체화
- 테스트 코드의 추가적인 리팩터링
2단계 구현
- 게임 라운드 진행 당 1초의 간격 추가
  1. setInterval을 추가, 실행한 라운드의 수 저장하고 실행 때 마다 업데이트
  2. 횟수 다 채우면 clearInterval 및 게임 종료 함수 실행
- 로딩 애니메이션 표시
  1. requestAnimationFrame을 이용해 스피너가 360도 회전하도록 설정
  2. 횟수 입력 버튼으로 게임 시작 시 자동으로 이름 하단에 생성
- 라운드 결과 표시
  1. 스텝을 담는 컨테이너를 추가, 그 dataset으로 car id를 추가
  2. 라운드 실행 결과 전진한 차인 경우 컨테이너에 스텝을 추가
  3. 모든 라운드가 종료되면 애니메이션 정지 및 스피너 삭제
- 테스트 코드로 타이밍을 테스트하기 위해서 clock과 tick 사용

이번 단계를 통해서 requestAnimationFrame을 처음 사용해봤는데 이렇게 사용하는 게 맞는지 잘 모르겠습니다.
저는 애니메이션을 repaint 하는 부분을 효율적으로 하기 위해서 사용하는 것으로 이해했습니다.
그래서 1초 간으로 게임을 진행하는 요구사항 자체는 비즈니스 로직에서 setInterval을 활용했고
별개로 view에서 애니메이션 처리를 구현했습니다.
제가 이해한 개념이 맞는지 궁금합니다!

이번 단계도 잘 부탁드리겠습니다!!

1. 잘못된 값을 입력했을 때 alert를 검증하는 테스트 수정 - 성공 케이스와 실패 케이스가 alert를 표시하는 부분을 제외하고 동일 - 따라서 묶어서 구현하고 인자로 올바른 입력값 여부를 추가해 한 함수 내에서 분기하도록 리팩터링 - alertStub는 인자로 주입하는 대신 클릭 시 alert을 보여주는 지 확인하는 명령 내에서 선언하고 활용 - alert 표시되는 지 여부 대신 올바른 메시지를 표시하는 지 확인하도록 변경 - 위 메시지를 검증하기 위해서 코드 내에서 error 자체를 alert 해주는 대신 error의 message 부분만 alert 하도록 수정 2. E2E 테스트의 취지에 더 적합하도록 테스트 설명 문구 수정 3. cypress/support/index.js를 gitignore에서 제외 (command를 부를 수 있도록)

https://velog.io/@apricotsoul/HTML-section%EC%9A%94%EC%86%8C-main-%EC%9A%94%EC%86%8C#%EC%A7%88%EB%AC%B8-%EC%84%B9%EC%85%98-%EC%9A%94%EC%86%8C%EC%9D%98-title%EC%9D%84-%EA%BC%AD-%ED%8F%AC%ED%95%A8%ED%95%B4%EC%95%BC%ED%95%9C%EB%8B%A4%EA%B3%A0-%ED%96%88%EB%8A%94%EB%8D%B0 https://developer.mozilla.org/ko/docs/Web/HTML/Element/section#%EC%82%AC%EC%9A%A9_%EC%9D%BC%EB%9E%8C

https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Classes/Private_class_fields

1. car의 인스턴스 생성을 game에서 직접하는 것이 아닌 manager 클래스에서 - carManager 클래스 구현 - game으로부터는 car의 생성 부분을, car에서는 이름의 검증 부분을 이관 - 이유: 이름이 valid 하지 않은 것이 입력 값의 첫 값이 아닌 경우, 그 앞의 이름까지 객체를 생성하고 오류를 반환. 따라서 이름 입력 값 전체를 먼저 검증한 후, 생성 시작하도록 변경 + 추가적인 역할 분담 2. carName의 검증 기준을 "5자 이하" => "1-5자"로 변경 - 0자의 입력 값(= 빈 칸)도 허용할 수 없음 - 관련된 상수명, 함수명, 오류 메시지, 테스트도 수정

1. 라운드를 1초 간격으로 실행하는 지 검증하기 위해서 1초 이전에 결과가 표시 되지 않는지 확인하는 테스트 추가 2. 전체 라운드 진행 후 결과와 alert가 호출되는 지 테스트 추가 3. spinner 상수 분리

- 각 라운드가 1초간 간격을 가지고 실행되도록 설정 - 입력한 값 만큼 라운드가 진행되면 실행 중지하고 결과 표시

1. 라운드 실행을 1초 간격으로 실행 - setInterval을 통해서 1초 간격으로 라운드 실행 - 라운드 수가 입력한 횟수에 다다르면 clearInterval로 정지 2. 스피너 뷰 추가 - 횟수 입력 버튼으로 게임 시작 시 자동으로 이름 및 스피너 추가 - requestAnimationFrame으로 회전 애니메이션 추가 3. 전체 게임 종료 시 종료 결과 표시 - 스피너를 삭제하고 결과를 표시

1. 1단계 테스트 코드 중 정상 게임 진행 테스트 코드 수정 - 변경된 요구사항에 맞게 일정 시간 이후 확인하도록 수정 2. 한 차례만 확인하고 넘어가야 하는 경우의 코드 수정 - DOM을 가져오는 코드의 경우 지속적으로 가져오기를 시도 - timeout 이용, 한 차례만 테스트하게 넘어갈 수 있도록 수정 - 없는 것(not.exist)과 안 보이는 것(not.be.visible)의 차이 유의

1. 시간 관련 상수 분리 - 라운드 진행 간격 (1초) 상수로 분리 - 축하 alert 표시 대기 시간 (1초) 상수로 분리 - 테스트 코드에도 반영 2. 이름 입력 값을 split하는 과정을 carManager로 이동 3. 라운드 별로 자동차가 움직였는지 여부를 보관하는 moved에 항목을 추가하는 위치 변경 - 기존에는 random 숫자를 뽑고 자동차를 움직이는 함수에서 moved 배열을 인자로 받아서 처리 - 인자로 받은 배열에 직접 값을 추가하는 것은 지양하려는 의도에서 움직인 경우 true를 반환하고 아닌 경우 false를 반환해서 함수를 실행한 후 반환 값으로 함수 호출 위치에서 (밖에서) moved에 추가하도록 변경 4. 명확하지 않은 변수명 수정

EungyuCho

안녕하세요 블링님!

이번에 step도 정말 깔끔하게 구현해주셨네요!

제가 구현했다면 스피너를 css animation으로 구현했을 것 같은데 rAF(requestAnimationFrame)으로 구현해주셨네요!! rAF를 이해하고 사용하기위한 위치로서 제일 적당한 위치라고 생각되는데 잘 구현해주셨어요!

rAF를 왜 사용하는게 repaint에 효율적인지에 대한 부분은 조금 생각해보면 좋을 것 같은데요. 왜 사용하는지에 대해 부연설명을 드리면 좋을 것 같아 조금 적어보겠습니다 ㅎㅎ

게임을 할때 FPS(Frame Per Second)라는 단어를 많이 사용하는데 1초동안 몇번의 화면을 보여주는지를 알려주는 단어인데 이 FPS가 낮아지게 되면 사용자 입장에서 부드러워야 할 화면이 뚝 끊기게 되어 사용자 입장에서 불편하게 느껴집니다. 이런 부분을 해소하기위해 rAF가 나왔습니다. rAF는 사용자가 이러한 끊김을 느끼지 않도록 1프레임마다 호출을 보장해주는 web api입니다. 디스플레이의 환경에 맞게 최적의 빈도로 실행을 해줘서 사용자 경험을 챙겨줄 수 있습니다.

setTimeout, setInterval로 구현해도 되는거 아니냐! 라고 할수도 있겠지만 rAF는 앞의 set~ api와는 동작하는 방식이 다르기 때문에 이러한 차이가 있습니다. 자바스크립트는 기본적으로 싱글스레드로 동작하는데 setTimeout, setInerval api는 지정해준 시간이 지나면 해당 콜백함수들을 task queue에 넣어주는데요~ 이전에 큐에 쌓여있던 함수에서 지연될 가능성을 배제하지 못하기 때문에 지정해준 타이머에 명확히 실행해준다는 보증이 없습니다. 그래비해 rAF는 Animation frames라는 브라우저 렌더링과 관련된 task를 별도로 처리하는 queue를 통해 실행시점을 보증할 수 있습니다.

rAF에 엮여있는 공부할 부분이 많아서 따라가며 공부해보는 것도 많은 도움이 될 것 같습니다.
브라우저의 렌더링 과정(+ reflow, repaint), 자바스크립트의 동작방식(이벤트 루프), 큐의 우선순위(Micro Task Queue, Animation Frames, Task Queue)가 묶여있는 영역이다 보니 천천히 살펴보시면 좋을 것 같네요 :)

테코블에도 이벤트 루프 관련한 글이 있네요!

추가로 구현해주신 부분에서 유저가 자동차 이름이나 횟수를 입력하면 해당 순서에 맞게 입력창과 버튼을 disabled 시켜주면 어떨까요?

현재는 횟수를 입력하는 프로세스까지 가면 버튼만 비활성화되게 되어있는데 스타일상으로도 더이상 누를 수 없다는걸 유저에게 직관적으로 알려줄 수 있으면 더 좋을 것 같고 해당 버튼이 비활성화 되었듯이 입력창도 더이상 입력할 수 없게 disabled 속성을 추가해주면 좋을 것 같네요!

개선하면 좋을 부분이나 버그가 있는 부분에 대한 코멘트를 아래에 남겼는데 확인해주세요!~

PR 커밋마다 어떤식으로 코드를 변경했고 그 근거를 꼼꼼히 작성해주셔서 정말 좋았어요 🥇
미션 시간이 촉박했을텐데 구현하시느라 정말 고생하셨습니다~

EungyuCho · 2022-02-19T00:02:38Z

+    const spinnerArray = Array.from(this.spinners);
+    spinnerArray.forEach((spinner) => {
+      spinner.remove();
+      cancelAnimationFrame(this.animationId);


cancleAnimationFrame는 한번만 실행되어도 괜찮을 것 같네요!

반영했습니다!

EungyuCho · 2022-02-19T00:15:22Z

+import CarManager from './carManager.js';
 import RacingCarGameView from './view.js';

 class RacingCarGame {


네이밍이 RacingCarGameController가 더 맞는 것 같네요!

폴더 분리가 안되어있어 모델로 착각할수도 있겠다 생각이 드는데 model, controller, view 폴더로 분리해보는건 어떨까요? 어플리케이션이 커지게 되고나서 분리해도 괜찮겠지만 미리 폴더 구조를 잡고 시작하는게 일반적이라 분리해보는걸 추천드려요!

car model에서 private를 적용하신김에 해당 controller, view에서도 메서드를 은닉해보는건 어떨까요?

Controller로 수정해서 반영했습니다! 폴더도 분리했습니다.
그 중에서 carManager가 모델을 관리하는 부분이라서 models 폴더에 추가했는데 올바른 구분일까요?

private을 사용해서 각 클래스의 시작 매서드를 은닉했습니다.
아직 이 부분에 있어서 모르는 것이 많은데, 혹시 은닉할 지 말 지를 판단할 수 있는 기준이 무엇이 있을까요?
지금은 생성 매서드 정도를 은닉했는데, 아마 지금 존재하는 매서드 중에서 많은 부분들이 외부에서 접근할 필요가 없는 경우가 많을 것 같아서 이들도 모두 숨기는 것이 맞는지 궁금합니다.

carManager가 모델을 관리하는 부분이라서 models 폴더에 추가했는데 올바른 구분일까요?

모델에 포함되어도 되겠습니다.
MVC패턴에서 모델은 화면에 나타낼 상태와 해당 상태를 관리하는 함수가 포함되면 모델이라고 봐주시면 됩니다!

은닉할 지 말 지를 판단할 수 있는 기준이 무엇이 있을까요? 지금은 생성 매서드 정도를 은닉했는데, 아마 지금 존재하는 매서드 중에서 많은 부분들이 외부에서 접근할 필요가 없는 경우가 많을 것 같아서 이들도 모두 숨기는 것이 맞는지 궁금합니다.

말씀하신대로 외부에서 노출될 필요가 없는 함수나 상태를 숨기기 위해 코멘트를 드렸습니다~
객체지향 패턴에 정의되어있는 정보 은닉의 원칙에 따르면 외부로 노출될 필요가 없는 정보는 숨기는걸 권장하고있기에 드린 코멘트였습니다.

클래스 기반으로 작성을 하다보니 OOP에 대해 공부해보시면 좋을 것 같아요!

코드를 보면서 궁금했던 부분인데 혹시 app과 lib 파일구조로 나누신 부분은 어떤 구조를 참고하신지 알 수 있을까요?? 혹시 모바일 app관련 폴더 구조일까요?

말씀해 주신 내용을 참고해서 은닉시킬 수 있는 매서드는 모두 은닉했습니다!

코드를 보면서 궁금했던 부분인데 혹시 app과 lib 파일구조로 나누신 부분은 어떤 구조를 참고하신지 알 수 있을까요?? 혹시 모바일 app관련 폴더 구조일까요?

사실 저는 우테코를 준비하는 과정에서 MVC 패턴에 대해서 처음 공부하게 되었는데요..
개념 자체는 이해했지만 가장 어려운 부분 중에 하나가 디렉토리 구조인 것 같습니다!

지금의 폴더 구조는 페어 프로그래밍을 할 때, 페어의 경험을 살려서 구현했습니다.
저는 앱 자체와 그 외 참고할 요소들을 분리하는 것으로 이해하고 수용했습니다.

혹시 이 부분에 있어서 참고할 수 있을 만한 기준이 있을까요?
다른 부분보다 디렉토리 구조에 있어서는 원하는 만큼 시원한 답을 못찾았네요 ㅜㅜ

혹시 이 부분에 있어서 참고할 수 있을 만한 기준이 있을까요?

아하.. 앱 프로젝트에서 몇번 본 폴더구조라 왠지 앱을 개발해보신 것 같아 드린 코멘트였는데 착각했네요 💦
디렉터리 구조는 의견도 많이 갈리는 부분이라 새로 프로젝트를 시작해도 어떻게 디렉터리 구조를 가져갈지 정말 많이 논의하는 부분이기때문에 쉽지않은 것 같아요. 🤔

현재 프로젝트 기준에서는 model, controller, view가 racing에 관련된 각각의 요소만 있으면 되기때문에 model/view/controller 폴더를 src하위에 생성해도 무방하다고 생각되지만 추가로 점수가 추가되고 랭킹시스템 대시보드 등 요구사항이 추가될수록 원하는 기능을 유지보수하기위한 코스트가 늘어난다고 생각합니다 😓

그래서 현재 상태보다 더 어플리케이션이 고도화 될 여지가 있다고 판단되면 폴더를 기준으로 나눠보는 것도 괜찮다고 생각되네요.
관련해서 mvc를 담아두는 폴더 구조 자체는 이 저장소를 참고해보시면 어떨까 싶어요!

이렇게 mvc를 규모있게 작성해본적이 없어 판단이 명확하진 않지만 제가 구현했다면 아마 mvc폴더 하위에 PostComments나 SinglePost등 관련 폴더들을 내려줄 것 같아요.

한번 확인해보시고 어떻게 생각하시는지 답변 부탁드립니다~

조금 코멘트가 늦기는 했지만 나름대로 제 생각을 정리해보겠습니다!
(머지가 끝났는데도 이 코멘트를 보실지는 모르겠네요 ㅎㅎ)

우선 말씀 해주신대로 저도 만약에 이 상태에서 개선을 한다면 mvc 폴더를 만들고 그 하위 수준에 기능 단위 폴더들을 내려줄 것 같습니다! 컴포넌트 폴더와 같은 수준에 기능별 폴더가 같이 있는 것이 어색하다고 느껴집니다.

근데 이왕 고민해보는 김에 좀 더 창의성을 발휘해서, 만약에 기능 단위로 구분해서 디렉토리를 구성하는 것이 아니라 역할 단위로, 그러니까 컨트롤러끼리, 뷰끼리, 모델끼리 묶어서 구성한다면? 이라는 가정을 해봤습니다. 분명 앱의 규모가 커지면, 동일한 기능 단위에서 공유하는 공통점보다 같은 역할 군에서 코드를 공유한다거나 패턴이 생겨서 파일로 묶어 추상화하거나 재사용하는 경우가 생길 것 같은데 그럴 때는 M끼리, V끼리, C끼리 묶는 것이 효과적일 때도 있지 않을까요? 혹시 이렇게 사용하는 것이 지양될 만한 이유가 있을까요?

머지가 끝났는데도 이 코멘트를 보실지는 모르겠네요 ㅎㅎ

모든 코멘트가 네이버 메일로 오고있어서 언제든 코멘트는 환영입니다!~ (슬랙 메세지로도 주셔도 괜찮아요!)

혹시 이렇게 사용하는 것이 지양될 만한 이유가 있을까요?

그렇게 사용하셔도 무방합니다~! 우선 제가 전 코멘트에 제가 말씀드리고자 하는바가 정확히 전달되었는지 모르겠네요 💦
'기능'이 관점이 아니라 '페이지'가 관점이었는데 의도가 잘못 전달되었을수도 있다고 생각이 드네요 😢

프로젝트 구조에 답은 없다고 생각합니다. 특정 폴더패턴에 묶이는 것 보다는 블링님이 생각하신대로 페이지 내부의 독립적인 작업보다 역할간의 의존성이 많을거라고 생각하시는 경우에는 말씀하신대로 역할단위로 프로젝트 구조를 설정해도 무방하다고 생각됩니다. 😄
추상화하는데 폴더별로 구분하는 것 보다 효율적이라고 판단되네요!! 좋은 생각이신 것 같습니다.

제가 위처럼 폴더단위로 페이지를 구성하는데 말씀을 드린 이유에 살을 붙혀보자면 보통 유지보수를 진행할 때 페이지단위로 개발을 하게되는데 대부분은 역할간의 상태나 모델의 데이터를 참고하기보다 해당 페이지에 맞는 모델의 상태나 뷰를 수정하는 상황이 많았기때문에 해당 코멘트를 드렸습니다.(처음 역할을 역할(모듈)별로 나누는게 편하다고 생각하게 된 계기는 특화된 NestJS의 시스템에서 영감을 받았습니다)

공유되는 데이터의 경우 대부분 공통적인 상태관리 라이브러리를 사용하기 때문에 아직까진 프로젝트 설정 시 페이지단위 폴더구성을 자주 사용하는 편입니다.

정말 깊게 생각해주셨네요! 👍 👍

EungyuCho · 2022-02-19T00:20:57Z

-      }
+    const moved = [];
+    this.carManager.cars.forEach((car) => {
+      const carMoved = RacingCarGame.processCarRandomMove(car);


isCarMoved는 어떨까요?
저는 true / false를 명확히 구분하는 변수라는 걸 인지시키기 위해 앞에 is 접두사를 붙이는걸 선호하는데 어떻게 생각하시는지 궁금합니다~

해당 변수가 자동차를 함수에 넣고 랜덤 움직이기를 실행한 뒤에 움직인 여부를 반환 받는 값이라는 의미로
isMovedCar로 수정했습니다!

EungyuCho · 2022-02-19T00:53:10Z

+  }
+
+  static isValidCarNameInput(names) {
+    return names.some((name) => checkStringInRange(name, CAR_NAME_MIN_LENGTH, CAR_NAME_MAX_LENGTH));


조건식이 잘못된 것 같은데 every를 사용해보는건 어떨까요?

테스트코드를 너무 신뢰하다보면 이런 사소한 부분을 놓칠 수 있는데 테스트코드에서 5자 이하의 자동차도 함께 input에 넣어줬다면 테스트가 실패하면서 조기에 발견할 수 있었을 것 같네요 😅

테스트코드도 이에 맞춰서 바꿔보면 좋을 것 같아요.

혹시 해당 부분을 고치면서 every에서 lint에러가 발생하시면 2가지 해결방법이 떠오르는데 첫번째는 해당 lint rule을 꺼버리는 방법(해당 줄만 disabled시키거나 lintrc에서 rules 수정)과 함수의 분리로 해결하시면 되겠습니다~

수정해서 반영했습니다!
코멘트 남겨주신 것처럼 테스트 케이스가 세세하지 못해서 아마 이런 부분을 잡아내지 못한 것 같습니다 😥..

그래서 생긴 궁금증인데,
동일한 로직으로 테스트하고 싶은 케이스가 아주 많다면 어떤 식으로 테스트 코드를 구현하는 게 가장 적절할까요?

예를 들자면 '잘못된 이름'을 테스트 하는 케이스가 경계 값을 기준으로 4개 (이름이 각각 0, 1, 5, 6자 일 때), 그리고 위와 같은 케이스를 체크하기 위해서 잘못된 이름이 하나 포함 되어있을 때, 잘못된 이름으로만 되어있을 때 등 분리해서 사용하고 싶은데 그렇게만 해도 이름 값만 다르게 추가하는 테스트가 6개가 되었습니다.
이들을 각각 테스트로 분리하자니 너무 세세한 테스트를 개별적으로 실행하는 느낌이 들었고,
하나의 테스트에서 다 확인하면 테스트 코드지만 함수가 여러가지 작업을 하는 게 옳지 않은 것 같고
또 어떤 오류가 발생하는지 정확히 판단하기 어려울 것 같아서 고민입니다.

조금 복잡해 보이더라도 각각 테스트를 개별적으로 작성하는 게 맞을까요?

수정해서 반영했습니다! 코멘트 남겨주신 것처럼 테스트 케이스가 세세하지 못해서 아마 이런 부분을 잡아내지 못한 것 같습니다 😥..

그래서 생긴 궁금증인데, 동일한 로직으로 테스트하고 싶은 케이스가 아주 많다면 어떤 식으로 테스트 코드를 구현하는 게 가장 적절할까요?

예를 들자면 '잘못된 이름'을 테스트 하는 케이스가 경계 값을 기준으로 4개 (이름이 각각 0, 1, 5, 6자 일 때), 그리고 위와 같은 케이스를 체크하기 위해서 잘못된 이름이 하나 포함 되어있을 때, 잘못된 이름으로만 되어있을 때 등 분리해서 사용하고 싶은데 그렇게만 해도 이름 값만 다르게 추가하는 테스트가 6개가 되었습니다. 이들을 각각 테스트로 분리하자니 너무 세세한 테스트를 개별적으로 실행하는 느낌이 들었고, 하나의 테스트에서 다 확인하면 테스트 코드지만 함수가 여러가지 작업을 하는 게 옳지 않은 것 같고 또 어떤 오류가 발생하는지 정확히 판단하기 어려울 것 같아서 고민입니다.

조금 복잡해 보이더라도 각각 테스트를 개별적으로 작성하는 게 맞을까요?

실제로는 테스트 범위를 어디까지 잡을것인지에 대한 개발자끼리의 협의에 따라 다르겠지만 해당 미션을 제가 구현했다면 이런 조건을 기준으로 구현했을 것 같습니다.

잘못된 이름이 포함된 경우 에러가 발생한다. (정상적인 입력 + 5자리를 초과하는 입력)

이름이 비어있을 경우 에러가 발생한다. (빈 입력)

입력 케이스가 정상일 때 (해당 정상 입력 이후에 프로세스가 있다면 생략할 수 있음)

어디까지 테스트할 것인가는 항상 고민되는 문제네요 ㅎㅎ;
최대한 케이스를 줄이면서도 예상치 못한 입력을 검증할 수 있는 방향성을 적절히 찾아가는 과정이 쉽지 않은 것 같습니다 😓
그래서 최근에는 프로젝트의 기획을 할 때 입력에 대한 예상범위를 산정하는 것 또한 기획의 범위 안이라고 생각하고 있습니다. 그래야 테스트가 용이하고 견고한 어플리케이션이 만들어진다고 생각하고 있습니다.

코멘트를 달다보니 말이 조금 딴길로 샌 것 같은데 저는 개별적으로 작성하는 편을 선호하고 해당 테스트 케이스가 너무 길어진다면 별도의 테스트 파일로 분리하는걸 추천드립니다!~

다행히 지금 제가 작성한 테스트의 범위가 리뷰어님이 기준으로 생각하신 부분과 잘 맞는 것 같습니다!
앞으로는 개발 계획 단계에서 이런 부분들을 어떤 기준을 잡고 갈 것인지 조금 더 고민해보겠습니다.

EungyuCho · 2022-02-19T01:00:08Z


 export const pickNumberInRange = (min, max) => Math.floor(Math.random() * (max - min) + min);

+export const createDivWithClass = (className) => {


createDivWIthClassName 네이밍은 어떻게 생각하실까요?
제 생각에는 최대한 구체적으로 적는편이 좋을 것 같아요

반영했습니다!
사실 함수명의 길이가 너무 길어져서 전체적으로 코드 스타일이 망가지는 바람에
요리조리 함수명을 변경 시키다가 누락된 것 같습니다.
Name이 들어간게 훨씬 명확하다고 생각해서 반영했습니다!

EungyuCho · 2022-02-19T01:00:34Z

+  rotateSpinner = () => {
+    this.spinners.forEach((spinner) => {
+      spinner.style.transform = `rotate(${this.currentDegrees % 360}deg)`;
+    });
+
+    this.currentDegrees += 10;
+    this.animationId = requestAnimationFrame(this.rotateSpinner);
+  };


rAF로 구현해주셨네요!~ 👍

EungyuCho · 2022-02-19T01:07:54Z

+      if (moved.includes(container.dataset.carId)) {
+        container.append(RacingCarGameView.generateStepTemplate());
+      }


innerHTML => Elemet기반 append로 변경해주셔서 화살표를 렌더링하는 부분이 효율적으로 개선되었네요! 💯

EungyuCho · 2022-02-19T01:30:10Z

    });
+    this.finishedCount += 1;
+    this.view.renderRoundResult(moved, this.count, this.finishedCount);
+    if (this.finishedCount === this.count) {


count가 범용적인 변수다보니 게임 횟수에 관련된 네이밍을 변경해보면 어떨까요?
round를 쓰는게 어떨지 추천드리고 싶었는데 차가 전진하는 개념이라면 round보다는 turn이 맞을 것 같은데 블링님의 생각도 궁금하네요 ㅎㅎ

count, round 등 변수명이 전체적으로 헷갈리는 것 같아서 아래처럼 정리해서 통일해 봤습니다.

사용자가 입력한 값은 count, countInput으로 통일

controller에 저장해서 실행 횟수로 사용할 부분은 round로 통일

입력한 count를 totalRounds로 저장

한 round 실행할 때마다 finishedRound를 업데이트

이렇게 바꾸면 좀 더 의미가 명확해질 것 같은데 어떻게 생각하시나요?

(turn 대신 round를 사용한 이유는 자동차들이 번갈아서 자신의 순서에 따라 움직이기 보다는
한 번의 라운드에 각각 자동차가 갈 지 말 지가 전체적으로 결정된다는 생각 때문이었습니다.
그러니까 의미적으로는 각 자동차의 순차 진행보다는 라운드 별 동시 진행으로 생각했습니다!)

count, round 등 변수명이 전체적으로 헷갈리는 것 같아서 아래처럼 정리해서 통일해 봤습니다.

사용자가 입력한 값은 count, countInput으로 통일

controller에 저장해서 실행 횟수로 사용할 부분은 round로 통일

입력한 count를 totalRounds로 저장

한 round 실행할 때마다 finishedRound를 업데이트

이렇게 바꾸면 좀 더 의미가 명확해질 것 같은데 어떻게 생각하시나요?

(turn 대신 round를 사용한 이유는 자동차들이 번갈아서 자신의 순서에 따라 움직이기 보다는 한 번의 라운드에 각각 자동차가 갈 지 말 지가 전체적으로 결정된다는 생각 때문이었습니다. 그러니까 의미적으로는 각 자동차의 순차 진행보다는 라운드 별 동시 진행으로 생각했습니다!)

그렇네요!
원래 round로 통일하는 방향을 추천드리려 했는데 입력과 모델의 변수명을 별도로 분리해주는게 더 깔끔해보이네요.
지금 상태가 명확해보입니다~

1. createDivWithClass => createDivWithClassName 2. count, round 등 게임 실행 횟수에 관련된 명칭 - 사용자가 입력한 값은 count, countInput으로 통일 - controller에 저장해서 실행 횟수로 사용할 부분은 round로 통일 - 입력한 count를 totalRounds로 저장 - 한 round 실행할 때마다 finishedRound를 업데이트 3. 자동차의 이동 관련 명칭 수정 - 한 round에서 이동한 자동차의 배열은 moved => movedCars - 한 자동차가 해당 round에서 이동했는지 여부는 carMoved => isMovedCar

원인 - 검증 과정에서 모든 이름을 체크할 때 every 대신 some 사용 - 이름들 사이에 조건을 만족하는 이름이 포함될 경우 나머지의 만족 여부 관계없이 통과됨 해결 - every로 변경, 모든 이름이 조건에 만족해야함 - 해당 내용을 테스트 할 수 있도록 테스트 케이스 수정

- 게임시작 버튼을 분리해서 시도 횟수 입력 후 바로 게임 시작 대신 게임 시작 버튼 클릭 시 시작하도록 변경 - 각 입력 값 입력 시 입력창과 버튼을 비활성화 - 비활성화 된 요소 스타일 추가로 활성/비활성 구분 명확하게 변경 - html 요소를 숨기고 나타내는 로직 변경 - 기존에는 view에서 스타일 직접 추가 - html에 hide라는 클래스를 추가해 숨기고, 표시하기 위해서는 view에서 해당 클래스를 제거 - 위 변경에 따른 테스트 케이스 수정

uk960214 · 2022-02-19T06:10:54Z

코멘트로 남겨주신 부분에 disabled 처리와 관련된 부분을 반영하고 좀 더 추가적으로 수정해봤습니다.

입력을 한 후에 각각의 입력창, 버튼을 비활성화 처리하고 버튼의 텍스트도 바꿔주었습니다.
그리고 마지막으로 횟수를 입력한 후에 바로 게임이 진행되는 것이 아니라 입력 필드 하단에 게임 진행 버튼을 추가해서
이 버튼을 눌러야지만 게임이 시작되도록 변경했습니다.
해당 버튼을 클릭하면 게임이 시작되며 버튼은 비활성화 되고 게임 진행 상황에 따라서 텍스트는 변경되도록 구현했습니다!

EungyuCho

블링님 코멘트를 빠르게 잘 반영해주셨네요!

블링님의 생각과 제 생각을 비교해볼 수 있는 부분이 많아서 좋았습니다 😄
궁금한점과 컨벤션관련하여 답글과 코멘트를 달아봤습니다~

확인해주시고 코멘트� 부탁드리겠습니다~

EungyuCho · 2022-02-19T08:17:29Z

+    this.#initSpinner();
+  }
+
+  renderRoundResult(movedCars, totalRounds, finishedRounds) {


Suggested change

renderRoundResult(movedCars, totalRounds, finishedRounds) {

renderRoundResult(movedCars, totalRounds, currentRound) {

현재 라운드가 더 맞는 네이밍인 것 같은데 어떨까요?

이 부분을 더 자세히 들여다 보니 근본적으로 리팩터링 할 수 있는 방법이 생각나서 아예 수정했습니다.

지금 저 두 인자를 받아서 하는 것이 두 가지가 동일한 지를 비교하는 게 전부인데
굳이 두 인자를 받아올 필요가 없다는 걸 발견했습니다.
그래서 애초에 컨트롤러 부분에서 뷰로 넘어올 때
두 변수를 비교해서 게임이 종료되었는지 여부만 넘겨주도록 수정했습니다.

// 정의 부분 renderRoundResult(movedCars, isGameOver) { ... if (isGameOver) { this.#removeSpinner(); } } // 호출 부분 const isGameOver = this.totalRounds === this.finishedRounds; this.view.renderRoundResult(movedCars, isGameOver);

EungyuCho · 2022-02-19T08:19:28Z

+    this.carNameInputField.addEventListener('click', this.onCarNameInputFieldClick);
+    this.countInputField.addEventListener('click', this.onCountInputFieldClick);
+    this.gameStartBtn.addEventListener('click', this.onGameStartBtnClick);


추후에 이벤트 위임에 대해서도 한번 알아보시면 좋겠네요!

위 두 가지만 있었을 때는 그렇게까지 눈에 띄지 않았는데 게임 시작 버튼이 추가 되니
말씀해주신대로 이벤트 위임에 대해서 고려할 필요가 있다고 생각됐습니다.

제가 아는 선에서 최대한으로 위임할 수 있도록 수정해보았습니다!

EungyuCho · 2022-02-19T08:29:08Z

 export const isNumberBelowZero = (number) => number <= 0;

-export const checkStringLengthOver = (str, standard) => str.length <= standard;
+export const checkStringInRange = (str, min, max) => str.length >= min && str.length <= max;


인자를 객체형태로 바꿔보는건 어떨까요?
저는 인자가 2개가 넘어가면 객체형태로 받는편입니다 :)
참고

막연하게 '너무 길다' 생각이 들 때만 객체로 받아왔는데 2개가 넘어가면이라는 통용되는 기준이 있었군요!
반영해서 수정했습니다. 앞으로도 남겨주신 참고자료 열심히 참고해보겠습니다!

EungyuCho

피드백을 굉장히 빠르게 반영해주셨네요!
이벤트 위임까지 이번에 반영해주실 줄 몰랐는데 정말 깔끔하네요~
완성도 높게 최종 스텝까지 완성된 것 같습니다 😄

다음 미션 진행까지 지금까지 드린 코멘트에 있는 키워드에 대해 알아보거나 자료들을 한번 훝어보는걸 추천드릴게요! 분명 블링님께 장기적으로 도움이 될거라고 생각해요. 💡

다음 미션 진행도 응원하겠습니다! 다음 작성하실 코드가 기대되네요!
자동차 미션 진행하시느라 고생하셨습니다~

uk960214 added 21 commits February 14, 2022 15:35

refactor: car 객체의 init 매서드 은닉

dfe4e3b

https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Classes/Private_class_fields

docs: 2단계 구현 기능 목록 작성

64e0814

feat(spinner): 로딩 시 스피너 마크업과 컨트롤 할 뷰 함수 작성 (임시)

360a154

feat(roundDelay): 라운드 진행 시간 간격 추가

d747d30

- 각 라운드가 1초간 간격을 가지고 실행되도록 설정 - 입력한 값 만큼 라운드가 진행되면 실행 중지하고 결과 표시

docs(README): 테스트 작성 완료 처리

eb56323

docs(README): 완료된 구현 기능 목록 체크

77cb0d1

feat(winnerAlert): 게임 종료 2초 뒤 alert 표시 구현, 테스트 수정

aec19b7

test: 시간 관리 방법을 wait에서 tick으로 변경

b014ef1

refactor: 게임 클래스 실행을 동적할당에서 매서드 호출로 변경

cf579e1

fix: html 언어 설정 오타 수정

e977cbd

refactor: innerHTML 제거, append로 수정

7ae4a81

refactor: 테스트 리팩터링

a363f25

refactor: 상수명 변경, 테스트 코멘트 위치 변경

0e3fa40

refactor: step을 담는 container 별도로 추가

a27a567

pocojang requested a review from EungyuCho February 18, 2022 11:26

pocojang assigned EungyuCho Feb 18, 2022

EungyuCho suggested changes Feb 19, 2022

View reviewed changes

uk960214 added 6 commits February 19, 2022 12:42

fix: cancelAnimationFrame 실행 위치 변경

a1f533c

refactor: 뷰와 컨트롤러에서 매서드 은닉

6f42b01

refactor: MVC에 따라 폴더 재구조화, 클래스 명칭 수정

b78df93

uk960214 added 2 commits February 19, 2022 14:44

refactor: 버튼 문구 상수 분리

a3737cb

style: 버튼 스타일 추가

e2deb52

EungyuCho suggested changes Feb 19, 2022

View reviewed changes

uk960214 added 5 commits February 19, 2022 22:24

refactor: util 함수 인자를 객체로 변경

362739f

refactor: 라운드 결과 렌더 함수 인자 정리

b556d64

refactor: inputField에 이벤트 위임

7b09cc2

refactor: 외부로 노출이 불필요한 매서드를 은닉

ee851d6

refactor: document에서 요소 찾는 케이스 최소화

108fe95

EungyuCho approved these changes Feb 19, 2022

View reviewed changes

EungyuCho merged commit 17498c6 into woowacourse:uk960214 Feb 19, 2022


		export const pickNumberInRange = (min, max) => Math.floor(Math.random() * (max - min) + min);

		export const createDivWithClass = (className) => {

	renderRoundResult(movedCars, totalRounds, finishedRounds) {
	renderRoundResult(movedCars, totalRounds, currentRound) {

Conversation

uk960214 commented Feb 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EungyuCho left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uk960214 Feb 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uk960214 commented Feb 19, 2022

Uh oh!

EungyuCho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

uk960214 commented Feb 18, 2022 •

edited

Loading

EungyuCho left a comment •

edited

Loading

uk960214 Feb 19, 2022 •

edited

Loading