백발의 개발자를 꿈꾸며

코드 리뷰

효율적으로 리뷰 하는 방법

코드 리뷰를 높은 우선 순위로 작업에 할당해야 한다.

코드 리뷰 작성자는 리뷰가 종료될 때 까지 해당 작업은 더 이상 진행할 수 없는 대기 상태에 빠진다.

코드 리뷰를 바로 시작 하면, 선순환이 된다.

코드를 읽고 피드백을 줄 때 까지 충분한 시간을 가지고 진행해도 되지만, 시작은 바로 해야 한다. 가장 이상적인 것은 코드 리뷰를 요청 받으면, 수분 내에 진행 하는 것

리뷰 라운드는 최대 하루 안에 어프로브 되도록 만들어라.

우선순위 높은 업무로 1일 내 불가하면 다른 리뷰어로 지정할 수 있다. 월 1회 이상 리뷰어를 재지정 한다면 속도를 줄여 건강한 개발 관습을 유지할 수 있도록 문화적으로 접근해야 한다.

PR 가이드

작고, 범위가 좋은 PR 은 대체적으로 쉽고 리뷰하기 편리하여, 빠르게 리뷰 라운드가 진행될 수 있다.

효율적 리뷰 방법

고수준으로 시작해서, 저수준으로 내려가라

리뷰 라운드에서 많은 의견을 남길수록, 코드 작성자가 당황할 위험이 커진다.

• 하나의 라운드에 20~50개 가 넘어간다면 위험한 리뷰 라운드라고 판단

초기 라운드에서는 고수준 피드백으로 제한

• 버그, 성능, 장애, 보안 등 크리티컬한 문제점을 우선으로 적용하도록 리뷰를 진행한다.

고수준의 피드백이 처리된 후, 저수준 이슈를 꺼낸다.

• 설계 개선
• 변수명 변경, 주석 작성, 코드 복잡도 등

예시를 제공하는 것에 관대하라

예제는 너무 길면 관대한 것이 아니라 억압적일 수 있다. 리뷰에서, 너무 많은 예제가 많으면 코드 작성자의 수준이 낮다고 인지할 가능성이 있다.

리뷰의 범위에 존중하라

태그를 활용하라

Nit 태깅은 고치면 좋지만 아니어도 그만을 의미로 사용되며, 보통은 그래도 고친다.

리뷰어는 항상 더 개선할 수 있는 의견을 자유롭게 남길 수 있어야 하는 SNS 소셜 코딩과 같은 존재이기 때문에, 코드 작성자가 이를 어느정도 의식하여 자신이 의사결정을 내릴 수 있어야한다.

한두 등급만 코드 레벨을 올리는 것을 목표로 하는 것이 리뷰이다.

D 등급의 PR 을 받았다고 하면, 코드 작성자가 C 나 B 등급을 받을 수 있도록 만들어라.

• 우리의 코드는 완전하지 않지만 좋은 코드가 되도록 리뷰를 제공해야 한다.
• F 등급을 제공해야 할 때는 기능적으로 잘못 되었거나, 너무 복잡해서 정합성에 확신이 없는 경우이다.

승인을 보류하는 이유는, 수 차례의 리뷰 라운드 후에도 코드 등급이 계속 F 상태일 때 보류된다.

피드백 방법

코드 리뷰의 핵심은 “무엇이 코드를 나아지게 했는가?”에 대한 주제를 나눠야지, 누가 그런 잘못된 코드를 작성했는지에 대해 얘기를 나누는 공간이 아니다.

비판의 대상은 코드일 뿐, 저자가 아니다. 피드백에서 사용하는 소통 방식은 상대방이 작성한 결과물로 하여금 나의 감정을 나타내는 화법을 이용하면 원활한 의사소통이 된다.

• “당신이 이런 코드를 만든건 …” ❌
• “이 결과물이 나는 어떻게 생각 된다.” ✅

건설적인 피드백을 하라

동료들간의 코드 리뷰는 서로간 누가 코드를 잘 작성하는지 경쟁을 하는 것이 아닌 팀의 생산성을 높이는 행위이다.

코드 리뷰를 자신의 코드에 대한 비판이 아닌, 학습 과정으로 인지하면 전체적인 프로젝트의 성공에 기여할 수 있다.

진정한 칭찬을 해라

대부분의 리뷰어가 잘못된 부분에만 집중하기 나름인데, 긍정적 행위 강화를 위한 값진 기회이기도 하다.

온라인 코드 리뷰를 많이 접하지 않은 코드 작성자는 매우 민감하고, 방어적으로 받아들일 수 있다.

피드백은 명령이 아니라 요청으로 표현해라

우린 일상에서 동료에게 명령하지 않는다. 그렇기에 리뷰에서는 강압적인 명령이 아니라 요청을 해야 한다.

“특정 클래스의 파일로 분리 하라” -> “특정 클래스가 너무 커지는 것 같은데 유지보수할 때 괜챃을까요?”

의견이 아니라 원칙에 기반하여 피드백하라

저자에게 의견을 줄 때는, 제안하는 변경 사항과 변경의 이유를 모두 설명해야 한다.

원칙으로 설명이 되지 않는 경우도 존재한다. 예를 들어, 직관적이지 않다는 주관적인 견해에 의해서 내용을 전달해야 할 때가 있다.

이러한 경우에는, 나의 생각 / 내가 느낀점을 설명한다.

코드 리뷰의 토론이 지속적으로 격양되어 교착상태에 빠지려고 한다면, 적극적으로 처리하라.

교착상태 판단 방법

• 토론의 톤이 점차 팽팽해지고 공격적으로 변한다.
• 리뷰 라운드당 코멘트가 줄어들지 않는다.
• 너무 많은 코멘트에 코드 작성자가 저항감을 표현한다.

코드 리뷰의 최악의 결과는 교착 상태이며, 코멘트를 반영하지 않으니 승인이 거부되고 코드 작성자는 이해할 수 없기 때문에 리뷰 반영을 거부한다.

이럴 경우에 대면 리뷰로 전환하여, 텍스트 기반으로 의견을 나누지 말고 상대방과 대화로 나누는 것이 가장 좋은 방법이다.

교착상태 처리 방법 상대방을 인정하고, 승인한다. - Agree to disagree

저수준 코드를 승인 했다고 해서 코드 품질이 나빠질 수 있다. 하지만, 동료와 다퉈서 관계가 악화된다면 고수준 품질을 얻을 기회를 날리게 된다.

• 저수준 코드가 승인 되고, 이후에 고수준으로 개선을 시도하는 작업을 제공하거나, 개선되도록 점진적으로 성장을 도모해야 한다.

인정을 못하겠다면, 상급자를 참여 시키거나 다른 리뷰어로 변경하라.

코드 리뷰 사례

리뷰를 반영한 커밋 제공하기

코드 작성자가 피드백에 대해 반영한 내용을 커밋 해시를 제공하여, 리뷰어가 해당 커밋으로 바로 이동하여 어떻게 반영 했는지 내용을 확인할 수 있다.

배움을 주고 받기

코드 리뷰를 아주 재밌게 하는 방법

코드 작성자가 PR을 올린 내용을 리뷰어 환경에서 코드를 직접적으로 수정하고, 개선하는 모습을 보여주며 개선점을 설명한다.

설명한 후, 변경된 코드 내역은 restore 하여 제거한 뒤 코드 작성자에게 개선사항을 반영해서 다시 PR 을 보내도록 요청한다.

이렇게 해서 얻을 수 있는 것은 코멘트로만 전달하기 어려운 내용을 직접적으로 전달할 수 있다.

체크 리스트

NPE 방지 eqauls 사용

code.equals("") // X

"".eqauls(code) // O

ThreadSafety

• 멀티스레드 환경에 적합한 데이터 구조체를 사용하는가?
• 잠금 전략을 제대로 사용 하고 있는가?
  • syncrhoized, lock, atomic variables
• 로컬 캐시에서 얻은 값이 상태를 변경하여, 하나의 JVM 에서 상태가 공유되는 경우 immutable 한 구성을 하고 있는가?
• 스레드간 클래스의 상태나 변수가 재할당 되어 경합 될 가능성이 있지는 않은가? -> final 로 선언할 수 없는가?

OOM

• static field 를 가지고 있는 부모 객체가 GC 되더라도, 필드는 GC가 되지 않는다.
• 스레드 풀과 스레드 로컬을 부적합하게 사용하면 GC 가 제대로 동작하지 않으니 주의해야 한다.
  • 스레드 로컬을 사용할 때는 명시적으로 remove 하는가?
• 무한으로 증가 가능한 데이터 구조체를 생성하는가?
  • 새로운 값이 지속적으로 List 나 Map 에 추가 되는가?
• stream session 또는 커넥션 정보를 제대로 닫고 있는가?
• 리소스 풀이 제대로 설정 되어 있는가?
  • non-static inner class 이 구성 되어 있는가?
  • 이너 클래스는 자신을 참조하고 있는 컨테이너 클래스에 대해 묵시적인 참조 관계가 존재함 -> 어플리케이션에서 이너 클래스는 더 이상 사용 되지 않더라도 컨테이너 클래스가 자주 호출된다면 이너 클래스는 GC 가 되지 않음 -> 스테틱 클래스로 정의 필요
• tomcat session-timeout 이 0이 무한대를 의미하는건지, 이런 환경 설정이 어떤 의도를 갖고 있는지 확인이 필요하다.

기능성

• 코드가 실제로 의도한 대로 동작 하는가?
  • CQS 에서 커맨드인데 상태를 반환 하는지, 쿼리인데 상태를 변경하는지?
• 미묘한 버그는 없는가?
  • 잘못된 변수 사용, and 대신 or 로 조건을 검사한다
• 선택한 해결책이 요구사항에 적합한가?
  • 더 좋은 해결책은 없었는지?

가독성 / 유지보수 용이성

• 코드 컨벤션 스타일을 제대로 준수하고 있는가?
• 네이밍이 적절한가?
  • 읽어서 코드 이해가 되는가?
  • 예외 메시지는 이해가 능한가?
• Magic Number 가 무엇을 의미하는지 변수나 상수 추출이 되어있는가?
• null 을 반환 하는가?
  • Optional 또는 Exception 으로 유도
• 메서드 파라미터 중 Boolean 이나 Optional 을 사용하지 않는가?
  • 만약, 해당 파라미터를 사용하여 분기 하고 있다면 true 인 경우와 false 인 경우를 별도로 추출해야 한다.
• 테스트가 해피 케이스 외에 엣지 케이스도 다루고 있는가?
• 코드를 나중에 사용할 예정으로, 코멘트 처리해서 사용하지 않는 코드를 남겨두지는 않았는가?
  • 커밋 로그에 남기면 삭제된 코드를 기록하여 재생성 권장

테스트

• 새로운, 변경된 코드에 대한 테스트가 존재하는가?
  • 테스트가 하는 일을 이해할 수 있는가?
• 한계를 문서화 하고 있거나, 테스트가 존재하는가?
  • 배치가 한 번에 1,000 개만 처리 가능한 경우 1,000개를 초과하면 예외가 발생하고 있는가?

자료 구조

• List 에서 데이터를 탐색하기 위해 너무 자주 반복한다면 Map 사용을 고려하라.
• 결과를 반환하기 전에 항상 정렬하고 있다면 TreeSet 사용을 고려하라.
• Map 을 외부에 직접 노출 하지 말아라. -> 클래스로 감싸서 외부에 반환
  • 원치 않는 키/값이 들어와도 방지 불가
  • get 을 한 뒤 null check 를 빠트리면 NPE 가능
  • equals & hashCode 사용

성능

도날드 크누스 - 미리 성능을 고려 하는 행위는 97% 악의 근원이다.

• 반복문에서 DB 나 외부 API 를 여러번 호출하지는 않는가?
• 클라이언트와 서버간의 네트워크 라운드 트립이 자주 일어나면 성능이 떨어진다.
• 락 전략을 적절하게 사용하고 있는가?
• 리플렉션은 사용하지 않는 경우보다 느린 경우가 많다.

설계

• SRP 를 준수하며, 적절한 응집도와 적절한 결합도를 준수하는가?
• DIP 를 준수하며, 고수준 컴포넌트가 저수준 컴포넌트에 의존하고 있는지?
• OCP 를 준수하며, 기능 확장을 코드 수정 없이 할 수 있는지?
• 다른 클래스가 가지고 있는 데이터를 조회 해서 조작하고 있는 Feature Envy가 있는지?
• 내가 가지고 있는 데이터를 외부에 노출하지 않고, 내가 가공해서 제공하도록 캡슐화 되어 있는지?
• 중복 - Locate duplicates 를 검사하여, 중복이 없도록 유지하고 있는지?
• 오버 엔지니어링
  • YAGNI: 필요하지 않지만, 필요할지도 모른다며 코드를 생성하지 않는지?
  • 재사용: 재사용 할 것이라고 예측 하고 있지는 않은지?
• 메타포 역할: Controller, Service, Dao, Repository, Entity, Value, Factory, Strategy
  • 메타포 역할이 의하는대로 동작 하는지?
• 대칭성: 코드를 일관적으로 작성하고 있는가?
  • 대칭성이 있는 코드는 비대칭인 코드에 비해 이해하기 쉬움
  • 예) “DB 에서 마지막으로 개싱된 문서 가져오기” 메서드가 여러 곳에서 사용 될 때
    • 메서드 이름이 다르지 않는가
    • 처리 순서가 다르지 않는가
    • 중요한 차이점이 있는지는 않은가
  • Composed Method 참고
• 반복문, 조건문 등에서 2가지 이상의 상태가 변경이 되는 경우
  • 반복문 두 개를 복사해서, 하나의 반복문에서 한 개의 상태를 변경하고 이를 메서드로 추출할 수 있음
• 파라미터 수가 3개 이상인 경우 -> Parameter Object 추출
• 불필요한 setter 제거하기 (주로 Lombok 에 의해 setter 가 생성됨)
• Guard Clause / Early Return 으로 코드를 읽는이가 읽을 때 복잡도를 줄일 수 있는가?
• 디자인 패턴을 이용해서 코드의 가독성, 유지보수성을 높일 수 있는지?

레거시 다루기

레거시 분석하기

레거시 분석의 한계

1. 코드 전반을 머릿속에 보관하기 어렵다.
2. 긴 코드를 한 번에 보는 것이 어렵다.
3. 코드를 이해하기 위한 보조 수단이 필요하다.

코드 시각화를 위한 몇 가지 표기법

액티비티 다이어그램: 단계적인 코드 실행 흐름

• 조건 분기가 어떤식으로 흘러 가는지 따라가기 쉬움
• 논리적으로 코드 블록을 추출하기 쉬움

코드가 시각적으로 어떤 행동을 하는지 잘 드러나기 때문에, 논리적으로 무슨 기능인지 파악하는데 많은 도움을 얻을 수 있다.

시퀀스 다이어그램: 구성 요소간 연동 흐름

• 시간 흐름에 따라서 연동을 표시 하고 있기 때문에, 실행 순서를 이해하는 데 도움이된다.
• 비동기를 표현할 수 있다.

의존/호출 관계 그래프: 클래스/변수/필드/메서드 간 의존 관계를 그림으로 표현

• 코드를 수정했을 때, 어떤 곳에서 영향을 받는지 파악하기 쉽다.

스크래치 리팩토링

실제로 리팩토링 하지 않고, 코드를 이해하는 것을 목적으로 리팩토링 하는 것을 의미한다.

리팩토링을 수행하며 코드를 이해하는 시간을 갖고, 코드를 되돌린 뒤 이해한 내용을 바탕으로 다음 기능을 만들어낸다.

레거시에 테스트 코드 만들기

범위를 좁혀서 테스트 만들기

테스트 할 대상을 기존 코드와 분리해서 테스트 작성

• 일부 코드, 로직, 기능만 테스트하고 싶을 때 사용
• 새로 추가한 코드만 테스트하고 싶을 때 사용

대체 구현을 사용해서 테스트 만들기

테스트 대상이 이미 사용하고 있는 객체, 기능이 존재하는 경우 의존 대상의 구현을 대체할 대역을 만들어서 테스트 코드를 실행 시킨다.

범위를 넓혀서 테스트 만들기 - 레거시 코드에서 가장 효과적인 테스트 ✅

• 일부 로직이 쿼리에 위치함

이 테스트 방식은, 범위를 가능한 넓혀 다양한 구성 요소 간 연동을 포함하는 테스트 코드 작성 방식이다.

• DB 연동 / 외부 연동을 포함 하기도 함
• API 또는 제공 기능 단위로 통합 테스트

예시)

테스트 환경 구성

로컬에 운영환경과 동일한 DBMS 구성

• 특정 DB에서만 동작하는 전용 쿼리 실행 위함
• 테스트 실행 후 변경된 데이터 확인이 용이함
• 다른 개발자와 동일한 데이터를 변경하지 않기 위해, 로컬에다 구성

필요한 DB 테이블 생성 방법

1. 로컬에 미리 테이블을 생성, 변경되면 반영
2. 테스트를 실행할 때 마다 테이블 초기화(create-drop)

테스트 흐름

리팩터링

코드 변경 비용을 낮추려면 이해/변경이 쉬운 구조로 점진적으로 개선해야 한다.

• 레거시에서 흔히 발생하는 상황으로 코드 이해에 2시간, 수정은 5분이 걸리는 일이 있다.
• 리팩터링을 통해 코드 이해 시간을 줄이 필요가 있다.

테스트가 있다면 과감하게 리팩터링이 가능하다.

• 코드 변경 후에 전과 동일하게 동작하는지 검증이 가능하기 때문이다.

테스트가 없어도 필요하면 리팩터링을 진행한다.

• 현재의 위험 회피 -> 미래에 더 큰 위험이 따른다.

미사용 코드 삭제

사용되지 않는 코드는 주석으로 날짜를 기록하고, 일정 기간 뒤에 삭제한다.

• 미사용 클래스
• 미사용 메서드
• 미사용 파라미터
• 미사용 조건 분기

매직 넘버

숫자, 문자, 리터럴은 의미를 알 수 없기 때문에 의미가 담긴 상수나 열거형으로 추출한다.

이름 변경

이름 짓기가 어려운 만큼, 이름이 갖고 있는 예측되는 행위와 실제 행위는 다른 경우가 많다. 또는, 이름이 어떤 행위를 하는지 예측할 수 없다.

• 클래스, 메서드, 파라미터, 변수 이름을 의미에 맞게 변경한다.

변수 선언과 사용

변수는 사용 직전 위치로 이동 시켜주는 것이 좋다. 이는, 변수 선언 위치와 사용 위치가 멀리 떨어져 있으면 이 변수가 어떤 값이었는지 어떤 값으로 변경 되지는 않는지 생각을 하기 때문에 부담 된다.

변수 제거

긴 코드에서 값이 바뀌는 변수가 많을수록 코드 추적이 어렵다.

• 필요하지 않은 변수는 가능하면 제거한다.
• 한 변수를 여러 의미로 사용하지 않는다.

조건 분기 줄이기

if-else 에서 if 가 길고, else 가 짧은 경우 조건을 뒤집어 구조를 단순화 한다.

• early-return 패턴으로 중첩된 if 제거로 코드 복잡도를 감소 시킨다.

메서드 분리

비슷한 행위을 하는 두 기능이, 하나의 메서드에서 동작하고 있는 상황에 사용한다.

• 두 기능 중 한 기능을 위한 메서드 추가
• 이 메서드는 내부에서 기존 메서드 호출
• 기존 메서드를 호출하는 코드가 새 메서드를 호출하도록 변경
• 기존 메서드의 코드를 새 메서드로 이동
• 기존 메서드 이름을 변경
• 분리 완료 후 추가 개선 진행

클래스 분리

클래스는 항상 커지기 마련이다. 클래스를 분리 할 경우 발생하는 복잡성 보다 클래스가 커졌을 때 복잡성이 더 크다. 이럴 때, 일부 기능을 별도 클래스로 분리 하여 증가하는 복잡도를 낮춰주도록 해야 한다.

Q. 클래스로 분리하게 전, 메서드 추출로 중복을 처리 했는데 분리하고 나면 중복이 발생하지 않나? A. 클래스 분리를 진행하는 과정에서 중복이 발생하는 것은 별도의 리팩터링으로 중복을 제거하면 된다.

메서드 추출

클래스 추출

좁은 범위에 대해 테스트를 작성하는 방법론에서, 리팩터링 단계에서 클래스로 추출할 경우 테스트 하기 수월 해진다.

파라미터 값 정리

파라미터는 적을수록 좋다. 개발을 하다보면 자연스레 개수는 증가하게 되고, 코드가 변할수록 사용하지 않는 파라미터가 생길 수 있다.

가능하면 메서드에서 사용하고 있는 값만 파라미터만 사용하는 것이 좋다.

• DTO 형태로 넘어오는 파라미터는 사용하지 않는 프로퍼티 제거하기
• 또는, 사용하는 값만 다용하고 있는 새 타입을 사용 한다.