코드 리뷰 가이드라인

(이 글은 @teradonburi님의 코드리뷰의 비결( コードレヴュー虎の巻 )를 한국 상황에 맞게 의역한 것임을 밝힙니다)

Review Guideline

여기에서 언급하는 리뷰는 피어 리뷰(Peer review)를 의미한다.

(작업성과물의 결함 및 개선 발생 여지를 찾는 리뷰)

‘최악을 최초로’ 를 기본으로 리뷰를 실행

예를 들어 리뷰를 할 때 사양과 알고리즘에 결함이 있는데도 오탈자에만 매달리는 경우가 자주 있다. 이 글은 무엇이 최악인지를 생각하고 이를 예방하기 위한 것을 리뷰의 목적으로 한다.

리뷰 대상을 고를 때

  • 오류가 프로덕트 전체에 영향을 준다
  • 오류 발생시의 수정 비용은 매우 비싸다
  • 실패할 것 같은 리스크는 없을 것인가

를 고려하여 리뷰의 대상을 고른다

예를 들어, 기본적인 초기 단계의 요구사양이나, 크리티컬한 결정을 기초로 한 사양, 사용 빈도가 높은 모듈등이 리뷰의 대상이 될 것이다.

아래에 작성한 항목은 리뷰어가 부담을 느끼지 않도록 하는게 전제이기 때문에 리뷰어에 제출되기 전에 체크해야 해야 할 항목들이다.

실제 리뷰를 했는지 증거로서 스크린 샷을 찍어 놓는 것도 중요하며. Demeter법칙, YAGNI, DRY, KISS, GoF 를 의식하며 실행, 리뷰하는 것도 중요하다.

중요 체크 포인트

  • 버그나 설계 변경이 있는 경우, 가장 위험할만한 부분을 중점적으로 본다
  • 모델의 변경은 위험한 작업이란 것을 염두하면서 본다.
  • 메일 발송 등 한번 보내버리면 되돌릴 수 없는 것들에 대해서는 반드시 시험해 본다.
  • 보안이 연관된 부분은 주의한다. 유저가 입력을 집어 넣은 부분들이 그렇다(XSS/SQL인젝션 등)
  • 서비스의 근본적인 가치 문제, 또는 돈과 연관된 부분들은 죽기살기로 본다.

기본 코드 리뷰

  • 함수 인자의 존재 체크 (undefined, null, nill)
  • 경계조건에서 정상적으로 동작할지
  • 결과 반환을 하지 않는 함수가 없는지
  • 계산 순서를 좀 더 짧게 쓸 수 없는지, DB 검색 쿼리 조건으로 해결할 수 없는지
  • 병렬 처리는 동기와 정합성을 적절히 취했는지
  • 캐시를 이용하는 곳이 있다면, 캐시 유무에 상관없이 정상 작동을 하는가
  • 매직 넘버1는 절대 쓰지 않는다. 쓰더라도 이는 어떤 의미인지 반드시 적는다
  • 처리가 복잡한 경우 사양 설명이 코멘트에 기술되었는지
  • 하나의 메서드에 if 문을 남발하고 있지 않는가(조기 return, continue 가 불가능한지)
  • 변수 이름이 잘못 붙여져 오해를 불러올 여지가 있을까?
  • 데드 코드(dead code)는 없는지
  • 필요없는 처리를 하고 있지는 않은지
  • CSRClient Side Redering, SSRServer SIde Redering의 문제는 없는지(SPASingle Page Application의 경우)
  • AMPAccelerated Mobile Page 의 경우 문제가 없는지(AMP를 실행하는 경우)

API리뷰

  • API 경로가 틀리지는 않는가?(다른 API경로를 덮어쓰고 있지 않은가)
  • 외부 서비스 API를 호출할 때는 에러 처리를 하고 있는가?
  • 리퀘스트 파라메터 존재 체크여부(undefined, null, nil)
  • 리퀘스트 파라메터에 의도하지 않는 데이터가 유입되면?(Validate)
  • API의 단위 테스트는 되어 있는지
  • 메일, 메시지 송신 기능(SMS, KAKAO, LINE) 은 실제 송신하고, 개봉한 메시지는 링크가 짤리지 않는지 확인했는가?
  • 과금 관련 처리는 정확히 처리되었는가. 에러 시에는 프론트엔드의 처리도 정확히 하고 있는가?
  • 구글 봇 등은 집계처리에서 처리되고 있는가?

DB 리뷰

  • DB쿼리 에러 케이스가 없는지(쿼리로부터 발생하는 에러가 없는지 체크가 가능한지)
  • 이상한 데이터가 혼입(混入)되는 케이스가 없는지(모델 validate를 하고 있는가)
  • 여러개의 API 조건 분기에 영향을 받는 케이스가 없는지
  • 시큐리티 문제가 있는 파라메터가 전달되지는 않는지(XSS, SQL인젝션)

DB필드의 변경, 삭제시 발생하는 수정은 실제 데이터를 localhost의 DB에 저장하고 반드시 마이그레이션했는지를 확인(실제 데이터는 의도치 못한 것이 유입되는 경우가 많음)

  • 모델의 마이그레이션이 발생할 경우, 폴 백(fallback)처리가 되어 있는지(릴리즈 후, 마이그레이션 스크립트 실행이 필요한 케이스로 릴리즈 단계를 나눌 필요가 있다)
  • index를 추가, 변경, 삭제하는 경우 문제가 없는지

프론트엔드 리뷰

  • 유저가 조작 불가능으로 빠질 플로우가 없는지
  • 자바스크립트 에러가 없는지
  • 링크가 이상한 곳은 없는지
  • 특정 브라우저 의존 기능이 있는가, 멀티 브라우저 대응이 되어 있는지
  • SPA의 경우, 다른 루틴을 덮어 쓰고 있지 않은지.
  • API경로가 정확한지
  • 맞는 데이터가 반영되고 있는지
  • API에러 경우 어떻게 처리하고 있는지
  • 유저 입력 데이터를 표시하는 곳은 XSS 세니타이징을 하고 있는지

웹 디자인 리뷰

  • 데이터 연동 컴포넌트(리스트 등)의 경우, 데이터가 비어 있을 때의 표시(또는 로딩중 표시)는 설정되어 있는지
  • chrome, iPhone safari, IE 지원을 다 하고 있는지
  • 화면 사이즈 변경시 레이아웃의 깨짐은 없는지
  • 표시되고 있는 화면의 사이즈는 적절한가
  • 여백이 반영되고 있는가?
  • 문자 크기가 적절한가?
  • 컬러 코드가 적절한가?

코드의 가독성 리뷰

The art of Readable Code2 라는 명저가 있으며 일단 이를 읽어보는 것을 추천한다 .

  • 명명규칙을 통일한다. 오탈자, 표기의 비일관성이 없어야 한다
  • 처리를 함수화하고 의미가 있는 코드 덩어리 단위로 개행(改行)을 입력
  • 복잡한 조건분기는 쓰지 않는다(조기 return, continue를 명심하라)

도메인 구동 개발

도메인 구동 개발(Domain driven Dev)라는 것이 있다. 시스템 요건의 컨텍스트(상황, 역할)에 맞게 모델(테이블)과 필드를 작성하는 것으로 사양 변경과 확장을 하기 쉽게 설계를 한다.

CRUD에 맞는 API 경로

모델 단위로 CRUD조작 API경로를 변경하지 않고 HTTP메소드 변경으로 대응해야 한다.

  • Create : POST
  • Read : GET
  • Update : PUT
  • Delete : DELETE

예를 들어 user모델의 CRUD조작의 경우

  1. create: POST /user -> 유저 생성
  2. Read : GET /user –> 유저 열람
  3. Read : GET /user/:id –> id로 유저 취득
  4. Update : PUT /user/:id –> 유저 갱신
  5. Delete : DELETE /user/:id –> 유저 삭제

가 된다.

테스트 자동화

일반적으로 동작확인은 수동 테스트를 하는게 당연하다고 생각할지도 모르겠다. 하지만 여기에 덧붙여 아래 테스트를 추가하는 것으로 시스템의 견고성이 증가한다. 구체적으로는 회피 테스트가 이에 해당한다.

테스트가 중요한 이유는 라이브러리의 버전 업그레이드시, 또는 사양변경을 하였을 때 다른 부분에서 부정합(오류)가 없는지 확인하는게 가능하기 때문이다.

Circle CI, Travis CI, Jenkins등의 CI(Continuous Integration)을 사용하여 git push 시 계속적인 테스트를 실시할 수 있다.

  • API 단위 테스트
  • End to End 테스트
  • 화면 리그레션 테스트

API단위 테스트에는 Jest(NodeJS의 경우, 여기에 sinon, supertest, nock, rewire, proxyquire등의 mock라이브러리를 같이 조합)를 사용한다.

프로덕트 코어 기능을 테스트하는 End to End테스트에는 Jest+ puppeteer 를 같이 조합하곤 한다.

UI 회귀 테스트인 화면 리그레션 테스트는 storybook+BackStopJS 로 실행한다.

테스트 커버리지

codecov 를 사용하여 테스트 코드 전체의 커버리지를 확인할 수 있다. 코드 커버리지를 100%로 보장할 필요는 없지만 시스템 코어 기능관련 부분은 테스트 누락 방지를 위해 지키는 것이 좋다.

(단, 사양 누락인 경우에는 테스트 수행시의 누락인 경우여서 테스트 커버리지가 아무리 높아도 누락은 발생한다.)

Lint

린트(Lint)는 프로그래밍 언어 문법 체크이다. 웹 서비스의 경우 ESLint 문법 체크 규칙 지정이 가능하다.

또는 husky 를 사용하여 git commit시 또는 git push전에 lint커멘드를 실행시킬 수 있다.

안드로이드의 경우에는 Android Lint, XCode의 경우에는 SwiftLint 등이 있다.

리뷰의 자동화

danger 를 사용하여 PR(Preview Release)의 특정 조건에 걸리면 경고를 발생시키는 것이 가능하다.

  • PR 1회 코드가 500라인이 넘어간다
  • 테스트 코드가 없으면 경고를 발생
  • 특정 파일에 손을 댄 경우 경고를 발생(ORM파일을 수정한 경우 등)

  1. http://blog.daum.net/question0921/1103 참조 ↩

  2. 한국에서는 ‘읽기좋은 코드가 좋은 코드다’ 라는 이름으로 임백준님이 번역함 ↩