📕 목차

1. 서론
2. Convention 확립
3. Commit을 잘게 쪼개자
4. 리뷰 요청자의 자세
5. 리뷰를 할 때의 자세

1. 서론

 

 

코드 리뷰 in 뱅크샐러드 개발 문화 | 뱅크샐러드

안녕하세요, 뱅크샐러드 BanksaladX iOS Engineer…

blog.banksalad.com

한 프로젝트에서 PM, Scrum Master, Backend 동시에 하느라 몸이 갈려나가는 중이라 포스팅이 상당히 줄었다.

매일같이 블로그를 보면서도 공백기가 길어질 수록 너무 슬펐는데, 프로젝트 도중에 너무 좋은 글을 읽어서 정리해보려 한다.

 

이번 포스팅은 팀 프로젝트를 진행하기 전에 Convention으로 미리 정해두고 시작하면 좋은 내용들이다.

뱅크샐러드 내용을 많이 참조해서 중복된 내용일 수 있지만, 최대한 내 경험과 엮어 필요한 이유를 몇 자 더 적어보고자 한다.

 

2. Convention 확립

 

📌 개요

코딩 스타일은 개발자마다 다르다.

특히 학부생 수준에서는 코딩 스타일이 확립되지 않은 경우가 많다.

코딩 스타일이 다르다고 해서 코드 리뷰 단계에 Blocker를 주기엔 애매한 경우가 많다.

 

문제는 이 작업이 누적되면 장기적으로 유지보수성이 저하될 수 있다는 문제가 생긴다.

따라서 프로젝트 시작 전에 Convention을 정해놓는 것은 필수적이다.

 

다만, 이를 강제적으로 제한하는 기능을 추가하는 것은 스트레스만 높아지고 개발 생산성을 저하시킬 수 있다.

자동화할 수 있는 도구가 있다면 자동화하여 사람의 리소스를 아끼는 것이 최선이다.

 

아래 명시한 Convention은 내가 자주 사용하는 방식이다.

참고해보고 본인 프로젝트와 팀원에게 맞는 컨벤션으로 수정하거나 추가/삭제해서 사용하면 좋을 것이다.

 

📌 변수명

사람마다 변수명을 작성하는 방법 또한 다양하다는 것을 명심하자.

자바 프로그래밍에 익숙한 사람은 당연히 camelCase를 사용하겠지만, C 계열 프로그래밍에 익숙한 사람은 대부분 lower_snake_case를 사용한다.

물론 Java를 사용하기로 했다면, 표준 언어 규칙을 지키는 것이 맞지만 항상 중요한 것은 팀의 생각이라 생각한다.

 

다음을 고려하여 명시하자.

  • 표기법 : camelCase
  • 조합: 명사
  • 상태 처리를 위한 변수: 접두사 `is-`
  • 자료 구조
    • 리스트: 복수명사
    • 맵: 접미사 `-Map`
  • idx, arr, cnt, btn과 같이 이해하기 쉬운 약자 외엔 사용하지 않아야 한다.
  • 상수는 반드시 UPPER_SNAKE_CASE

이 외에도 고려할 것이 있다면 미리 약속하자.

 

📌 함수
  • 표기법: camelCase
  • 조합: 동사 + 명사 (ex. getData)
  • 서비스 계층 메서드 명
    • 상위 서비스: 기능 관점에서 명확한 동사 (ex. signIn, signUp)
    • 하위 서비스: 구체적인 작업을 명시하는 메서드명 (ex. findById, save)
      - search를 위한 메서드는 반드시 `find-`로 시작하며, 필요한 매개변수는 `By-`뒤에 명시한다.
      - 존재 여부를 반환하는 메서드는 `exists-`로 시작한다.
  • 이벤트 핸들러 조합: handle + 명사 + 동사 (ex. handleBtnTabClick)

 

📌 클래스
  • 표기법: PascalCase
  • 조합: 명사 (ex. Domain)
  • 불변성을 보장해야하는 데이터 타입(ex. Dto)은 record 타입을 사용한다.

 

📌 코드 스타일

코드 스타일 컨벤션은 잡으려면 끝이 없다.

최소한으로 정하고 코드 리뷰를 꼼꼼하게 하는 것이 경험적으로 가장 좋은 방법이라고 생각한다.

 

  • 들여쓰기는 반드시 tab (space 4번 X)
  • magic number는 반드시 상수로 정의하여 의도를 전달할 것
  • for, stream 모두 사용 가능하다.
    • 단, stream이 과하게 복잡한 경우 1차로 도우미 메서드를 사용하여 가독성을 확보한다.
    • 불가능할 경우 for문으로 풀어서 작성한다.
  • 코드 분기 처리는 다음 구조를 따른다. (else-if 구문 사용 지양할 것)
if (state) {

} else {

}

 

📌 브랜치 & 커밋
  • angular commit convention을 준수한다.
    • feat: 신규 기능 추가
    • fix: 버그 수정
    • docs: 문서 수정
    • rename: 주석, 로그, 변수명 등 수정
    • style: 코드 포맷팅, 세미콜론 누락 (코드 변경 없는 경우)
    • refactor: 코드 리팩토링
    • test: 테스트 코드, 리팩토링 테스트 코드 추가
    • chore: 빌드 업무 수정, 패키지 매니저 수정
  • git branch strategy
    1. main
      • 배포 가능한 상태의 코드만을 관리하는 프로덕션용 브랜치
      • PM 승인 후 병합 가능
    2. dev
      • 개발 전용 브랜치
      • 한 명 이상의 팀원의 승인 후 병합 가능
      • 기능 개발이 완료된 브랜치를 병합하여 테스트를 진행
    3. 이슈 기반 브랜치
      • 이슈는 `{티켓번호}-{브랜치명}`을 포함한다. (나는 Jira를 사용한다.)
        ⇒ Github Project의 Kanban Board를 사용한다면 `#{이슈 번호}`를 포함하도록 한다.
      • `feat/{티켓번호}-{브랜치명}`: 신규 기능 개발 시 브랜치명
      • `fix/{티켓번호}-{브랜치명}` : 리팩토링, 수정 작업 시 브랜치명
      • `hotfix/{티켓번호}-{브랜치명}` : 빠르게 수정해야 하는 버그 조치 시 브랜치명
  • merge strategy
    • Merge, Squash, Rebase 중 한 가지 전략 혹은 경우에 따라 병합 전략 선택
    • 모든 대화가 종료될 때까지 병합할 수 없으며, 종료 판단은 Reviewer가 결정한다.
    • 병합은 PR 게시자가 직접 한다.

 

📌 기타 고려 사항

위는 대부분의 프로젝트에서 공통적으로 고려하는 항목이라면, 프로젝트 별로 디테일한 Convention을 정의할 필요가 있을 수 있다.

 

예를 들어, 현재 진행 중인 프로젝트에선 멀티 모듈화를 사용하며 Facade Pattern을 적용해 서비스 계층을 논리적으로 구분하였다.

  • 모듈
    • application 모듈(ex. internal-api, external-api, admin-api), domain 모듈, infra 모듈, common 모듈 관심사
  • Service 규칙
    • 상위 서비스의 이름은 `-UseCase`가 되며, `@UseCase` 어노테이션으로 명시한다.
    • 코드는 서술적 구조를 준수하며, 핵심 비지니스 로직을 처리한다.
    • 하위 서비스 이름은 `-Service`로 하되, 메서드가 방대해지는 경우 `-SearchService`, `-SaveService`로 분리한다.
    • 단계를 더 나누어 DomainService 모듈을 생성하면 규칙이 추가될 것이다.
  • Database
    • 테이블 명은 lower_snake_case를 준수한다.
    • PK: id 컬럼을 필수로 갖는다.
    • 사용자의 이름과 닉네임과 아이디 컬럼명은 각각 name, nickname, username으로 구분한다.
    • FE: `{참조 테이블명}_id`
    • 레코드 관리용 날짜 컬럼은 `created_at`, `updated_at`, `deleted_at` 
    • 이 외에도 작성자, 수정자의 정보나 상태 정보를 기록하는 필드명에 대해서도 정하면 편하다.
  • 패키지
    • 구분자를 사용하는 것은 되도록 지양할 것 (필요하다면 이유를 명시하여 commit)
    • 패키지 구조 예시...
  • API
    • `https://api.{domain name}.com` 으로 시작한다.
    • `/v1`, `/v2`, `/v3` 버전 구분 사용 여부 및 구분 방법
    • RESTful Naming 컨벤션

 

3. Commit을 쪼개자

 

📌 무지성 커밋의 위험성

기존에 사용하던 방식

협업 경험이 많이 없기도 했고, 있어도 딱히 커밋으로 인한 문제가 발생한 적이 없어서 지금까지는 뭉텅이로 작업을 하고 한 번에 커밋을 했었다.

예를 들어, 사용자의 api에 필요한 5가지 기능이 있다면 5가지 기능을 모두 구현하고 한 번에 commit을 하는 식이였다.

 

그러다 한 번 된통 당했던 적이 있는데, 작업 사항을 rollback 해야 하는 상황이 발생한 것이다.

작업 사항 중 극히 일부만 되돌리고 싶은데 commit의 단위가 너무 커서 오히려 rollback이 더 비용이 클 정도였다.

 

그렇다면 커밋은 대체 언제 해야할까?

 

 

커밋을 잘게 쪼개자 - 커밋은 언제 하는 것이 가장 좋을까?

"깃 커밋은 언제 하는 것이 가장 좋을까?" 들어가면서 커밋의 단위는 개발자들마다 생각이 각기 다를 것이다. 누구는 크게, 누구는 작게 말이다. 이 글에서는 커밋을 어느 단위로 할지에 대해 이

jaeheon.kr

나는 그 해결 방안을 위 블로그에서 찾았다.

내가 작성할 내용은 모두 여기서 확인할 수 있다.

 

📌 커밋의 기본 원칙
💡 1 Action, 1 Commit

 

1️⃣ 하나의 액션, 하나의 커밋

커밋 메시지가 길어진다면 더 잘게 나눌 수 있진 않았는지 고민해보는 것이 좋다.

예를 들어 5가지 기능이 담긴 api를 커밋한다고 하면, 최소 5개로 나눌 수 있다.

그리고 각각의 api를 구현하기 위해 controller, service, domain service 로직을 추가했다고 하면, 이 또한 잘게 나눌 수 있다.

 

어차피 마지막에 병합할 때는 squash 전략으로 커밋을 하나로 축약할 수 있음을 기억하자.

 

2️⃣ 전역으로 영향을 주는 커밋을 구분하자

여러 클래스에서 동일한 수정 작업을 수행했다고 하더라도 하나의 커밋으로 묶는 것은 위험할 수 있다.

커밋을 revert 할 때 영향을 받는 코드의 범위가 광범위해지기 때문이다. (변경 사항 추적이 힘들다.)

 

spring에서 여러 모듈의 설정을 한 번에 고치고 "application.yml profile 수정"이라고 하면, rollback을 했을 때 모든 모듈의 application.yml 설정이 영향을 받는다.

이보다는 "common 모듈 application.yml profile 수정"이 낫다.

 

3️⃣ 커밋 메시지는 명료해야 한다.

커밋 메시지만 봐도 어떤 작업이 수행되었고, 어떤 클래스들이 영향을 받았는지 확인 가능해야 한다.

그렇지 않으면 revert를 시킬 때마다 변경된 코드를 모두 추적해야 하는 시간 손해를 감수하게 된다.

 

4️⃣ 합칠 수는 있지만 나눌 수는 없다.

가장 중요하다고 생각하는 내용이다.

이러한 방식의 commit은 너무 과하다고 생각할 수 있다.

마치 게임을 하면서 F5를 연타하는 거랑 차이를 구분하기 힘들 정도다.

 

하지만 커밋을 합치는 것은 쉽지만, 나눌 수는 없다는 것을 기억하자.

가장 좋은 방식은 작업할 때는 잘게 나누고, 마지막에 합치는 전략을 택하는 것이다.

 

설령 고치기 힘들더라도 작은 커밋이 언제나 낫다.

 

📌 언제 커밋을 해야 하는가?

1️⃣ 한 번에 커밋해도 좋은 경우

작업 사항이 모두 독립적이면 한 번에 커밋해도 문제가 없을 수 있다.

A, B, C, D 파일이 변경되었지만, 각각 외부에 어떠한 변경사항도 전파하지 않으며 수정될 일이 거의 없음을 보장한다면 한 번에 커밋하는 게 나을 수도 있다.

 

하지만 이 것도 되도록 4개의 파일을 별도로 커밋하는 것이 좋다.

왜냐하면, 어쩌다 보니 B의 작업을 revert해야 하는데 A, C, D의 파일이 모두 rollback 되기 때문이다.

 

2️⃣ 나누어서 커밋해야 하는 경우

작업 사항이 외부에 영향을 주거나, 하나로 묶일 수 없는 거의 대부분의 케이스에 해당한다.

 

하나의 Util 클래스를 정의하기 위해 다음과 같은 일련의 작업을 수행했다고 가정하자.

  1. 환경 변수 주입
  2. enum class로 상수 지정
  3. Util 내 5가지 메서드 정의

이 경우엔 최소 3개의 커밋으로 나누는 것이 옳다.

다만, 3번은 경우에 따라 다를 수도 있다.

5가지 메서드가 모두 외부에 노출된다면 나누는 것이 좋을 수도 있다.

 

4. 리뷰 요청자의 자세

 

📌 큰 PR의 문제점 

어차피 백엔드가 나 혼자밖에 없는 프로젝트였기에 당시에는 뭉텅이로 PR을 올렸었다.

다만, 해당 PR을 리뷰해줄 팀원이 있었다면 그야말로 끔찍한 상황이었을 것이다.

 

한 번은 5,000줄에 가까운 PR을 리뷰한 적도 있었는데, 코드의 길이가 길어질 수록 집중도가 급격하게 저하된다.

코드를 이해하는 시간이 길어질 수록, 코드 리뷰에 대한 부담감이 높아지게 되었으며

결과적으로 PR이 병합되는 시기가 딜레이되어 프로젝트 전체 진행도에 영향을 주게 되었다.

(깨진 창문 이론이 이런 걸까)

 

이러한 경험은 PR을 더 작은 단위로 가져가는 것이 맞다는 생각으로 귀결되었고, PR을 세분화 하기 위해 어떤 행동을 해야 하는가에 대해 고민하게 되었다.

 

📌 작은 PR 규칙

PR을 세분화 하려면, 단순히 제한을 걸기 전에 "작업이 충분히 세분화 되었는가?"를 고려해봐야 한다고 생각한다.

 

현재 진행 중인 프로젝트는 애자일 스크럼 방식을 도입하였고, 하나의 User Story를 완성하기 위한 Sub Task를 스프린트 계획 회의 단계에서 정한다.

단순히 "[BE] 회원가입 API" 하나만 있었다면, 해당 작업을 수행한 PR에는 각종 Util부터 환경 설정, 핵심 비지니스 로직에 걸친 방대한 작업량이 진행될 것이다.

 

최선의 방식은 "[BE] 회원가입 API"를 구현하기 위해 필요한 작업 사항들을 최대한 세세하게 분리하는 것이다.

작업량은 되도록 하루에 끝낼 수 있을 정도의 단위로 정하면 좋다.

 

물론, 처음부터 이게 가능하지는 않을 것이므로 시행 착오법을 통해 분리하는 것도 좋다고 생각한다.

 

📌 PR LOC(Line Of Code) 제한

1개의 PR은 1,000 Line을 넘을 수 없다는 제한을 걸어보자.

이러한 제한을 걸어둔 가장 최근의 PR 수도 무려 839 라인에 달하는데, 아마 인터페이스를 정의하는 작업과 구현체를 정의하는 작업을 나눠볼 수도 있었을 것 같다.

 

PR을 나누면, 그만큼 승인 받기 위해 대기하는 시간이 더 소요될 수 있지 않을까란 우려도 있을 수 있다.

하지만 작은 PR은 팀원이 리뷰를 하는데 드는 부담감을 줄일 수 있다.

300줄 이하의 라인 수를 유지하면 리뷰 완료에 드는 속도가 분명하게 줄어든다. (애초에 팀원이 게으른 건 논외 사항)

 

하지만 1,000줄에 가깝거나 그보다 큰 PR은 일주일이 지나도 리뷰가 진행되지 않는 경우도 있었다.

아무래도 회사에서 하는 것이 아닌 사이드 프로젝트일 수록 그 문제가 분명하게 드러난다.

 

🤔 테스트 코드도 라인 수에 포함하는가?

테스트 코드의 경우 Mock를 생성하는 라인 수가 굉장히 많이 들어간다.

이러한 이유로 뱅크 샐러드에서도 테스트 코드의 라인 수를 제한하지는 않는다.

 

📌 PR 내용에 포함되어야 할 정보
 

✨ JwtProvider 인터페이스 및 AT, RT Provider 구현체 정의 by psychology50 · Pull Request #6 · CollaBu/pennyway-was

작업 이유 😅 하다보니 너무 많이 작업하고 한 번에 PR 올려버렸습니다. 다음부터는 더 잘게 쪼개볼게요. OCP 원칙을 준수하기 위해 전략 패턴 기반 JwtProvider 인터페이스 정의 AT, RT Provider 구현체

github.com

 

나는 PR에 필수적으로 4가지를 포함시킨다.

1. 작업 이유
2. 작업 사항
3. 리뷰어가 중점적으로 확인해야 하는 부분
4. 발견한 이슈

여기서 테스트를 수행한 부분을 따로 명시하는 것도 좋다고 생각한다.

 

3번이 가장 중요하다고 생각하는데, 2번은 때때로 내용이 길어지기도 한다.

그러면 리뷰어는 '대체 어떤 부분을 중점적으로 봐야하는 거지?'라는 혼란에 빠질 수도 있다.

 

나는 3번을 다음과 같은 항목에 해당된다면 반드시 작성한다.

  • 리뷰어가 확인해야 할 핵심 기능 코드
  • 더 나은 해결책을 질의하기 위한 경우

 

뱅크 샐러드에선 다음 리스트를 채택한다고 한다. (Figure 8)

1. 제품 기능 기획
2. 기술적인 구현 계획
3. 현재 PR에서의 변경점
4. 예상/구현된 디자인
5. 어떤 내용들을 테스트 했는지 / 해야 하는지

 

 

📌 얼마나 상세하게 기술할 것인가?

전역적으로 상태를 미치는 상위 모듈 수정 사항이나, 편의성을 위해 만들었으며 베이스가 될 법한 코드(에러 처리 클래스)들은 최대한 자세하게 적는 편이다.

주석도 가능한한 예시 코드까지 작성하는 편인데, 당장은 불필요할지 몰라도 추후 의사소통 문제로 인한 에러가 발생하지 않을 수 있다는 측면에서 더 낫다고 생각하기 때문이다.

그리고 해당 코드를 사용해야 하는 입장에서도 매번 나에게 찾아오지 않아도, PR과 주석만 읽으면 사용 가능하기 때문에 불필요한 곳에 소모되는 시간을 줄일 수 있다.

 

하지만 모든 작업을 이렇게 작성한다면 리뷰어가 읽어야 할 PR의 내용이 많아지므로, 이 또한 부담을 주게 된다.

그래서 나는 3가지 기준을 정하여 작성한다.

 

  1. 중요한 작업 사항이 반영되어 있으며, 반드시 리뷰어가 이해해야 하는 내용
    • 최대한 상세하게 적으며, 가능하다면 클래스 다이어그램으로 명시적으로 알려주는 것이 좋다.
    • 추후 오해로 인해 작업이 딜레이 될 수 있으므로, 리뷰어가 충분히 이해할 수 있도록 쉽게 설명한다.
  2. 중요도는 상이할 수 있지만, 리뷰어가 인지는 해야하는 내용
    • 자세하게 기술하지 않되, 명시는 해둔다.
    • 리뷰어가 해당 부분에 대해 질문하면 설명한다.
  3. 리뷰어가 몰라도 되는 내용
    • 작성하지 않는다.
    • 리뷰어가 해당 부분에 대해 질문하면 설명한다.

 

5. 리뷰를 할 때의 자세

 

📌 Reviewer의 자세

실력이 미숙한 사람은 용서해도, 리뷰를 게을리 하는 사람은 피의 숙청을..

까지는 과장이지만 리뷰를 성실하게 임해야 한다는 생각은 변함이 없다.

 

다만, 지금까지 나는 단순히 "리뷰를 성실하게"라는 마인드만 강조했지 구체적으로 어떠한 자세를 가져야 할 지는 생각을 해보지 않았었다.

 

뱅크 샐러드에서는 다음과 같은 자세를 제시하고 있다.

  • 일관된 아키텍처를 유지하고 있는지
  • 다른 해결 방법에 대한 의견 제시
  • 버그가 발생할 수 있는 가능성 제시
  • 기술적인 지식, 노하우 공유
  • 히스토리 전달

 

이러한 자세를 프로젝트 팀원들에게 전파했고, 지금은 전보다는 다소 건전한 리뷰 문화를 가질 수 있게 되었다.

 

📌 Pn Rule

가장 핵심적인 내용이라 생각했던 부분.

PR이 올라오면 Request changes, Comment, Approve 3가지 중 하나를 선택하게 된다.

 

그렇다면 각각은 어떠한 기준으로 선택해야 할까?

  • P1: 꼭 반영해주세요 (Request changes)
  • P2: 적극적으로 고려해주세요 (Request changes)
  • P3: 웬만하면 반영해 주세요 (Comment)
  • P4: 반영해도 좋고 넘어가도 좋습니다 (Approve)
  • P5: 그냥 사소한 의견입니다. (Approve)

자세한 내용은 뱅크 샐러드에서 확인.

 

📌 저 문맥(Low Context) 커뮤니케이션

누군가를 가르쳐본 사람은 당연히 알 만한 내용이지만, 그렇지 않은 사람은 쉽게 착각할 수 있는 것이 내가 안다고 상대도 알지는 않다는 것이다.

질문하거나 답변을 할 때, 최대한 상대가 이해할 수 있는 언어를 사용하는 것이 좋다.

 

그리고 PR을 작성할 때 상세하게 작성해야 하는 지에 대한 여부를 고려하게 될 때도 이 점을 되짚어 봐야 한다.

리뷰 요청자는 리뷰어가 사전 지식이 없다는 전제를 기반으로 PR을 작성해야 한다.

 

또한 리뷰어도 리뷰 작성자에게 새로운 방식을 제안할 때, 그 개념을 모를 것임을 전제로 자세히 알려주어야 한다.

 

이러한 약속을 지키지 않으면 질의응답으로 인한 추가적인 커뮤니케이션 비용과

잘못된 이해로 인해 유발되는 추가 비용이 더 클 수 있다는 점을 유의하자.

저작자표시 비영리

티스토리툴바