[1단계 - 자동차 경주 구현] 인치(한창희) 미션 제출합니다.

[hchayan]

자동차 게임 미션 1단계

한줄 코멘트

첫 코드리뷰입니다. 잘부탁드립니다.

구현하며 확신이 없는 부분을 모험적으로 시도해봤습니다!

평소와 다르게 진행한 부분

• valid 부분을 모듈화해서 세분화 했습니다.
• 테스트 코드를 입력 유효성 테스트 부분을 describe으로 나눴습니다.
• 게임 진행 일련의 과정을 테스트 it 코드의 흐름으로 테스트 했습니다.
• 클래스를 내보낼때, 인스턴스를 내보내는 형태로 구현했습니다. export const validator = new Validator();

[Vallista]

안녕하세요~ 코드 리뷰를 맡게 된 Vallista 입니다!

잘 부탁 드립니다 😄

[Vallista]

인치님!

먼저, 1단계 - 자동차 경주 구현 수고 많으셨습니다!
디테일한 내용은 라인마다 적어놓았습니다.

전반적인 코드 리뷰는 아래와 같습니다!

1. export 단위로 동적할당을 진행한 부분들이 있었습니다. Template과 같은 코드의 부분은 공개함수를 추출하는 느낌이라 좋기도 했으나, 이렇게 되면 객체로 생성하는 의미가 없어보입니다. 객체로 생성하는 이유는 캡슐화나 은닉화를 통해 외장 객체의 객체단위의 커뮤니케이션을 기대하는 의미가 있습니다. 그런데, 이렇게 구현하게 되면 파일 안에서만 객체단위의 커뮤니케이션이 이루어질 수 있습니다. 그렇다는 것은 굳이 class가 아니라, function으로 만들고 function을 export만 해도 된다는 이야기 입니다.

2. export 시 객체를 생성하는 것의 또 다른 문제는, 객체 생성 시점이 코드를 불어들이는 시점과 동일하다는 것 입니다. 그렇다면 전혀 사용하지도 않을 때 메모리 점유를 계속 하고 있다는 것 입니다. 일반적으로 메모리 할당의 영역은, member 변수나 scope 안의 휘발성 변수가 stack 영역에 쌓이고, 동적할당과 같은 경우 heap에 쌓아서 메모리를 유용하게 쓰는데 초점을 맞춥니다. (물론 브라우저에서는 이런 메모리 관리를 가비지 컬렉터나 heap, stack 메모리 관리를 알아서 해주지만요) export로 가져오게 되면, countSelectionTemplate을 가져오는 import 단계에서 파일이 쓰이고 있는 경우 계속 메모리를 갖게되므로 메모리 해제 시점이나 메모리 생성 시점이 비효율적이게 됩니다. 즉 이미 메모리 풀을 크게 들고 있다는 의미이죠. 이러한 자그마한 소프트웨어의 경우 큰 문제는 없겠으나. 커다란 대형 어플리케이션에서는 싱글톤이나 멀티턴 패턴을 제외하고는 이러한 흐름으로 코딩을 작성하는건 별로 좋지 못한 패턴으로 보입니다.

3. 전반적으로 MVC 패턴을 잘 구분해서 사용하신 것 같아요 👍 디테일하게 적어둔 대로 model을 조금 더 고도화를 진행해도 괜찮을 것 같아요! 단순한 setter 보다는 행동단위의 메소드를 만들면 좋을 것 같습니다.

4. 다른 코드들은 정말 잘 나눠주셔서 좋았습니다 👍 최고!

코드를 크게 수정할 부분은 없어서 Approve 하겠습니다!
위에 말씀드린 부분을 한번쯤 고민 해보시면 좋을 것 같아요!

[Vallista]

입력 테스트 (비즈니스), 레이아웃 테스트를 나눈 것은 아주 센스있는 코딩인 것 같습니다 😄 👍 👍

[Vallista]

#***-input을 입력하고 #***-btn을 클릭하는 동작이 중복되는 것 같습니다 😄
해당 일련된 동작을 제공하는 함수를 만들어보는 건 어떨까요?

[Vallista]

Input -> btn -> should 와 같은 패턴이 많이 보이네요~ 이런 것도 함수로 제공하면 좋을 것 같습니다 :)
아래와 같이 짤 수도 있겠네요!

class CypressWrapper {
  type = (name, value) => {
    try {
       this._getCy(name).type(value)
    } catch (err) {
      new Error(err)
    }

    return this
  }

  click = (name, params) {
    try {
      this._getCy(name).click(params)
    } catch (err) { 
      new Error(err)
    }
    return this
  }

  should(name, ...param) {
    try {
      this._getCy(name).should(...param)
    } catch(err) {
      new Error(err)
    }

    return this
  }

  _getCy(name) {
    return cy.get(name)
  }
}

const cw = new CypressWrapper()

function testCar() {
  cw
    .type('#car-input', 'a,b,c,d')
    .click('#car-btn')
    .should('@count', 'have.css', 'display', 'block');
}

describe('ui-input-vaild-check', () => {
  ...
  it("입력한 시도 횟수가 소수면 alert 출력", () => {
    testCar()
    cw
      .type('#count-input', 4.21)
      .click('#count-btn')
      .should('@alertStub', 'be.calledWith', '시도 횟수는 소수가 될 수 없습니다.')
  })
})

[Vallista]

cypress는 빌드에 연관있는 툴은 아니므로, --dev 옵션을 주어 devDependencies로 이관하는게 좋지 않을까요?

[Vallista]

상수를 별도의 파일에 저장하는 기법은 아주 좋은 방법입니다 👍 👍

[Vallista]

RacingCar의 init 함수를 만들어서

const racingCarGame = new RacingCar()
racingCarGame.init()

을 하는게 더 좋아보입니다.
new RacingCar() 만 하고 끝내면 추후 메모리 해제를 동적으로 한다던가.. 내부의 변수나 함수를 사용한다던가 하는 범용적인 행위에서 자유롭지 못하므로, 생성만 하는 행위는 지향하는게 좋습니다 😄

[Vallista]

여기도 동일합니다. 생성만 하고 대입을 하지 않는건 별로 좋지 못한 코딩 방식입니다.

[Vallista]

그리고, RacingCar 클래스가 하는 일이 없고 객체만 갖고 있어서 굳이 만들 필요는 없어보입니다 :)
위의 설명처럼 RacingCar의 init 함수를 만들고,
init 함수에서 멤버변수로 받은 controller = RacingCarController() 를 실행하고,
index.js에서 사용하는 형태가 더 올바르게 보입니다.

[hchayan]

안녕하세요 리뷰어님! 꼼꼼하게 리뷰해주셔서 감사합니다 ㅠㅠ
다름이 아니라 이부분에 대해서 궁금증이 생겨서 추가 질문 드립니다.

말씀 해주셔다 싶이 controller와 index.js 사이의 불필요한 클래스 파일을 제거했고 아래와 같이 코드를 변경했습니다.

// index.js
import RacingCar from "./racingcar/controller.js";

const racingCar = new RacingCar();
racingCar.init();

이때 드는 의문점이 controller.js 에서 constructor 내에 있는 내용들을 init 메서드를 만들어 옮겨 주는게 맞나요?

// controller.js
// 기존
class RacingCarController {
  constructor() {
    this.model = new RacingCarModel();
    this.view = new RacingCarView();
    this.handleCars();
  }
  ...

// 이후
class RacingCarController {
  constructor() {}     
  
  init() {
    this.model = new RacingCarModel();
    this.view = new RacingCarView();
    this.handleCars();
  }

만약 아니면 index.js 파일에서 아래와 같이 생성자를 변수 할당할 필요가 없지 않나 싶은 의문을 가지게 됩니다.

// 기존
const racingCar = new RacingCar();           // racingCar 변수를 사용해줄 필요없이 이미 생성됨


// 이후
new RacingCar();  

[Vallista]

네넵 init의 분리가 필요 없다면, constructor에 단순히 담는것도 좋아보입니다.
다만, 객체를 할당하고 안의 메서드를 사용하는 목적이 아니라면 단순 함수로 표기해도 되어 보입니다.

class로 사용하는 목적이 없어보이는데 어떻게 생각하시나요~?

[hchayan]

네 이번 프로젝트와 같이 index.js에서 관리해줘야 할 controller가 하나만 있을때는 클래스 없이 단순히 객체를 할당하는 방식이 가장 적당한것 같습니다.

그래도 이번 미션에서 연습으로
controller에 init 메서드를 만들어줘서 해당 메서드에서 handler를 호출해주는 방식으로 개선했습니다!

[Vallista]

getCarsInput() {
}

요렇게 getter를 만드는 것도 좋지만,

get CarsInput() {
}

형태로 만드는 ES6 문법 방식도 있습니다 :)

[Vallista]

document.querySelector('blahblah').value를 사용하는 패턴이 많이 보이네요. 이러한 패턴은 util로 빼서 관리해도 좋아보입니다.
그리고 그 안에서 해당 dom object가 있는지에 대한 여부도 함께 예외처리를 진행하시면 좋을 것 같습니다.

[Vallista]

모델에서 setter는 별로 사용하지 않는게 좋습니다. 이렇게 되면 사실상 this.cars에 직접 접근하는 것과 다름이 없어서..

getter만 쓰되, 행동 단위로 수정해보면 좋을 것 같습니다.

위의 controller에서 보면, reset, maxForward, move 등도 모델에서 구현할 수 있을 것 같습니다.

[hchayan]

안녕하세요 리뷰어님! 궁금했던 부분뿐만 아니라 생각치 못했던 부분까지 잡아주셔서 감사합니다

리뷰해주신내용을 토대로 아래와 같은 부분을 리팩토링 했습니다!

1. Cypress에서 자주사용한 type, click, should, get 부분을 클래스화해서 재사용성을 높혔습니다.

2. template 파일을 constants 디렉터리에서 layouts 디렉터리로 옮겼습니다.

3. 클래스를 export해 사용하는 곳에서 받아서 메모리 할당하는 방식으로 변경했습니다.

   // 기존
   export const validator = new Validator();     // 메모리 할당후 export
   ...
   import { validator } from "../validator/validator.js";
   ...
   validator.isCarValid(carNames)
   
   // 변경
   export default Vaildator;
   ...
   import Vaildator from "../validator/validator.js";
   ...
   constructor() {
   	this.validator = new Vaildator();     // 실제 클래스를 사용하는 곳에서 메모리 할당
   }
   this.validator.isCarValid(carNames)

4. 중간에 불필요한 RacingCar 클래스를 제거하고 바로 RacingCarController 클래스를 사용합니다

5. ES6의 접근자 프로퍼티 (getter, setter)을 사용해보았습니다.

   • controller.js의 getCarsInput, getCountInput, getWinners 메서드
   • model.js 의 get, set 메서드들

6. document.querySelector("선택자") 를 자주사용해서 util.js 에 아래와 같이 단순화 했습니다.

   // util.js
   const $ = selector => { 
   	
   	return document.querySelector(selector)
   }
   
   // controller.js
   const $countInput = $("#count-input") && $("#count-input").value; // 존재할때 수행.

7. model의 단순 set을 수행하는 메서드 대신, controller에서 수행하는 내용을 일부 가져와 아래와 같은 메서드를 구현했습니다.

   reset, moveCar, initCar 

고민한 부분

1. return alert false 취급되는것을 이용한 구현이 적잘한 구현인지 궁금합니다

   isCarNameBlank(carNames) {
       if (carNames.some(carName => carName.length < VALID.CARNAME_MIN_LENGTH)) {
         return alert(ERROR_MESSAGE.BLANK_CARNAME);   // false
       }
   
       return true;
     }

2. 불필요한 상황에서 클래스 생성자 변수 할당에 대해
   : 고민하다 handler 이벤트를 init 메서드로 분리구현했습니다.

   import RacingCar from "./racingcar/controller.js";
   
   const racingcar = new RacingCar();
   racingcar.init();

   class RacingCarController {
     constructor() {
       this.model = new RacingCarModel();
       this.view = new RacingCarView();
       this.validator = new Validator();
     }
   
     init() {
       this.handleCars();
     }

[Vallista]

전반적으로 객체나 MVC의 정의, 개념을 잘 파악하고 계신 것 같아서 좋았습니다! 👍 👍

질문주신 부분에 대한 답변입니다.

1. 넵 좋아보입니다. 해당 구현의 경우 Validate layer를 분리해서 Controller에 의존되는 방식으로 만드셨는데요, 문제가 될 만한 부분은 없어보입니다. model이나 view에 의존적이지만 않으면 됩니다. 조금 더 개선점을 찾자면, validator는 business(여기서는 자동차 경주 구현 이겠죠?)에 의존적인 코드 보다는 범용적으로 사용할 수 있는 코드 메소드를 갖고있는 클래스면 더 좋을 것 같습니다. ERROR_MESSAGE.BLANK_CARNAME등과 같은 message는 business에 의존적이므로, 해당하는 코드들은 controller 레벨에서 제어하면 좋을 것 같다는 생각입니다.

2. constructor로 객체 생성 및 할당시에 불필요한 작업이면 별도의 함수로 추출하여 외부에 공개하는 것은 아주 좋은 것 같습니다 👍 👍

[Vallista]

이렇게 사용해보시니까 어떤가요?
이런 패턴은 depth가 깊어지게 만들지 않으며,
promise등에서 사용하는 자기 자신을 반환하는 패턴입니다 :)

API class를 만들때도 요긴하게 쓰일 수 있으니 잘 익혀두시는 걸 추천드립니다.

[hchayan]

생각보다 가독성의 측면에서도 더 좋아진거같고,
Cypress에서도 다른 JS 코드처럼 클래스화, 함수화하는 방법이 너무 좋은것 같습니다.

그리고 알려주신 방법을 사용해 체이닝 방식으로 구현하는 방법을 알려주셔서 적용했고,
this 바인딩 개념에 대해 다시 한번 공부할 수 있었던 계기였습니다!

[hchayan]

한달전 셀프 코드 리뷰 회고

이 코드를 작성한지 한달이라는 시간이 지났다.
처음에 시작할떄는, 지금 코드와 별 차이가 없을꺼 같아, 별로 코드리뷰를 할 부분이 없지 않을까? 라는 생각을 했다.

하지만 막상 코드 리뷰를 해보니,
코드의 부족함이 눈에 많이 띄었고, 생각보다 많은 부분에서 지적을 할 수 있었다.
이러한 점에서 한달동안 생각보다 많이 성장했다는 생각과,
내가 그동안 리뷰를 받으며 고민했던 것들을 다시한번 더 생각해볼 수 있었고,
리뷰어 분들의 리뷰를 잘 듣고 고민하고 잘 적용해 나가고 있다는 생각을
할 수 있게 된 계기가 되었다.

또, 지금 진행하고 있는 유투브 프로젝트도 한달뒤에 보면,
지금과 같이 부족한 점이 많이 보이지 않을까 하는 기대감과,
스스로 더욱더 성장할 수 있을거라는 자신감이 생긴것 같다.

동시에, 이번 유투브 미션을 하면서 실수 했던 부분들이 생각이 났다.
언젠가 봤던 글중에 교통사고가 제일 많이 나는 시점은
면허를 갓 취득했을때가 아니라, 면허를 따고 운전을 한두달정도 했을때라고 한다.

처음의 그 긴장감과 생각이 시간이 지나며 헤이해져 기초적인 실수를 하는 것이다.
우리가 지금 진행하는 코드리뷰가 정확히 이 시점이라고 생각한다.
적당한 긴장감을 가지고, 자만심이 아닌 자신감을 가지는것이 이 시점에서 가장 중요한것 같다.

그래도 생각보다 잘 해왔고, 앞으로도 잘 해나고자 하는 좋은 회고였다 😁😁

[hchayan]

input 태그의 다양한 속성들을 이용해 사용자의 입력에 대한 접근을 제한해보면 어떨까요?

[hchayan]

클래스에 대한 init을 상수로 관리하는 건 상황에 따라 다르겠지만,
무작정 사용하지 말고, 다시 한번 생각해보고 사용해보면 좋을꺼 같아요!

[hchayan]

model과 view를 의존성 주입 방식을 사용해서 연동하면 어떨까요?
그렇게 하면 model과 view의 파일 위치에 따른 종속성으로 인한 문제를 제어하기 좋을꺼같아요

[hchayan]

굳이 template화를 해주지 않아도 되는 부분을 고려해 보세요

[hchayan]

validator를 생성자를 사용하지 않아도 되는 방향으로 구현해보면 어떨까요?
굳이 모든 파일의 형태를 class 형태로 맞춰줄 필요는 없습니다!

[hchayan]

manage라는 변수명은 함수의 내용을 한번에 파악하기 어려운 변수명이니 지양하는게 좋을거 같아요

[hchayan]

controller의 역할에 대해 좀 더 고민해보면 좋을거같아요.
controller에서는 다른 곳에 잇는 메서드를 조합하는 개념으로 생각을하시고,
components 와같은 파일을 나누어 역할별 메서드를 분리하고,
controller에서는 그 메서드를 가져와 조합해준다는 생각으로 구현해보는 것도 좋을거 같아요!

[hchayan]

car를 carModel로 클래스화 해서 해당 car에 대한 정보를 가지고 있는 식으로 분리해보는 건 어떨까요?

[hchayan]

template도 마찬가지로, class의 형태를 고집하지 않아도 됩니다!

[hchayan]

굳이 class 를 사용하려면 static을 사용해, 생성자 생성을 안하는 방식으로도 고려해봐요