서론
꼭 필요하지만 개발자에게는 불편하게 느껴질 수 밖에 없는 코드리뷰! 그러니까 얼른 코드리뷰 문화를 정착시키는 것이 중요하다. 이후에 새로 오는 개발자들은 더욱 거부감 없이 받아들일 수 있기 때문!!
코드 리뷰, 왜 필요한가!
•
개발자 서로가 작업 내용에 대한 이해의 폭이 커지고, 궁극적으로는 프로젝트에 대한 이해도가 커짐
•
자주 들어오고 빠져나가는 이쪽 업계 특유의 환경도 두렵지 않아진다!
•
유지보수에 매우 긍정적인 효과를 준다 (개발 시간이 들어가지만 궁극적으로 개발 시간이 단축된다?! 잘 하면!)
•
코드리뷰의 핵심은 단순히 코딩 스타일을 일관되 게 유지하거나, 예상되는 문제를 일찍 파악하는 데에 그치지 않고 코드에 대한 책임이 그 코드를 만든 사람 혼자에게 있지 않고 우리 모두에게 있다는 문화를 만드는 데에 있습니다.
리뷰에 앞서 일단 마인드 장착!
잘못된 생각들 (그러나 내가 실제 했던 생각들)
•
내가 리뷰를 하면 기분 나쁘다고 하지 않을까?
•
저 사람이 나를 싫어하나?
•
부끄러워
•
나를 허접 쓰레기라고 생각하면 어떻게하지
코드 리뷰의 형태와 범위!
명쾌한 기준이 있어야한다! 명확한 기준이 있어야 명확한 코드 리뷰가 이루어진다.
3가지 원칙
1. 리뷰는 언제나 상호 합의가 되어진 상황에서 진행되어야 한다.
2. 리뷰어의 해당 결과물에 대해서 객관성을 가지고 서로 인지해야 한다
3. 개발자 자신의 작업물에 대해서 정말 객관적으로 바라볼 수 있는 작성가가 선정되어야 한다.
2가지 원칙
1. 코드 검토는 1시간 이내에 끝낼 분량으로 검토한다.
2. 코드는 200라인 이상을 한 번에 검토하지 마라
해당 기준이 어겨지면 리뷰어는 제대로된 리뷰를 하기 힘들어진다.
코딩 규칙 가이드라인 준수 여부를 체크한다.
팀에 존재하는 스타일 가이드를 준수 해야한다. 물론 팀의 가이드라인이 매우 세부적인 부분까지 규정되어 있고 이를 100% 맞추려고 하는 것은 오히려 역효과가 있을 수 있겠지만... 그래도 가능하면 가이드라인은 준수하는 것이 좋다.
테스팅!
사실 이부분을 코드 리뷰 보다 더 우선시하고 중요하다고 생각하는 편이다. 그런데 굳이 여기에 쓴 이유는 코드 리뷰를 할 때 규칙에서 테스트 코드를 포함하는가 를 꼭 체크해야한다고 생각했기 때문이다.
코드리뷰에 대한 기법
코드 인스펙션
Code Inspection은 개발팀에서 작성한 개발소스 코드를 분석하여 개발 표준에 위배되었거나 잘못 작성된 부분을 수정하는 작업을 말한다. 잘못된 부분이란, 코드가 중복되거나 작성규칙에 맞지 않거나 잘못 구현된 부분들을 말한다.
•
상당히 명쾌한 리뷰가 진행된다. 그러나 리소스가 많이 든다.
•
리소스가 풍부하고 일정에 여유가 있는 경우에 가능하다 (주로 대기업이나 대형 플랫폼에서 가능하다)
롤(Role) 구분
1.
Moderator 실질적인 매니저로 팀 간의 인터페이스와 리소스, 인프라를 확보하고, 프로세스에 대한 정의와 산출물의 정리를 담당한다.
2.
Reade. 각 산출물을 읽고, 리뷰하고, 방향성을 제시한다. 보통, 지식이 많은 사람이 담당한다.
3.
Designer/Coder Reader의 지시에 따라서 코드를 검증하고 잠재적인 발견 등의 수정 방안을 만든다.
4.
Tester 진행 중인 코드와 권장 수정 코드에 대해서 검증한다.
진행단계
1.
Planning 계획 수립
2.
Overview 교육과 역할 정의
3.
Preparation 인터뷰와 필요한 문서 습득, 툴 환경 구축
4.
Meeting(Inspection) 각자의 역할대로 수행
5.
Rework 보고된 Defect 수정
6.
Follow-up 보고된 Defect가 수정되었는지 확인
팀 리뷰
•
일정한 계획과 프로세스만 따른다. (좀 덜 정형화 되어있다.)
•
일정 주기(일주일에 한 번)에 팀 리뷰를 수행하거나 특정 모듈 및 기능이 완료되는 시점에 진행
•
위험하거나 의견이 필요할 때도 진행
•
자유로운 토론을 할 수도, 좋은 사례를 전체적으로 공유하는 시간을 가질 수도 있다.
Walkthrough
•
발표자가 리뷰의 주제나 시간을 정해서 발표하고 동료들로부터 의견이나 아이디어를 듣는 시간을 가짐.
Peer review or over the shoulder review
•
보통 2~3명이 진행하는 코드리 뷰로 코드의 작성자가 모니터를 보면서 코드를 설명하고, 다른 한 사람이 설명을 들으면서 아이디어를 제안하거나 Defect를 발견하는 방법이다.
•
신입사원이나 인턴사원의 경우에 업무 이해도를 높이면서 해당 코드를 사용할 수 있는 수준으로 활용할 경우에 의미 있는 방법
•
개발자의 인력 투입이 거의 두배 이상으로 증가하는 것으로써, 고품질의 영역을 개발하거나, 빠른 시간 안에 신입 개발자의 업무 이해도를 높이는 경우가 아니라면 시행하지 않는다.
돌려 보기 방법
•
온라인이나 실시간성이 아니라, 리파지토리나 이메일 등을 사용하여 천천히 리뷰하는 방식
•
속도는 느리지만, 중요한 코드이거나, 제품의 기능 개선이 필요한 경우에는 아주 의미가 있다
온라인 코드리뷰
일반적으로 관련 툴(gitlab, github, jira 등등)들은 버젼 관리 시스템, 이슈 트레킹 시스템과 연동해서 해당 코드가 어떤 부분을 수정했는지, 어떤 이슈와 관련되어 있는지 쉽게 파악할 수 있도록 도와줍니다.
요약
중요한 것은 어느 정도 개발 조직이 서로 커뮤니케이션하고, 목적의식을 통일하고, 적절한 시간 분배를 통해서 리뷰를 할 수 있는 시간을 만들어 내느냐가 리뷰의 핵심이라고 할 수 있다.
Check Lists
리뷰 체크리스트 (일단 복붙)
1.
시스템의 요구사항이 제대로 반영되었는가?
2.
시스템의 설계의 규격대로 구현되었는가?
3.
과도한 코딩을 하고 있지 않는가?
4.
같은 기능 구현을 더 단순하게 할 수 있는가?
5.
함수의 입출력 값은 명확한가?
6.
빌딩 블록들( 알고리즘, 자료구조, 데이터 타입, 템플릿, 라이브러리, API )등이 적절하게 사용되었는가?
7.
좋은 패턴과 추상화( 상태도, 모듈화 )등을 사용해서 구현하고 있는가?
8.
의존도가 높은 함수나 라이브러리 등의 의존관계에 대해서 별도 기술하고 있는가?
9.
함수의 반환(exit)은 한 곳에서 이루어지고 있는가?
10.
모든 변수는 사용 전에 초기화하고 있는가?
11.
사용하지 않는 변수가 있는가?
12.
하나의 함수는 하나의 기능만 수행하고 있는가?
스타일 및 코딩가이드 체크리스트 (일단 복붙)
1.
코딩 스타일 가이드를 준수하고 있는가?
2.
각 파일의 헤더 정보가 존재하는가?
3.
각 함수의 정보를 코드에 대해서 설명하기에 충분한가?
4.
주석은 적절하게 기술되어있는가?
5.
코드는 잘 구조화되어있는가? ( 가독성, 기능적 측면 )
6.
헤더, 함수 정보를 도구로 추출해서 자동으로 문서화할 수 있는 구조인가?
7.
변수와 함수의 이름이 일관되게 기술되어 있는가?
8.
프로젝트의 가이드를 통한 네이밍 규칙을 준수하고 있는가?
9.
숫자의 경우 단위에 대해서 기술하고 있는가?
10.
숫자를 직접 서술하지 않고, 상수를 사용하고 있는가?
11.
어셈블리 코드를 사용하였다면 이를 대체할 방법은 없는가?
12.
수행되지 않는 코드는 없는가?
13.
주석 처리된 코드는 삭제가 되었는가? ( 버전 체크가 되었는가? )
14.
간결하지만 너무 특이한 코드가 존재하는가?
15.
설명을 보거나 작성자에게 물어봐야만 이해가 가능한 코드가 있는가?
16.
구현 예정인 기능이 있다면, ToDo주석으로 표시되어 있는가?
아키텍쳐 검토 체크리스트 (일단 복붙)
1.
함수의 길이는 적당한가? ( 화면을 넘기면 안 된다. )
2.
이 코드는 재사용이 가능한가?
3.
전역 변수는 최소로 사용하였는가?
4.
변수의 범위는 적절하게 선언되었는가?
5.
클래스와 함수가 관련된 기능끼리 그룹화가 되었는가? ( 응집도는 어떤가? )
6.
관련된 함수들이 흩어져 있지 않는가?
7.
중복된 함수나 클래스가 있지 않는가?
8.
코드가 이식성을 고려하여 작성되었는가? ( 프로세스의 특성을 받는 변수 타입이 고려되어있는가? )
9.
데이터에 맞게 타입이 구체적으로 선언되었는가?
10.
If/else구분이 2단계 이상 중접되었다면 이를 함수로 더 구분하라
11.
Switch/case문이 중첩되었다면 이를 더 구분하라
12.
리소스에 lock이 있다면, unlock은 반드시 이루어지는가?
13.
힙 메모리 할당과 해제는 항상 짝을 이루는가?
14.
스택 변수를 반환하고 있는가?
15.
외부/공개 라이브러리 사용하였을 경우에 MIT 라이선스를 확인했는가? GPL의 경우에는 관련된 영역에서만 사용해야 한다.
16.
블로킹 api호출시에 비동기적인 방식으로 처리하고 있는가?
예외처리 관련 체크리스트 (일단 복붙)
1.
입력 파라미터의 유효 범위는 체크하고 있는가?
2.
에러코드와 예외(exception)의 호출 함수는 분명하게 반환되고 있는가?
3.
호출 함수가 어려와 예외처리 코드를 가지고 있는가?
4.
Null포인트와 음수가 처리되는 구조인가?
5.
에러코드에 대해서 명쾌하게 선언하고 처리하고 있는가?
6.
switch문에 default가 존재하고, 예외처리를 하고 있는가?
7.
배열 사용시에 index범위를 체크하는가?
8.
포인트 사용시에 유요한 범위를 체크하는가?
9.
Garbage collection을 제대로 하고 있는가?
10.
수학계산시에 overflow, underflow가 발생할 가능성이 있는가?
11.
에러 조건이 체크되고 에러 발생 시 로깅 정보를 남기는가?
12.
에러 메시지와 에러코드가 에러의 의미를 잘 전달하는가?
13.
Try/catch 에러 핸들링 사용방법은 적절하게 구현되었는가?
검증과 시험 체크리스트 (일단 복붙)
1.
코드는 시험하기 쉽게 작성되었는가?
2.
단위 테스트가 쉽게 될 수 있는가?
3.
에러 핸들링 코드도 잘 테스트되었는가?
4.
컴파일, 링크 체크 시에 경고 메시지도 100% 처리하였는가?
5.
경계값, 음수값, 0/1등의 가독성이 떨어지는 코드에 대해서 충분하게 경계하고 있는가?
6.
테스트를 위한 fault 조건 재현을 쉽게 할 수 있는가?
7.
모든 인터페이스와 모든 예외 조건에 대해서 테스트 코드가 있는가?
8.
최악의 조건에서도 리소스 사용은 문제가 없는가?
9.
런타임 시의 오류와 로그에 대비한 시스템이 있는가?
10.
테스트를 위한 주석 코드가 존재하는가?
우리 (피츠미)팀에서는?!
•
현재 모두가 프로젝트 파악이 잘 되어있지 않은 상황이다.
그렇기 때문에 코드 리뷰를 하는 것이 더 좋다! 파악과 동시에 리팩토링!!
생각해볼점
•
일정
스프린트 기한은 정해져있다. 코드 리뷰를 하는데는 시간이 든다. 우선순위를 스프린트에 두게 된다면 실상 코드리뷰 룰은 크게 의미가 없어지기 쉽다. 강제력이 필요하긴 한데... 어느 수준으로....
•
너무 긴 코드는 리뷰하기가 힘들다. 그렇다면 어떻게 나누는 것이 좋을까?(커밋 전략과도 연관이 있을까?)
•
기본적인 규칙들은 가능한 자동화를 하고, 규칙들을 지키도록 권장되어야한다.
현재 자동화는 되어있다. 규칙을 어떻게할까!?