좋은 리팩토링 VS 나쁜 리팩토링

💡개요

프로젝트에서 리팩토링을 하면서 과연 어디까지 리팩토링 하는게 맞는지, 어떤 부분을 해야 하는지 많이 헷갈리고 고민이 되었다. 리팩토링에 대한 정보를 찾던 중, 좋은 리팩토링과 그렇지 않은 리팩토링에 대한 글이 있어 정리하려고 한다.

 

 

💡피해야 하는 리팩토링

❗1. 코드 스타일 180도 바꾸기

기존 코드

나이 18 이상인 유저의 성인 속성을 추가하고 이 조건을 만족하는 유저 정보를 배열로 반환하는 코드이다.

function processUsers(users: User[]) {
  const result = [];
  for (let i = 0; i < users.length; i++) {
    if (users[i].age >= 18) {
      const formattedUser = {
        name: users[i].name.toUpperCase(),
        age: users[i].age,
        isAdult: true,
      };
      result.push(formattedUser);
    }
  }
  return result;
}

 

나쁜 리팩토링 예시

아래는 Ramda 라는 함수형 프로그래밍 라이브러리를 활용해서 같은 기능을 함수형 스타일로 구현한 코드이다.

import * as R from 'ramda';

// 🚩 완전히 다른 스타일과 라이브러리를 채택했습니다.
const processUsers = R.pipe(
  R.filter(R.propSatisfies(R.gte(R.__, 18), 'age')),
  R.map(
    R.applySpec({
      name: R.pipe(R.prop('name'), R.toUpper),
      age: R.prop('age'),
      isAdult: R.always(true),
    })
  )
);

 

processUsers에 필터링과 데이터 변환을 처리하는 함수를 합성해서 값을 처리하는 파이프라인을 만든 코드이다.

이렇게 Ramda함수는 작은 단위로 분리되어 있어 다른 곳에서도 쉽게 재사용 가능하다는 장점은 물론 있지만 기존에 작성한 코드와는 완전히 다른 스타일의 코드가 되었다.

 

이렇게 익숙하지 않은 방식을 적용하게 되면 여러 사람의 손을 거치는 팀 단위의 유지보수에서는 좋은 방법은 아닐 수 있다.

 

그럼 여기서 말하는 좋은 리팩토링은?

이렇게 기존 코드의 체계는 유지하되, filter와 map 함수를 사용하여 간결하게 나타낼 수 있다. (완전히 새로운 패러다임 NO, 외부 의존성 도입 NO)

 

 

Before(기존 코드)

function processUsers(users: User[]) {
  const result = [];
  for (let i = 0; i < users.length; i++) {
    if (users[i].age >= 18) {
      const formattedUser = {
        name: users[i].name.toUpperCase(),
        age: users[i].age,
        isAdult: true,
      };
      result.push(formattedUser);
    }
  }
  return result;
}

 

 

After(코드 스타일 유지한 리팩토링)

function processUsers(users: User[]): FormattedUser[] {
  return users
    .filter(user => user.age >= 18)
    .map(user => ({
      name: user.name.toUpperCase(),
      age: user.age,
      isAdult: true,
    }));
}

❗2.불필요한 추상화하기

재사용성을 높이고 의존성을 줄이기 위해서 무작정 추상화하는 것에 집중하여 코드의 일부를 서로 다른 방향으로 분리하거나 해서 일부 설정을 잘못 통합하기도 하는 등의 사례가 있었다고 한다.

기존 코드에서 여러 메서드를 가진 클래스를 도입한 건데 객체 지향적으로 하려다가 오히려 이렇게 더 복잡해지고 한 눈에 이해하기 어려워진다.

 

나쁜 리팩토링 예시

// 🚩 여기에는 필요 이상으로 많은 레이어와 추상화가 있습니다.
class UserProcessor {
  private users: User[];

  constructor(users: User[]) {
    this.users = users;
  }

  public process(): FormattedUser[] {
    return this.filterAdults().formatUsers();
  }

  private filterAdults(): UserProcessor {
    this.users = this.users.filter(user => user.age >= 18);
    return this;
  }

  private formatUsers(): FormattedUser[] {
    return this.users.map(user => ({
      name: this.formatName(user.name),
      age: user.age,
      isAdult: true,
    }));
  }

  private formatName(name: string): string {
    return name.toUpperCase();
  }
}

const processUsers = (users: User[]): FormattedUser[] => {
  return new UserProcessor(users).process();
};

 

 

좋은 리팩토링

// ✅ 컨벤션을 지키면서 더 깔끔합니다.
const isAdult = (user: User): boolean => user.age >= 18;

const formatUser = (user: User): FormattedUser => ({
  name: user.name.toUpperCase(),
  age: user.age,
  isAdult: true,
});

function processUsers(users: User[]): FormattedUser[] {
  return users.filter(isAdult).map(formatUser);
}

만약 로직을 재사용할 수 있게 작게 분해하고 싶다면 이렇게 불필요한 복잡성을 도입하지 않고 분해하면 된다.!

 

 

❗3.일관성 부족하게 구현하기

예를 들어 이렇게 리액트 쿼리를 사용해서 데이터 패칭을 하는 애플리케이션이 있다면 이 기존 코드 베이스는 유지한 채 개선할 수 있도록 해야 한다.

// 앱 전체에서
import { useQuery } from 'react-query';

function UserProfile({ userId }) {
  const { data: user, isLoading } = useQuery(['user', userId], fetchUser);

  if (isLoading) return <div>Loading...</div>;
  return <div>{user.name}</div>;
}

 

한 개발자가 하나의 컴포넌트에서만 리액트 툴킷을 사용하기로 결정한다면 이 특정 한 컴포넌트에서만 완전히 다른 상태 관리 패턴을 도입하는 것이므로 더 적절한 접근 방식은 기존 리액트 쿼리를 계속 사용하는 것이다.

한 컴포넌트만을 위해서 팀원들이 새로운 패턴을 배우는 것 즉 일회성 불일치는 비효율적이라는 것이다.

❗4.코드 이해 없이 리팩토링하기

기존 코드

// 🫤 여기에는 너무 많은 하드 코딩이 있습니다.
function fetchUserData(userId: string) {
  const cachedData = localStorage.getItem(`user_${userId}`);
  if (cachedData) {
    return JSON.parse(cachedData);
  }

  return api.fetchUser(userId).then(userData => {
    localStorage.setItem(`user_${userId}`, JSON.stringify(userData));
    return userData;
  });
}

이게 기존 코드인데 현재 로컬스토리지를 사용하고 있다. localstorage는 모든 데이터를 문자열로 저장하기 때문에 변환을 해줘야 해서 데이터를 저장할 때는 문자열로, 가져올 때는 객체로 변환하기 위해 JSON(파싱).parse와 JSON.stringify를 사용했다.

 

나쁜 코드 예시(캐싱 메커니즘이 사라짐)

// 🚩 캐싱은 어디로 갔나요?
function fetchUserData(userId: string) {
  return api.fetchUser(userId);
}

단순화한 코드라고 생각할 수 있지만 기존 코드에 있는 데이터 캐싱하는 과정이 사라져 항상 서버에 지속적으로 요청을 해야 하는 상황이 발생하고 서버 부하가 일어나며 성능이 떨어질 수 있다.

 

좋은 리팩토링 예시

// ✅ 기존 동작을 보존하는 깔끔한 코드
async function fetchUserData(userId: string) {
  const cachedData = await cacheManager.get(`user_${userId}`);
  if (cachedData) {
    return cachedData;
  }

  const userData = await api.fetchUser(userId);
  await cacheManager.set(`user_${userId}`, userData, { expiresIn: '1h' });
  return userData;
}

리팩토링된 코드에서는 cacheManager를 사용하였다. 기존 코드에서는 형식 변환이 필요했다면, 여기서는 cacheManager가 내부적으로 변환해주거나 객체를 그대로 저장할 수 있어 JSON 파싱이 필요없다. (+만료시간도 자동으로 만료되게 하여 기존에 수동으로 관리해야 하는 불편함을 줄였다.)

이렇게 하면 캐싱 기능을 유지하면서 만료 기능을 갖춰 동작을 개선할 수 있다!

 

 

❗5.비즈니스 맥락 이해없이 구현하기

현재 비즈니스에서 우선시되어야 하는 요소는 무엇인지 아는 것이 중요하다 !

글 작성자의 경우에는 이커머스 회사에서 개발했었는데 빠르고 특히 SEO가 중요한 서비스였지만 매우 느리고 버그가 많이 발생하는 문제를 겪었다고 한다.

 

나쁜 리팩토링 예시

// 🚩 SEO 중심 사이트에 단일 페이지 앱은 나쁜 생각입니다.
function App() {
  return (
    <Router>
      <Switch>
        <Route path="/product/:id" component={ProductDetails} />
      </Switch>
    </Router>);
}

보기엔 깔끔해보여도 느렸던 이유는 클라이언트 사이드 렌더링으로 구현했기 때문이다.

 

좋은 리팩토링 예시

// ✅ 서버가 SEO 중심 사이트를 렌더링 합니다.
export const getStaticProps: GetStaticProps = async () => {
  const products = await getProducts();
  return { props: { products } };
};

export default function ProductList({ products }) {
  return <div>...</div>;
}

개선한 건 페이지를 정적생성하는 방법 중 하나인 getStaticProps를 사용하여 데이터를 가져와 빠르게 렌더링이 되게 했다.

 

❗6.과도하게 코드 통합하기

기존 코드

// 😕 코드베이스에 같은 코드가 40번 이상 있었다면, 아마도 통합할 수 있을 것입니다.
export const quickFunction = functions
  .runWith({ timeoutSeconds: 60, memory: '256MB' })
  .https.onRequest(...);

export const longRunningFunction = functions
  .runWith({ timeoutSeconds: 540, memory: '1GB' })
  .https.onRequest(...);

 

 

나쁜 코드 예시

// 🚩 통합해서는 안 되는 설정을 맹목적으로 통합하는 경우
const createApi = (handler: RequestHandler) => {
  return functions
    .runWith({ timeoutSeconds: 300, memory: '512MB' })
    .https.onRequest((req, res) => handler(req, res));
};

export const quickFunction = createApi(handleQuickRequest);
export const longRunningFunction = createApi(handleLongRunningRequest);

이 코드는 깔끔해보이지만 모든 API에 동일한 설정을 적용해서 설정을 개별적으로 재정의 할 수 없다.

이것보다 좋은 방식은 Firebase 옵션을 API 별로 전달할 수 있도록 하는 것이다.

 

 

좋은 리팩토링 예시

// ✅ 적절한 기본값을 설정하되 누구나 재정의할 수 있도록 허용합니다.
const createApi = (handler: RequestHandler, options: ApiOptions = {}) => {
  return functions
    .runWith({ timeoutSeconds: 300, memory: '512MB', **...options** })
    .https.onRequest((req, res) => handler(req, res));
};

export const quickFunction = createApi(handleQuickRequest, {
  timeoutSeconds: 60,
  memory: '256MB',
});
export const longRunningFunction = createApi(handleLongRunningRequest, {
  timeoutSeconds: 540,
  memory: '1GB',
});

💡올바른 리팩토링 방법

이번에 렌더링 최적화, Zustand → Tanstack Query+Zustand로 바꾸는 등 리팩토링을 하면서 이게 하나라도 변경하자 마음 먹는 순간 많은 것의 변화가 일어난다는 것을 체감했다,,, 리팩토링을 하게 되면 명확한 기준과 팀의 컨벤션을 잘 지키며 반영하는 것이 좋을 것 같다.

 

참고 글

https://www.builder.io/blog/good-vs-bad-refactoring

Good Refactoring vs Bad Refactoring

https://ykss.netlify.app/translation/good_refactoring_vs_bad_refactoring/

(번역) 좋은 리팩터링 vs 나쁜 리팩터링