안녕하세요, 톡FE파트에서 톡명함 서비스를 개발하고 있는 Kay입니다.
저는 2022년 신입 공채 기술 온보딩 교육의 코드 리뷰어로 활동을 했는데요, 이를 통해 얻었던 경험과 효과적인 코드 리뷰를 위한 리뷰어의 자세에 대해 소개하고자 합니다.
신입 공채 기술 온보딩 교육은 6주간 미션 중심 과제로 진행되었으며 교육 동안 주 2회 PR(Pull Request)를 바탕으로 코드 리뷰를 진행하였습니다. 교육은 코드스쿼드 주관으로 진행이 되었고, 사전에 코드스쿼드로부터 코드 리뷰 가이드(리뷰의 5가지 규칙)를 받아 학습했습니다.
이 글에서 많이 등장하는 ‘리뷰어’, ‘리뷰이’ 용어 정의는 다음과 같습니다.
- 리뷰어 : 다른 사람이 작성한 코드를 리뷰하는 사람들
- 리뷰이 : 본인이 작성한 코드를 다른 사람들에게 리뷰 받는 사람
코드 리뷰란
코드 리뷰란 한 개발자가 코드를 작성하면 다른 개발자가 정해진 방법으로 피드백을 주고받는 과정을 말합니다. 이 과정을 통해 본인이 발견하지 못한 실수를 다른 사람이 발견하여 코드의 부작용(Side effect)과 오류를 조기에 대응할 수 있으며, 개발 내 정해진 컨벤션 규칙을 유지하고 기술 부채를 줄일 수 있습니다. 또한 여러 명의 개발자가 참여함으로써 문제 해결을 위한 기술 구현 방법론에 대해 공유하기도 합니다.
효과적인 코드 리뷰를 지속하기 위해서는 리뷰어의 자세가 굉장히 중요한데요. 리뷰의 5가지 규칙을 토대로 리뷰어가 어떤 자세로 임하면 좋은지 알아보도록 하겠습니다.
리뷰의 5가지 규칙
1. 왜 개선이 필요한지 이유를 충분한 설명해 주세요.
리뷰어가 코드 개선의 필요성을 느끼고 리뷰를 남긴다면 충분한 이유가 뒷받침되어야 합니다. 만약 코드 리뷰가 주관적이거나 추상적이라면 리뷰이는 혼란을 느낄 수 있습니다. 그래서 피드백을 작성할 때 리뷰이가 개선의 필요성을 느낄 수 있도록 구체적인 이유를 작성하면 좋습니다.
const data = [
['데이터베이스', 'A', 3],
['교양영어', 'B+', 1],
['철학', 'A', 2]
];
위 코드는 2차원 배열을 사용한 자료구조이며 학점 정보를 담고 있는 data라는 변수가 있습니다. 변수명만 봐서는 어떤 의도를 가진 변수인지 파악하기 힘들고, 데이터가 확장되거나 비슷한 자료구조가 추가될 때 문제가 야기될 수도 있습니다. 따라서 리뷰어는 구체적이고 의도를 가진 변수명으로 변경하고자 합니다.
안 좋은 리뷰
“data 변수 말고 다른 변수명으로 하세요."
“data 변수 말고 grade로 하세요.”변경에 대한 이유를 설명하지 않고 제안도 애매하게 하는 리뷰입니다. 리뷰이 입장에서는 왜 다른 변수명으로 변경을 해야 하는지 알 방법이 없겠죠.
좋은 리뷰
“data라는 이름은 현재의 자료구조가 무엇인지 그 의도를 알기가 어렵네요.
학점 정보를 담고 있는 자료구조 같은데 이와 관련된 변수명을 짓는다면
현재 정의한 자료구조가 무엇인지 그 의도를 쉽게 파악할 수 있을 것 같아요.”개선이 필요한 이유를 구체적으로 설명한 피드백입니다. 답을 알려주는 것이 아니라 개선의 필요성만 설명하고 있기 때문에 리뷰이는 스스로 생각하고 학습할 계기가 됩니다.
2. 답을 알려주기보다는 스스로 고민하고 개선 방법을 선택할 수 있게 해주세요.
업무와 코드 리뷰를 병행하는 건 쉽지 않은 일입니다. 업무량은 많은데 코드 리뷰까지 신경 쓰려고 하다 보니 상세히 코드를 살펴보지 못하는 경우도 생기는데요, 그럴 때 리뷰어는 “000로 변경하세요”, “000을 쓰세요”와 같이 현재 코드의 개선점을 찾아 답을 알려주는 리뷰가 늘어나기도 합니다. 이러한 리뷰가 늘어날수록 리뷰이는 주어진 리뷰의 코드만 변경할 뿐 스스로 고민하는 시간은 줄어듭니다. 답을 알려주는 리뷰가 많아질수록 코드에 대한 애정도 사라지고 수동적인 개발자가 될 수 있으므로 스스로 고민하고 개선 방법을 찾는 것이 중요합니다.
const result = [];
arr.forEach(item => {
if(item % 2 === 0) {
result.push(item);
}
});
위 코드는 배열을 순회하며 짝수인 값만 result 배열에 추가하는 코드입니다. 리뷰어는 배열 내장 메서드를 통해 코드를 변경하고자 합니다.
안 좋은 리뷰
“arr.filter(item => item % 2 === 0);으로 사용하세요."filter의 사용 코드를 알려주는 피드백입니다. 리뷰이 입장에서는 filter의 동작 방식을 제대로 이해하지 못한 채 수정하게 됩니다. 또한 비슷한 코드가 여러 군데 있음에도 불구하고 이곳만 수정할 수도 있습니다.
좋은 리뷰
“자바스크립트에는 배열에는 다양한 내장 메서드들이 존재합니다.
그중 filter 메서드를 활용해 보세요.
이 메서드를 활용하면 코드량을 줄일 수 있습니다.”답을 알려주기보단 키워드(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. 리뷰 과정이 숙제검사가 아닌 학습과정으로 느낄 수 있게 리뷰해 주세요.
코드 리뷰를 하는 목적은 서로 생각을 공유하고 좀 더 깔끔한 코드를 작성하기 위함입니다. 그러나 코드 리뷰를 받아본 경험이 적은 개발자에게는 리뷰가 숙제로 받아들일 수도 있습니다. 이러한 상황을 예방하기 위해 리뷰이가 코드리뷰를 학습 일부로 받아들이기 위해 피드백을 자주 주고받고, 성장하고 있다는 생각을 심어주는 것이 좋습니다.
class Component {
constructor(node) {
this.node = node;
}
render(child) {
this.node.appendChild(child);
}
}
class Header extends Component {
constructor(node) {
super(node);
}
}
class List extends Component {
constructor(node) {
super(node);
}
}
Header, List 클래스가 Component 클래스를 상속받는 코드입니다. 기능 구현에는 큰 문제가 없어 보입니다. 타입 에러가 발생하지도 않을 것 같고요. 리뷰이의 숨은 의도를 파악하기 위해 먼저 충분한 대화를 통해 의도를 파악하는 것이 좋습니다. 리뷰이는 향후 확장성을 고려하여 미리 클래스를 나눴을 수도 있기 때문입니다. 만약 그런 것이 아니라면, 몇 가지 아쉬운 부분은 보입니다. 자식 클래스의 constructor에서 아무 일을 하지 않고 부모 클래스를 호출만 합니다. 그리고 Component 클래스에서 선언한 메소드도 사용하지 않습니다. 이러한 무의미한 상속은 불필요하다는 것을 리뷰하고자 합니다.
안 좋은 리뷰
“클래스 상속은 필요 없습니다.”리뷰이의 의도는 파악하지 않은 채 클래스 상속은 필요 없다고 단정을 짓고 있습니다. 먼저 왜 이렇게 짰는지 의도를 파악하는 것이 좋습니다.
좋은 리뷰
"Component 클래스를 상속 받는 건 좋네요.
그러나 자식 클래스에서 부모 클래스를 호출만 하기 때문에 모든 클래스에서
상속받는 건 오히려 중복 코드 같아 보이기도 하는데 이렇게 작성해 주신 이유가 있을까요?"먼저 ‘중복 코드’라는 단어를 사용하여 상속의 불필요한 이유를 설명하고 있습니다. 그리고 리뷰이의 생각은 다를 수 있으므로 의도를 물어봅니다. 리뷰이는 이 피드백을 보며 본인이 왜 이렇게 코드를 작성했는지 다시 되돌아보겠죠. 그 과정에서 어떤 부분을 수정해야 되는지 고민하고 학습할 수 있는 계기가 됩니다.
5. 리뷰를 위한 리뷰를 하지 마세요. 피드백 할 게 없으면 칭찬해 주세요.
리뷰할 부분이 없는데 흔적을 남기기 위해 리뷰할 경우 서로 어색해지는 경우도 발생합니다. 예를 들어, 깔끔하게 짜인 코드에 다른 디자인 패턴을 권유하는 것입니다. 리뷰이 입장에서는 잘 짜인 코드임에도 불구하고 안 좋은 코드라 여기고 잘못된 방향으로 아키텍처가 설계될 수 있습니다. 또는 앞뒤의 연관 관계없이 “코드가 복잡해 보입니다.”, “코드가 읽기 어렵습니다.”와 같은 리뷰는 서로 감성소비만 심해집니다. 따라서 리뷰할 부분이 없으면 칭찬 피드백을 주는 것도 한 가지 방법입니다.
아래는 칭찬 피드백에 대한 예시입니다.
- 코드량이 적당해서 읽기 편하네요.
- 많은 고민이 코드에서 엿보이네요.
- 성능에 아주 유리한 코드라고 생각되네요.
- 전에 코드보다 훨씬 좋아진 거 같네요.
- 예외 처리가 꽤 꼼꼼해서 좋네요.
- 함수, 변수명이 직관적이어서 좋네요.
‘칭찬은 고래도 춤추게 한다.’라는 속담도 있듯이 리뷰할 때 좋은 코드, 깔끔한 코드를 보고 칭찬한다면, 서로 시너지 효과를 증대시킬 수 있습니다.
추가 팁
“작은 커밋 단위만을 보지 말고, 전체 코드의 맥락을 살피면서 리뷰를 해주세요.”
하나의 PR 안에는 작업 단위로 쪼갠 커밋들이 존재합니다. 운영 업무를 오랫동안 한 개발자는 커밋 단위로 리뷰해도 전체적인 흐름을 파악할 수 있습니다. 그러나 신규 프로젝트 혹은 업무에 투입된 시간이 적은 개발자는 커밋 단위로 리뷰할 경우 전체적인 흐름을 파악하기 힘듭니다.
작은 단락의 코드에서 드러나지 않은 이슈가 전체 코드에서 확인할 수도 있거나, 작은 단락의 코드만 봤을 땐 깔끔해 보였으나 전체 코드상으로는 어색해 보이는 부분을 발견할 수도 있습니다. 정확한 리뷰를 위해서는 전체적인 코드의 맥락과 흐름을 파악하는 것이 좋습니다.
“Pull Request templates을 활용해 보세요.”
‘처음’이라는 단어는 설레기도 하지만 불확실성에서 오는 긴장감, 떨림, 두려움을 느끼기도 하는데요. 저는 첫 PR을 생성할 때 몇 십 분간 어떤 내용을 작성해야 되는지 고민하고 내가 실수한 부분은 없는지 계속 검사하며 비슷한 감정을 느꼈던 경험이 있습니다. 이러한 고민의 시간과 실수를 줄이고자 이번 온보딩 코드 리뷰 기간에는 PR을 등록할 때 Pull Request templates를 활용했는데요.
Pull Request templates이란 템플릿을 미리 등록해두고 PR을 생성할 때마다 자동으로 템플릿을 불러오는 것을 말합니다. 프로젝트 동안 target 브랜치를 master/main브랜치로 설정하지 않는 것, 그리고 리뷰가 필요할 때는 ‘review’와 같은 라벨을 달아주는 것과 같은 다양한 규칙들이 있었습니다. 이전에는 리뷰이가 규칙을 깜빡하고 target 브랜치를 master/main으로 설정하거나 Approve 된 PR을 머지를 하는 경우도 있었다고 해요. 그래서 저희는 템플릿에 체크리스트를 한 번 더 숙지시킴으로써 예시와 같은 실수를 줄이고자 했습니다. 또한, 온보딩 교육은 2주마다 새로운 프로젝트를 진행했었는데요. 업무를 병행하면서 제한된 시간 안에 리뷰를 하다 보니 프로젝트 구조를 한눈에 파악하기 어려웠고, 설계 목적과 이유를 들을 시간이 부족했었습니다. 이를 해결하기 위해 템플릿에 작업 내용과 참고한 링크를 추가할 수 있는 항목을 만들었습니다. 리뷰이는 말로 설명하기 어려운 부분은 이미지를 첨부하거나 마크다운 문법을 활용하여 정리하고 본인의 생각을 최대한 PR 내용에 작성함으로써 리뷰어가 코드를 파악하는 시간을 줄일 수 있었습니다.
“개발자의 성향을 리뷰하는 것을 지양해 주세요.”
함께 프로젝트를 만들어 나가는 과정에서 개발자의 성향을 리뷰한다면 리뷰하는 장점이 사라지게 됩니다.
function getDay(date) {
if (isWeekend(date)) {
return weekend;
} else {
return weekday;
}
}
위 코드는 주말인지 파악하여 주말일 경우 weekend 주 중일 경우 weekday를 반환하는 함수입니다. 조건문 안에서 추가 연산하거나 다른 함수를 호출하지 않고 바로 값을 반환합니다.
안 좋은 리뷰
“return isWeekend(date) ? weekend : weekday; 로 작성해 보세요.
코드가 깔끔해집니다.”코드가 깔끔해진다는 이유로 if/else 조건문을 삼항 연산자로 변경해달라는 리뷰입니다. 위 예제 코드의 경우 추가적인 연산이 없으므로 조건문에 대한 가독성은 주관적일 수 있습니다. 누군가는 if/else 조건문 작성이 더 읽기 편할 수도 있고, 누군가는 삼항 연산자가 더 읽기 편할 수 있겠죠. 만약 추가적인 연산이 없음에도 불구하고 삼항 연산자를 사용해야 한다면 피드백을 남기기 전에 서로 충분한 논의가 필요합니다.
마치며
코드 리뷰는 커뮤니케이션 비용이 많이 드는 일입니다. 저는 온보딩 기간 동안 온라인으로 리뷰하다 보니 상대방의 기분, 표정, 상황을 알 수 없어 피드백을 작성하는 데 시간을 평소보다 더 소비했었습니다. 피드백 한 줄이 상대방의 기분을 상하게 할 수도 있기 때문이죠. 위 예제를 보면 시간을 더 소비하는 이유가 명확히 나타납니다. 안 좋은 리뷰는 피드백을 작성한 이유 없이 변경 요청만 하므로 비교적 짧습니다. 이와 반대로 좋은 리뷰는 깁니다. 변경했을 때 이점이나 변경해야만 하는 이유나 근거가 있기 때문입니다. 큰 비용에도 불구하고 다양한 장점들이 존재하기 때문에 코드 리뷰는 꼭 하면 좋다고 생각합니다.
이렇게 5가지 원칙과 추가 팁에 대해 알아보았습니다. 사실 5가지 원칙은 개별로 나누었지만 서로 연관된 내용입니다. 이 글을 통해 리뷰를 할 때 어떤 식으로 피드백을 주고받아야 하는지 한 번쯤 생각하는 기회가 되었으면 좋겠습니다.
