코드리뷰 신청합니다. by YeonsuBaek · Pull Request #5 · ericjypark/code-review

YeonsuBaek · 2024-01-01T01:37:22Z

Pull Request 템플릿

배포

배포된 페이지가 있다면 링크를 남겨주세요

링크

로컬 환경

로컬 환경 확인이 필요하다면 예시와 같이 남겨주세요

# install
npm install

# run
npm run dev

리뷰 요청 ✍️

- 신청하게 된 계기, 간단한 사연

Eric Park님의 리액트 최적화 포스팅과 애니메이션 개선 포스팅을 보고 어떤 프로젝트와 공부를 하시는 지 더욱 궁금해져 깃헙을 방문하게 되었습니다.

저는 컴퓨터공학부에 재학하면서 2023년 7월부터 12월까지 프론트엔드 개발 인턴으로 활동하였습니다.
인턴 생활 중에 가장 도움이 되었던 것은 사수님께 받는 코드리뷰였습니다. 처음에는 지적 받는 것이 부끄러웠지만 그 과정에서 제 코드 퀄리티가 많이 성장한 것을 느꼈습니다.
이제는 인턴을 수료해서 제 코드를 적극적으로 리뷰해줄 사람이 없어서 다시 혼자 고군분투를 해야합니다.
이 때 Eric님의 코드리뷰 레파지토리를 보고 한 줄기 빛이 보여 바로 신청하게 되었습니다.

- 중점적으로 리뷰받고 싶은 부분

컴포넌트를 올바른 레벨로 분리하였는가
커스텀 훅이 코드 최적화에 긍정적인 영향을 미쳤는가
state와 props가 적절하게 사용되었는가
필요 없는 렌더링이 발생할 가능성이 있는가

- 나누고 싶은 고민

저는 코딩테스트(알고리즘)에 취약한 편입니다.
2024년 상반기 안에 취업하는 것이 목표라면 코딩테스트 공부를 꾸준히 하는 것이 좋을 지, 아니면 프로젝트와 기능 구현에 매진하여 코딩테스트를 보지 않는 기업에만 지원을 해야할 지 고민입니다.

ericjypark · 2024-01-01T13:22:13Z

안녕하세요!
PR과 함께 프로젝트의 소스코드를 업로드 해주셔야만 코드리뷰가 가능합니다!

YeonsuBaek · 2024-01-01T13:27:58Z

@Bokdol11859 소스코드 업로드 하였습니다! 알려주셔서 감사합니다!

ericjypark · 2024-01-01T13:53:58Z

빠르게 업로드 해주셔서 감사합니다!
연수님 이전에 먼저 신청해주신 분이 계셔서 그 분 코드 리뷰 후 일주일 내로 리뷰해드릴게요!
신청해주셔서 감사합니다!

ericjypark

안녕하세요 연수님! 코드리뷰 신청해주셔서 감사합니다!

사연이 되게 공감이 되네요!
저도 예전에는 코드리뷰 받는 것을 부끄러워하기도 했고, 이런걸 하나하나 다 따져야 하나라고 생각을 했었던 시기가 있었는데, 코드리뷰를 통해 제 코드의 퀄리티가 많이 발전한 것을 체감하기도 했었고, 사소한 디테일 하나하나가 협업을 위해서는 정말 중요한 요소들이라고 느끼게 되어 그 이후부턴 저도 코드리뷰를 되게 중요시 여기게 된 것 같네요..!
당연히 연수님의 사수분 만큼 리뷰를 잘 해드리지는 못하겠지만, 그래도 도움이 되셨으면 하는 바램에 꼼꼼히 읽으면서 재밌게 리뷰했습니다!

우선 전체적인 코드의 구조와 폴더의 구조는 잘 짜여진 것 같습니다!
다만 구조를 살펴보면서 두가지 생각이 들었습니다!

components라는 폴더 안에 blocks와 features/main 폴더 두개가 존재하는 것을 확인했습니다.
혹시 그 두 폴더를 나누는 기준이 따로 있으실까요?? 뭔가 처음 딱 봤을 때는 atomic pattern과 유사한가 싶었는데, 그러면 modal이 blocks가 아니라 features/main 에 위치해있어야 하지 않나 싶더라구요!
개인적으로 프로젝트의 폴더 구조는 정말 정말 중요하다고 생각하기에, 한번 깊게 고민해보시고 본인이 생각하기에 가장 편한 폴더 구조를 하나 구축하는 것도 도움이 되실 것 같습니다!
hooks 폴더 내부에 모든 훅들이 위치한 것 같은데, 분명 다 훅인 것은 맞지만, 역할이 조금씩 다르기에 구분을 지을 수 있지 않을까 싶었습니다.
예를 들어, useAuth, useLogin, useLogout의 경우 프로젝트의 모든 곳에서 재사용할 수 있는 훅들인 반면에, useGetOgsm, useSaveOgsm, useMutation은 Ogsm에 강결합되어있는 훅들로 보입니다!
분명 모두 훅인 것은 맞지만, 공통 훅들과, 서비스 종속적인 훅들 사이에는 꽤 큰 차이가 존재하지 않나라는 생각이 듭니다!
그렇기에 더 확실한 관심사 분리를 위해서는 hooks 폴더 내부에 ogsm이라는 폴더를 만들어 Ogsm관련 훅들을 관리하고, common이라는 폴더를 만들어 공통적으로 사용하는 훅들을 관리하는게 좋지 않을까라는 생각이 들었습니다!

더불어 필요 없는 렌더링이 발생할 가능성이 있는가에 대해서 질문을 주셨는데, 아마 매우 높은 확률로 불필요한 리렌더링이 발생하고 있을 것으로 보입니다!
하지만 이 불필요한 리렌더링이 과연 사용하는데에 불편함을 주고있나에 대해서는 확실하지 않은 것 같습니다.

서비스가 엄청나게 커질 것을 이미 알고 있다면 처음부터 최적화에 신경을 쓰면서 개발하는 것이 장기적으로 좋은 선택이지만, 그렇게 커지지 않을 서비스라면 최적화에 신경을 쓰는 것이 개발 속도를 꽤나 늦추곤 합니다.
그런 경우에는 오히려 최적화가 필요해질 때쯤 신경쓰더라도 충분했던 것 같네요!
다만 처음부터 서비스가 커지는 것이 예상이 된다면, 나중가서 최적화를 신경쓰기에는 늦었을 수도 있기에 일찌감치 신경쓰는 것이 좋다고 생각이 됩니다!

각각의 장단점들이 존재하기 때문에 상황에 맞춰서 최적화를 신경쓰는게 좋지만, 개인적으로 취준 시기에는 한번쯤 최적화를 과할 정도로 신경써보는게 꽤나 도움이 된다고 생각합니다! 저는 그렇게 했던게 취업하고도 꽤 도움이 많이 됐었던 것 같네요!

저는 코딩테스트(알고리즘)에 취약한 편입니다. 2024년 상반기 안에 취업하는 것이 목표라면 코딩테스트 공부를 꾸준히 하는 것이 좋을 지, 아니면 프로젝트와 기능 구현에 매진하여 코딩테스트를 보지 않는 기업에만 지원을 해야할 지 고민입니다.

연수님께서 남겨주신 고민은 정말 수많은 개발 취준생 분들이 많이 하는 고민이고, 되게 어려운 고민이라고 생각합니다. 저도 예전에 이 고민을 되게 많이 했었던 것 같네요.

조금 예민한 부분이기에 딱 잘라서 어떻다 라고 말씀드리기는 쉽지 않지만, 제 개인적인 생각을 말씀드리겠습니다!

결론부터 말씀을 드리자면, 알고리즘이 어렵다고 코딩테스트를 아예 포기하기에는 너무나도 많은 기회들을 시작하기 전부터 날리고 시작하는거라고 생각합니다.

제가 취준하던 시기에 발견했던 사실은, 큰 회사일수록 알고리즘같은 기본기를 더 중요시 여긴다는 점이었습니다. 가고싶은 회사일수록 코딩테스트가 없는 경우는 진짜 흔치 않더라구요. 특히 외국계 회사들, 흔히 말하는 빅테크 회사들은 코딩 인터뷰가 없는 경우가 없고, 그 과정 및 결과를 정말 중요하게 여기곤 합니다.

알고리즘 몰라도 다 취업한다 라는 말을 저도 꽤나 많이 들었었습니다. 그리고 전혀 틀린 말이 아닙니다. 알고리즘을 몰라도 실무 능력이 뛰어난 사람은 당연히 취업이 가능하고, 실제로 저희 회사에도 그런 분이 계셔서 저도 저 말에 백번 공감합니다.

하지만 저는 알고리즘이 너무나도 중요하다고 생각합니다. 물론 잘못된 논리일 수도 있겠지만, 만약 알고리즘이 중요하지 않은 요소였다면, 빅테크 기업들은 왜 아직까지도 코딩 인터뷰를 그렇게 중요시 여기는걸까에 대한 대답은 저는 아직까지도 찾지 못했습니다.

그렇기에 연수님이 갖고 계신 고민에 대해 제 생각을 말씀을 드리자면,

코딩테스트를 보지 않는 기업에만 지원을 하여 취업을 하는 것은 분명 가능합니다. 하지만 요즘 같은 불경기에는 채용공고의 수가 이미 너무 적은데, 처음부터 선택지를 줄이고 시작하는게 과연 좋은 선택일까? 라는 의문이 듭니다.

저는 알고리즘을 아예 못하던 상태에서 딱 6개월 죽어라 하니까 취준 시절 한번도 코테에서 떨어진 적은 없었습니다.
남들은 코테 무서워서 코테 없는 회사들 지원하려고 하는데, 저는 오히려 코테 있는 회사들을 더 찾고 싶어지더라구요. 딱 6개월만 죽어라 하니까 제 단점이 오히려 장점이 됐었고, 제가 지원할 수 있는 회사의 풀 역시 훨씬 더 넓어졌고, 회사의 평균 규모 역시 훨씬 더 커졌습니다.

정답은 존재하지 않지만, 연수님께서 가고 싶으신 꿈의 회사들을 떠올려보시고, 그 회사들이 코테를 중요시 여기는지 안여기는지를 고민해보시면 고민에 도움이 되시지 않을까 싶습니다!

좋은 프로젝트 만드시느라 수고 많으셨습니다. 혹시라도 궁금하거나 이해가 되지 않는 부분이 있으시다면 편하게 코멘트 남겨주세요!

ericjypark · 2024-01-08T12:34:18Z

+  width: 100%;
+  height: 100%;
+  background-color: #fff;
+  border-radius: 0 !important;


꼭 !important가 필요한 이유가 있으셨을까요??
외부 라이브러리의 스타일을 강제로 변경하려는게 아니면, 전 개인적으로 진짜 진짜 최대한 피하려고 하곤 합니다!

MUI에 기본적으로 설정된 border-radius를 강제로 변경하느라 사용하였습니다!
사실 다음에 리팩터링 하면서 제가 만든 UI 라이브러리를 적용해볼 예정이라 !important를 제거할 수 있을 것 같습니다:)
https://github.com/YeonsuBaek/yeonsui

ericjypark · 2024-01-10T07:43:19Z

+    "@emotion/react": "^11.11.1",
+    "@emotion/styled": "^11.11.0",
+    "@mui/icons-material": "^5.14.18",
+    "@mui/material": "^5.14.18",
+    "@mui/x-date-pickers": "^6.18.2",


리뷰랑은 조금 별개이긴 하지만, app router를 100% 활용하려면 Runtime-CSS-in-JS에서 벗어나야합니다,,
최근에는 tailwindcss 또는 vanilla extract를 많이들 사용하더라구요!

코드를 조금 살펴본 결과, emotion을 사용하지 않는 것 같은데, mui 때문에 emotion가 딸려온걸까요??

네! mui를 설치하면서 emotion도 같이 설치된 것 같습니다! 따로 emotion을 사용한 부분은 없습니다

ericjypark · 2024-01-10T07:51:32Z

+export enum NUMBER_SUFFIX {
+  NULL = "",
+  FIRST = "st",
+  SECOND = "nd",
+  THIRD = "rd",
+  OTHER = "th",
+}


이 경우 const enum을 찾아보시면 도움이 되실 것 같아요!

enum은 번들사이즈를 늘린다는 단점이 있는데, const enum은 그 단점을 어느정도 해소해줍니다!

물론 은탄환은 없기에 const enum에서는 사용할 수 없는 기능이 존재하게 되는데, 그게 무엇인지도 한번 살펴보시면 좋을 것 같아요!

ericjypark · 2024-01-12T01:12:47Z

실무에서는 보통 CSS-in-JS나 TailwindCSS 등등을 많이 사용하는 것 같아요!
아예 전부 다 한 css 파일에서 관리하는 경우는 많지 않은 것 같아서, 실무에서 많이 사용하는 방식에 익숙해지면 도움이 되시지 않을까 싶습니다!

ericjypark · 2024-01-12T01:13:55Z

+        <Header refetch={() => refetch()} />
+        <Content ogsmList={ogsmList} refetch={() => refetch()} />


둘 다 refetch 함수를 바로 넘겨줘도 될 것 같습니다!

ericjypark · 2024-01-14T06:57:17Z

하나의 모달에는 정말 많은 정보가 들어갈 수 있기 때문에 코드가 길어지기 쉽습니다!
하지만 그렇게 되면 코드를 잘 파악하고 있는 사람이 아니라면 이해하기가 쉽지 않아지겠죠..!

더불어 현재 이 OgsmModal 컴포넌트가 너무 많은 상태를 다루고 있기 때문에 하나의 상태 변경이 발생하면 그로 인해 모든 불필요한 부분들까지 다 리렌더링이 되게 될 것 같습니다!

분명 하나의 모달인 것은 맞지만, 이 모달 역시 여러개의 컴포넌트로 나눌 수 있지 않을까요??

그렇게 하면 코드의 가독성 측면에서도, 성능 측면에서도, 더 좋을 것 같습니다!

ericjypark · 2024-01-14T06:59:21Z

+          const list = response.data()["ogsm"].map((item: any) => {
+            return {
+              ...item,
+              id: item.id || `${id}-${item.goal}`,
+            }
+          })
+          setData(list)


여기서 (item: any)는 깨지기 정말 쉬운 약속이 될 것 같습니다!
item의 인터페이스를 미리 알 수 있는 방법이 없으셨을까요??

ericjypark · 2024-01-14T07:03:17Z

+  const onRefetch = () => {
+    setIsRefetch(true)
+  }


onRefetch라는 이름은 refetch가 발생할 때 어떤 로직을 실행하기 위한 함수처럼 느껴집니다!
하지만 현재 이 함수는 refetch를 발동시키는 함수로 보이는데, 그렇기에 함수의 역할과 함수의 이름 사이에 불일치가 존재하는 것 같습니다!

refetch, triggerRefetch, 등등의 이름이 함수의 역할과 더 어울리지 않을까요??

ericjypark · 2024-01-14T07:06:28Z

+  const mutate = async (data: DATA_TYPE, mutationFn: MUTATION_FN_TYPE) => {
+    const provider = data
+    const { onSuccess, onError } = mutationFn
+    setIsLoading(true)
+
+    try {
+      const response = await signInWithPopup(auth, provider)
+      if (response) {
+        onSuccess(response)
+      }
+    } catch (error) {
+      setError(error)
+      onError()
+    } finally {
+      setIsLoading(false)
+    }
+  }


일반적으로 커스텀훅에서 외부로 전달하는 함수 또는 변수의 경우 많은 곳에서 참조하게 됩니다.
그렇기에 더더욱 커스텀훅 내부의 함수/변수들은 useCallback과 useMemo의 사용이 중요하지 않을까요??
그러지 않으면 불필요한 리렌더링들이 커스텀훅 하나 때문에 계속 발생할 수도 있으니까요!!

ericjypark · 2024-01-14T07:11:36Z

+  isOpen: boolean
+  setIsOpen: (isOpen: boolean) => void
+  ogsm?: OGSM_TYPE
+  ogsmList: OGSM_TYPE[]
+  onDelete: (id: number) => void
+  onSave: (newOgsm: OGSM_TYPE) => void


되게 사소한 부분이지만, 저는 optional한 프로퍼티들은 하단에 같이 묶어두곤 합니다!

interface AddItemModalProps { isOpen: boolean setIsOpen: (isOpen: boolean) => void ogsmList: OGSM_TYPE[] onDelete: (id: number) => void onSave: (newOgsm: OGSM_TYPE) => void setSelectedItem: (id: undefined) => void ogsm?: OGSM_TYPE otherProp?: boolean otherProp?: boolean otherProp?: boolean }

이렇게 하면 한눈에 어떤 값들이 optional인지를 파악할 수 있어서 좋더라구요!

YeonsuBaek · 2024-01-17T00:22:47Z

꼼꼼한 답변과 코드 리뷰 정말 감사합니다! 너무 자세히 설명해주셔서 많이 배웠습니다. 하나씩 살펴보며 더 좋은 방향으로 바꿔나가도록 하겠습니다. 소중한 시간 할애해주셔서 정말 감사합니다 :)

Update pull_request_template.md

4bd9f79

코드 업로드

50a1348

ericjypark reviewed Jan 14, 2024

View reviewed changes

		<Header refetch={() => refetch()} />
		<Content ogsmList={ogsmList} refetch={() => refetch()} />

Conversation

YeonsuBaek commented Jan 1, 2024

Pull Request 템플릿

배포

로컬 환경

리뷰 요청 ✍️

- 신청하게 된 계기, 간단한 사연

- 중점적으로 리뷰받고 싶은 부분

- 나누고 싶은 고민

Uh oh!

ericjypark commented Jan 1, 2024

Uh oh!

YeonsuBaek commented Jan 1, 2024

Uh oh!

ericjypark commented Jan 1, 2024

Uh oh!

ericjypark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YeonsuBaek commented Jan 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants