대수술이었다. 약 10시간 정도 걸렸다. 이 일을 한 발단은 원티드 프리 온보딩 코스를 들으면서 관심사의 분리를 듣고 시작했다. 할 일 목록 생성 어플을 만들다가 내 프로젝트에 일괄적으로 적용해봐도 괜찮겠다 싶었다. 나의 코드를 열어서 다시 보는 순간 분명 약 1개월 정도밖에 되지 않은 코드인데 썩은 내가 나는 것 같았다. 무슨 의도로 이렇게 난해하게 작성을 했는지 모르겠다. 아마 그때 당시에는 ‘동작’하니까 이렇게 작성했을 것 같다. 만약에 이게 회사였다면 PR 승인도 못받았을 것 같은 느낌적인 느낌. 이번 리펙토링의 큰 줄기는 관심사의 분리다. 그리고 세부적으로 API를 호출하는 함수와 뷰를 최대한 분리하였다. 마침 몇개월 전에 스프린트를 끝내면서 이슈를 하나 남겨놓았었다. 그때도 이 부분이 고민이긴 했나보다.

불분명한 이름 변경하기

아래 코드는 서버에 Get 요청을 위해서 만들어진 코드다. 제네릭 타입을 지정할 때, T라고 그냥 아무 생각 없이 지정했었다. 이런 불분명한 이름은 나중에 다른 사람이 나의 코드를 사용해서 기능을 추가하거나 유지 보수 해야 할 때, 정말 화가날 것 같은 부분이었다. 설명도 없는데 그냥 T라니… 아마 useGet 함수를 사용하기 위해 마우스를 올려다 놓으면 그냥 T라고만 되어있어서 사용하기 어렵다.

1interface IFetchProps<T>
2  extends Omit<
3    UseQueryOptions<T, unknown, T, QueryKey>,
4    "queryKey" | "queryFn"
5  > {
6  url: RequestInfo;
7  queryKey: string | string[];
8}
9
10const useGet = <T,>({ url, queryKey, ...rest }: IFetchProps<T>) => {
11  return useQuery<T>(
12    queryKey,
13    async () => {
14      const response = await fetch(url, getRequest);
15      const { data } = await response.json();
16      return data;
17    },
18    {
19      ...rest
20    }
21  );
22};

그래서 API를 호출하는 훅을 만들 때, 최대한 불분명한 이름을 짓지 않으려고 노력했다. 그 결과가 아래 코드다. 일단 useGetEducation이라는 이름을 보고 대강 교육에 관련된 것을 가져오는 훅이라는 것을 짐작할 수 있다. useQuery에 들어간 제네릭 타입도 EducationGetData라고 이름을 지었다. EducationGet의 Data의 모형을 나타내겠구나라고 짐작할 수 있다. 위의 코드보다 아래 코드가 훨씬 더 명확하다. 목적에 맞는 이름을 잘 짓는 것이 중요한 것 같다.

1import { API } from "@/lib/api";
2import { useQuery } from "react-query";
3import { EducationGetData } from "./interface";
4
5const useGetEducations = () => {
6  const api = new API();
7  return useQuery<EducationGetData[]>(
8    ["educations"],
9    () => api.getData<EducationGetData[]>("/api/education/groups"),
10    { onSuccess: () => {} }
11  );
12};
13
14export default useGetEducations;

하나의 함수, 하나의 일

useDelete는 삭제만 하게 해주세요.

나의 코드의 함수는 여러 일을 하느라 힘들다. 아니 함수는 힘들어보이지 않는데 그걸 보는 내가 힘들다. 대표적인 함수 useDelete를 살펴보고 개선 한 사례를 살펴보자.

1import React, { useEffect, useState } from "react";
2import {
3  QueryKey,
4  useMutation,
5  UseMutationOptions,
6  useQueryClient,
7  UseQueryOptions
8} from "react-query";
9import { deleteRequest } from "@/lib/utils";
10import useGetCSRFToken from "./useGetCSRFToken";
11
12interface IuseDeleteProps<T>
13  extends Omit<UseMutationOptions<T, unknown, void, unknown>, "mutationFn"> {
14  url: RequestInfo;
15  queryKey: string;
16}
17
18const useDelete = <T,>({ url, queryKey, ...rest }: IuseDeleteProps<T>) => {
19  const [isConfirmModal, setIsConfirmModal] = useState(false);
20  const [isDelete, setIsDelete] = useState(false);
21
22  const { csrf, csrfToken } = useGetCSRFToken();
23
24  const deleteHelper = async () => {
25    const response = await fetch(url, deleteRequest(csrfToken));
26    return response.json();
27  };
28
29  useEffect(() => {
30    csrf();
31  }, []);
32
33  const mutation = useMutation<T>(deleteHelper, { ...rest });
34
35  return {
36    isConfirmModal,
37    setIsConfirmModal,
38    isDelete,
39    setIsDelete,
40    ...mutation
41  };
42};
43
44export default useDelete;

이 훅은 delete 요청을 보내 DB에서 무언가를 삭제하기 위해 만든 훅이다. 그런데 보면 용도를 모르겠는 것이 있다.

1const [isConfirmModal, setIsConfirmModal] = useState(false);
2const [isDelete, setIsDelete] = useState(false);

이 코드는 모달을 컨트롤 하기 위해 만든 상태다. 아마 설명이 없다면 이 훅을 보고 당장 사용해야겠다고 생각이 들지 않을 것이다. 누군가 이 코드를 사용해야한다면 내부를 보지 않고는 delete가 일어나는 과정을 유추할 수 가 없다.(열어봐도 유추하기 어렵다.)

나는 이 코드를 하나는 모달을 컨트롤하기 위한 함수, 하나는 서버에 delete 요청을 보내는 함수로 쪼개기로 했다.

1import { API } from "@/lib/api";
2import { useMutation, useQueryClient } from "react-query";
3import { useNavigate } from "react-router";
4
5interface NoticeDeleteVariable {
6  id: string;
7}
8
9interface NoticeDeleteData {
10  data: string;
11}
12
13const useDeleteNotice = () => {
14  const api = new API();
15  const queryClient = useQueryClient();
16
17  return useMutation<NoticeDeleteData, Error, NoticeDeleteVariable>(
18    ({ id }) => api.deleteData(`/api/notice/${id}`),
19    {
20      onSuccess: () => {
21        queryClient.invalidateQueries(["notices"]);
22      }
23    }
24  );
25};
26
27export default useDeleteNotice;
1import { useState } from "react";
2
3const useModalContorl = () => {
4  const [isConfirm, setIsConfirm] = useState(false);
5  const [isModal, setIsModal] = useState(false);
6
7  return { isConfirm, setIsConfirm, isModal, setIsModal };
8};
9
10export default useModalContorl;

함수를 쪼개면서 여러가지가 변경되었다. 특히 이름이 변경되면서 함수가 어떤 일을 하는지 더 명확해졌다. 이전에는 isDelete, isConfirmModal과 같은 이름을 보면 어떻게 써야하는지 조금 햇갈렸다. 하지만 isModal, isConfirm으로 변경해서 모든 모달에서 두루두루 사용할 수 있을 것 같다.

post와 patch 나누기

usePostOrPatch라는 훅을 만들 때, 정말 근사한 아이디어라고 생각했다. method를 prop으로 받아서 함수 안에서 역할을 나눠주는 것이다. 하지만 나중에 가서 usePostOrPatch라고 이름이 붙어있는 것을 보면 곧바로 어떤 일을 하는지 알 수 없다. props를 살펴봐야지만 ‘지금 post를 하고 있구나, patch를 하고 있구나'라고 유추할 수 있다.

1import React, { useEffect } from "react";
2import { useMutation, useQueryClient } from "react-query";
3import { postOrPatchRequest } from "../utilities/httpMethod";
4import useGetCSRFToken from "./useGetCSRFToken";
5
6interface IusePostProps {
7  url: RequestInfo;
8  queryKey: string | string[];
9  method: "POST" | "PATCH";
10}
11
12const usePostOrPatch = <TData, TError, TVariables>({
13  url,
14  queryKey,
15  method
16}: IusePostProps) => {
17  const { csrf, csrfToken } = useGetCSRFToken();
18  const queryClient = useQueryClient();
19
20  const postHelper = async (postData: TVariables) => {
21    const response = await fetch(
22      url,
23      postOrPatchRequest(postData, csrfToken, method)
24    );
25
26    if (!response.ok) {
27      const error = await response.json();
28      throw new Error(error.message);
29    }
30    return response.json();
31  };
32
33  useEffect(() => {
34    csrf();
35  }, []);
36
37  return useMutation<TData, TError, TVariables, unknown>(postHelper, {
38    onSuccess: () => {
39      queryClient.invalidateQueries(queryKey);
40    }
41  });
42};
43
44export default usePostOrPatch;

하지만 이번 리펙토링에서는 전부 분리하기로 했다. 그냥 하나의 함수에 너무 많은 아이디어를 넣어서 사용 방법을 복잡하게 하는 것보다 쉽게 사용할 수 있게 변경하는 편이 낫다고 생각했기 때문이다. 함수 내부가 비슷하기 때문에 반복 되는 것 같은 느낌이 든다. 하지만 페이지 컴포넌트 안에서 사용될 때, 정말 복잡하다.

1import { API } from "@/lib/api";
2import { useMutation, useQueryClient } from "react-query";
3import {
4  EducationGroupInfoData,
5  EducationGroupInfoVariable
6} from "./interface";
7
8const usePatchGroupInfo = () => {
9  const api = new API();
10  const queryClient = useQueryClient();
11  return useMutation<EducationGroupInfoData, Error, EducationGroupInfoVariable>(
12    ({ id, body }) => api.patchData(`/api/education/groups/${id}`, body),
13    {
14      onSuccess: () => {
15        queryClient.invalidateQueries(["groupInfo"]);
16      }
17    }
18  );
19};
20
21export default usePatchGroupInfo;
1import { API } from "@/lib/api";
2import { useMutation, useQueryClient } from "react-query";
3import { EducatioCreateGroupVariable, EducationGroupData } from "./interface";
4
5const useCreateGroup = () => {
6  const api = new API();
7  const queryClient = useQueryClient();
8  return useMutation<EducationGroupData, Error, EducatioCreateGroupVariable>(
9    ({ id, body }) => api.postData(`/api/education/groups/${id}/group`, body),
10    {
11      onSuccess: () => {
12        queryClient.invalidateQueries(["groupInfo"]);
13        queryClient.invalidateQueries(["groups"]);
14        queryClient.invalidateQueries(["people"]);
15      }
16    }
17  );
18};
19
20export default useCreateGroup;

꼭 분리가 답은 아니다.

분리 해서 편한것도 있지만 같은 역할을 하는 것끼리 통합하는 것도 때론 문제 해결의 방법이 된다. 아래 코드는 fetch option을 생성해서 넣어주는 객체 또는 함수다. 이런 분리가 더 좋지 않았다. fetch를 사용할 때, option을 반복해서 넣어 주어야했다. 그래서 그냥 이 기능을 하나로 합쳐버렸다.

1export const getRequest: RequestInit = {
2  method: "GET",
3  headers: {
4    "Content-type": "application/json"
5  },
6  credentials: "include",
7  mode: "cors"
8};
9
10export const getWeekliesData = async () => {
11  const response = await fetch(`/api/worship`, getRequest);
12  const { data } = await response.json();
13  return data;
14};
15
16export const postOrPatchRequest = (
17  body: any,
18  csrfToken: string,
19  method: "POST" | "PATCH"
20): RequestInit => {
21  const data = JSON.stringify({ ...body });
22  return {
23    method,
24    headers: {
25      "Content-Type": "application/json",
26      "X-CSRF-Token": csrfToken
27    },
28    body: data,
29    credentials: "include",
30    mode: "cors"
31  };
32};
33
34export const postRequestMultipartFormData = (
35  body: any,
36  csrfToken: string
37): RequestInit => {
38  return {
39    method: "POST",
40    headers: {
41      "X-CSRF-Token": csrfToken
42    },
43    body,
44    credentials: "include",
45    mode: "cors"
46  };
47};
48
49export const deleteRequest = (csrfToken: string): RequestInit => ({
50  method: "DELETE",
51  headers: {
52    "Content-type": "application/json",
53    "X-CSRF-Token": csrfToken
54  },
55  credentials: "include",
56  mode: "cors"
57});

API라는 class를 만들어서 get, post, patch, put, delete를 기능별로 만들었다. 그래서 오히려 생성자를 선언하고 method 별로 함수를 불러와 바로 사용할 수 있게 했다. 변경 후에 커스텀 훅 내부가 훨씬 더 알아보기 쉬워졌다. hook의 이름으로 기능을 유추할 수 있지만, 내부를 열었을 때도 변경도 쉽다.

1class API {
2  async getData<TData>(url: string) {
3    const response = await fetch(url, {
4      method: "GET",
5      headers: {
6        "Content-Type": "application/json"
7      },
8      credentials: "include",
9      mode: "cors"
10    });
11
12    const { data } = await response.json();
13
14    return data as TData;
15  }
16
17  async postData<TData, TVariable>(url: string, body: TVariable) {
18    const CSRFToken = await this.getData<string>("/api/csrf-token");
19
20    const response = await fetch(url, {
21      method: "POST",
22      headers: {
23        "Content-Type": "application/json",
24        "X-CSRF-Token": CSRFToken
25      },
26      body: JSON.stringify(body),
27      credentials: "include",
28      mode: "cors"
29    });
30
31    const { data } = await response.json();
32
33    return data as TData;
34  }
35
36  async patchData<TData, TVariable>(url: string, body: TVariable) {
37    const CSRFToken = await this.getData<string>("/api/csrf-token");
38
39    const response = await fetch(url, {
40      method: "PATCH",
41      headers: {
42        "Content-Type": "application/json",
43        "X-CSRF-Token": CSRFToken
44      },
45      body: JSON.stringify(body),
46      credentials: "include",
47      mode: "cors"
48    });
49
50    const { data } = await response.json();
51
52    return data as TData;
53  }
54
55  async putData<TData, TVariable>(url: string, body: TVariable) {
56    const CSRFToken = await this.getData<string>("/api/csrf-token");
57
58    const response = await fetch(url, {
59      method: "PUT",
60      headers: {
61        "Content-Type": "application/json",
62        "X-CSRF-Token": CSRFToken
63      },
64      body: JSON.stringify(body),
65      credentials: "include",
66      mode: "cors"
67    });
68
69    const { data } = await response.json();
70
71    return data as TData;
72  }
73
74  async deleteData<TData>(url: string) {
75    const CSRFToken = await this.getData<string>("/api/csrf-token");
76
77    const response = await fetch(url, {
78      method: "DELETE",
79      headers: {
80        "Content-type": "application/json",
81        "X-CSRF-Token": CSRFToken
82      },
83      credentials: "include",
84      mode: "cors"
85    });
86
87    const { data } = await response.json();
88
89    return data as TData;
90  }
91}
92
93export default API;
1// 다른것을 다 생략하고 보더라도 통합된 함수를 사용하는 것이 훨씬 더 효율적이다.
2
3// 분리
4const useDelete = <T,>({ url, queryKey, ...rest }: IuseDeleteProps<T>) => {
5  const deleteHelper = async () => {
6    const response = await fetch(url, deleteRequest(csrfToken));
7    return response.json();
8  };
9
10  const mutation = useMutation<T>(deleteHelper, { ...rest });
11};
12
13// 통합
14const useDeleteGroup = () => {
15  const api = new API();
16  return useMutation((id: string) =>
17    api.deleteData(`/api/education/group/${id}`)
18  );
19};

최악의 코드

그래서 이번 리펙토링을 통해서 뷰와 비동기 코드를 분리해서 조금은 개선할 수 있었다. 하지만 여전히 부족한 부분이 많이 보인다. 대표적으로 가장 최악이라고 뽑은 코드를 개선 예제로 넣었다. 전체 코드를 보면 정말 토나온다.

1const Group = ({ item }: IGroupProps) => {
2  const queryClient = useQueryClient();
3
4  const [isUpdate, setIsUpdate] = useState(false);
5  const [isOpenPeopleInput, setIsOpenPeopleInput] = useState(false);
6  const [searchPerson, setSearchPerson] = useState<People[] | null>();
7  const { data, refetch } = useGet<People[] | null>({
8    url: `/api/education/search?person=${searchPersonName}`,
9    queryKey: "search",
10    enabled: false,
11    onSuccess: (response) => {
12      setSearchPerson(response);
13    }
14  });
15
16  const { data: people } = useGet<People[]>({
17    url: `/api/education/group/${item._id}/people`,
18    queryKey: ["people", item._id]
19  });
20
21  const { mutate: addNewPeople } = usePost<
22    FetchDataProps<People[]>,
23    Error,
24    SendPeople
25  >({
26    url: `/api/education/group/${item._id}/people`,
27    queryKey: ["people", item._id],
28    method: "POST"
29  });
30
31  const { mutate: updateGroup } = usePost<
32    FetchDataProps<GroupProps>,
33    Error,
34    {
35      _id?: string;
36      name?: string;
37      place?: string;
38      type?: "student" | "worker" | "new" | "etc";
39    }
40  >({
41    url: `/api/education/group/update`,
42    queryKey: "groups",
43    method: "PATCH"
44  });
45
46  const {
47    mutate: deleteGroupMutate,
48    isConfirmModal,
49    setIsConfirmModal,
50    isDelete,
51    setIsDelete
52  } = useDelete({
53    url: `/api/education/group/${item._id}`,
54    queryKey: "groups",
55    onSuccess: () => {
56      queryClient.invalidateQueries("groups");
57    }
58  });
59
60  const deleteGroup = () => {
61    setIsConfirmModal(true);
62  };
63
64  const onSubmitNewPeopleName = handleSubmit((data) => {
65    if (selectedNodeId && searchPerson && searchPerson.length !== 0) {
66      const [item] = searchPerson?.filter(
67        (value) => value._id === selectedNodeId
68      );
69      selectItem(item);
70      setIsOpenPeopleInput(!isOpenPeopleInput);
71      reset({ name: "" });
72      setSearchPersonName("");
73      setSearchPerson([]);
74      return;
75    }
76
77    addNewPeople(
78      {
79        name: data.name,
80        type: item.type
81      },
82      {
83        onSuccess: () => {
84          setSearchPerson([]);
85          reset({ name: "" });
86        },
87        onError: (err) => {
88          setSearchPerson([]);
89          reset({ name: "" });
90          setError("name", { message: err.message });
91        }
92      }
93    );
94    setIsOpenPeopleInput(!isOpenPeopleInput);
95    reset({ name: "" });
96    setSearchPersonName("");
97    setSearchPerson([]);
98  });
99
100  const onSubmitUpdateGroupName = handleSubmit((data) => {
101    updateGroup({
102      _id: item._id,
103      name: data.groupName,
104      type: data.type,
105      place: data.place
106    });
107    reset();
108    setIsUpdate(false);
109  });
110
111  const selectItem = (person: People) => {
112    const hasHuman = item.humanIds.some((value) => value === person._id);
113    if (hasHuman) {
114      setSearchingBoxError(true);
115      return;
116    }
117    addNewPeople({ _id: person._id });
118    reset({ name: "" });
119  };
120
121  useEffect(() => {
122    if (isDelete) {
123      deleteGroupMutate();
124      setIsConfirmModal(false);
125      setIsDelete(false);
126    }
127  }, [isDelete]);
128
129  const personName = useRef<HTMLInputElement>(null);
130  return (
131    <>
132      {isConfirmModal && (
133        <ConfirmDeleteModal
134          setIsModal={setIsDelete}
135          setIsConfirm={setIsConfirmModal}
136          title="그룹을 삭제하시겠습니까?"
137          subtitle="그룹을 삭제하면 복구할 수 없습니다. 참가자는 그대로 남습니다."
138        />
139      )}
140    </>
141  );
142};
143
144export default Group;
1const Group = ({ item }: IGroupProps) => {
2  const { isModal, isConfirm, setIsConfirm, setIsModal } = useModalContorl();
3
4  const { mutate: addNewPeople } = useAddNewPerson();
5  const { mutate: deleteGroupMutate } = useDeleteGroup();
6  const { mutate: updateGroup } = useUpdateGroup();
7  const { data: people } = useGetPeople(item._id);
8  const { refetch } = useEducaionSearchPerson(
9    searchPersonName,
10    setSearchPerson
11  );
12
13  const deleteGroup = () => {
14    setIsModal(true);
15  };
16
17  const onSubmitNewPeopleName = handleSubmit((data) => {
18    if (selectedNodeId && searchPerson && searchPerson.length !== 0) {
19      const [selectedItem] = searchPerson?.filter(
20        (person) => person._id === selectedNodeId
21      );
22      selectItem(selectedItem);
23    } else if (data.name) {
24      addNewPeople(
25        {
26          id: item._id,
27          body: {
28            name: data.name,
29            type: item.type
30          }
31        },
32        {
33          onSuccess: () => {
34            setSearchPerson([]);
35            reset({ name: "" });
36          },
37          onError: (err) => {
38            setSearchPerson([]);
39            reset({ name: "" });
40            setError("name", { message: err.message });
41          }
42        }
43      );
44    }
45
46    setIsOpenPeopleInput(!isOpenPeopleInput);
47    reset({ name: "" });
48    setSearchPersonName("");
49    setSearchPerson([]);
50  });
51
52  const onSubmitUpdateGroupName = handleSubmit((data) => {
53    const id = item._id;
54    const body = {
55      _id: id,
56      name: data.groupName,
57      type: data.type,
58      place: data.place
59    };
60    if (id) {
61      updateGroup({ body });
62      reset();
63      setIsUpdate(false);
64    }
65  });
66
67  const selectItem = (person: People) => {
68    const hasHuman = item.humanIds.some((value) => value === person._id);
69    if (hasHuman) {
70      setSearchingBoxError(true);
71      return;
72    }
73    addNewPeople({ id: item._id, body: { _id: person._id } });
74    reset({ name: "" });
75  };
76
77  useEffect(() => {
78    const id = item._id;
79    if (isConfirm && id) {
80      deleteGroupMutate(id);
81      setIsConfirm(false);
82      setIsModal(false);
83    }
84  }, [isConfirm]);
85
86  return (
87    <>
88      {isModal && (
89        <ConfirmDeleteModal
90          setIsModal={setIsModal}
91          setIsConfirm={setIsConfirm}
92          title="그룹을 삭제하시겠습니까?"
93          subtitle="그룹을 삭제하면 복구할 수 없습니다. 참가자는 그대로 남습니다."
94        />
95      )}
96    </>
97  );
98};
99
100export default Group;

마무리

관심사의 분리는 그나마 나중에 코드를 돌아보고 과거의 나에게 욕을 덜하게 하는 좋은 도구인 것 같다. 이번에는 목표가 비동기와 뷰를 분리하는 것이었다. 그것을 개선한 것 만으로도 코드를 읽으면서 마음이 편안해진다. 무엇을 분리하고 무엇을 통합해서 반복을 줄여나가고 우아한 코드를 작성할지는 계속 고민하고 연습해야할 것 같다. 원티드 프리온보딩 코스가 그런 의미에서 많은 도움이 됐던 것 같다.

이번에 리펙토링을 하면서 과제가 하나 더 생겼다. 스타일 컴포넌트를 쓰면서 스타일 코드와 뷰가 뒤섞여있어서 수직으로 스크롤을 왔다 갔다 하는게 만만치 않다. 어떻게 분리해야할지 아직 감은 안잡히지만 배웠던 것을 바탕으로 개선을 해봐야겠다.(성능 개선도 해야하는데 ㅜㅠ)

이번 기회에 코드를 돌아볼 수 있어서 다행이었다. 운이 좋았다. 내가 코드를 작성하면서 일단 동작을 하게 하는 것을 중요하게 생각했던 것 같다. 그리고 나중에 고쳐야지라고 생각했을지도 모른다. 팀 프로젝트를 하면서도 ‘일단 동작시키고 나중에 고쳐요'라는 말을 듣고 그때는 그냥 동의를 했었다. 하지만 지금은 아니다. 왜냐하면 팀 프로젝트 때 작성했던 코드를 지금 돌아보지 않기 때문이다. 동작하는 코드는 기본이다. 다른 사람이 나중에 봤을 때, 키보드 샷건 치는 코드는 작성하지 않는 것이 필요하다.

우테코 라이브에서 강사님이 건축 설계와 소프트웨어의 차이를 설명하시면서 소프트웨어는 설계를 철저하게 하지 않으면 미래의 개발 비용이 비약적으로 늘어날 수 있다는 이야기를 했었다. 클린 코드에서는 회사가 망한 사례도 이야기한다. 회사에 들어가서 내가 열심히 코드를 작성하는 것도 일이지만 미래의 개발 부채를 최소한으로 하기 위해 코드를 작성하는 개발자가 되고 싶다.