테트리스 코드 리팩터링

메이커 준 님의 리팩터링 글을 토대로 리팩터링을 진행해봤습니다.

네이밍

1. 재사용성 및 한 가지 기능만 할 수 있도록 함수 분리

• checkMatch()는 함수명과 다르게 ①한 줄이 블록으로 모두 차있는지 확인하고, ②모두 차있다면 해당 줄을 제거하고, ③점수를 추가하는 세 가지 기능을 하는 코드가 혼재되어 있다고 생각해 라인 제거와 점수 변경을 removeLine()과 addScore()로 분리함

function checkMatch(){
	...
    removeLine(matched, child)
  	...
}


function removeLine(matched, child) {
  if (!matched) {
    return;
  }

  child.remove();
  prependNewLine();
  addScore(10);
}

function addScore(point) {
  score += point;
  scoreDisplay.innerText = score;
}

• init()에서도 점수를 초기화하기 때문에, 코드의 재사용을 위해 addScore()를 setScore()로 변경하고, point가 0일 경우 score에 0을, 아닐 경우 score에 점수를 추가해주는 코드를 추가

function setScore(point) {
  point ? (score += point) : (score = 0);
  scoreDisplay.innerText = score;
}

➕init()은 시작할 때 한번만 실행되기 때문에 점수 초기화도 한번만 하면 되는데, 굳이 두 줄되는 코드 중복을 줄이기 위해 조건문을 추가해서 setScore()로 수정할 필요가 있을까 의문이 들었다. 코드 중복을 줄인 효용보다 연산이 증가해 발생하는 비용이 더 큰 것 같아 그냥 addScore로 원상복구 시켰다.

2. 좀 더 명확한 의미전달을 위해 함수 및 변수 이름 변경

1) 게임 오버 시 보여주는 메세지이므로 gameText에서 gameoverText로 변경

2) 블록이 해당위치로 이동이 가능한지 확인하는 함수이므로 checkAvailable()에서 isMovable()로 변경

3) Object.entries()에서 Object.keys()로 변경하면서 blockArray를 blockTypes로 변경

구조

1. boolean을 바로 return하도록 변경

수정 전

function isMovable(target) {
  if (!target || target.classList.contains("seized")) {
    return false;
  }
  return true;
}

수정 후

function isMovable(target) {
  return target && !target.classList.contains("seized");
}

2. 중간 변수 제거

굳이 중간변수를 사용해야 할 이유가 없다고 생각하여, 중간 변수를 제거하고 조건문에 isMovable()을 바로 사용하였다.

수정 전

const isAvailable = isMovable(target)

if(isAvailable){
  ...
}

수정 후

if(isMovable(target)){
  ...
}

기타 수정사항

1. 게임 오버 시 코드가 종료되도록 수정

• 기존 코드에서는 게임이 종료되고 나서도 코드가 계속 작동하는 것을 우연찮게 발견하였다. 이유는 게임이 끝났음을 알리는 showGameoverText()가 호출된 후에도 renderBlocks()을 재귀적으로 호출하기 때문이었다.

• 따라서 종료문구를 띄운 후 some()을 중단시키기 위해 종료 문구 함수 호출 코드 다음에 return true를 추가하여 이를 해결하였다.

BLOCKS[type][direction].some((block)=>{
	  ...
      
      if (moveType === "retry") {
        clearInterval(downInterval);
        showGameoverText();
        return true;    // 이 코드를 추가해서 코드 종료시킴
      }
      setTimeout(() => {
        renderBlocks("retry");
		...
      }, 0);
 })

🔨추가 수정사항

게임 오버 메세지가 나와도, 방향키를 누르면 게임이 동작한다.

2. Object.entries에서 Object.keys로 수정

• generateNewBlock()에서 랜덤으로 블록을 정하기 위해 0~블록 개수 사이의 난수를 생성하였고 강의에선 이 과정에서 Object.entries()를 사용했다.

• Object.keys()로 key값(각 블록의 이름)만 가져오면, 좀 더 효율적으로 해당 기능을 구현할 수 있다고 생각하였다. (BLOCK객체를 다 불러오지 않아도 되므로)

수정 전

  const blockArray = Object.entries(BLOCKS);
  const randomIndex = Math.floor(Math.random() * blockArray.length);

  movingItem.type = blockArray[randomIndex][0];

수정 후

  const blockTypes = Object.keys(BLOCKS);
  const randomIndex = Math.floor(Math.random() * blockTypes.length);

  movingItem.type = blockTypes[randomIndex];

수정 후 randomIndex가 어떤 값인지, type 프로퍼티에 어떤 값을 추가하는지 좀 더 잘 알 수 있게 된 것 같다.

3. generateNewBlock()에서 movingItem 프로퍼티 값 변경 코드 수정

4줄밖에 되지 않아 굳이 코드를 수정할 필요는 없다고 생각했지만, 변경해야할 프로퍼티가 많은 경우를 대비해서 한번 코드를 수정해봤다.

수정 전

  movingItem.type = blockTypes[randomIndex];
  movingItem.top = 0;
  movingItem.left = 3;
  movingItem.direction = 0;

수정 후

  const defaultValues = [blockTypes[randomIndex], 0, 0, 3];

  Object.keys(movingItem).forEach((key, i) => {
    movingItem[key] = defaultValues[i];
  });

• 값을 일일이 하드코딩할 때보다 수정 후 방식이 좀 더 나은 것 같다. 프로퍼티가 많을 경우 코드는 더 깔끔해질 것 같다.

• ❓defaultValues를 객체 리터럴로 작성하면 더 나을까?

추가로 renderBlocks()에서도 tempMovingItem의 값을 movingItem에 복사하는 코드를 위와 같은 방식으로 수정했다.

  Object.keys(movingItem).forEach((key) => {
    movingItem[key] = tempMovingItem[key];
  });

굳이 객체를 순회하지 않고 Object.assign()을 활용해도 될 것 같아서 이대로 수정하였다.

Object.assign(movingItem, tempMovingItem);

❓ assign사용시 주의할 점

• Object.assign()은 속성의 값을 복사하기 때문에 출처 값이 객체에 대한 참조라면 참조 값(주소)만 복사함. 깊은 복사를 하려면 다른 방법을 사용해야함.
• 출처 값에 null이나 undefined가 있어도 예외발생 x

4. keycode대신 key를 사용

keycode는 더이상 사용하지 않아 권장되지 않는다고 해서 key를 사용하기로 했다.

스페이스의 e.key는 공백문자이다.

느낀점

아직 함수를 어떤 기준으로 분리시켜야할 지(특히 랜더링 함수에서), 어떤 코드가 좋은 코드인지 감이 잘 오지 않는다. 한 번 리팩터링에 대해 좀 더 자세히 찾아봐야겠다.