[리팩토링] custom hooks를 사용한 리팩토링 by codeisneverodd · Pull Request #91 · prgrms-fe-devcourse/FEDC2_TimeDivider_Root

codeisneverodd · 2022-06-20T18:47:16Z

개요

타이머를 만드는 과정 들에 사용되는 함수와 변수들을 custom hooks로 묶는 리팩토링을 진행하였습니다.
추가적으로 타이머를 만드는 과정 중 뒤로가기를 해도 이전 값이 유지되어 표시되도록 하였습니다.

작업 내용

세세한 내용은 작업 내용이 많아 코드에 담당하시는 분의 이름을 멘션하여 리뷰를 남기겠습니다.

페이지 라우터 이름 변경
custom hooks 를 이용한 리팩토링

참고 자료

codeisneverodd

다른 분들이 기존에 작성해주신 코드를 변경한 것이 마음에 걸리네요!
제가 짠 코드가 더 낫다는 것이 아닌, 저희 대화중 리팩토링 거리로 나온 것들을 이렇게 했으면 좋겠다라고 제안하는 것 정도로 이해해주시면 좋을 것 같습니다!(말로 하는 것보다 코드가 더 명확하니까요!)

상세하게 리뷰를 남긴다고 남겼지만, 부족하거나 궁금하신 사항이 생긴다면 언제든 말해주세요 😄

codeisneverodd · 2022-06-20T18:48:39Z

src/pages/CreateNameIds/hooks/useNameTag.js

+import { useState } from 'react'
+import { useCreatingTimers } from '../../../shared/hooks/useCreatingTimers'
+
+const useNameTag = () => {


@dahye1013
아래 캡쳐의 각 태그들을 조작하는데 사용되는 함수들을 모은 hooks 입니다.

codeisneverodd · 2022-06-20T18:49:52Z

src/pages/CreateNameIds/hooks/useNameTag.js

+
+const useNameTag = () => {
+	const { nameIds, addNameId, updateNameIds } = useCreatingTimers()
+	const [nameTag, setNameTag] = useState('')


@dahye1013
현재 input에 적고 있는 namTag의 값입니다.
최종적으로 timers의 name에 해당하게 됩니다.

codeisneverodd · 2022-06-20T18:51:46Z

src/pages/CreateNameIds/hooks/useNameTag.js

+	const handleNameTagSubmit = e => {
+		e.preventDefault()
+		const trimmedName = nameTag.trim()
+
+		if (trimmedName) {
+			addNameId(`${Date.now()}${nameIds.length}`, trimmedName)
+		}
+
+		setNameTag('')
+	}


@dahye1013
input에 내용을 적고 추가를 눌렀을 때 발생하는 이벤트입니다.
addNameId(새로운 Id, 새로운 name) 함수를 이용 하여, 새로운 Name-Id 항목을 추가하게됩니다.

Name-Id 은 기존의 코드 tasks 내부에 들어있던 {id,task} 쌍을 의미하며(task의 이름이 혼용되어 name으로 변경하였습니다)
nameIds 는 기존 코드의 tasks에 해당합니다.

저는 개인적으로 함수의 인자를 객체 형태로 인자를 넘겨주는걸 선호해서 ㅎㅎ 이 부분은 수정해서 사용할게요.
함수로 넣으면 순서를 지켜야해서 휴먼 에러가 생길 확률이 높다고 생각해요.

codeisneverodd · 2022-06-20T18:52:54Z

src/pages/CreateNameIds/hooks/useNameTag.js

+	const removeNameTag = removeId => {
+		const filteredNameIds = nameIds.filter(({ id }) => removeId !== id)
+		updateNameIds(filteredNameIds)
+	}


@dahye1013
클릭시 태그가 사라지는 기능을 위한 함수입니다.
삭제할 태그의 아이디를 받고, Name-Id 모음(기존의 tasks) 중에서 해당 아이디의 Name-Id 항목을 제거한 값으로, Name-Id 모음을 갱신합니다.

filteredNameIds는 id들의 조합 같은데 실제 반환값은 name객체의 모집군이라 filteredNames가 더 적합하지 않을까요?
해당 name를 사용해서 타이머를 생성하는 updateNameIds를 살펴보니 객체 구조이네용.

codeisneverodd · 2022-06-20T18:56:21Z

src/pages/CreateNameIds/index.js

-	const location = useLocation()
-	const { tasks, task, spareTime, setSpareTime, setTask, removeTask, handleSubmit, isValidTasks } =
-		useCreateTasks()
+const CreateNameIds = () => {


@dahye1013
네이밍을 변경하고, 함수들을 커스텀 훅스들로 이관하였습니다.
주요 이름 변경사항은
tasks => nameIds
task => name
변경이유는 다음과 같습니다

tasks라는 이름이 향후 사용될 timers와 혼동될 여지가 있고, 해당 변수가 name과 id 쌍들을 가진 배열이므로 변경하였습니다.

또한 기존 코드에서 tasks는 timer의 id와 name , task는 name을 뜻하는등. tasks가 task의 복수형을 의미하지 않아 혼동이 있을 수 있습니다.

마이너한 이름 변동은 fileChanged에서 확인하시는 것이 좋을 것 같습니다!

name은 뒷플로우의 로직이 몰려있으니까 경현님 기준으로 맞추는게 맞다고 생각해요!
근데 nameIds는 조금 어색한 느낌이네요 ㅠㅠ 실제로는 name객체들의 모집군이지 않나요?

name.map(({id})=>id)

요 친구들 같이 느껴져요!

codeisneverodd · 2022-06-20T19:06:14Z

src/pages/DivideTime/hooks/usePendingTimers.js

+	const changeTime = time => {
+		const { hour, minute } = time
+		const inputTime = convertHourMinuteToSeconds(time)
+		const currentTime = selectedTask.time
+
+		if (!checkTimeValidation(inputTime, currentTime)) {
+			setIsTimeOver(true)
+			return
+		}
+
+		const newPendingTimers = pendingTimers.map(task =>
+			task.id === selectedTask.id ? { ...task, hour, minute, time: inputTime } : task,
+		)
+
+		setPendingTimers(newPendingTimers)
+		setIsTimeOver(false)
+		setSelectedTask(null)
+	}


@ljw0096
각 pendingTimer의 시간을 바꾸는 함수라고 생각되어 함수명을 changeTime으로 정하였습니다. 기존에 handleSubmit에 있던 함수를 가져왔습니다.

const handleSubmit = time => { const { hour, minute } = time const inputTime = convertHourMinuteToSeconds(time) const currentTime = selectedTask.time if (!checkTimeValidation(inputTime, currentTime)) { setIsTimeOver(true) return } const nextTasks = tasks.map(task => task.id === selectedTask.id ? { ...task, hour, minute, time: inputTime } : task, ) setTasks(nextTasks) setIsTimeOver(false) setSelectedTask(null) }

codeisneverodd · 2022-06-20T19:07:54Z

src/pages/DivideTime/index.js

+	INVALID: '시간을 입력해주세요',
+})
+
+const DivideTime = () => {


@ljw0096
시간을 나눈다는 의미를 가진 divideTime으로 이름을 변경하였습니다.
변경 이유는 createTimeDivider라는 명칭이 TimerDivider라는 것을 만든다는 의미인데, TimeDivider라는 것을 만드는 것이 아니라 Time Divide 라는 행위를 하는 것이라 생각되어서 입니다.

codeisneverodd · 2022-06-20T19:10:24Z

src/pages/DivideTime/index.js

+	const handleNextPageClick = () => {
+		const newTimers = {}
+		pendingTimers.forEach(({ name, time, id }) => {
+			newTimers[id] = { time, name }
+		})
+		resetTimers(newTimers)
+	}


@ljw0096
setTimers를 직접사용하는 것이 아닌 userTimers 훅의 resetTimers 함수를 가져와 사용하였습니다.
resetTimers 함수는 아래와 같습니다.

const resetTimers = (newTimers = {}) => { setTimers(newTimers) }

resetTimers이며 default 값으로 초기화할 것 같은데, newTimers가 인자로 필요한가요? 👀

인자가 없으면, default 값으로 초기화 하고
인자가 있으면, 인자 값으로 초기화하는 함수입니다!

codeisneverodd · 2022-06-20T19:12:06Z

src/pages/DivideTime/index.js

+					{pendingTimers.map(timer => (
+						<TaskBox
+							key={timer.id}
+							timer={timer}
+							onClick={() => {
+								handlePendingTimerBoxClick(timer)
+							}}
+						/>
+					))}


@ljw0096
파일명 변경으로 인해 기존 코드와 비교가 어려울 것 같아 첨부합니다!

{tasks.map(task => ( <TaskBox key={task.id} task={task} onClick={() => { handleTaskBoxClick(task) }} /> ))}

codeisneverodd · 2022-06-20T19:14:33Z

src/shared/hooks/useCreatingTimers.js

+export const useCreatingTimers = () => {
+	const [spareTime, setSpareTime] = useRecoilState(spareTimeState)
+	const [nameIds, setNameIds] = useRecoilState(nameIdState)
+
+	const updateSpareTime = (timeType, value) => {
+		setSpareTime({ ...spareTime, [timeType]: value })
+	}
+	const addNameId = (id, name) => {
+		setNameIds([...nameIds, { id, name }])
+	}
+	const updateNameIds = newNames => {
+		setNameIds(newNames)
+	}
+
+	return {
+		spareTime,
+		nameIds,
+		updateSpareTime,
+		addNameId,
+		updateNameIds,
+	}
+}


@dahye1013 @ljw0096
useCreatingTimers 훅스는 DivideTime 페이지에 오기 전 페이지들에서 생성된 데이터들을 관리하는 훅입니다.
간단한 setter 함수로만 이루어져있습니다.

dahye1013

리팩토링 해주신 덕분에 코드를 전반적으로 훑어 볼 수있었네요 ㅎㅎㅎ
페이지 별로 각자 작업하는 바람에 로직이 꼬여있어서 설계만 수정하시는줄 알았는데, 코드를 새로 짜고 있으실 줄이야 🥲 ...
리팩토링 할 때 참고 할게요~! 협업하는 작업물이라 코드 스타일을 통일해야하는 건 맞지만, 다른 사람이 작업하던걸 또 다른 사람이 한번에 수정하면 기존 작업자가 코드를 파악하기 어려울 것 같아요 ㅠㅠ 서로 합의가 필요한 부분도 있구요.
그래도 리뷰 상세하게 남겨주셔서 파악하기는 수월했습니당~ 중간 중간에 서로 맞춰야하는 부분을 코멘트 주고 받으면서 고치면 될 것 같아요!

dahye1013 · 2022-06-20T19:29:22Z

src/pages/CreateNameIds/hooks/useNameTag.js

+	const removeNameTag = removeId => {
+		const filteredNameIds = nameIds.filter(({ id }) => removeId !== id)
+		updateNameIds(filteredNameIds)
+	}


filteredNameIds는 id들의 조합 같은데 실제 반환값은 name객체의 모집군이라 filteredNames가 더 적합하지 않을까요?
해당 name를 사용해서 타이머를 생성하는 updateNameIds를 살펴보니 객체 구조이네용.

dahye1013 · 2022-06-20T19:38:09Z

src/pages/CreateNameIds/hooks/useNameTag.js

+	const handleNameTagSubmit = e => {
+		e.preventDefault()
+		const trimmedName = nameTag.trim()
+
+		if (trimmedName) {
+			addNameId(`${Date.now()}${nameIds.length}`, trimmedName)
+		}
+
+		setNameTag('')
+	}


저는 개인적으로 함수의 인자를 객체 형태로 인자를 넘겨주는걸 선호해서 ㅎㅎ 이 부분은 수정해서 사용할게요.
함수로 넣으면 순서를 지켜야해서 휴먼 에러가 생길 확률이 높다고 생각해요.

dahye1013 · 2022-06-20T19:50:06Z

src/pages/CreateNameIds/index.js

-	const location = useLocation()
-	const { tasks, task, spareTime, setSpareTime, setTask, removeTask, handleSubmit, isValidTasks } =
-		useCreateTasks()
+const CreateNameIds = () => {


name은 뒷플로우의 로직이 몰려있으니까 경현님 기준으로 맞추는게 맞다고 생각해요!
근데 nameIds는 조금 어색한 느낌이네요 ㅠㅠ 실제로는 name객체들의 모집군이지 않나요?

name.map(({id})=>id)

요 친구들 같이 느껴져요!

dahye1013 · 2022-06-20T19:50:43Z

src/pages/CreateNameIds/index.js

-				<Link to="/createTimeDivider" state={{ spareTime, tasks }}>
-					<Button disabled={!isValidTasks}>
-						{!isValidTasks ? BUTTON_TEXT.INVALID : BUTTON_TEXT.VALID}
+				<Link to="/divideTime">


ㅎㅎㅎpath가 깔끔해서 좋네요

dahye1013 · 2022-06-20T19:52:00Z

src/pages/CreateSpareTime/index.js

+	const handleSpareTimeChange = e => {
 		const { name, value } = e.target
-		setSpareTime({ ...spareTime, [name]: value })
+		updateSpareTime(name, value)


change가 들어가니까 확실히 의미가 더 명시적인것 같네요 👍

dahye1013 · 2022-06-20T19:52:26Z

src/pages/DivideTime/components/TaskBox.jsx

+const TaskBox = ({ timer, ...props }) => {
+	const { hour, minute, name } = timer

 	return (
 		<BoxContainer {...props}>
 			<BoxWrapper>
 				<Text size={1.4} color={themeColors.primary}>
-					{task.task}
+					{name}


오 구조분해할당 👍

dahye1013 · 2022-06-20T20:21:04Z

src/pages/DivideTime/index.js

+		const newTimers = {}
+		pendingTimers.forEach(({ name, time, id }) => {
+			newTimers[id] = { time, name }
+		})


요기도 고차함수로 한번에 반환해도 될 것 같네용

Suggested change

const newTimers = {}

pendingTimers.forEach(({ name, time, id }) => {

newTimers[id] = { time, name }

})

const newTimers = Object.entries(timers).reduce((acc, [id, timer]) => {

acc[id] = { time, name }

return acc

},{})

dahye1013 · 2022-06-20T20:23:05Z

src/pages/DivideTime/index.js

+	const handleNextPageClick = () => {
+		const newTimers = {}
+		pendingTimers.forEach(({ name, time, id }) => {
+			newTimers[id] = { time, name }
+		})
+		resetTimers(newTimers)
+	}


resetTimers이며 default 값으로 초기화할 것 같은데, newTimers가 인자로 필요한가요? 👀

dahye1013 · 2022-06-20T20:24:46Z

src/shared/hooks/useCreatingTimers.js

+	const [nameIds, setNameIds] = useRecoilState(nameIdState)
+
+	const updateSpareTime = (timeType, value) => {
+		setSpareTime({ ...spareTime, [timeType]: value })


spareTime 입력 화면에서는 입력값에 대한 시간/분이 유효성 검사가 필요해서 timeType을 나누었는데
전역 상태에서도 굳이 이걸 알고 있어야할까요? 그냥 Number 로만 가지고 있어도 될 것 같네용.
검색해보니까 다른 화면에서는 TIME_TYPE가 전혀 사용되지 않고 있어서ㅎㅎ...
아니면 여기서 useMemo를 사용해서 Number 타입으로 파싱해주는 애를 추가해줘도 좋을 것 같아요

dahye1013 · 2022-06-20T20:30:23Z

src/shared/hooks/useCreatingTimers.js

+	const addNameId = (id, name) => {
+		setNameIds([...nameIds, { id, name }])
+	}
+	const updateNameIds = newNames => {
+		setNameIds(newNames)
+	}


요기는 위에도 이야기했지만 이름이 setNameById,addNameById, updateNameByIds 혹은 그냥 id 접두사를 제거하는게 편이 좋지 않을까요? 파라미터는 names인데 함수명랑 좀 괴리가 있는 것 같아요!

codeisneverodd added 6 commits June 21, 2022 03:38

feat: timer 리셋을 위한 함수 추가

f4aab22

feat: createSpareTime, createNameIds 페이지에서 사용될 useCreatingTimers 훅 구현

1aa5a18

feat: 페이지 네이밍 변경

1455160

etc: 페이지 이름 변경으로 인한 파일 이동

d645548

refactor: 페이지 이름 변경으로 인한 link 변경

6a6cbd4

refactor: state 관리를 위한 커스텀 hooks 구현 방식으로 리팩토링

ad28969

codeisneverodd added 라우터 페이지 리팩터링 REVIEW labels Jun 20, 2022

codeisneverodd requested review from dahye1013 and ljw0096 June 20, 2022 18:47

codeisneverodd self-assigned this Jun 20, 2022

codeisneverodd commented Jun 20, 2022

View reviewed changes

dahye1013 reviewed Jun 20, 2022

View reviewed changes

Conversation

codeisneverodd commented Jun 20, 2022

개요

작업 내용

관련 이슈

참고 자료

Uh oh!

codeisneverodd 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dahye1013 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants