[PR 코드리뷰] 우아한테크코스 프론트엔드 레벨1 - 자동차경주 미션

 

우아한테크코스 프론트엔드 과정 레벨1 <자동차경주 미션>을 수행하며 ODK미디어의 @ysm0622 님께 리뷰를 받았다. 내 미숙한 코드를 꼼꼼하게 보시고 피드백해주셔서 코드를 개선하는데 많은 도움을 받았다.

 

미션 1단계  피드백

👉 PR 바로가기

1. Model-View 각각의 클래스로 분리

$cars 와 같이 직접 DOM을 다루고 직접 접근해서 data 값을 변경하기보다는 Model과 View를 분리하자. 그리고 Model과 View를 분리하는게 왜 중요한지 고민해보자.

getWinners()에서도 DOM에 직접 접근하는 부분과 maxScore인 car들을 뽑아내는 로직을 분리할 수 있다. 이렇게 하면 테스트에서 똑같은 로직을 다시 작성하는 일도 없앨 수 있다. 

 

2. 변수명을 더 명확하게

'NAME_BLANK' 보다는 'NAME_CANNOT_BE_BLANK'과 같이 더 명확한 이름으로 짓자. 'VALIDATOR'는 변수명보다는 무언가를 검증하는 함수나 객체 이름에 사용된다. 임계값은 'THRESHOLD'라는 단어를 많이 사용한다.

'List'는 배열 자료구조를 담을 때 많이 쓰인다. 객체를 sectionList와 같이 명명하면 혼란을 줄 수 있으므로 피하자.

 

3. 함수명은 의도를 드러내도록

toggle은 현재 상태에 의존하기 때문에 import { toggleVisibility as setInvisible } 처럼 이름을 바꾸면 의도를 숨기게 된다. 함수 이름은 동사+명사(적용할 값) 형태로 짓는 것이 일반적이다. setVisibility($target, true)와 같이 바꾸는 것도 괜찮다.

Boolean을 반환하는 함수의 이름은 특수한 경우를 제외하고는 부정문이 들어가지 않는 것이 좋다. isNotBlank 대신에 isBlank 를 쓰도록 하자.

 

4. 함수는 이름에 맞는 한 가지 기능만

isValidRacingCount()에서 alert가 호출되는 식의 사이드이펙트가 없도록 하자.

 

5. 사용하지 않는 반환값의 리턴은 지양

코드를 짧게 작성하는 것보다 의도를 명확히 드러내는 것이 더 중요하다는 점을 상기하자.

// 개선 전
return ($racingCountInput.value = '') 

// 개선 후
$racingCountInput.value = ''; 
return; 

 

6. 기타

instance화 하기 위해서는 new 생성자 키워드는 필요하지 않다. 오타도 수정하자.

 

 

미션 2단계 피드백

👉 PR 바로가기

1. bind 메서드로 this 전달

화살표 함수와 일반 함수에서의 this의 차이를 먼저 이해하자. bind 메서드를 이용하면 이벤트 핸들러에 this를 전달할 수 있다.

const car = {
  name: 'tico',

  logNameRegFunc: function () {
    console.log(this); // { name: 'tico', logNameRegFunc: f ... }
    console.log(this.name); // 'tico'
  },
  logNameArrowFunc: () => {
    console.log(this); // Window 전역객체
    console.log(this.name); // ''
  },
};
car.logNameRegFunc();
car.logNameArrowFunc();

$('#car-name-submit').addEventListener('click', handleCarNameInput.bind(this));

 

2. 시간지연의 실행 주체는 Controller

"N초 뒤에 ~를 실행한다" 는 작업은 도메인 로직이다. 예를 들어, "N초 뒤 DB를 업데이트하고, 완료되면 다른 페이지로 이동 후 팝업을 3초 보여주었다가 숨긴다."라는 요구사항은 View에서 수행하면 안 되는 로직이다. View는 Model 의 상태를 단순히 표현(representation)만 해야 한다. 게임이View는 Model의 상태변화를 감지하여 isPopupShowing 값이 변경되면 팝업을 보여준다.

 

3. every/some 사용을 적절하게

every와 some을 적절하게 사용하면 코드를 간결화할 수 있다.

// 개선 전
if (!cars.every((car) => isValidLength(car.getName()))) { ... }

if (!cars.every((car) => !isBlank(car.getName()))) { ... }

// 개선 후
const carNames = cars.map((car) => car.name);
 
if (!carNames.every(isValidLength)) { ... }

if (carNames.some(isBlank)) { ... }

 

4. 변수 선언은 필요한 곳에서

변수는 꼭 시작 부분이 아니더라도 필요한 곳에 선언해도 된다. 이렇게 하면 대부분의 경우는 let 선언 없이도 작성할 수 있다. 따라서 let을 사용했다면 다시 한번 꼭 let을 사용해야 하는 상황인지 고민해보자.

let winners;

....
winners = getWinners();

....
const = getWinners();

 

5. 클래스 안에는 관련 있는 로직만

예를 들어 getRandomNumber()는 Car라는 도메인과 관련이 없기 때문에 Car 내부에 있는 것은 맞지 않다. this를 전혀 사용하지 않고 있다면 클래스 메서드로 선언할 필요가 있는지 다시 한번 확인해보자. 

 

6. 꼭 getter setter를 통해 접근할 필요는 X

getter에서 return this.name 과 같이 바로 반환할 것이라면 car.name 으로 접근해도 괜찮다.

 

 

 

다른 크루의 리뷰 메모 👀

피드백 내용 앞의 번호는 순번이 아니라 PR 번호이다.

 

아키텍쳐/패턴

#07 MVC패턴에서 모델에 데이터를 저장할 때 필요한 검증과정은 모델보다 컨트롤러가 더 적절한 위치이다.

#08 화면과 관련된 로직을 처리하는 View의 네이밍은 addHidden, removeHidden 보다는 hide, show 와 같은 네이밍이 좋다. class를 추가하고 제거하는 것은 Controller의 관심사가 아니기 때문이다.

#09 element.addEventListener() 가 이미 옵저버 패턴이다.

#09 event를 지워주는 코드 없이 또 이벤트를 등록하는 것은 피하자. 단, 새로운 innerHTML을 할당하면 이벤트가 삭제된다.

#14 당장 패턴을 만들어 적용하려고 하기보다 문제를 직접 겪어서 문제를 인지하고 문제를 어떻게 하면 해결할 수 있는지 고민해보자. 결국 코드를 많이 보아야 '역할'과 '책임'을 인식할 수 있다.

#15 폴더 구조를 Model / View / Controller 로 잘 나눠놓더라도 실제 코드에서 View 쪽에 함수들이 View 외에 View를 Control 하는 기능을 담당하지 않도록 주의하자. 돔을 직접 숨기거나 보이거나 삭제하는 것이 Control이다.

#19 흔히 사용하는 프레임워크는 render-mount 순서이다. DOM 객체를 생성 후, mount 과정에서 이벤트를 바인딩한다. addEventListener를 하나로 묶는 함수는 일반적이다.

#19 상수 변수들을 constants에 몰아두는 것은 좋은 SoC(Seperation of Concerns, 관심사의 분리)이다.

#23 MVC 패턴에서 M과 V와 C는 하나씩만 존재해야 하는 것은 아니다. 그렇게 되면 M과 V를 알고 있는 C가 모든 것을 통제하게 되면서 기능이 추가될수록 비대해질 수 있다.

#25 레이어 패턴이란 레이어를 어떤 특정한 형태로 나눈다기보다, 개발을 하면서 어떤 특정한 일을 하는 레이어를 구축하고, 레이어 단위로 커뮤니케이션을 할 수 있도록 개발하는 패턴이다. 따라서 MVC도 일종의 레이어 패턴이라고 볼 수 있다.

#48 Controller는 View를 알고 있지만, 이 View가 웹브라우저에서 실행되는 뷰라는것은 모른다. Controller와 Model의 코드는 그대로 놔두고 웹브라우저의 DOM이 아니라 캔버스, CLI로 텍스트로만 출력되는 화면 등으로 View를 변경하더라도 동작 가능해야 한다. (현실에서는 적정선에서 타협하지만 엄밀히 말하면) 웹브라우저에서만 존재하는 alert을 Controller에서 직접 호출하거나 DOM 셀렉팅을 해오는 것은 Controller의 엄밀히 따지면 위배되는 사항이다. alert로 띄울지 커스텀 다이얼로그로 띄울지는 Controller가 아닌 View의 관심사이다. 따라서 Controller가 뷰에 요청하는 메서드의 이름은 alert이면 안된다. 둘 사이의 인터페이스는 바뀌지 않게끔 추상적으로 선언하고 화면이 바뀐다고 메서드 이름이 바뀌어 Controller까지 수정 여파가 미치지 않도록 해야한다.

 

함수/클래스

#05 객체를 생성한다면 function 보다 class 로 객체 단위 관리를 하는 게 더 편하고, 은닉 및 상속에도 쉽게 사용된다.

#05 코드의 범용성을 위해 아래와 같은 default 값은 없는 게 낫다.

export const getRandomNumber = (min = GAME.MIN_SCORE, max = GAME.MAX_SCORE) => {}

#07 메서드 A -> 메서드 B -> 메서드 C -> 메서드 D 처럼 꼬리를 무는 형태로 호출하고 있다면 혹시 단일 책임원칙을 벗어난 것은 아닌지 생각해보자.

#07 특정 횟수의 반복이 아니라면 실수하기 쉬운 for문 보다는 forEach를 사용하자.

#15 클래스, 함수 중 더 선호되는 것은 팀의 컨벤션에 따라 다를 수 있다. 클래스는 syntatic sugar이다. 두 가지 모두 사용법을 정확하게 알고 있는 것이 중요하다.

#15 함수나 메서드를 작성할 때 클래스 안에 두어야 할지 밖으로 빼도 될지 많이 고민해서 작성하자. 어떤 기준으로 해당 위치에 작성한 것인지 명확한 이유를 얘기할 수 있어야 한다.

#15 특정 값(상수)이나 일련의 작업(함수)을 보고 어떤 값인지, 어떤 작업을 하는지 쉽게 유추할 수 있다면 분리할 필요가 없다. 또한, 재사용되지 않는다면 처음부터 분리할 필요는 없다.

#16 this를 사용하지 않는다고 꼭 클래스 밖으로 내쫓아야 하는 것은 아니다. 만약 반드시 클래스 안에 있어야 하는데 this를 사용할 필요가 없다면 static method로 선언해도 좋다.

#18 호이스팅되면 함수를 사용하기 전에 반드시 함수를 선언해야 한다는 규칙을 위반하고 가독성도 떨어지므로 함수 선언식을 지양해야 한다.

#19 JavaScript에서는 변수의 접근이 쉬워서 접근을 getter로 제어하는 게 안전한 코딩 방법 중 하나이다. 

#20 객체를 할당하고 객체 안의 메서드를 활용할 목적이 아니라면 class를 사용할 필요가 없다.

#20 단순한 setter 는 지양하고 행동 단위의 메서드를 만드는 게 좋다.

#20 new RacingCar() 만 하고 끝내면 범용적으로 사용할 수 없다. 추후 메모리 해제를 동적으로 할 수 없고, 내부의 변수나 함수를 사용할 수도 없다. 따라서 생성만 하는 행위는 지양하는 것이 좋다.

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

#22 파라미터가 없고 단순히 하나의 함수를 호출해주는 경우에는 bind 를 쓰면 코드도 간결해지고 읽기도 좋다.

Object.addEventListener('click', () => Handler());
Object.addEventListener('click', Handler.bind(this));

#25 함수를 작성할 때는 매개변수에 대한 예외처리를 고려하는 것이 중요한다. 함수를 작성하는 순서는 다음과 같다. 함수의 행동 정의 - 매개변수 정의 - bad case 정의해서 리턴 구문 작성

#25 보통 크리티컬한 에러가 나오지만 계속 실행이 되어야 할 때 try-catch와 함께 throw new Error를 사용한다.

#33 클래스일 필요가 없다면 단일 함수들로 export 하는 것이 좋다.

#45 get, set 만드는 이유는 멤버변수를 다른 곳에서는 변경하지 못하고 클래스의 함수로 변경할 수 있도록 하기 위해서이다. 그래서 보통 멤버변수는 prefix를 붙여 숨기고 public한 get, set에 진짜 원하는 변수명을 함수명을 짓는다.

constructor(name) {
  this._name = name;
}

get name() {
  return this._name;
}

#47 비동기 로직이 도메인 로직과 결합되면 재사용하기 어려워지기 때문에 분리하는 것이 좋다.

 

 

컨벤션/폴더링

#07 숫자는 작은 것에서 큰 것으로 읽히는 것이 자연스럽다. (size <= 5 && size > 0) 보다 (size > 0 && size <= 5)이 더 좋다.

#12 함수명의 get 네이밍은 getter로서만 사용하는 것이 좋다. 무언가를 생성한다는 의미를 담고싶다면 create가 좋다.

#14 함수명의 get 네이밍은 get 뒤에 오는 내용을 리턴할 것으로 예상이 되고, set 네이밍은 setSectionId(id)와 같이 set 해줄 인자를 받을 것으로 예상이 된다. 특정 데이터를 초기화하는 경우라면 init 네이밍이 더 잘 어울린다.

#15 함수, 상수, 클래스의 위치는 그 코드를 사용하는 곳과 가장 가까운 곳에 두자. 보통 그 파일 안에서만 사용되는 코드라면 그 파일 안 에두는 것이 좋다. 그리고 해당 코드를 import 해서 가져다 쓰는 곳이 점점 많아진다면 범용적인 함수라는 의미이기 때문에 범용성을 나타내는 다른 폴더나 파일로 분리하면 좋다.

#15 함수의 이름만 보고 파라미터로 어떤 값을 넘겨야 하는지 유추할 수 있는 것이 좋다.

#16 util/domUtil.js 의 domeUtil.js는 이미 util 폴더 안에 있기 때문에 파일 이름에는 Util을 빼도 좋다.

#16 좋은 에러 메시지는어느 부분이 잘못되었는지 '안내', 맞는 방향으로 '제안' 2가지를 모두 해준다.

#19 네이밍 컨벤션에서 행동을 먼저 말하고 뒤에 서사를 풀어쓰는 느낌이 있다. 따라서 typeCarAndClick 보다는 clickAfterTypeCarName 이 더 좋다.

#19 commit에 올릴 때는 코드를 다시 보면서, console.log를 삭제하는 게 좋다.

#20 HTML template은 constants 보다는 layout의 영역이다.

#22 constants는 유틸성 파일은 아니다.

#23 당장 전역적으로 관리되어야 하는 데이터가 아니라면 static 변수로 사용해서 구역을 작게 제한하고 그 후에 전역적으로 관리되어야 하는 데이터로 판단되었을 때 범위를 넓히는 것이 좋다.

#32 CSS 네이밍은 BEM(Block-Element-Modifier)로 짓는다. 예를 들면 '.block', 'block__elem', 'block--hidden' 'block--size-big' 과 같다. (참고)

#33 도메인과 상관없이 사용될 수 있는 함수들은 util에 모아보자.

#42 resolve, reject는 줄여 쓰지 않는 것이 좋다.

#43 클래스라면 파일명도 PascalCase로 작성하자.

#50 게임이 끝나고 메시지를 alert하는 함수의 이름은 alertNextGamepossible 보다 alertGameOver 이 낫다. 얼럿메세지가 "한 게임 더 하시겠습니까?"에서 "축하합니다!"로 변경되었을 때 함수이름도 변경해야하는 상황을 방지하기 위해서다. 메시지의 내용으로 함수의 이름이 정해지는게 아니라 함수가 하는 일에 따른 이름이 정해진다고 생각하자.

 

DOM

#02 자주 쓰는 querySelector는 아래와 같은 snippet으로 쓰면 좋다.

// util.js
export const $ = selector => document.querySelector(selector);
export const $$ = selector => document.querySelectorAll(selector);

// app.js
const carNameInput = $('#car-name-input');

#07 input의 값을 조작할 때는 특히 유의하자. 유저가 입력한 값에 대한 검증은 빡빡하게 해야한다.

#14 JavaScript에서 계산한 값을 data- 에 저장하고 다시 가져오는 패턴은 HTML과 JavaScript를 강하게 엮기 때문에 좋지 않다.

#14 추후 HTML 태그가 추가될 경우를 대비해 셀렉터는 tagName 보다 id 나 data-xxx로 컨트롤하는 것이 좋다. 

#15 innerHTML에 템플릿을 += 또는 +로 더해주는 해주는 것은 display 행위가 아니라 add 행위이다.

#16 $container.classList.add(...['d-flex', 'justify-center', 'mt-5', 'racing-result-container']) 와 같이 스프레드 연산자를 사용하면 class를 깔끔하게 추가할 수 있다.

#19 $div.style.display = 'none', $div.innerHTML = ''와 같은 코드는 내부 메서드로 만들어 코드를 관리해도 좋다.

해당 노드가 있는지 여부도 함께 예외처리를 해주는 것이 좋다.

#23 selector 를 상수화하는 것은 좋다 나쁘다 할만한 부분은 아니다. (취향 차이다.)

#24 map은 동일한 length의 새 배열을 반환하는 데 사용되므로 어떤 동작을 반복하여 실행만 하고 싶을 경우는 forEach를 사용하는 것이 좋다.

#39 DOM을 지우고 새로 append 하는 것은 비용이 크기 때문에 렌더링 해두고 show/hide 하는 것이 좋다.

export function $(selector) {
  const element = document.querySelector(selector)
  return {
    show() {
      element.style.display = "block"
    },
    hide() {

    },
    get innerHTML() {

    },
    set innerHTML(v) {

    },
    addEventListener(eventName, cb) {

    }
  }
}

#50 셀렉터를 #try-input > div > button 이렇게 파고 들어가는 것은 지양해야 한다. HTML 구조 변경에 취약하다.

 

테스트

#16 테스트의 제목은 주어진 상황에서 ~했을 때 ~한다 형태로 Given, When, Then 3가지 모두 포함되도록 작성하는 것이 좋다.

#19 테스트 코드를 작성할 때 테스트를 읽는 사람이 이 테스트를 왜 해야 하는지?를 이해할 수 있도록 구현한다.

#19 테스트 코드의 describe를 레이아웃 관점과 비즈니스 로직(입력 테스트)으로 두 가지를 만드는 것도 좋다.

#20 테스트 코드의 비슷한 패턴은 이렇게 체이닝해서 작성할 수 있다.

#20 cypress는 빌드에 연관 있는 툴은 아니므로, --dev 옵션을 주어 devDependencies로 이관하는 게 좋다

#51 함수 내부에서 외부에 접근하거나, 함수가 함수 외부의 상태를 변경하면 테스트 하기가 어려워진다. 순수함수(pure function)와 같이 파라미터로 받은 값들에 대해서만 변경하면 함수단위로 테스트하기 쉬워집니다. 

 

 

그 외 키워드: 덕 타이핑(#3), 디바운스와 쓰로틀(#5), 메서드 추출(#7), 전략 패턴(#23)
도서 추천:  조영호님의 객체지향의 사실과 오해, 오브젝트(#14)