코드 리뷰에서 반복 지적받는 패턴 10가지, 이렇게 고쳐라

작성자:

코드 리뷰를 받다 보면 “또 이거야?” 싶은 지적이 반복된다. 처음엔 몰라서 틀리지만, 두 번 세 번 같은 코멘트를 받는다면 패턴을 인식 못 하고 있는 거다. 리뷰어도 매번 같은 말을 하기 지치고, 리뷰이도 개선이 없는 것처럼 보여 서로 답답한 상황이 된다.

이 글은 실무 리뷰에서 반복적으로 등장하는 패턴 10가지를 정리한다. 단순히 “이렇게 하면 안 된다”가 아니라, 왜 지적을 받는지 구조적으로 이해하고 스스로 걸러낼 수 있게 하는 게 목표다.

GitHub Pull Request 코드 리뷰 코멘트 화면 예시

패턴 1: 함수 하나가 너무 많은 일을 한다

리뷰어가 “이 함수 너무 길지 않나요?” 또는 “역할을 분리해주세요”라고 말한다면 단일 책임 원칙(SRP) 문제다. 함수가 데이터 조회, 가공, 저장, 이메일 발송을 한꺼번에 하면 테스트도 어렵고, 일부만 바꾸려 해도 전체를 건드려야 한다.

js

//  잘못된 예: 한 함수에 너무 많은 책임
async function processOrder(orderId) {
  const order = await db.query('SELECT * FROM orders WHERE id = ?', [orderId]);
  const total = order.items.reduce((sum, item) => sum + item.price * item.qty, 0);
  order.total = total;
  order.status = 'processed';
  await db.query('UPDATE orders SET total = ?, status = ? WHERE id = ?', [total, 'processed', orderId]);
  await sendEmail(order.userEmail, `주문 ${orderId} 처리 완료`);
  await slackNotify(`#orders`, `새 주문 처리됨: ${orderId}`);
}

//  올바른 예: 각 책임을 분리
async function processOrder(orderId) {
  const order = await getOrder(orderId);
  const total = calculateTotal(order.items);
  await updateOrderStatus(orderId, total, 'processed');
  await notifyOrderProcessed(order);
}

함수명이 and나 then으로 연결되는 설명이 필요해질 때가 분리 신호다.

패턴 2: 의미 없는 변수명과 함수명

data, result, temp, flag, val — 코드베이스에 이런 이름이 많으면 “읽히는 코드”가 아니다. 리뷰어는 변수가 무엇을 담고 있는지 추론하느라 시간을 쓰게 된다.

js

//  잘못된 예
const data = await fetch('/api/users');
const result = data.filter(x => x.age > 18);
const flag = result.length > 0;

//  올바른 예
const users = await fetchUsers();
const adultUsers = users.filter(user => user.age > 18);
const hasAdultUsers = adultUsers.length > 0;

네이밍이 어렵게 느껴진다면, 변수에 뭘 담아야 할지 아직 명확하지 않다는 신호일 수 있다. 설계를 먼저 다듬자.

패턴 3: 에러 처리를 빠뜨리거나 너무 뭉뚱그린다

js

//  잘못된 예: 에러를 삼켜버림
try {
  await saveUser(userData);
} catch (e) {
  console.log(e); // 로그만 남기고 아무것도 안 함
}

//  잘못된 예: 모든 에러를 똑같이 처리
try {
  const user = await getUser(id);
  await sendWelcomeEmail(user.email);
} catch (e) {
  return res.status(500).json({ error: '서버 에러' });
  // 사용자 없음(404)과 이메일 실패(502)를 동일하게 처리
}

// 올바른 예: 에러 종류에 따라 구분
try {
  const user = await getUser(id);
  if (!user) return res.status(404).json({ error: '사용자를 찾을 수 없습니다' });
  await sendWelcomeEmail(user.email);
} catch (e) {
  if (e instanceof EmailServiceError) {
    return res.status(502).json({ error: '이메일 발송 실패' });
  }
  logger.error('예상치 못한 오류', { error: e, userId: id });
  return res.status(500).json({ error: '서버 오류가 발생했습니다' });
}

주의: catch 블록에서 에러를 조용히 무시하는 건 디버깅 지옥의 시작이다. 최소한 로그를 남기고, 호출자에게 의미 있는 정보를 전달해야 한다.

패턴 4: 조건문 중첩이 3단계를 넘는다

중첩된 if가 깊어질수록 읽는 사람의 머릿속에서 스택 오버플로우가 난다. 얼리 리턴(early return)으로 평탄하게 만드는 게 정석이다.

js

//  잘못된 예: 화살촉 코드(arrow code)
function processPayment(order) {
  if (order) {
    if (order.isPaid) {
      if (order.items.length > 0) {
        if (order.user.isActive) {
          // 핵심 로직이 여기 있음
          return fulfillOrder(order);
        }
      }
    }
  }
}

//  올바른 예: 얼리 리턴으로 평탄화
function processPayment(order) {
  if (!order) return;
  if (!order.isPaid) return;
  if (order.items.length === 0) return;
  if (!order.user.isActive) return;

  return fulfillOrder(order);
}

패턴 5: 매직 넘버와 매직 스트링

코드 안에 맥락 없이 박힌 숫자나 문자열은 “이게 뭔데?”라는 질문을 남긴다. 6개월 후의 내가, 또는 팀원이 이 값을 바꿔야 할 때 어디를 고쳐야 할지 모른다.

js

//  잘못된 예
if (user.loginAttempts > 5) lockAccount(user);
setTimeout(cleanup, 86400000);
if (order.status === 'P') processPayment(order);

//  올바른 예
const MAX_LOGIN_ATTEMPTS = 5;
const ONE_DAY_MS = 24 * 60 * 60 * 1000;
const ORDER_STATUS = { PENDING: 'P', COMPLETED: 'C', CANCELLED: 'X' };

if (user.loginAttempts > MAX_LOGIN_ATTEMPTS) lockAccount(user);
setTimeout(cleanup, ONE_DAY_MS);
if (order.status === ORDER_STATUS.PENDING) processPayment(order);

패턴 6: 불필요한 주석 — 코드가 이미 말하고 있다

주석이 나쁜 게 아니다. 문제는 코드를 그대로 번역한 주석이다. “무엇”을 설명하는 주석은 대부분 불필요하다. 주석은 “왜”를 설명할 때 빛난다.

js

//  잘못된 예: 코드 번역 주석
// 사용자 나이가 18 이상인지 확인
if (user.age >= 18) { ... }

// i를 1 증가
i++;

//  올바른 예: 이유를 설명하는 주석
// GDPR 규정상 미성년자 데이터 수집 불가 (2018.05 정책)
if (user.age >= 18) { ... }

// IE11 호환을 위해 Array.from 대신 slice 사용 (2024년 말 EOL 이후 제거 예정)
const arr = Array.prototype.slice.call(nodeList);

패턴 7: 중복 코드를 추상화하지 않는다

Copy-paste 코드는 리뷰어의 눈에 바로 보인다. 지금 당장은 괜찮아도, 로직이 바뀔 때 한쪽만 수정하고 나머지를 빠뜨리면 버그가 된다.

js

//  잘못된 예: 거의 같은 코드가 두 군데
function getAdminUsers() {
  return db.query('SELECT id, name, email FROM users WHERE role = "admin" AND deleted_at IS NULL');
}

function getEditorUsers() {
  return db.query('SELECT id, name, email FROM users WHERE role = "editor" AND deleted_at IS NULL');
}

//  올바른 예: 파라미터로 추상화
function getUsersByRole(role) {
  const ALLOWED_ROLES = ['admin', 'editor', 'viewer'];
  if (!ALLOWED_ROLES.includes(role)) throw new Error(`허용되지 않은 역할: ${role}`);
  return db.query('SELECT id, name, email FROM users WHERE role = ? AND deleted_at IS NULL', [role]);
}

“3번 이상 복붙했다면 함수로 만들어라”는 Rule of Three는 여전히 유효하다.

패턴 8: 비동기 처리를 잘못 다룬다

async/await를 쓴다고 비동기가 안전해지는 게 아니다. 병렬로 처리할 수 있는 걸 직렬로 돌리거나, Promise 체이닝을 섞어서 쓰다 보면 미묘한 버그가 생긴다.

js

//  잘못된 예: 독립적인 작업을 순서대로 실행 (느림)
const user = await getUser(userId);
const orders = await getOrders(userId);
const notifications = await getNotifications(userId);

//  올바른 예: 독립적인 작업은 병렬로
const [user, orders, notifications] = await Promise.all([
  getUser(userId),
  getOrders(userId),
  getNotifications(userId),
]);

//  잘못된 예: await를 빠뜨려 에러가 안 잡힘
async function saveData(data) {
  try {
    db.save(data); // await 누락! 에러가 catch에 잡히지 않음
  } catch (e) {
    logger.error(e);
  }
}

팁: Promise.all은 하나라도 실패하면 전체가 reject된다. 개별 실패를 허용하면서 병렬 처리하려면 Promise.allSettled를 쓴다.

패턴 9: 보안을 코드 레벨에서 빠뜨린다

“보안은 나중에”라는 생각이 리뷰 단계에서 가장 많이 걸린다. 특히 입력값 검증, SQL 인젝션, 민감 정보 노출 세 가지는 리뷰어가 민감하게 보는 영역이다.

js

//  잘못된 예: 사용자 입력을 그대로 쿼리에 사용 (SQL 인젝션)
const query = `SELECT * FROM users WHERE email = '${req.body.email}'`;

//  올바른 예: 파라미터 바인딩 사용
const query = 'SELECT * FROM users WHERE email = ?';
await db.query(query, [req.body.email]);

//  잘못된 예: 민감 정보를 로그에 출력
logger.info('로그인 시도', { email: user.email, password: req.body.password });

//  올바른 예: 민감 정보 제외
logger.info('로그인 시도', { email: user.email, userId: user.id });

주의: API 응답에 불필요한 필드를 포함하는 것도 보안 이슈다. DB 모델 전체를 그대로 직렬화해서 반환하지 말고, 응답 스키마를 명시적으로 정의한다.

패턴 10: PR 하나에 너무 많은 변경이 담겨 있다

이건 코드 자체의 문제가 아니라 협업 방식의 문제다. 500줄짜리 PR은 리뷰어가 제대로 검토하기 어렵고, 피드백 품질도 떨어진다. 결과적으로 버그가 머지된다.

 나쁜 PR 예시
- 기능 A 구현
- 기능 B 구현
- 기존 모듈 리팩토링
- 버그 수정 3건
- 의존성 업데이트

 좋은 PR 예시
- [feat] 유저 프로필 이미지 업로드 기능 추가 (#123)
  - S3 업로드 유틸 추가
  - 프로필 API 엔드포인트 추가
  - 업로드 제한(5MB, jpg/png)

경험상 200줄 이하, 하나의 목적, 명확한 PR 제목이 리뷰 품질을 가장 높인다.

10가지 패턴 요약

#패턴핵심 원칙
1함수 과부하함수는 한 가지 일만
2불명확한 네이밍이름이 곧 문서
3에러 처리 부실에러 종류별 대응
4깊은 중첩 조건얼리 리턴으로 평탄화
5매직 넘버/스트링상수로 의미 부여
6번역형 주석“왜”를 설명하라
7중복 코드Rule of Three
8비동기 처리 오류await 빠뜨리지 않기, 병렬 처리
9보안 구멍입력 검증, 파라미터 바인딩
10PR 범위 과다하나의 PR, 하나의 목적

FAQ

Q. 리뷰어마다 스타일이 달라서 기준을 모르겠어요. 어떻게 해야 하나요? 팀 전체가 따르는 ESLint/Prettier 설정과 코딩 컨벤션 문서를 만드는 게 근본 해결책이다. 개인 취향 지적은 nit: 접두사로 구분하는 관행도 도움이 된다. 자동화로 잡을 수 있는 건 사람이 지적하지 않는 환경을 만드는 게 좋다.

Q. 코드 리뷰가 너무 오래 걸려서 PR이 쌓여요. PR 크기 문제일 가능성이 높다. 패턴 10에서 다룬 대로 PR을 작게 나누고, 리뷰 요청 시 “어디를 집중적으로 봐달라”는 컨텍스트를 함께 남기면 리뷰 속도가 체감상 달라진다.

Q. 리뷰 코멘트가 너무 많으면 사기가 꺾여요. 어떻게 받아들여야 하나요? 코멘트 수가 코드 품질의 지표가 아니다. 리뷰어가 꼼꼼하다는 뜻이기도 하다. nit(사소한 의견)과 must(반드시 수정)를 구분해달라고 요청하는 것도 방법이다. 코멘트 20개 중 18개가 nit라면 실제로 고칠 건 얼마 없다.

Q. 지적받은 내용을 다음 PR에서 또 반복하는 경우가 많은데, 어떻게 개선하나요? 리뷰 코멘트를 받을 때마다 개인 노트나 팀 위키에 정리하는 습관이 효과적이다. 반복되는 패턴은 PR 전에 스스로 체크리스트로 만들어 셀프 리뷰에 활용한다.

Q. 함수를 너무 잘게 쪼개면 오히려 추적이 어렵다는 말도 있던데요. 맞다. 추상화에도 적정 수준이 있다. 5줄짜리 함수를 2줄씩 쪼개는 건 과분리다. 기준은 “이 함수를 테스트 코드 없이 이름만 보고 동작을 예측할 수 있는가”다. 예측 가능하면 분리 수준이 적당한 것이다.

Q. 이 패턴들을 자동으로 잡을 수 있는 도구가 있나요? 상당 부분은 도구로 자동화 가능하다. ESLint(JS/TS), Pylint/flake8(Python), SonarQube(다국어) 등이 패턴 2, 3, 5, 8을 상당 부분 잡아준다. 패턴 1, 7은 복잡도 측정 도구(cyclomatic complexity)로 보조할 수 있다. 패턴 10은 GitHub PR 크기 제한 봇을 활용하기도 한다.

PR 제출 전 코드 자가 점검 체크리스트

댓글 남기기