코드리뷰에 대하여

김소희·2024년 3월 24일

왜 코드리뷰를 해야할까?

사업은 더 빨리 혁신해야 하고, 개발은 더 빠르고, 안정적으로, 자주 배포해야 한다.
엉클밥의 저서인 클린 아키텍처 1장에는 배포를 많이 할 수록 투여되는 개발자의 수가 증가하고, 배포별 라인당 개발 비용도 여덟번째 배포는 첫번째 릴리즈에 비해 40배나 비용이 증가한 연구 결과가 있다.
개발초기에는 새로운 기능을 추가하기만 하므로 생산성이 높지만 시간이 지날 수록 영향을 받는 것들이 많아지기 때문에 점진적으로 생산성이 떨어지게 된다. 그렇게 되면 개발자 입장에서는 야근도 했고, 헌신도 했는데 변화가 없는 상황으로 느껴지게 된다. 이때쯤 되면 뭔가 새로운 기능을 구현하는데 시간을 쏟기보다는 기존코드를 이해하고 수정하는데 더 많은 시간을 쏟게 된다.

Robert C. Martin은 SW 개발의 단순한 진리는 "The only way to go fast is to go well."이라고 말했다. 빨리 갈 수 있는 유일한 방법은 잘하는 것이라는 뜻으로 생산성 저하와 비용 증가를 뒤집을 수 있는 유일한 방법은 개발을 잘하는 것. 즉 SW 품질을 높이는 것이라고 얘기할 수 있다.

품질을 높인다는 건 어떤 걸까? SW는 현 시점의 사용자의 요구사항을 만족(behavior)시키면서 지속적으로 변화하는 요구사항을 수용(soft)해야하는데 애자일에서는 동작하지만 고칠수 없는 SW보다 고칠 수 있지만 동작하지 않는 SW가 훨씬 더 중요하다고 이야기한다. 개발자들은 개발자를 위해서 지속가능한 SW을 개발하는 것이 아니라 품질을 높여야만 사업도 잘 될 수 있음을 염두해야 한다.

학교에서 배운것은 소프트웨어를 최초에 돌아가게 만드는 것까지 배우기에 그 후에 유지 보수하고 기능을 추가하는 것들에 대해서는 많이 배울 수가 없다. 그래서 소프트웨어 장인정신이라고, 후배들에게 지식과 경험을 공유해야 전문성을 갖춘 개발자를 육성할 수 있다는 말로 쓰이는데 이를 실천하기에 코드리뷰가 유리하다.

코드리뷰는 내가 알고 있는 것을 내 주변의 개발자들과 공유할 수 있는 가장 손쉬운 방법이고, 다른 사람이 소스 코드를 보면서 확인하는(배포해도 될지, 버그나 장애는 없는지..) 소프트웨어 품질 보증 검수를 했다고도 볼 수 있다. 부수적으로는 더 좋은 코드로 개선할 수도 있고, 참여한 모든 사람들에게 배움의 기회를 주기도 한다. 조직내에서 실천하면 집단 코드 오너십으로 우리 팀에서 변경된 코드에 대해서 우리 팀원들 모두가 책임감을 갖게되는 효과도 얻을 수 있다.

코드리뷰의 절차

풀리퀘스트를 작성 할 때 저자가 노력을 들이면 리뷰어들의 시간을 절약해 줄 수 있다.

왜 코드리뷰가 어려울까?

저자는 본인이 생각하기에 멋지다고 생각하는 변경을 만들어서 풀리퀘스트를 만들어서 리뷰어들에게 요청을 한다. 그런데 간혹 리뷰어들은 네가 멋지다고 생각한 게 왜 안 멋진지 낱낱이 증명하겠다는 마음으로 임할 때가 있다. 이럴 때 서로 간에 갈등이 증폭될 수 있는데 저자는 겸허한 마음으로 임할 필요가 있고, 코드에 대한 비판을 자신에 대한 비판으로 이해하는 순간 움츠려들게 된다. 리뷰어들도 객관적인 코드에 대해서만 이야기를 해야하지 '너 왜 이렇게 했어?'와 같은 실수를 저지르지 않게끔 조심스럽게 피드백을 해야한다.

코드리뷰 기법들

효율적 PR 방법

지루한 작업은 컴퓨터로 처리한다.
Formatting Tool을 이용해서 공백과 들여쓰기 오류나, 별로의 커밋/PR로 리뷰가 불필요한 부분을 기술하여 생산성을 높일 수 있다.
PR을 올릴 때 주석을 단다.
PR을 저자가 먼저 보고 리뷰어들을 위한 설명이나 궁금한 점이나 의견을 코멘트로 남겨둔다면 시간을 절약할 수 있다.
리뷰어에 모두를 포함하라.
많은 사람이 볼수록 버그나 개선사항을 더 잘 찾아낼 수 있고, 많은 사람이 본다는 것을 알기에 대게 더 잘하려는 경향이 있다.

효율적 리뷰 방법

리뷰는 즉시
코드리뷰를 올린 사람은 리뷰가 완료되서 머지가 어프로브 될 때까지 대기의 상태에 빠지게 된다. 따라서 최대한 빠르게 리뷰를 해주는 것이 좋다. 리뷰 라운드를 최대 하루 정도로 유지하는 것이 좋고, 만약 선임이 우선순위 높은 업무로 1일 내에 리뷰가 불가능하고 다른 사람에게 리뷰를 받을 수 있다면 리뷰어를 변경하는 것도 좋지만 너무 잦게 리뷰어를 변경한다면 조직의 개발 관습에 문제가 없는지 의심을 해봐야 한다.
고수준으로 시작, 저수준으로 내려가라.
리뷰 라운드에서 많은 의견을 남길수록, 저자가 당황할 가능성도 커진다. 초기에는 버그, 장애, 성능, 보안, 메소드가 너무 크다, 메소드의 추상화가 안맞는다 등 고수준 피드백으로 제한하고 이가 처리된 다음에 설계 원칙을 준수하지 못한 점, 변수명 변경, 주석이 모호해요 등 저수준 이슈를 처리하는 것이 좋다.
리뷰의 범위를 존중하라.
많이하는 실수로 이번 풀리퀘스트의 변경분이 아닌(빨간색상코드) 부분의 코드까지 고칠점을 알려주진 않는편이 좋다. 하지만 PR이 둘러싼 코드에 영향을 미칠 때는 예외적으로 알려준다.
Nit 태그를 활용
Nit 태그 '고치면 좋지만 아니여도 그만' 이라는 의미인데 대부분은 고치기는 한다.
리뷰어들이 개선할 수 있는 의견을 자유롭게 남길 수 있어야 하지만 중요치 않다면 저자가 무시할 수 있도록 태그를 붙일 수 있다. ex)null대신 Optional을 반환하면 어떨까요?
한두 등급만 코드레벨을 올리는 것을 목표로.
지금보다 요만큼만 좋아지면 좋겠다. 완전하지는 않아도 충분히 좋은 코드가 될 정도로 리뷰를 해야지. 한번에 너무 많은 것을 바라면 해내기가 어렵다.

피드백 유의사항

누가 그런 아이디어(잘못)을 냈는지가 아닌 무엇이 코드를 나아지게 하는가(생산성)에 집중
"이렇게 해야합니다.", "이부분이 틀렸습니다." 가 아닌 "이렇게 저는 제안합니다.", "이렇게 하는 것은 어떨까요?"라고 명령이 아닌 요청을 통해 표현하라.
제안하는 변경과 변경의 이유를 같이 제공한다. ex) "이 클래스를 2개로 분리해야 해요." 보다는 "지금 이 클래스는 파일 다운로드와 파싱의 2가지 책임을 가지고 있어요. 다운로더와 파서 2개의 클래스로 분리하여 SRP를 준수하는 것이 어떨까요?"

코드리뷰 체크리스트

버그/장애

Null Point Exception
Thread Safety
java.util.LinkedList나 java.util.HashMap 등 특정 데이터 구조체들은 java.util.concurrent 패키지에 존재하지 않으면서 java.util.Collection을 구현하는 클래스들, 모든 output stream, java.util.Calender는 Thread Safety하지 않는다.
스레드가 꼬여서 가끔씩 날짜가 1970년대가 나온다던가 하는 에러가 발생될 수도 있고, 다른 사용자의 데이터가 보여지는 보안상의 사고가 날 수도 있다.
lock을 사용할때도 API문서를 통해서 synchronized, lock, atomic variable 등 정해진 것을 잘 사용하고 있는지 확인해야 한다.
aliasing problem(별칭문제)는 캐시처럼 JVM 내에서 한 곳에서 관리하는 값이 리턴됐을 때 뮤터블한지 인뮤터블한지 확인하고 뮤터블하다면 클롬 같은 걸 통해서 안전하게 만들고 값을 변경해야 한다.
스레드 간의 경합 발생 가능성이 있는지도 살펴야 하는데 클래스의 상태나 변수의 리어사인이 너무 빈번하면 혹시 멀티스레드 환경에서 경합이 발생할 수도 있지 않나, final 키워드로 선언할 수는 없는지 생각해 보아야 한다. 예를들어 오토믹하지 않은경우에 get와 set을 함께 사용하는 경우.
Out of Memory Error
memory leak 가능성이 있는지 살펴보아야 하는데 스태틱 필드는 어플리케이션의 전체 라이프타임과 동일한 라이프타임을 가지므로 스태틱필드를 가지고 있는 부모 클래스의 객체가 더이상 사용되지 않아서 gc가 되더라도 스태틱 필드는 gc가 안 될 수도 있다. 그런 스태틱 필드에 리스트나 맵을 넣고 무한정 데이터가 증가할 수 있게 해두면 OOME가 발생할 소지가 높아진다.
스레드풀과 스레드로컬을 부적절하게 사용하면 의도하지 않은 객체의 보존이 일어나서 gc가 안되는 상황이 생길 수 있으니 조심해야 한다. 예를들어 톰캣같은 waas가 스레드 재사용을 하기위해 스레드풀을 이용하는데 스레드로컬이 remove를 명시적으로 호출하지 않으면 스레드로컬에 담았던 것들이 스레드풀에 있기 때문에 유지될 수 있으므로 더이상 사용하지 않을 때 반드시 remove 콜을 해야 한다.
waas를 리스타트하면 새로운 클래스 로더가 생성되며, 이전클래스 로더는 gc대상이 되는데 이전 클래스 로더가 가지고 있는 어떤 객체를 다른 클래스 로더가 참조한다면 이 클래스 로더가 가지고 있던 모든 객체들이 gc되지 않는다. 그래서 이전 waas를 hot deploy한 경우에서 클래스 로더를 여러개 쓰게 되는 경우 gc를 고려해야하는데 리스타트 없이 재배포/재로딩을 할 수 있는 환경에서 작업할때 OOME가 발생할 수 있다.
list 나 map 처럼 크기가 무한정 증가가 가능한 데이터 구조체를 생성할 때에도 지속적으로 엘리먼트가 추가되거나 개수 제한이 있는지, 아니면 주기적으로 제거하는지를 확인해야 한다.

단위 테스트 없는 중요 로직

비즈니스 핵심 로직(결제, 포인트 계산, 계정 생성, 재고 차감 등)이 단위 테스트 없이 운영으로 배포될 경우
예상하지 못한 사이드 이펙트나 로직 오류가 그대로 사용자에게 노출될 가능성이 높다.
중요 로직일수록 작은 변경에도 전체 시스템에 영향을 줄 수 있으므로
해당 부분만큼은 Mocking 기반의 단위 테스트와 시나리오 기반의 통합 테스트를 반드시 포함해야 한다.
특히 if/else 분기나 외부 API 호출 전후의 로직은 테스트가 없으면 회귀(regression) 버그가 쉽게 발생한다.

경계값 테스트 미비

입력값의 최소·최대 범위, 0·1 같은 edge case, 배열·리스트의 empty/null 상태에 대한 테스트가 없으면
특정 조건에서만 재현되는 치명적 버그가 발생할 수 있다.
예를 들어 페이지네이션에서 page=0, 금액 계산에서 quantity=1, 문자열 파싱에서 empty 문자열 등
경계값 주변에서 오류가 가장 자주 발생하므로
테스트 케이스를 작성할 때 범위의 양 끝단을 반드시 포함해야 한다.
이는 수학적 경계값 분석(Boundary Value Analysis)의 기본 원칙이다.

외부 URL(DB, API)을 n번 반복 호출하는 로직

외부 API나 DB를 호출하는 로직이 루프 내부에 있거나
불필요한 반복 호출이 발생하는 구조라면 성능 저하뿐 아니라 장애까지 유발할 수 있다.
예를 들어 반복문 안에서 findById()를 매번 호출하거나,
API 서버에 필요한 데이터 수만큼 개별 요청을 보내는 형태는 “N+1 문제”를 만들어낸다.
이러한 외부 의존성 호출은 반드시 캐싱, 벌크 조회, 트랜잭션 내 최소 호출 최적화,
레디스·로컬 캐시 적용, API 배치 호출 등으로 개선해야 한다.
또한 외부 시스템의 장애가 내부 서버로 전염되지 않도록
timeout, retry, circuit breaker 설정도 필수적으로 확인해야 한다.