[리팩토링] custom hooks를 사용한 리팩토링 #91

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

[리팩토링] custom hooks를 사용한 리팩토링 #91

Are you sure you want to change the base?

[리팩토링] custom hooks를 사용한 리팩토링 #91

Uh oh!

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!

3 participants