🔎 코드 리뷰

✒️ 코드 리뷰에 앞서

코드 리뷰를 진행하기에 앞서 어떤 방식으로 진행할지 설명하도록 하겠다. 먼저 프로젝트 설계 와 코드 가독성, 유지보수 두 단계로 나눠서 리뷰하도록 하며, 프로젝트 소개 에서 작성한 구현 기능 목록 단위로 코드를 리뷰하도록 하겠다. 또한 , 우테코 선발 과정에서 평가 대상이 아니었던 HTML, CSS 코드는 생략하고 JavaScript 코드만 리뷰하도록 하겠다.

마지막으로 코드 리뷰 체크리스트 의 요구사항에 얼마나 부합하는 코드였는지 하나씩 체크하며 살펴보도록 하겠다.

---

🗃️ 프로젝트 설계

이전 글을 읽고 왔다면 알겠지만 이 프로젝트는 설계 단계가 없었던 프로젝트다. 설계를 하지 않아 프로젝트가 어떻게 잘못되었는지 리뷰를 체크리스트를 하나씩 읽어보며 해보자.

• ❌ README.md 파일에 프로그램의 구현할 기능들을 분리해 작은 단위로 작성했는가?

• ❌ 프로그램을 작성하면서 예외 케이스 등을 추가하는 등, README.md 파일을 업데이트했는가?

왜 작성해야할까?

사실 문벅스, 우테코 프리코스를 접하기 전엔 모든 프로젝트를 위의 요구사항을 지키지 않고 시작했었는데 프리코스가 끝나고나서 저 두 가지의 중요성을 깨닫게 되었다.
일단 요구사항을 분리해 작은 단위로 만들면 막막했던 요구사항이 단순하게 보이기 시작하고, 이는 단순한 코드로 이어져 코드 가독성 또한 높일 수 있었다. 또한 어떤 예외가 존재할 지 미리 정리해놓으면 예상치 못한 방향으로 프로그램이 튀는 것도 상당히 많이 막을 수 있었다.
또 코드에 문제가 생겼을 때, 어디에서 문제가 생긴 것인지도 찾아내기가 쉽다.

결론은 시간이라는 중요한 리소스를 상당히 아낄 수 있다.

---

🕵️ 코드 가독성, 🗃️ 유지보수

이제 프로젝트 설계 부분은 리팩토링을 하는 부분에서 두 가지 요구사항을 어떻게 지키면서 프로젝트를 진행할 수 있는지 다음 글에서 살펴보도록 하고, 이젠 먼저 전체적으로 보이는 문제가 뭔지를 먼저 살펴보고, 구현 기능 목록 의 기능을 구현한 코드를 순서대로 뜯어보면서 코드 가독성 과 유지보수 에 대해 리뷰해보도록 하자.

먼저 HTML, CSS로만 구현된 기능들, 단순히 유튜브 따위를 참고해 만든 기능을 먼저 빼놓고 리뷰를 시작하겠다.

• Home 화면에 진입시 버거킹 로고를 활용한 애니메이션이 재생되도록 구현.

• 메뉴가 열린 상태에서 이동할 메뉴를 클릭하면 해당 메뉴로 이동하도록 구현.

⬆️ 위 두 가지 기능은 HTML, CSS 만으로 구현된 기능이다.

• Menu 화면에서 드래그로 회전하는 다각형 구현

• Restaurants 화면에서 위치 정보를 통해 사용자를 중심으로 버거킹 매장을 보여주는 지도를 보여주는 기능을 구현.

• 사용자의 위치정보를 가져오도록 구현.

⬆️ 위 세 가지 기능은 유튜브와 카카오맵을 참고해 구현된 기능이다.

---

💻 보이는 프로그램 전체 문제점들

커밋메세지

제일 먼저 얘기하고 싶은 부분은 커밋 과 디렉토리 이다. 코드리뷰를 위해서 오랜만에 코드를 뜯어보기 위해 당연히 기능단위나 의미있는 단위로 커밋을 했을거라 생각하고 커밋을 살펴봤는데...

도대체 커밋메시지만 보고는 뭘 구현한 커밋인지를 알 수가 없었다. (아니 ???는 대체 뭐냐고 ㅋㅋㅋㅋ) 일관성도 하나도 없어서 커밋으로 코드의 역할을 파악하는 것보다 그냥 코드 전체를 보면서 읽어보는게 나을 것 같다는 생각이 들었다.

이제는 커밋은 우테코 프리코스를 진행하면서 알게된 커밋작성법을 이용하고 있다. 어떻게 커밋메세지를 작성해야하는지는 링크를 통해 대신하겠다.

➡️ AngularJS Commit Message Conventions

디렉토리

그래서 VSCode를 켜서 코드를 확인하려했는데...
그런데 JS file이 모두 JS폴더에 박혀있었다!

CSS 파일은 그래도 목적에 맞게 분리를 해놓아서 어디에 쓰이는 파일인지 바로 바로 알 수 있었는데 JS 파일은 파일명으로 용도를 예상하고 전체를 뜯어보면서 프로그램의 흐름을 파악할 수 밖에 없었다...

그러나 아직 나도 명확히 디렉토리를 어떻게 나눠야하는지는 몰라서 나 스스로 보고 이해할 수 있게만 나누고 있다. 이 부분은 추후 공부해서 덧붙이도록 하겠다.

코드 컨벤션

코드 컨벤션을 미리 정해놓지 않았으니 당연히 코드가 자기 마음대로이다. 밑에서 코드를 살펴보면 알겠지만 2space, 4space 를 혼용해서 사용하고 있고, '', "" 도 혼용하는 등 통일성이 없는 모습을 보여준다.

우테코 프리코스 때부터는 eslint, prettier를 적극 활용하고 airbnb의 코드 컨벤션을 채택해 프로젝트에 사용했고, 딱히 코드 컨벤션이 정해져있는 않은 개인 프로젝트라면 그대로 airbnb의 코드 컨벤션을 사용할 것 같다.

정해진 코드 컨벤션이 없으니 일단 코드 컨벤션에 대한 리뷰는 하지 않고 리팩터링 때 코드 컨벤션에 맞춰 코드를 작성하도록 하겠다.

➡️ Airbnb JavaScript 스타일 가이드 한국어 번역본

console.log

디버그 때 사용한 console.log가 너무 많이 남아있다. 리뷰하는 동안은 건드리지 않고 리팩터링을 할 때 모두 제거해줄 생각이다.

---

💻 구현 기능 목록 코드 리뷰

• Home 화면에서 화면을 드래그하면 섹션이 위 아래로 드래그 되도록 구현.

//index.js

const $ = (selector) => document.querySelector(selector);

//scrollScreen.js

const lastPage = document.querySelectorAll('.section').length;
let curPage = curHeight = 0;

function scrollScreen(event) {
  if($(".menu-tree").classList.contains("is-active")){
    return;
  }

  const pageHeight = window.innerHeight;
  if (event.deltaY > 0 && curPage < lastPage){ 
    curPage++;
    window.scrollTo({top:curPage*pageHeight, left:0, behavior:'smooth'})
  }
  else if (event.deltaY < 0 && curPage > 1) {
    curPage--;
    window.scrollTo({top:curPage*pageHeight, left:0, behavior:'smooth'})
  }
}

window.setTimeout(window.addEventListener("wheel", scrollScreen),2800);

위는 전체 코드이고, 이제 한 줄씩 뜯어보면서 살펴보도록 하자.

let curPage = curHeight = 0;

curPage, curHeight 라는 변수를 전역공간에 만들었다. 심지어 curHeight 라는 변수는 프로그램 전체에서 한 번도 사용 안 된다.

• 전역공간 은 어디서나 접근할 수 있기 때문에 위험하다. 전역변수 는 만들지 않아야한다.
• 필요없는 변수는 제거해야한다.

function scrollScreen(event) {
  if($(".menu-tree").classList.contains("is-active")){
    return;
  }

  const pageHeight = window.innerHeight;
  if (event.deltaY > 0 && curPage < lastPage){ 
    curPage++;
    window.scrollTo({top:curPage*pageHeight, left:0, behavior:'smooth'})
  }
  else if (event.deltaY < 0 && curPage > 1) {
    curPage--;
    window.scrollTo({top:curPage*pageHeight, left:0, behavior:'smooth'})
  }
}

window.setTimeout(window.addEventListener("wheel", scrollScreen),2800);

아... 참 할 말이 많은 코드다. 하나씩 살펴보자.

• scrollScreen 이라는 함수를 함수선언문 으로 선언했는데, 함수도 호이스팅이 되기 때문에 함수표현식 을 사용하는 것이 안전하다.

• if, else if 문 안에 페이지를 바꿔주는 비즈니스 로직 과 바뀐 페이지를 드래그해주는 UI 로직이 함께 들어가있다. 분리해줘야한다.

• 위의 원칙이 위배되었으니, 하나의 기능만 하는 작은 함수 도 아니다.

• if, else if 조건문에 의미를 알 수 없는 0, 1 이라는 매직넘버들이 사용되었다.

• window.onload 부분은 setTimeout을 사용한 부분들이 보인다. 이는 모두 애니메이션 종료시간에 맞추어 다음 애니메이션을 실행하기위함인데...
  모두 async, await 으로 바꿔줄 수 있을 것 같다.

• if문과 else if문 속에 있는 window.scrollTo 부분은 각각 한 번씩 쓸 필요 없이 함수 마지막에 한 번만 써도 된다. 한 마디로 중복된 코드이다.

사실 해결법을 몰라 굉장히 꼼수스럽게 짠 부분이 많아 코드 가독성,유지보수 체크리스트에 걸리는 부분들만 다루고, 코드 자체를 잘못 짠 부분은 리팩터링 부분에서 다루도록 하겠다.

---

• header 에 커서를 올리면 header 가 흰 색으로, header 내부 아이콘들이 검은 색으로 변하도록 구현.

• 메뉴 아이콘 을 클릭할 시 상태에 맞게 메뉴가 열리고 닫히도록 구현.

• 화면 좌측 상단의 메뉴 아이콘 을 클릭할 시 메뉴 아이콘 의 모양이 상태에 따라 ☰, X 로 변하는 애니메이션이 재생되도록 구현.

//menuTreeOpen.js

const wholeMenuTree = document.querySelectorAll(".menu-tree-class-for-toggle");

function wholeMenuTreeToggle(){
     for (let i=0; i < wholeMenuTree.length; i++) {
          wholeMenuTree[i].classList.toggle("is-active");
     }
     $("header").classList.toggle("is-active");
}

$(".menu-icon").addEventListener("click", wholeMenuTreeToggle);

위의 세 가지 기능은 모두 상태가 toggle 되면 상태에 따라 CSS 애니메이션을 작동시키도록 되어있기때문에 JS 코드는 공통이다.

• wholeMenuTreeToggle 이라는 함수도 역시 위와 동일한 이유로 함수표현식 으로 바꿔주는게 좋다.

• 변수명, 함수명 이 직관적으로 어떤 역할인지가 바로 와닿지가 않는다. wholeMenuTree 라는 말 자체가 무엇을 의미하는지 정말 모르겠다.

• 배열의 요소를 순서대로 탐색하는 for문이라면 초기식, 조건식, 증감식을 다른 방식으로 표현할 수 있어 보인다.

---

• resize 이벤트 발생 시, 화면사이즈에 맞게 배경사진 사이즈를 바꿔줌

// windowResizeEvent.js

window.onresize = function () {
    section = document.querySelectorAll(".section");
    section.innerHeight = window.innerHeight;
    const pageHeight = window.innerHeight;
    window.scrollTo({top:pageHeight*curPage, left:0, behavior:'auto'});
}

눈에 아주 거슬리는 한 줄이 있다...

• section 이라는 변수가 let이나 const 키워드 없이 선언이 되었다. 이게 되나? 싶었지만 JS에서는 키워드 없이 선언하면 선언과 동시에 할당해줄 경우에만 전역변수로 선언이 된다고 한다.. (물론 권장하지 않는 방식이다)
  자세한 것은 아래 링크에서 확인할 수 있다.

• 함수선언식 대신에 함수표현문 을 사용할 것!

  https://www.tutorialsteacher.com/javascript/javascript-variable

---

• 새로고침 이벤트 발생 시, 섹션 페이지가 1번 페이지로 드래그 되도록 구현.

//windowReloadEvent.js

function reloadScrollScreen(){
    const pageHeight = window.innerHeight;
    window.scrollTo({top:pageHeight, left:0, behavior:'smooth'});
    curPage++;
}
window.onload = () => {
    console.log("work");
    setTimeout(() => {window.scrollTo(0, 0)},100);
    window.setTimeout(reloadScrollScreen,2500);
}

엄청나게 많은 꼼수들이 보인다..

• 일단 먼저 console.log가 보인다.

• 함수선언식 대신에 함수표현문 을 사용할 것!

• window.onload 부분은 setTimeout을 사용한 부분들이 보인다. 이는 위와 동일하게 모두 애니메이션 종료시간에 맞추어 다음 애니메이션을 실행하기위함인데...
  역시 모두 async, await 으로 바꿔줄 수 있을 것 같다.

---

📋 체크리스트 확인

• ❌ README.md 파일에 프로그램의 구현할 기능들을 분리해 작은 단위로 작성했는가?

• ❌ 프로그램을 작성하면서 예외 케이스 등을 추가하는 등, README.md 파일을 업데이트했는가?

• ❌ 프로그램 전체에 동일한 코딩 컨벤션을 정해놓고 이를 사용했는가?

• ❌ 각각의 커밋이 어떤 커밋인지 알아볼 수 있도록 커밋메세지를 작성했는가?

• ❌ 변수명 등을 통일성 있게, 축약 또는 생략하지 않고 작성했는가?

• ❌ 어떤 프로그램인지 프로그램을 소개하는 내용을 README.md 파일에 작성했는가?

• ❌ 값을 하드코딩하지 않고 상수를 활용하며, 해당 상수에 의도를 드러낼 수 있는 이름을 부여했는가?

• ❌ JavaScript에서 제공하는 api를 활용하여 함수가 명백히 무슨 일을 하는 지 알 수 있도록 코드를 작성했는가?

• ❌ 전역공간을 사용하지 않았는가?

• ✔️ var를 사용하지 않았는가?

• ❌ 하나의 함수를 하나의 기능만을 하는 작은 단위로 구현했는가?

• ❌ 중복된 코드가 없는가?

• ❌ 불필요한 변수, 임시변수들을 제거해줬는가?

• ❌ 비즈니스 로직과 UI로직을 분리했는가?

처참하다... 과거의 나..

---

✍ 글을 마치며

burgerking-order-site 프로젝트는 나의 성장을 기록하는 프로젝트로 꾸준히 사용하려고 한다. 그래서 이 글의 이름이 코드 리뷰 -1- 인 것이고, 꾸준히 기능을 추가해나가고 내가 나의 성장을 느낄 때마다 코드 리뷰와 리팩터링을 하는 시간을 가져볼까 한다.

우테코 시작 전, 총 11일 동안 계속했던 프로젝튼데 뜯어보고 나니까 정말 만든게 없다. 제대로 된 체계 없이 시작해서 헤메는 시간이 너무 길었던 탓인 것 같다.

다음 리팩터링 글에서 더 좋은 코드로 다시 만나자!