코드 리뷰 그거 왜함?

gakko 2023. 1. 28. 12:18

2023. 1. 28. 12:18

부스트캠프에서 chosimhe 팀과 진행한 프로젝트 모여모여는 1년 개발 인생 중 가장 값진 경험이다. 팀원들에게 코드리뷰와 피드백을 받는 시간은 가장 소중한 경험 중 하나였다. 우리 팀(4인 팀)은 코드리뷰를 의무화하기 위해 2명 이상의 리뷰를 받아야만 Merge를 할 수 있도록 레포지토리를 설정했다. 비용이 굉장히 많이 들었지만 기술적으로 단단해지는 시간이었다. 고민을 공유하고 서로 놓친 것들을 잡아주며, 자신의 생각을 정리할 수 있었다. 나의 코드를 자신의 코드처럼 검토해주는 동료들이 있었기 때문에 코드의 품질을 더 신경쓸 수 있었다.

코드리뷰는 왜 해야할까?

우리 팀은 코드리뷰를 하면서 굉장히 많은 비용을 소모했다. 코드리뷰는 생각보다 많은 시간이 드는 작업이다. 코드 읽는 것 자체도 어려운 일이지만 많은 문제점들이 있다. 다음 과정으로 넘어가기 위해 꼭 머지되어야만 PR이 blocking되어버리는 경우가 굉장히 많았다. 또 긴급한 PR을 처리하기 위해 하던 일을 중간에 멈추고 리뷰를 진행해야하는 경우도 있었다. 이외에도 여러 문제점들이 존재한다. 그럼에도 어째서 코드리뷰를 해야하는 걸까?

1. 개인이 파악하지 못한 문제를 찾아낼 수 있다.

Ep1. API 명세

코드 리뷰를 통해 API 명세가 잘못되었다는 것을 알아내게 되었다. API 명세를 만든 사람과 API 코드를 만든 사람, 그리고 API를 사용할 프론트 개발자들의 이해가 다 달랐다. 개발자는 문서로 일한다는 마스터 조은 님의 말을 이제야 이해하게 되었다. 문서의 단어 하나 하나가 엄청난 차이를 만들어낸다. 단어 하나 차이로 해석이 갈리고, 그동안 만들어놓은 코드를 갈아엎어야할 수도 있다. 결국 내가 만들어놓은 코드를 개편하기로 했고 추가적인 비용이 발생할 수 밖에 없었지만, 코드리뷰를 통해 미리 캐치해내지 못했다면, 더 많은 비용이 발생할 수 있었다.

2. 팀의 실력 차이를 줄일 수 있다.

프로젝트는 혼자 만드는 것이 아니라 팀 전체가 만드는 것이다. 잘하는 사람 혼자서 다 할 수는 없는 노릇이니 분업을 진행할 수 밖에 없고 팀원의 실력 차이가 클수록 코드의 품질은 떨어질 수 밖에 없다. 코드리뷰는 이 문제를 해결해준다. 코드리뷰를 통해 비교적 실력이 떨어지는 사람을 성장시킬 수 있다. 잘 만든 코드를 보거나, 자신의 코드에 대한 피드백을 받으면서 실력이 좋아질 수 있다. 잘하는 사람도 기존에 알았다고 생각했던 것들을 다시 정리하는 시간을 가질 수 있고, 개발교육에 눈을 뜨게 될 수도 있다.

Ep2. N+1 Problem

GroupApplication 엔티티에서 User를 매번 가지고 있을 필요가 없었기 때문에 성능을 위해 Lazy Loading하여 가져오도록 만들어놨다. 아래의 경우 application을 돌면서 User를 Lazy Loading해오고 있는데 relations를 설정하지 않았기 때문에 User를 한 번에 가져오지 않고 N+1번 쿼리를 날려 정보를 가져오고 있는 상태였다.

PR 링크: 모임 신청자 조회 API - Pull Request

그래서 N+1 문제가 뭐야?

연관 관계때문에 발생하는 이슈로 연관 관계가 설정된 엔티티를 조회할 경우에 조회된 데이터 갯수(n) + 1 만큼 연관관계의 조회 쿼리가 추가로 발생하여 데이터를 읽어오는 문제를 말한다.

Typeorm에서 Lazy Loading을 할 때 발생할 수 있는 문제로 데이터베이스의 성능을 떨어뜨리는 주범이다. (+추가정보: JPA에서는 Eager, Lazy Loading 모두 발생할 수 있다고 합니다.) typeorm은 JPA와 다르게 Eager로 놓으면 해당 문제가 발생하지 않는다고 한다. 그렇다고 무조건 Eager Loading으로 정보를 가져오는 것이 좋은 것은 아니다. 연관 데이터를 한 번에 가져오기 때문에 로딩 시간이 길어질 수 있고, 백엔드에서 DTO 처리를 방만하게 한다면 프론트엔드에서는 받기 싫은 데이터를 받아야 하는 오버 패칭이 발생할 수 있다. Entity가 많으면 많을수록 더 많은 join이 발생하기 때문에 성능이 안 좋아질 수 밖에 없다. 그렇다면 어떻게 해결해볼 수 있을까? 해결 방법을 보기 전에 코드부터 살펴보도록 하자.

프로젝트에서의 코드

GroupApplication Entity

@Entity()
@Unique('UNIQUE_USER_ID_GROUP_ID_STATUS', ['userId', 'groupId', 'status'])
export class GroupApplication {
  @PrimaryGeneratedColumn({ unsigned: true })
  id: number;

  @Column({ unsigned: true })
  userId: number;

  @ManyToOne(() => User, { lazy: true, createForeignKeyConstraints: false })
  @JoinColumn({ referencedColumnName: 'id', name: 'user_id' })
  user: Promise<User>;

  @Column({ unsigned: true })
  groupId: number;

  @ManyToOne(() => Group, { lazy: true })
  @JoinColumn({ referencedColumnName: 'id', name: 'group_id' })
  group: Promise<Group>;
  
  ....
}

Group으로 GroupApplication을 모두 가져오고 하나씩 돌면서 User 정보를 가져오는 로직

private async getApplicationWithUserInfo(group: Group) {
  const allApplication =
    await this.groupApplicationRepository.findAllApplicationByGroup(group.id);

  const applicationWithUserInfoList = allApplication.map(
    async (application) => {
      const user = await application.user;
        return ApplicationWithUserInfoResponse.from(
          UserInfo.from(user),
          application,
        );
      },
    );

  return await Promise.all(
    applicationWithUserInfoList.map((applicationWithUserInfo) => {
      return applicationWithUserInfo;
    }),
  );
}

현재의 예시는 모임 신청자를 조회하는 로직이다. 모든 신청서(application)을 한 번에 가져온 후 application 엔티티에 Lazy Loading 관계로 존재하는 user를 찾아오는 식으로 구성되어 있다. 아무런 조치도 취하지 않았기 때문에 application을 가져올 때 1번의 쿼리문, 4명의 유저마다 각각 쿼리가 나갔고 총 5번의 쿼리가 발생하게 된다. 한 번에 할 수 있는 걸 5번에 나눠서 하면 당연히 많이 비용이 발생할 것이다. 나중에 유저가 훨씬 많아져서 100명, 1000명, 아니 만명이 된다면 어떻게 될까? 모임 신청자를 조회하기 위해 엄청나게 많은 시간이 들 것이다.

해결과정

N+1 Problem을 해결하기 위해 User를 한 번에 가져와야한다는 사실을 인지하고 있었기 때문에 typeorm 공식 문서(typeorm - relation)를 확인해봤다. Typeorm에서는 이 문제를 해결하는 2가지 방법이 있다. find 메소드의 relation 옵션을 추가하거나, Query Builder를 작성해 해결하는 것이다. 나는 코드 일관성을 위해 find 메소드에 relation 옵션을 추가하는 방식으로 문제를 해결해봤다.

본래 아래와 같이 되어있었던 코드에 relations 옵션을 추가했다.

findAllApplicationByGroup(groupId: number) {
  return this.find({
    where: {
      groupId,
      status: GROUP_APPLICATION_STATUS.REGISTER,
    },
  });
}

findAllApplicationByGroup(groupId: number) {
  return this.find({
    relations: {
      user: true,
    },
    where: {
      groupId,
      status: GROUP_APPLICATION_STATUS.REGISTER,
    },
  });
}

Lazy Loading을 통해 해당 요소를 가져오는 방식을 취할 때 Promise로 묶어놓았기 때문에 배열의 Map으로 만든 Promise 배열을 Promise.all()에 넣어 사용하는 방식은 유지했다. 결과는 성공! 쿼리가 길어지긴했지만 5번의 쿼리로 나눠서 날리는 것보다 훨씬 효율적으로 보인다.

3. 질문받은 내용을 스스로 정리해볼 수 있다.

질문에 대답하려면 공부해야지!

Ep3. Docker

이번 프로젝트에서 동경만 하던 Docker를 처음 사용해봤다. CI/CD를 하고 개발 환경, 배포 환경을 만드는 것이 나의 중점 역할이었기 때문에 자연스럽게 Docker 설정을 내가 도맡게 되었다. PR을 올린 뒤 코드 리뷰를 받았다. 마찬가지로 팀원 중에서도 도커를 처음 접하는 분이 있었기 때문에 아래와 같은 질문이 들어왔다.

도커에 아무것도 모르던 내가 아래와 같이 답할 수 있었다는 사실이 뿌듯했다. 성취감이 느껴진달까? 또, 답변을 작성할 때 생각이 정리되면서 어떤 부분이 문제인지, 어떤 것에 대해 무지한지 정확히 인지할 수 있어서 좋았다. 짧은 프로젝트 기간으로 인해 현재 Buildx에 대한 학습을 어쩔 수 없이 미뤄둔 상태이지만, 프로젝트가 끝난 이후 공부해야할 목록에 추가해두었다.

PR 링크: 백엔드 Docker 환경 설정 - Pull Request

Ep4. Nest Guard

(마찬가지로 NestJS도 이번 프로젝트에서 처음 다뤄봤습니다. ~~다 처음해보네.. 도대체 아는게 뭐야??~~ NestJS에 대한 이해도가 부족하다는 사실을 인지하고 봐주세요...)

해당 작업을 진행하기 전에 Guard에 대한 이해가 필요한 상태였다. 로그인과 인증 부분의 로직을 동료가 맡아서 했기 때문에 해당 부분에 대한 이해도가 직접 코드를 만든 사람보다는 떨어질 수밖에 없었다. 시간은 부족하고 할 일은 많았다. 코드를 읽어보고 동료가 추천해준 링크(공식 문서)를 간단히 훑어봤었고 "AuthGuard에서 User에 대한 정보를 가져와주는 무언가가 있구나. Custom Decorator를 사용하면 User로 싸서 전달할 수 있구나" 정도만 알고 있었는데, 코드 리뷰를 받을 때 여기에 대해 질문이 들어왔다.

정리되지 않은 정보를 전달할 수는 없었기 때문에 공식문서를 꼼꼼히 읽었고 필요한 정보들을 인용해 정보를 정확히 알려줬다. 이 과정을 통해 내가 부족한 부분을 더 공부할 수 있었고 해당 부분을 정확히 이해할 수 있었다.

PR 링크: 내 정보 조회 API

4. PR을 통해 고민을 공유할 수 있다.

Ep5. DTO의 범위에 관하여

학습 스프린트를 진행할 때 내 코드를 리뷰해주는 리뷰어님을 배정받았었다. "Controller에서 Service로 정보를 넘길 때 꼭 해당 정보를 풀어헤쳐서 넣어야한다. 그대로 전달하는 것은 좋지 않다." 라는 피드백을 이전 리뷰어님한테 들은 적이 있었고, 이것이 왜 좋지 않을까. 팀원과 얘기를 나눈 적이 있었다.

이 고민이 멘토링까지 이어지게 되었는데, 멘토님은 웬만하면 DTO를 통째로 넘기지 않는다고 하셨다. 만약 포스트맨 같은 것으로 api body에 아무 값이나 담아서 보냈을 때 Request를 그대로 넘겨버리면 서버가 위험해질 수 있다고 하셨다. 재수가 없으면 repository에 그냥 가버릴 수도 있다고 한다. 실수로 업데이트가 그냥 되어버리는 문제가 생길 수 있는 것이다.

PR 링크: 프로필 수정 API - Pull Request

Ep6. 에러처리

동료가 PR에 자신의 고민과 함께 정리한 글을 아래와 같이 공유해주었다. 백엔드를 맡아 개발하고 있었기 때문에 프론트엔드에 대한 이해도가 떨어져 코드리뷰만으로 코드를 완벽히 이해하기 어려운 상황이었는데 글을 보면서 어떤 고민을 가지고 해당 코드를 만들게 되었는지, 에러 로직이 무엇인지 파악할 수 있었고 코드에 대한 납득이 되었다.

에러 처리 정리: 노션 페이지

Ep7. 순환 참조 오류

내가 만든 API의 모듈과 동료가 만든 API의 모듈이 순환 참조를 일으키는 문제가 발생하게 되었다. 일단 우회해서 해결해놓은 상태였는데 배드 스멜이 느껴지고 있었다. nest 공식문서에서 순환참조를 해결하기 위한 문제를 제공해주고 있었지만, 순환참조로 인해 컴포넌트 간의 명확한 경계가 사라지고 이로 인해 예상치 못한 문제가 발생할 수 있기 때문에 순환참조만큼은 피하고 싶었다. 앞으로의 개발과 코드 리팩토링에도 악영향을 줄 것이라고 판단했다. 아래 멘토님과의 대화를 확인해보자!

PR 링크: 모집 게시글 채팅방 url 조회 API 추가 - Pull Request

결국 멘토님께 여쭤보기로 했고 아래와 같은 답변을 얻을 수 있었다.

이 대화를 나눈 이후에 깨달은 것은 Module에 하나의 Controller만 있어야된다는 고정관념을 가지고 코드를 만들었기 때문에 아래와 같은 문제가 발생했다는 것이다. 단순히 라우터 상으로 분리가 일어났기 때문에 Module도 따로 가져가야한다고 생각했던 것이다. 아직도 해답을 완전히 찾지는 못했다. 명일이와 나는 멘토님이 추천해준 "객체지향의 사실과 오해"를 구매하게 되었고 그 책을 읽으며 조금 더 고민해보기로 했다.

5. 프로젝트에 대한 이해도가 올라간다.

Ep8. 너와 나의 연결 고리

코드를 읽어보고 이해되지 않는 내용이 있으면 아래와 같이 코멘트를 남기면서 서로를 이해하려고 노력했다. 팀원들은 내가 이해하지 못하는 것들을 자세히 설명해주고, 더 좋은 코드를 만들 수 있도록 도와줬다. Typeorm value transformer부터 시작해, Swagger, Docker Compose 등등 내가 잘 모르는 것들을 배울 수 있는 시간이었다.

해당 PR 링크: 모집 신청하기 API - Pull Request

해당 PR 링크: 모집 완료 API - Pull Request

리뷰의 5가지 규칙

카카오 기술 블로그에서 가져온 것으로 코드스쿼드 주관으로 진행된 교육의 내용이라고 한다. 코드리뷰를 할 때 아래 5가지 원칙을 마음에 새기면서 코드리뷰를 하면 더 건강한 리뷰를 할 수 있을 것이라고 생각한다.

1. 왜 개선이 필요한지 이유를 충분한 설명해 주세요.

안 좋은 리뷰

“data 변수 말고 다른 변수명으로 하세요."
“data 변수 말고 grade로 하세요.”

좋은 리뷰

“data라는 이름은 현재의 자료구조가 무엇인지 그 의도를 알기가 어렵네요. 학점 정보를 담고 있는 자료구조 같은데 이와 관련된 변수명을 짓는다면 현재 정의한 자료구조가 무엇인지 그 의도를 쉽게 파악할 수 있을 것 같아요.”

2. 답을 알려주기보다는 스스로 고민하고 개선 방법을 선택할 수 있게 해주세요.

안 좋은 리뷰

“arr.filter(item => item % 2 === 0);으로 사용하세요."

좋은 리뷰

“자바스크립트에는 배열에는 다양한 내장 메서드들이 존재합니다. 그중 filter 메서드를 활용해 보세요. 이 메서드를 활용하면 코드량을 줄일 수 있습니다."

3. 코드를 클린 하게 유지하고, 일관되게 구현하도록 안내해 주세요.

const checkNumber = (comNum, myNum) => {
  return myNum.reduce((prev, curr) => {
    if (comNum[curr]) return prev + 1;
    return prev;
  }, 0);
}

function calculateEarningRate(list, totalPrizeMoney, investMoney) {
  const result = [];
  for(let i=0; i < list.length; i += 1) {
    result.push((list[i] / investMoney) * 100);
  }
  return result;
}

두 가지 함수가 있고 같은 애플리케이션에서 동작합니다.
먼저 함수 선언 방식이 일관되지 않습니다. 아무 의미 없이 함수 표현식과 함수 선언식을 사용하고 있습니다. 그리고 두 함수의 자료구조 반복문 방식이 일관되지 않습니다. checkNumber 반복문은 reduce를 사용했고 calculateEarningRate 반복문은 for문을 사용하였습니다. 리뷰어는 이 두 가지 상황에 대해 리뷰하고자 합니다.

안 좋은 리뷰

"함수 표현식을 사용하셨군요. reduce의 사용도 잘 활용했네요."

이 피드백은 코드의 전체를 본 것이 아니라 독립적으로 본 경우입니다. 일관된 코드인지 확인하기 위해서는 코드 전체를 봐야 합니다.

좋은 리뷰

"함수를 선언하는 두 가지 방식(함수 표현식, 함수 선언식)을 사용했는데 특별한 이유가 아니라면 함수를 선언하는 방식을 스스로 정하고 그 컨벤션 규칙을 따르도록 해보세요. 일관되게 동작하는 코드는 훨씬 보기 좋고 쉽게 수정할 수 있습니다. 그리고 reduce를 통해서 새로운 자료구조를 만든 건 잘했습니다. 하지만 같은 반복처리를 하는 과정에서 calculateEarningRate 에서 for문을 사용한 건 아쉽네요. reduce와 비슷한 방식의 다른 고차 함수를 찾아서 써보면 더 일관된 코드를 유지할 수 있을 거 같습니다."

함수를 반복적으로 선언하는 곳은 프로젝트에서 정한 컨벤션 규칙을 지키면서 코드를 작성하는 게 좋습니다. 만약 프로젝트 내 컨벤션 규칙이 없다면 본인이 정한 예상할 수 있는 수준의 컨벤션 규칙도 좋습니다. 리뷰이가 작성한 코드에서 일관되지 않은 부분을 알려주고 어떤 방식으로 수정하면 좋을지 방법을 제안하는 피드백입니다.

4. 리뷰 과정이 숙제검사가 아닌 학습과정으로 느낄 수 있게 리뷰해 주세요.

안 좋은 리뷰

“클래스 상속은 필요 없습니다.”

좋은 리뷰

"Component 클래스를 상속 받는 건 좋네요. 그러나 자식 클래스에서 부모 클래스를 호출만 하기 때문에 모든 클래스에서 상속받는 건 오히려 중복 코드 같아 보이기도 하는데 이렇게 작성해 주신 이유가 있을까요?"

5. 리뷰를 위한 리뷰를 하지 마세요. 피드백 할 게 없으면 칭찬해 주세요.

코드량이 적당해서 읽기 편하네요.
많은 고민이 코드에서 엿보이네요.
성능에 아주 유리한 코드라고 생각되네요.
전에 코드보다 훨씬 좋아진 거 같네요.
예외 처리가 꽤 꼼꼼해서 좋네요.
함수, 변수명이 직관적이어서 좋네요.

앞으로 나의 코드리뷰는? (결론)

네이버 부스트캠프에서 열정적인 동료들의 코드리뷰를 받으며 성장하는 좋은 경험을 했다. 하지만 코드리뷰에 좋지 않은 점은 없을까? "코드 리뷰의 목적은 성장이어야 한다"라는 블로그 글의 내용 중 하나이다.

이젠 리뷰라는 행위에 대한 인식을 맞춰보자.
리뷰하고자 하는 관점을 코드가 아닌, 코드를 작성한 엔지니어에게, 제품을 만드는 메이커에게 옮겨보면 어떨까.
일시적으로 코드의 품질을 높이려 하지 말고, 함께 성장하기 위한 수단으로 리뷰를 활용해보자.
리뷰를 지적이라고 느끼는 순간, 앞에서 언급한 부작용(병목이 되는 코드 리뷰, 의미없는 코드 리뷰, 싸움이 되는 코드 리뷰, 책임 소재를 묻는 코드 리뷰)들이 발생하게 된다. 지금 당장의 코드를 세세하게 변경(Change Request)하려고 하기 보단 메이커가 성장해서 제품을 더 잘 만들 수 있도록 리뷰를 해보면 어떨까.

코드리뷰 굉장히 매력적인 활동이지만 리뷰를 받는 사람이 어떻게 생각하느냐에 따라서 코드리뷰가 망할 수도 있다. 코드리뷰 문화가 없는 회사에서 리뷰 문화를 도입하려하는데 팀원이 코드리뷰를 원하지 않는다면 어떻게 될까?

조직 내에서 아무 계획 없이 그저 개발 문화를 위해 코드 리뷰를 도입하게 되면 제대로 동작하지 않는 리뷰 프로세스를 마주하게 되고 부작용을 겪어 ‘우리 다신 보지 말자.’가 될 수도 있다. 코드 리뷰를 한다면, 먼저 구성원들이 그것을 당연하다고 느껴야 한다. 리뷰가 어떤 규칙이나 제약이 되면 반발심이 생길 수 있다. 처음부터 그럴 수는 없겠지만, 당연하다고 생각하는 것이 문화이다.

코드 리뷰에 참여하는 구성원들의 인식이 같은 곳을 바라보는가? 문화로 받아들일 준비가 되어있는가? 확인해보자. 좋은 코드에 대한 기준을 먼저 얼라인하자. 어떤 코드가 좋은 코드인가? 함께 토론해보자. 전체적인 코드 컨벤션 확립과 코드 일관성을 유지하기 위한 도구 도입하자. 불필요하고 소모적인 리뷰로 인해 시간 낭비, 리소스 낭비를 예방해야 한다. 먼저 테스트를 작성해보면 어떨까?

인프콘의 강의를 듣는데 코드리뷰 문화를 만들려고 노력했다는 분의 얘기를 들었다. 나도 현업에 나갔을 때 회사에 리뷰 문화가 없다면 만들기 위해 노력하는 사람 중 하나가 되고 싶다고 생각했다. 공식적으로 할 수 없다면, 나와 친분이 있는 동료들과 비공식적으로 진행하고 싶다고 생각했다. 하지만 위의 사항을 전혀 고려해본 적이 없었다. 프로젝트를 코드리뷰에 열려있는 사람들과 함께하고 있기 때문에 코드리뷰를 하는 것이 당연하다고 생각했던 것 같다.

단순히 코드리뷰를 하겠다가 아닌 비용을 따져보고 코드리뷰가 무조건 필요한 지, 또 건강하게 진행될 수 있을지 가능성을 타진하고 고민한 후에 시작하는 것이 좋을 것 같다!

Reference

N+1 문제: https://choidr.tistory.com/entry/TypeORM-N-1-%EB%AC%B8%EC%A0%9C
Typeorm N+1: https://tristy.tistory.com/36
우아한 형제들 기술블로그, 코드리뷰가 쏘아올린 작은공: https://techblog.woowahan.com/2712/
카카오 기술 블로그, 효과적인 코드리뷰를 위한 리뷰어의 자세: https://tech.kakao.com/2022/03/17/2022-newkrew-onboarding-codereview/
코드 리뷰의 목적은 성장이어야 한다: https://jbee.io/essay/code-review-goal/

MYVELOP 마이벨롭