[Refactor] useQuiz hook 구현 #170
Conversation
There was a problem hiding this comment.
고생하셨습니다 👍
전반적인 리뷰도 남겼습니다
1.
이 PR의 목적은 useQuiz Hook 구현인데 그에 비해 파일 변동이 너무 많은 것 같습니다
혹시 파일을 열면서 발생하는 린트 에러 때문에 동작에 문제가 생겼나요?
만약에 동작에 문제가 없다면 useQuiz Hook 구현 및 적용에만 집중된 PR이었으면 더 좋은 PR이 되었을 것 같아요 👍
2.
커밋 분리를 하실 때 작업흐름 그대로 남기시는 편인가요?
작업흐름대로 남기면 리뷰어 입장에서는 오히려 리뷰하기가 힘들기 때문에 커밋 방식이 작업흐름이라면 PR에 남겨주시면 좋을 것 같습니다 :)
저는 개인적으로 모든 파일 변화 이후에 파일 분리를 통해 커밋 분리를 하면 좋다고 생각합니다!
| @@ -2,10 +2,10 @@ import { useState } from 'react'; | |||
|
|
|||
| import * as S from './styles'; | |||
|
|
|||
| import type { Quiz as QuizInterface } from '@/interfaces/Quiz'; | |||
| import type { Quiz as QuizType } from '@/hooks/useQuiz/useQuiz.helper'; | |||
There was a problem hiding this comment.
P2
Quiz 를 QuizType 으로 네이밍을 하신 이유가 궁금합니다
| class QuizService { | ||
| static async getShuffledQuizzes(count: number) { | ||
| try { | ||
| const posts = await getPosts(); | ||
| const quizzes = posts.map((post) => createQuiz(post)); | ||
|
|
||
| return shuffle(quizzes, count); | ||
| } catch (error) { | ||
| throw new Error('error occurred at QuizService.getShuffledQuizzes.'); | ||
| } | ||
| } | ||
|
|
||
| static async getQuizzesFromQuizSet(channelId: string) { | ||
| try { | ||
| const posts = await getPostsFromChannel(channelId); | ||
| const quizzes = posts.map((post) => createQuiz(post)).reverse(); | ||
|
|
||
| return quizzes; | ||
| } catch (error) { | ||
| throw new Error('error occurred at QuizService.getQuizzesFromQuizSet.'); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
이 부분에 대한 질문 답변드리기 전에 질문이 이해가 안 가네요 ㅠ
"일단 유즈케이스와 관련이 있기 때문에 나중에 데이터 스키마 형태로 사용할 수 있을 것 같아서 설정했다" 는 말이 정확히 무슨 말인가요?
질문과 별개로 리뷰를 드리자면
이 코드에서 비동기 로직을 분리할 수 있을 것 같습니다
이 부분에 대해서 어떻게 생각하시나요?
| import { updateTotalPoint } from '@/api/UserServices'; | ||
| import Icon from '@/components/Icon'; | ||
| import { POINTS, POST_IDS, USER_ANSWERS } from '@/constants'; | ||
| import { useAuthContext } from '@/contexts/AuthContext'; | ||
| import { useQuizContext } from '@/contexts/QuizContext'; | ||
| import Quiz from '@components/Quiz'; | ||
| import { useQuiz } from '@hooks/useQuiz'; | ||
| import { calculateScore } from '@hooks/useQuiz/useQuiz.helper'; |
There was a problem hiding this comment.
P2
useQuiz Hook에 대한 Helper 함수로 파일분리를 해두신 것 같은데 페이지에서도 함수를 사용한다면 범위를 넓혀야 하지 않을까요?
| // eslint-disable-next-line @typescript-eslint/no-floating-promises | ||
| fetchData(); |
| type ReturnType = [ | ||
| Quiz[], | ||
| (count: number) => Promise<void>, | ||
| (channelId: string) => Promise<void> | ||
| ]; |
There was a problem hiding this comment.
P1
리턴 타입을 배열로 해두면 배열 인덱스에 종속되어 일부만 사용하는 경우에 문제가 생길 것 같습니다
객체 타입으로 변경하는 것은 어떤가요?
📌 PR 설명
useQuiz hook
useQuiz hook의 역할은 서버로부터 퀴즈 데이터를 받아와, 데이터를 useState로 정의된 자료 구조와 연결하는 역할을 합니다.
useQuiz.helper
useQuiz를 구현하면서, api 폴더에 위치하고 있는 퀴즈 관련 도메인을 해당 파일 내부로 이동시켰습니다. 최종 목표는 api 폴더 내부에는 서버에서 원시 데이터를 불러오는 순수 함수만 존재하도록 하는 것입니다.
useQuiz.helper로 이동한 함수는 다음과 같습니다.
QuizSolvePage와 QuizResultPage의 사용 데이터가 달라, 해당 도메인을 분리하였습니다. QuizSolvePage: Quiz, QuizResultPage: Post
QuizService.ts의 파일명 변경은 이번 PR에서 변경하기에는 다소 변경점이 많은 것 같아 변경하지 않았습니다.
함수 선언형으로 작성된 문법을 함수 표현식으로 변경하였습니다.
💻 요구 사항과 구현 내용
✔️ PR 포인트 & 궁금한 점
질문
제안합니다
현재
@typescript-eslint/requiring-type-checkingrule을 사용하고 있는데, 여기서@typescript-eslint/no-floating-promisesrule이 자꾸 문제를 일으킵니다. useEffect의 callback 함수로는Promise<void>형태의 return type이 올 수 없는데(즉 async await형식으로 사용이 불가능한데), 실제 useEffect 내에서 async function을 정의하고, 바로 호출하는 형태로 사용하고 있어서 해당 부분마다 문제를 일으킵니다.해당 eslint 옵션을 off설정하는 것을 제안합니다. 끄지 않고 사용할 수 있는 다른 방법이 있다면, 알려주시면 감사하겠습니다.
close #169