코드 리뷰는 개발자의 스킬 향상 뿐만 아니라, 개발 문화 형성에 아주 중요한 요소라고 생각한다. 그런 관점에서 코드 리뷰가 지속가능한 문화로 자리를 잡게하기 위해서 필요한 것들을 여기에 정리한다.
여기서는 코드 리뷰에서 따라야할 기본 원칙들을 정리한다. 될 수 있으면 최소한의 원칙만 작성하는 것으로 한다.
- 리뷰도 프로젝트의 공식 일정과 자원에 포함시킨다.
- 목표는 사람(사람에 대한 선입견 포함)이 아니라 프로그램의 결과물을 검토한다.
- 문제를 명확하게 할 순 있어도 모두 해결하려고 하지 않는다.
- 성능과 이해도에 영향을 주지 않는 한 그 사람의 스타일을 문제제기하지 않는다.
- 세세한 버그를 볼 것이 아니라, 전체 구성과 흐름에 문제가 없는지 주목하자.
- 리뷰는 2시간을 넘지 않는다.
- 초기에는 비공식 리뷰나 교육을 자주하고 일정시간이 지나면 공식 리뷰로 전환한다.
- 필요하면 체크리스트를 만들어서 리뷰에 임한다.
- 토론을 장려하고 반론은 대다수가 동의가 이루어졌거나 대상자가 불특정 다수가 되었을때 진행하자.
| 구분 | 포인트 | 우선순위 | 코드 검토 기준 | 비고 |
|---|---|---|---|---|
| 사내 표준 | 안전성, 성능 | MUST | 테스트 작성 | 테스트 코드 가이드 참조 |
| 사내 표준 | 보안 | MUST | 비밀번호와 같은 민감한 정보를 로그에 남기지 않아야 함. | 개발보안 가이드 참조 |
| 요구사항(기획) | MUST | 기획에 맞게 개발이 이루어졌는지 여부 | 기획서 참조 | |
| 공통 | 가독성, 유지 보수성 | MUST | 코드가 복잡성. 순환 복잡도 등 linter에서 오류가 있는 경우 수정 | 코딩 가이드 참조 |
| 언어 공통 | 가독성 | MUST | 매직 넘버를 쓰지 않아야 함 | 코딩 가이드 참조 |
| 언어 공통 | 가독성 | MUST | 네이밍 룰이 적절한지? | |
| 언어 공통 | 안전성 | MUST | 잘못된 값이 포함되지 않도록 유효성 검사 | |
| 언어 공통 | 안전성 | MUST | 예외 처리가 빠진 곳이 없는지 | |
| 언어 공통 | 안전성 | MUST | 적절한 오류 메시지인지? | |
| 언어 공통 | 유지 보수성 | MUST | 구현이 적절한 레이어에 구현되어 있는지? | |
| 언어 공통 | 성능 | MUST | 루프 내에서 DB를 처리하는 경우 대안 고민. | DB접속 횟수가 증가하고 성능이 저하됨 |
용어는 굳이 외우려하지 말며, 리뷰에서 넓은 관점을 유지하지 위해 정리한다. 리뷰에서 공식적으로 거론하지는 않으며 강요하지도 않는다.
- DC(Duplicate Code) - 중복된 코드(중복된 코드나 조건들).
- BPC(Bad Performance Code) - 성능에 좋지 않는 코드.
- BN(Bad Name) - 변수나 클래스, 함수 등에 너무 길거나 가독성 떨어지고 일관되지 않은 이름.
- VS(Violates Specifications) - 가이드나 지침을 따르지 않음.
- TCL(Too Complicated/Larged) - 불필요하거나 너무 크고 복잡한 함수나 클래스.
- LP(Long Parameter) - 불필요하거나 너무 긴 인자값들.
- II(Inconsistent Indentation) - 비일관적인 들여쓰기.
- NEC(Not Enough Test Cases) - 테스트 케이스가 너무 적음.
- TMIS(Too Much If/Switch) - 조건 분기가 많음.
- NC(No Comments) - 주석이 너무 없음.
- SS(Shotgun Surgery) - 하나의 함수나 클래스를 변경했는데 많은 것들이 영향을 받는다.
- FE(Feature Envy) - 다른 클래스의 속성과 메소드들을 사용해 다른 클래스와 강한 관계가 성립한다. 모듈(클래스) 독립적이지 못하다.
- BC(Boilerplate Code) - Getter/Setter 등 맹복적으로 추가되는 반복적인 코드들.
- TE(Throws Exception) - 예외 처리.
- NG(Not General) - 너무 범용적이지 않음.
- DD(Dead Code) - 사용되지 않거나 미리 만들어진, 불필요한 코드들.
- MN(Magic Number) - 매직 넘버.
- PD(Platform Dependency) - 플랫폼(언어, 환경 등) 종속성이 있다.
- AOS(Abuse Open Source) - 오픈 소스 남용.
벨을 붙이면 액션을 명확하게 전달할 수 있고, 코멘크의 우선순위가 명확해져 효율성을 가져올 수 있다. GithubDocs >Setting >회신템플릿 만들기로 처리하면 외우지 않아도 된다.
| 라벨 | 의미 | 의도 |
|---|---|---|
| Q | 질문 (Question) | 질문. 상대방은 답변이 필요합니다. |
| FYI | 참고로 (For your information) | 참고로 공유. 액션은 불필요 |
| IMO | 제안 (In my opinion) | 개인적인 견해와 제안. |
| MUST | 필수(Must) | 이것을 고치지 않으면 승인할 수 없다. |
- 단일 책임의 원칙(SRP): 클래스와 메소드는 하나의 명확한 책임(역할)을 가져야 한다.
- 고응집·소결합: 관련성이 높은 코드는 가까이에 정리(고응집), 모듈간의 의존관계는 최소한으로 억제한다(소결결합).
- 적절한 클래스 설계: 불필요한 가치 객체나 과도한 범용화(YAGNI 위반)를 피하고 단순하고 목적에 맞는 클래스 설계 한다.
- 로직의 적절한 배치: 비즈니스 로직과 데이터 조작 로직이 적절한 클래스에 배치되었는지 확인한다.
- 상속보다 위임을 선호한다: 코드의 재이용이나 확장성을 높이기 위해서, 계승보다 위임(composition)을 우선적으로 검토한다.
- 결과의 국소화: 변경의 영향 범위가 제한 되도록, 관련성이 높은 코드가 정리되어 있는지 확인한다.
- 로직과 데이터 통합: 데이터와 데이터를 조작하는 로직이 같은 클래스나 모듈에 적절하게 배치되어 있는지 확인한다.
- 관심 분리(SoC): 다른 관심사(예: UI, 비즈니스 로직, 데이터 액세스)가 적절히 분리되어 각각의 모듈이 독립적으로 변경·이해할 수 있도록 되어 있는지 확인한다.
- 인터페이스와 구현 분리 : 구현 클래스에 직접 의존하는 것이 아니라, 인터페이스나 추상 클래스를 통해 의존하는 것으로, 구현의 변경이 용이하게 되어 있는지 확인한다.
- 재사용성: 모듈화, 컴포넌트화, 설정 가능성 등을 통해 코드가 다른 컨텍스트에서도 재사용하기 쉽게 설계되어 있는지 확인한다.
- 간단함(KISS): 필요 이상으로 복잡한 코드가 없는지 확인한다.
- YAGNI(You Ain't Gonna Need It): 현재 필요하지 않은 기능과 범용화(오버엔지니어링)는 구현하지 않는다.
- 표준적인 방법의 이용: 보다 단순하고 표준적인 방법(예: HTML 표준 태그 vs JS)로 구현할 수 없는지 검토한다.
- 적절한 추상화 레벨(SLAP): 같은 루틴이나 코드 블록 내에서 추상 레벨이 혼합되어 있는지 확인한다.
- 중복 로직의 추상화: 로직이 중복되어 있는 경우는, 적절한 클래스나 메소드에 추출해 공통화한다.
- 충실성, 무결성, 프리미티브성: 추상화의 레벨이나 크기가 적절한지 검토한다.
- 변경 예측 및 격리: 앞으로 변경될 가능성이 높은 부분(외부 API 연계, 비즈니스 규칙 등)을 파악하여 변경이 용이하도록 설계한다.
- 개방/폐쇄 원칙(OCP): 소프트웨어 엔티티(클래스, 모듈, 함수 등)는 확장에 대해 열려 있고 수정에 대해 닫혀 있어야 한다.
- RESTful 디자인: 웹 API 등은 리소스 지향을 기반으로 RESTful 디자인 원칙을 준수하는지 확인한다.
- 인터페이스 일관성: 관련 API 및 메소드 간에 인터페이스 (매개 변수, 응답 형식 등)에 일관성이 있는지 확인한다.
- 파라미터 설계: API 등의 파라미터가 필요 충분�한지 확인한다.
- 상호 운용성: 표준 프로토콜 및 데이터 형식(예 : JSON, REST)을 채택하는 등 다른 시스템과 연계하기 쉬운 설계가 되어 있는지 검토한다.
- 프레임워크 기능 활용: 프레임워크가 제공하는 기능을 적절히 활용하여 코드를 간단하게 유지하고 있는지 확인한다.
- 의존성: 새로운 라이브러리를 추가하는 경우, 그 필요성이나 이점이 충분한지 검토한다.
- 바퀴 재발명의 회피: 표준 라이브러리, 프레임워크, 또는 신뢰할 수 있는 기존의 라이브러리로 실현할 수 있는 기능을 자체적으로 구현하고 있지 않은가를 확인한다.
- 스타일 가이드 준수: 프로젝트 또는 언어 커뮤니티에서 정한 스타일 가이드를 따르는지 확인한다.(Google, Code Complete)
- 일관성: 프로젝트 전체에서 코딩 스타일(들여 쓰기, 공백, 괄호 위치 등)이 일관성이 있는지 확인한다.(Code Complete).
- 시각적 구조: 들여쓰기나 빈 줄이 코드의 논리 구조를 시각적으로 알기 쉽게 표현하고 있는지 확인한다.(Code Complete)
- 대칭 원리: 쌍이 되는 조작(예:
open/close,lock/unlock,create/destroy)이 고려되어 무결성이 유지되고 있는지 확인한다.
- 명확하고 구체적인 이름: 변수, 메소드, 클래스, 상수 등의 이름이 그 역할이나 내용을 정확하고 명확하게 전달되는지 확인한다.
- 일관성: 프로젝트 내에서 명명 규칙(캐멜 케이스, 스네이크 케이스 등)과 용어가 일관되어 있는지 확인한다.
- 효과와 목적: 이름이 그 요소가 무엇을 하는지(효과)뿐만 아니라, 왜 그것을 하는지(목적)도 전하고 있는지 확인한다.
- 부작용의 명시: 부작용이 있는 메소드에는 동사를, 없는 메소드에는 명사를 사용하는 등, 메소드의 성질을 이름에 반영시킨다.
- 명확성과 의도: 코드가 읽기 쉽고 개발자의 의도가 명확하게 전달되는지 확인한다.
- 구조화 프로그래밍: 제어 흐름(if, while, case 등)이 최소화되고 로직의 이해가 쉬운지 확인한다.
- 중첩 삭감: 가드절, 조기 리턴, 메소드 추출, 폴리모피즘 등으로 중첩을 얕게 한다.(Code Complete)
- 논리적 순서와 그룹화: 문이 논리적인 순서로 기술되고 관련 코드가 빈 줄 등으로 적절히 그룹화되어 있는지 확인한다.(Code Complete)
- 코드의 단순성: 필요 이상으로 복잡한 로직이나 제어 플로우가 되어 있지 않은지 확인한다.
- 조기 최적화 방지: 성능 튜닝은 측정에 따라 필요한 부분에만 수행해야 한다.
- 정수화: 코드중에 직접 기술된 수치나 캐릭터 라인(매직 넘버, 매직 스트링)에 의미가 있는 경우, 의미가 있는 이름을 가지는 정수로서 정의한다.(Code Complete)
- 미사용 코드: 사용되지 않는 변수, 메서드, 클래스, 파일, 설정, 가져오기 등이 있는지 확인한다.
- 데드 코드: 도달할 수 없는 코드나 실행되어도 의미가 없는 코드가 없는지 확인한다.
- 코멘트 아웃된 코드: 더 이상 필요하지 않은 코드는 코멘트 아웃하지 않고 버전 제어 시스템을 신뢰하고 삭제한다.
- 로직 중복: 동일하거나 유사한 코드가 여러 위치에 존재하지 않는지 확인한다. 툴로 확인한다.
- 설정 및 상수 중복: 설정값과 상수가 여러 위치에서 정의되지 않는지 확인하고, 오픈소스에서 제공되면 활용한다.
- 어설션: 개발 중에 코드가 특정 전제 조건을 충족 함을 표현하고 검증하기 위해 어설션을 사용한다.
- 입력값 검증: 루틴이나 메소드의 경계로, 받은 파라미터가 기대되는 범위나 형태인지 검증한다.(Code Complete)
- 에러 처리 전략: 에러 발생시 프로그램을 안전하게 정지시킬지, 에러를 통지하여 처리를 계속할지 등 일관된 에러 처리 전략을 가지고 따르는지 확인한다.(Code Complete)
- 안전성: "있을 수 없다"고 생각되는 케이스나 잠재적인 에러 조건도 고려하여 시스템이 안전하게 동작하도록 설계·구현되어 있는지 확인한다.
- 신뢰성: 예기치 않은 입력, 부정한 조작, 외부 시스템의 이상 등에 대해, 시스템이 크래쉬하거나, 부정한 상태가 되지 않게 방어되고 있는지 확인한다.
- 예기치 않은 입력에의 대응: 사용자 입력이나 외부 데이터 등, 예기치 않은 값(null, 공문자, 부정 포맷 등)에 대한 고려가 되어 있는지 확인한다.
- 적절한 예외 처리: 예외를 단순한 제어 흐름으로 사용하지 않고(예: 루프 탈출 등), 진정으로 예외적인 상황에서만 사용한다.
- 오류 정보 기록: 오류 발생 시 원인 규명에 필요한 정보(파라미터, 컨텍스트 등)가 로그나 에러 감시 툴에 기록되도록 한다.
- 리소스 해제: 파일 핸들, 네트워크 접속, 락 등의 리소스가 에러 발생시도 포함해 확실하게 릴리즈 되는지 확인한다.(
try-finally,ensure등) (Code Complete) - 트랜잭션 관리: 여러 데이터 업데이트가 포함된 프로세스가 올바르게 트랜잭션 관리되고 있는지 확인한다.
- 적절한 데이터 유형: 변수 또는 데이터베이스 열에 저장할 데이터 유형(숫자, 문자열, 날짜, 열거 등)이나 범위가 적합하게 선택되었는지 확인한다.
- 데이터 구조: 복잡한 데이터를 처리할 때 클래스, 구조체, 해시 등이 적절하게 이용되고 있는지 확인한다.
- 효율적인 알고리즘과 데이터 구조: 처리 내용에 대해 효율적인 알고리즘이나 데이터 구조가 선택되어 있는지 확인한다.
- 데이터베이스 액세스:
- N + 1 문제: 루프에서 매번 쿼리가 발행되지 않는지 확인한다.
- 불필요한 데이터 검색: 필요 이상으로 열이나 레코드를 검색하지 않는지, 필요한 데이터만 얻는도록 되어 있는지 확인한다.
- DB 처리의 활용: 대량 데이터의 집계나 필터링 등, 데이타베이스가 특유의 처리는 어플리케이션측이 아닌 DB측에서 실시한다.
- 인덱스: 빈번하게 검색 조건이 되는 컬럼에 인덱스가 적절히 설정되어 있는지 확인한다.
- 메모리 사용량: 대량 데이터를 취급할 때에, 모든 데이터를 메모리에 로드하지 않고, 배치 처리등으로 메모리 효율을 고려하고 있는지 검토한다.
- 신뢰성(장애 조치 및 허용): 특정 구성요소에 장애가 발생해도 시스템 전체가 중단되지 않고 가능한 한 계속 작동할 수 있는 메커니즘(중복, 폴백, 자동 재시도 등)이 고려되고 있는지 검토한다.
- 참조의 일점성(단일 대입): 변수나 객체의 상태가, 초기화 후에 변경되지 않는(Immutability) 것을 노력하고 있는지 확인한다.
- 참조 투명성: 함수의 실행이 외부의 상태에 영향을 주지 않고(부작용이 없다), 같은 입력에 대해서 항상 같은 결과를 돌려주고 있는 것인지 확인한다.
- "왜"를 설명한다: 코멘트는 코드가 무엇을하고 있는지 (How) 가 아니라 그렇게 되어 있는지 (Why) 나 의도 (Intent) 를 설명하기 위해서 사용한다.
- 유익한 코멘트: 복잡한 알고리즘, 정규 표현, 워크 어라운드, 설계상의 결정 사항 등 코드만으로는 이해가 어려운 점을 보충한다.
- 정확성과 최신성: 코멘트의 내용이 코드와 일치하고 최신 상태로 유지되고 있는지 확인한다.
- 자기 문서화 코드: 코드 자체를 알기 쉽게 쓰는 것으로 코멘트의 필요성을 줄이는 것을 목표로 한다.
- 마커 코멘트:
TODO,FIXME등의 마커는, 팀의 규약에 따라 사용해, 적절히 관리(태스크화 등) 한다.(Code Complete)
- 적절한 테스트 레벨: 변경 사항에 대해 적절한 유형의 테스트(유닛 테스트, 결합 테스트, 시스템 테스트/E2E 테스트)가 균형있게 작성되었는지 확인한다.
- 유닛 테스트: 개별 클래스와 메소드의 로직을 검증한다.
- 시스템 테스트: 사용자 시점에서의 조작과 화면 천이를 검증한다.
- 테스트 용이성: 코드가 테스트하기 쉽도록 설계되어 있어야 한다.(의존관계의 주입, 부작용의 분리 등)
- 범위: 정상, 오류, 경계값 등 고려해야 할 케이스가 적절히 테스트되고 있어야 한다.
- 구체성: 테스트 케이스 설명이 테스트 대상, 조건, 기대 결과를 구체적으로 보여주어야 한다.
- 독립성: 테스트 케이스가 다른 테스트 케이스에 의존하지 않고, 단독으로 실행 가능해야 한다.
- 안정성: 실행할 때마다 결과가 바뀌는 불안정한(flaky) 테스트가 되어 있지 않아야 한다.
- 정확한 어설션: 검증 내용에 대해 적절하고 명확한 어설션이 사용되고 있어야 한다.
- 예: 문자열 비교에서는 완전 일치가 아닌 부분 일치나 정규 표현을 사용해야 하는지를 검토한다.
- 구조화: 테스트 코드가 준비(Arrange), 실행(Act), 검증(Assert)의 구조로 알기 쉽게 쓰여져 있어야 한다
- 기대치의 명확성: 테스트 코드 단독으로 기대치의 근거를 알 수 있어야 한다.
- DRY 원칙의 균형: 테스트 코드의 중복을 피하는 것과 각 테스트의 독립성·가독성을 유지하는 것의 균형을 이루어야 한다.
- 불필요한 테스트 제거: 프레임 워크 및 라이브러리의 표준 기능 자체를 테스트하는 것과 같은 중복 테스트는 작성하지 않는다.
- 디자인 독립: 시스템 테스트 등으로, UI의 외형(CSS 클래스명 등)에 의존하지 않고, 테스트용의 속성등을 이용해 요소를 특정한다.
- 모의 / 스텁의 적절한 사용: 외부 의존 (API, DB 등)을 분리하기 위해 모의와 스텁이 제대로 사용되고 있어야 한다?
- 테스트 데이터: 테스트 데이터 준비 방법이 적절하고 테스트 의도를 명확히 하고 있어야 한다.
- 코드의 의도의 명확화: 코드만으로는 이해가 어려운 로직이나 설계 판단에 대해서 코멘트로 의도가 보충되고 있어야 한다.(Code Complete)
- 관련 문서 업데이트: 코드 변경에 따라 관련 문서(README, API 사양, 설계서 등)도 업데이트되고 있어야 한다.
- 도큐멘테이션의 필요성: 필요한 문서가 존재하지 않는 경우, 작성을 의뢰한다.(Google)