[42World] Mypage 코드 리팩토링

코드 리팩토링하기

짧막한 내생각

긴 프리온보딩 기간이 끝나고 오랜만에 기존에 하던 프로젝트들에 복귀하였다 (몇개없지만…)

프리온보딩 기간동안 나름 성장해서 그런지 내가 작성한 코드를 슬쩍 봐도 아… 이건좀… 싶은 요소들이 많이 보였다… 마침 프로젝트 전체 리팩토링 및 배포 준비 과정에 들어가 내 코드를 리팩토링하였음

기능은 이전과 똑같이 동작하나 몹쓸 코드들만 개편한 것에 가깝다

합의한 사항들

폴더 구조

src/components
└── pages
    └── Mypage
        ├── assets       // folder  
        ├── components   // folder
        ├── constants.js
        ├── hooks        // folder
        ├── index.jsx
        ├── styles       // folder
        └── utils        // folder

src/components 하위의 pages 폴더 안에 각 페이지 컴포넌트를 넣고, 페이지별로 hooks, styles, utils, assets 등 페이지 내에서만 쓰는 요소들을 묶어놓기로 한다

모든 페이지에서 공통으로 사용하는 컴포넌트들은 src/common 폴더에 모아두고, 각 페이지에서 import해오는 구조를 채택했다

기존에는 아토믹 디자인을 도입하고자 했었는데, 아토믹 디자인에 대한 이해가 부족한 상태에서 폴더 구조를 잡으려 하니 어느 요소가 organisms에 들어가야 하고, 어느 요소가 atoms에 들어가야 하는지 명확한 기준이 없어 혼란만 가중되었다… (나는 그랬음)

차라리 공용 컴포넌트만 common 등의 폴더에 모아두고, 페이지 내에서만 사용하는 컴포넌트나 훅 등은 해당 페이지 폴더 안에 모아두는 것이 나중에 알아보기에도 훨씬 간단하다

평소에도 components나 router 내에 페이지 컴포넌트별로 폴더를 구분하고 공용 컴포넌트만 별도의 폴더로 분리하는 구조를 자주 채택하였어서 지금 선택한 구조가 훨씬 익숙하고 작업하기에도 쉬웠다 (절대경로 적을 때 헷갈리지 않는 것은 덤)

상수는 별개의 파일로 분리하기

const Mypage = () => {
    ...
  const ARTICLE = 1,
    COMMENT = 2,
    LIKED = 3;
    ...

기존에는 이 상수들을 일일히 컴포넌트 내에 선언해가면서 썼었는데, 해당 상수들을 공통적으로 사용하는 컴포넌트가 많음에도 불구하고 모든 컴포넌트에 일일히 선언하는 방식은 거창하기 그지없다

또한 만약에 상수명을 쓰지 않고 1, 2, 3 등 값을 그대로 써 버리면.. 이게 뭐하는 값인지도 알 수가 없다

옆에 주석을 달아놓는 방법도 매 로직 줄마다 주석을 달 수는 없는 노릇이기에…

const constants = {
  ARTICLE: 1,
  COMMENT: 2,
  LIKED: 3,
};

export default constants;

상수를 별개 파일 내의 객체로 분리하여 이 상수들을 사용하는 컴포넌트들은 해당 객체를 불러오기만 하면 되도록 작업하였다

모든 컴포넌트가 공통의 상수값을 가지니 혹시나 값에 변동이 있더라도 오류를 예방할 수 있다

1 파일 1 컴포넌트

어지간한 예외 상황을 제외하고, 파일 하나당 컴포넌트 하나를 유지하고, export되는 컴포넌트 또한 딱 하나만을 유지한다 (export default)

파일 하나에 여러 개의 컴포넌트가 존재할 경우, 관심사가 분산되기도 하고, 파일명으로만 보았을 땐 이 파일이 어떤 컴포넌트를 담고 있는지 확연하게 들어오지 않아 다른 사람이 코드를 읽을 때 어려움이 발생할 수 있다

컴포넌트 최소화

컴포넌트를 쪼갤 수 있을 때까지 잘게 쪼개보자

다만 나는 한 가지 규칙을 더 추가했는데, 다른 곳에서 공용으로 사용되지 않는 5줄 이하의 컴포넌트는 굳이 분리할 필요가 없다고 판단하였다

예를 들어, 페이지 내부에서 버튼의 스타일을 통일하고 싶을 땐 css-in-js 컴포넌트로 감싼 형태의 새로운 컴포넌트를 분리하여 만들면 요긴하게 사용되지만, 특정 컴포넌트 내에서만 사용되는 하위 컴포넌트라면 간단한 형태는 굳이 분리할 필요성을 느끼지 못해 다 합쳐주었다

오히려 파일 개수가 너무 늘어나다 보면 혼란을 줄 수 있지 않나? 라는 생각도 있는데 이 부분은 개인차인 것 같다… 장단이 있다보니…

컴포넌트명과 상수명은 대문자, 디렉토리명은 소문자

Mypage
    └── index.jsx 

예외로 컴포넌트 폴더를 만들고, 내부의 index.jsx에서 컴포넌트를 선언했을 경우, 폴더명만으로 import가 가능하므로 (import Mypage from ‘components/Mypage’ 이런 식으로) 폴더명 = 컴포넌트명 이라고 생각하여 대문자로 시작하게 하였다

왠만하면 let 대신 const 쓰기

리액트에서 변동가능한 값은 대부분 useState를 사용하므로, let을 사용하는 경우가 거의 없다

로직은 최대한 깔끔하게

남이 코드를 읽어봐도 이해하기 쉽도록 작성하기

삼항 연산자 등의 구문은 중첩 방지

삼항 연산자가 깔끔하게 쓰면 정말 좋은 구문은 맞지만, 여러 번 중첩하는 등 복잡하게 사용하면 오히려 if문이나 switch-case문을 쓰는 것보다 못한 결과물이 나올 수 있다

내가 그랬음… (ㅋ) 삼항 연산자를 삼중으로 중첩했던가…

정말 정말 지양해야 한다 삼항연산자 특성상 엄청 간결하게 생겼기 때문에 중첩할수록 더 헷갈림과 혼란만 가중된다

View 파트 안에 로직 넣지 않기

조건부 렌더링 등의 이유로 삼항연산자, 배열 안의 값들 렌더링을 위해 map 메서드까지는 사용할 수 있어도 변수를 선언한다던지, 값을 연산한다던지 하는 행위는 지양하자

왠만하면 커스텀 훅으로 분리된 로직, 또는 그냥 로직 부분에서 모든 것을 계산하고, 그 결과값만으로만 뷰 렌더링을 해야 한다

핸들러 이름은 handle + 명사 + 동작

예시: handleButtonClick

다른 프로젝트에서는 handleClickButton과 같이 handle + 동작 + 명사의 형태를 취하고 있어서 리팩토링 시에 무의식적으로 다 바꿔줬다가… 원상복구했다

(개인적인) 리팩토링 규칙

어지간하면 파일 하나의 길이를 100줄 이하로 작성하기

파일이 길어질 수록 시선이 분산된다… 여러 프로젝트를 거치면서 이 부분을 제일 중요하게 생각하게 된 것 같다

특히 컴포넌트 안에 로직을 작성하다 보면 useEffect 등이 기하급수적으로 길어지면서 내가 지금 작성하고 있는게 A 컴포넌트인지 B 컴포넌트인지 계속 파일명을 확인해야 하는… 그런 헷갈리는 일들이 다소 발생했다

button이나 input이 여러 개인 컴포넌트일 수록 핸들러의 개수도 엄청나게 많아지는데… 컴포넌트 안에 십수 개의 핸들러가 존재한다고 생각하면 아득하다… 내가 내 코드 보면 차라리 상관없는데 남이 내 코드를 본다고 생각하면 하하

로직과 스타일 등을 최대한 분리하고, 컴포넌트를 최소단위로 쪼개가면서 (그렇다고 너무 잘게 쪼개는 것도 좀 그렇다) 파일을 간결하게 - 최대한 스크롤을 많이 내릴 필요가 없게 - 작성하려고 했다

로직은 커스텀 훅으로 분리하기

추가 룰로 컴포넌트 코드 내에서 React, react-router-dom 불러오지 않기…가 있었다

그리고 로직을 전부 훅으로 분리하고 필요한 핸들러나 상태값만 가져와서 사용하니 react-router-dom의 Link 사용할 때 말고는 이 규칙을 다 지킬 수 있었다

Link도 버튼이랑 onClick, useNavigate() 를 사용할까 했는데 다른 분이 작업하신 (다른 페이지에서 사용하는) 컴포넌트를 감싸는 형태로 만들어야 해서 일단은 Link로 감싸주었다 ⇒ 추가 작업이 필요할 수도 있겠다

아무튼… 커스텀 훅으로 분리하면 훅 내에서 useEffect, useState와 같은 작업들을 전부 처리하고, 특정 핸들러에서만 사용하는 상태값을 굳이 컴포넌트로 가져오지 않아도 함수만 가져와 사용할 수 있어서 매우 간편하다

예전에는 커스텀 훅 하면 되게 어렵게 느껴지고 손이 잘 안 갔는데 지금은 많이 익숙해진 편… 사실 훅 안에 있는 내용물이 전부 컴포넌트 안에 전개된다고 생각하면 쉽다

const useTestHook = () => {
    const [test, setTest] = useState(false);

    useEffect(() => {
        if (test) console.log("aaa");
    }, [test]);
}

const TestComponent = () => {
    useTestHook();
}

위의 코드가

const TestComponent = () => {
    const [test, setTest] = useState(false);

    useEffect(() => {
        if (test) console.log("aaa");
    }, [test]);
}

이것과 결과적으로 같은 동작을 하기 때문… 대신 컴포넌트가 짧고 간결해진다

또한 위의 예시 코드에서 상태값 test는 useEffect 내에서만 사용되므로 TestComponent에서는 굳이 받아올 필요가 없다

이처럼 커스텀 훅 안에서만 쓰는 상태값들은 훅 안에만 배치하면 돼서 컴포넌트 내에서 사용하는 불필요한 코드가 많이 줄어든다

const handleClickGoBack = () => {
    navigate('/mypage');
  };

  const handleClickPrev = () => {
    setCurPage(1);
  };

  const handleClickPageNum = page => {
    setCurPage(page);
  };

  const handleClickNext = () => {
    setCurPage(pageCount);
  };

  useEffect(() => {
    fetchMyArticles();
  }, [articleType, curPage]);

  useEffect(() => {
    setPageList(getPageList());
  }, [pageCount]);

MyArticleBoard 컴포넌트 내에 위와 같은 핸들러와 useEffect 모음이 있다고 하자

pageCount 등의 상태값과 getPageList() 함수는 해당 핸들러와 useEffect 내에서만 사용되는 값이다

const MyArticleBoard = ({ articleType }) => {
    const [articles, setArticles] = useState([]);
  const [curPage, setCurPage] = useState(1);
  const [pageCount, setPageCount] = useState(1);
  const [pageList, setPageList] = useState([]);
  const navigate = useNavigate();

    ...

원래대로라면 이렇게 컴포넌트 내에 상태값을 전부 선언해주었어야 한다

무수히 많은 상태값은 혼란만 가중시킨다

const MyArticleBoard = ({ articleType }) => {
  const {
    articles,
    articleInfo,
    curPage,
    pageList,
    handleClickGoBack,
    handleClickPrev,
    handleClickNext,
    handleClickPageNum,
  } = useMyArticleBoard(articleType);

  return (
        ...

하지만 훅으로 로직 부분을 전부 분리하면 컴포넌트 내에서 실제로 사용하는 핸들러만 반환값으로 받아와 사용하면 된다

pageCount, getPageList()와 같이 핸들러 안에서만 사용하는 상태값이나 함수는 가져오지도 않았다

그냥 훅 안에만 선언해주면 내부의 핸들러들이 알아서 가져다 쓰기 때문이다

이처럼 로직을 전부 훅으로 옮겨주면 관심사 (뷰 / 로직) 가 분리되고, 코드가 짧고 간결해지며 import 라인도 확연히 짧아진다는 장점이 있다

단점이라면… 컴포넌트만으론 이 함수가 무슨 역할을 하는지 알기 힘드니 코드를 두개 열어놓거나 이름을 찰떡같이 붙여야 한다는 것 정도…? 그래도 이름짓기는 나름 자신있다?

스타일 컴포넌트도 별도의 파일로 분리하기

scss를 사용할 때는 어차피 확장자가 달라 필연적으로 파일을 분리할 수밖에 없었는데 css-in-js 라이브러리를 쓰다 보니까 스타일 컴포넌트를 컴포넌트 파일 안에 같이 배치하는 일이 왕왕 있다

하지만 스타일 특) 엄청 긺… 클래스 안의 클래스나 가상선택자까지 포함하면 엄청 길어지므로 무조건 분리하는 것이 좋다

개인 프로젝트를 진행할 때는 컴포넌트와 하위 컴포넌트를 전부 폴더로 구분하고 폴더 안에 scss 파일을 배치하는 방식을 사용했는데 이번에는 페이지 컴포넌트 안에 styles 폴더를 만들어서 모아두는 방식을 채택했다

css-in-js이다 보니까 확장자가 js인 터라 스타일들을 별도 폴더로 분리해서 모아놓지 않으면 뷰나 로직 컴포넌트랑 조금 헷갈릴 수도 있겠다 싶기도 하고, 어차피 hook이나 utils 폴더도 분리해놓으므로 styles도 분리하는 게 통일성 면에서 좋아보인다

import할 땐 절대경로로

import { QuickLink } from 'components/organisms/main';
import { MyArticlePreview, MyArticleBoard, MypageProfile } from 'components/pages/Mypage/components';
import constants from 'components/pages/Mypage/constants';
import { useMypage } from 'components/pages/Mypage/hooks';

import { StyledMypage } from 'components/pages/Mypage/styles';

경로가 많이 길어지면서 안 예뻐보일 수 있지만, 개인적으로 ../가 많아질 수록 (깊이가 깊어질 수록) 이전 경로가 어딘지 찾기 힘들어 가독성이 더 떨어진다

../는 한 경로당 3개 미만으로 사용하고, 어지간하면 절대경로를 사용하.. 려고 했는데 어쩌다 보니 전부 절대경로로 작성해 버렸다

폴더 구조가 조금이라도 변동되었을 때 파일을 못 찾는 오류가 발생하면 난감하기도 하고, components/pages/Mypage와 같은 공통부분만 제외하면 맨 끝의 단어만 보고 어느 부분을 담당하는 파일인지 (constants, hooks, styles…) 단박에 알 수 있기 때문

앞의 공통부분을 환경변수로 묶어줄 수 있다는데 도입해볼까..?

결과

로직, 뷰, 스타일을 전부 개별 폴더로 분리해 놓으니 한 파일당 길어야 80~90줄 내외로 작성을 마칠 수 있었다

페이지의 어떤 부분을 담당하는 파일인지도 알기 쉽게 되었고, 해당 페이지 안에서만 사용하는 애셋 (svg) 들을 따로 모아놨기 때문에 전체 프로젝트의 애셋 폴더가 복잡해질 염려도 없다

리팩토링 막 시작할 때 내가 담당한 페이지 딱 열어보고 되게 참담했는데 (너무… 너무 장황한 더티코드임…) 막상 정리를 끝내고 나니 방 대청소와 분리수거를 끝낸 기분이 들어 개운하다

풀 리퀘스트 리뷰 후 조금 더 보완하고, 이번 경험을 토대로 남이 읽기 쉬운 코드를 작성하도록 더욱 심신을 갈고닦아야겠다

내가 임의로 세운 규칙도 나름의 이유로 설명할 수 있어야 한다고 생각이 들어서… 글로 생각을 정리하다 보면 설명이 좀 더 쉬워지지 않을까 싶네