안녕하세요! TA 장태영입니다. 간단하게나마 리뷰를 남겨봤습니다!

안녕하세요! 저는 지금 참여하고 계신 스터디에서 TA 역할을 맡고 있는 장태영이라고 합니다!

영서님께서 만들어 주신 노션 페이지에 깃헙 레포를 공유해주셔서 간단하게나마 코드 리뷰를 해드리려고 합니다 제가 고수는 아니라서 가볍게 이런 생각을 하고 있는 사람도 있구나 .. 하고 봐주세요. 🥹

지금은 프로젝트 규모가 작다 보니 크게 리뷰할 게 없을 수도 있습니다! 참고해주세요. 🙏

서론이 길었네요! 바로 시작해보겠습니다. 🚀

$ 표시

styled-components로 만든 컴포넌트들의 커스텀 속성에 $ 표시가 붙어 있는 것 같네요. 혹시 붙이시는 이유가 있을까요? ImgBox 컴포넌트의 경우 backface에는 없고 zIndex에는 있다 보니 어떤 기준이 있는건가 해서요!

img 태그를 래핑한 컴포넌트들의 img 태그 내장 src 속성에 커스텀 속성을 연결해주는 부분도 있다 보니 궁금해서 여쭤봅니다!

상수 활용

반복해서 사용되는 숫자를 대문자 상수로 만들어 재사용하신 부분은 좋네요!

이런 숫자들은 보통 매직 넘버라고 불립니다. 왜 “매직” 넘버라고 부르냐면 제 3의 개발자가 이 코드를 봤다고 생각해보면 해당 숫자들이 무엇을 의미하는지, 왜 하필 그 숫자인지 알기가 쉽지 않겠죠? 마법 같은 숫자라고 느끼다 보니 매직 넘버라고 합니다.

문자열도 동일해요. 텍스트도 제 3자가 보기엔 충분히 헷갈릴 수 있겠죠? 그리고 알아보기 어렵다는 점 외에도 여러 군데 반복해서 사용되면 이를 상수화 해놨을 때 수정하기도 쉽고 추가로 사용하기도 쉬울 겁니다! 여러모로 이득이 있는거죠. 👍

뭐든 그렇겠지만 무분별하게 늘리는건 또 오히려 독(이것 또한 관리 포인트니까요)이 될 수 있어서 적절히 사용하는게 가장 좋습니다! :)

관심사 분리해보기

저라고 잘 하는건 아니지만 평소에 최대한 관심사를 분리하려고 노력합니다.

리액트에는 훅이라는 좋은 도구가 있기 때문에 특정 작업에 관련된 코드만 모아서 훅에다가 담아주면 해당 훅을 사용하는 곳에선 특정 작업에 대한 상세 내용은 몰라도 되고 훅에서 반환된 값만 받아서 사용하면 되는 그림이 나오게 됩니다.

예를 들어, Detail이라는 페이지 컴포넌트를 구현하고 있을 때 해당 컴포넌트에서 API URL이나 이펙트 함수 내부 로직이 페이지 컴포넌트 입장에서 궁금한 부분일까요?

UI를 표현하기 위한 컴포넌트는 UI를 표현하기 위한 데이터만을 받아서 뿌려주는 걸로 역할이 충분할 수 있지 않을까요?

사실 이건 컴포넌트가 복잡해지기 시작하면 간단한 부분은 아닌데 이런 맥락이다 라고만 생각하시면 좋습니다. :) 그리고 이것도 기준없이 모든걸 커스텀 훅 안으로 넣어버리면 코드만 숨겨질 뿐 내부가 복잡한 건 여전하거든요! 그러면 보기 힘들어 지게 뎁스만 늘어날 뿐인 거죠.

그래도 연습은 필요하다고 생각해요! 조금씩 해봐야 이건 별로인 것 같고 이건 좋은 것 같다라는 기준이 생기니까요! 제가 느끼기에 Detail 컴포넌트 내부 titleRef와 그에 관련된 로직들을 커스텀 훅으로 분리해볼 수 있을 것 같아요.

스크롤에 관련된 로직은 해당 훅에 모두 존재하고 titleRef와 isScrolled 정도만 반환해주면 Detail 컴포넌트 입장에서는 상세 로직은 모르되 필요한 값들만 매핑해서 쓸 수 있게 되니까요! 이 작업을 통해 만든 커스텀 훅은 title과 해당 로직이 있던 컴포넌트에 의존성이 찐하게 있는거라 항상 이런 형태로 만드는게 정답은 아닙니다. 동일한 로직을 다른 컴포넌트에서도 사용하고 싶으실 수도 있겠죠? 재사용 가능한 형태로 만들 수도 있으니 이런 저런 형태를 구현해보시는 것에 의의를 두셨으면 좋겠습니다. 🥸

추가로 이런 작업들을 하다 보면 지금 데이터 패칭을 해오는 부분도 동일하게 관심사를 분리해볼 수 있지 않을까 고민할 수도 있어요. 제가 담님에게도 해드렸던 리뷰이기도 합니다. 다음의 코드를 추상화해서 API URL이나 함수를 넘겨 데이터 패칭이 발생하고 패칭해온 결과값만 반환받아서 쓸 수 있는 형태로 만들기도 한답니다.

const getHero = async () => {
  const data = await fetch(
    "https://marvel-proxy.nomadcoders.workers.dev/v1/public/characters?limit=50&orderBy=modified&series=24229,1058,2023"
  );
  const json = await data.json();

  if (json.code === 200) {
    setData(json.data.results);
  }
};

useEffect(() => {
  getHero();
}, []);

React Query라는 라이브러리의 useQuery의 모양새나 usehooks-ts의 useFetch 같은 형태인 같은거죠!

제가 적은 내용과 100% 관련된 내용은 아니지만 아주 좋은 내용을 담은 영상 링크를 남기면서 이 주제는 마무리할게요!

토스ㅣSLASH 21 - 실무에서 바로 쓰는 Frontend Clean Code

react-router-dom 경로

import {
  Link,
  useHistory,
  useParams,
} from "react-router-dom/cjs/react-router-dom.min";

이것도 궁금해서 여쭤보는건데 여기서 import 해오시는 이유가 있을까요??

`List`와 `ListItem` 컴포넌트

요건 정답이 없는 부분이긴 해요. 제가 이런 의문이 들었을 때 여러 가지 찾아보고 참고해서 전 이런 형태로 정착한 상태라 참고만 해주세요!

다음의 코드에서 저는 보통 ListItem 컴포넌트를 만들어 주는 편입니다. 목록 컴포넌트가 있으면 해당 목록의 아이템 격인 컴포넌트를 짝으로 만들어 줍니다(아닐 때도 있습니다. 하지만 보통은 짝으로 만듭니다).

<React.Fragment>
  <HomeTitle>Marvel Characters</HomeTitle>
  <GridContainer>
    {datas.map((data) => {
      const pathData = data.thumbnail.path.split("/");
      if (pathData[pathData.length - 1] === "image_not_available") {
        return null;
      }
      // 이미지가 이상함
      if (data.name === "Gorilla Man") return null;

      return (
        <Hero
          key={data.id}
          data={data}
          name={data.name}
          id={data.id}
          imgURL={`${`${data.thumbnail.path}.${data.thumbnail.extension}`}`}
          comicsAvail={data.comics.available}
        />
      );
    })}
  </GridContainer>
</React.Fragment>

이것 또한 어찌 보면 관심사 분리라고 할 수 있겠네요. ListItem 컴포넌트를 만들면 아래와 같은 부분들은 ListItem 컴포넌트 내에서 해결할 수도 있기도 하고 List에서 할 일과 ListItem에서 할 일이 명확하게 구분된다고 생각하는 편이거든요.

const pathData = data.thumbnail.path.split("/");
if (pathData[pathData.length - 1] === "image_not_available") {
  return null;
}

근데 이런건 진짜 사소한 부분이라 이렇게도 생각하는구나 하고 넘어가주시면 좋을 것 같습니다!

폴더명

이것도 진짜 사소한 부분인데 일관성있게 지어주는 것도 좋을 것 같아요. 첫 글자를 대문자로 하고 싶으면 routes도 Routes로 하는 식으로요!

왜 일관성있게 하는게 좋냐면 규모가 커짐에 따라 일관성은 더 중요해질 수 밖에 없어요. 폴더가 많아지고 파일이 많아졌는데 거기에 작업을 하는 개발자도 많아진 상황에서 일관적이지 않게 무엇인가 자꾸 만들어져 있다면 생각보다 훨씬 골치가 아프거든요!

우선 이 정도로 마무리 해보겠습니다! 더 궁금하거나 이해가 안가는 부분이 있으시다면 언제나 편하게 여쭤봐주세요! 🙇‍♂️

@taeyoungs 안녕하세요 태영님! 우선 코드 리뷰 남겨주셔서 정말 감사드립니다. 남겨주신 코드 리뷰를 바탕으로 리팩토링을 해보았는데 시간 나실 때 한번 봐주시면 감사드리겠습니다 😉 https://github.com/ICE0208/marvelous-react/pull/3

$ 표시

backface에는 까먹고 적지 않았는데 props에 $를 안붙이면 로그창에서 아래와 같은 경고가 뜨더라고요.

styled-components: it looks like an unknown prop "backface" is being sent through to the DOM, which will likely trigger a React console error. If you would like automatic filtering of unknown props, you can opt-into that behavior via <StyleSheetManager shouldForwardProp={...}> (connect an API like @emotion/is-prop-valid) or consider using transient props ($ prefix for automatic filtering.)

그래서 일단 경고를 없애기 위하여 props 앞에 $를 붙여놓았는데, 이게 제대로 된 해결 방법인지 궁금하네요!

img태그에 직접 attrs로 src를 지정한거는 제 실수였던 것 같습니다!

상수 활용

매직 넘버라는 용어는 처음 들어보네요. 이번 기회에 알아갑니다!

관심사 분리해보기

관심사 분리!!! 좋은 말씀 감사합니다. 아직 리액트가 익숙하지 않아서 그냥 한 곳에 코드를 다 넣어버렸네요. 남겨주신 영상과 다른 자료들 찾아보면서 연습해보겠습니다!

react-router-dom 경로

import 자동완성으로 해서 저렇게 된거 같은데, 확인해보니 from “react-router-dom” 이 안뜨더라구요. 여러가지 시도해 본 결과 @types/react-router-dom 를 install 하니 자동 완성에도 뜨고 경고도 사라졌습니다!

List와 ListItem 컴포넌트

음… 이 부분은 잘 이해가 안돼서 코드를 좀 수정해보긴 했는데 맞게 이해한 건지 모르겠습니다. 시간되실 때 코드 한 번 봐주시고 피드백 해주시면 감사드리겠습니다..!

폴더명

이런, 왜 components 폴더명이 대문자로 되어있는지 ㅋㅋㅋㅋ.. 소문자로 통일시켜줬습니다!

추가로 궁금한 사항

모든 페이지의 컴포넌트들을 components 폴더에 넣어버리면 어떤게 어떤 페이지의 컴포넌트인지 헷갈릴 것 같은데 보통 어떻게 구분하는지 궁금합니다. 또한 Hero.js 파일을 보면 Container, ImgContainer, TextContainer 등 styled components가 너무 많아서 코드가 길어지는데 이렇게 한 곳에 다 넣는게 맞는지 아니면 다른 폴더에 분리를 시켜주면 좋은지 궁금합니다. 분리를 시킨다면 보통 어떤식으로 처리하는지도 궁금하네요! 아! 그리고 Character.js에서 Title이 특정 조건일 때 배경이 검정색으로 되게 하기 위해서 class를 사용했는데, 더 좋은 방법이 있을 것 같아서 태영님이라면 어떻게 하셨을지 궁금하네요 🥹

다시 한번, 바쁘신 와중에 정성스럽게 코드 리뷰 남겨주셔서 너무 감사드립니다. (꾸벅)

ICE0208 / marvelous-react