코드 리뷰 이야기(2)

이글은 지난 글 "코드 리뷰 이야기(1)"에 연속된 글입니다. 지난 글에서는 현재 제가 있는 개발팀에 "코드 리뷰"를 적용하는 과정을 공유했습니다. 이번 글에서는 실제 코드 리뷰를 어떻게 하고 있는지에 대해 공유하려고 합니다. 지난 글에서 밝혔듯이 각 개발팀의 상황, 코드 작성자 등에 따라 다양한 관점으로 코드를 볼 수 있기 때문에 이 글은 그냥 참고만 하셨으면 합니다.

이 글의 작성 목적 중의 하나는 개발팀내에서 코드 리뷰가 어느 정도 가능한 개발자를 중심으로 코드 리뷰를 확산하려고 하는데, 이들에게 코드 리뷰의 방향을 알려주려는 것입니다.

(이미지 출처: http://whallalabs.com/how-to-write-a-better-code-using-peer-code-review/)

(실제 리뷰 사례의 많은 부분이 영어로 코멘트되어 있는데 필자의 영어 실력이 짧아 독자들이 이해하기 어렵지 않을까 하는 걱정이 있습니다. 영어의 잘못된 부분에 대해서는 무시하고 봐주세요. 정말로 이해 못할 정도면 댓글로 피드백 주시면 반영하겠습니다.)

코드 리뷰는 왜 하나?

코드 리뷰를 수행하는 팀원이 코드 리뷰를 왜 하는지에 대해서 잘 알지 못한다면 코드 리뷰를 어떤 방식으로 해야 하고 어떤 관점에서 코드를 봐야 하는지 알 수 없습니다.

코드 리뷰는 개발자가 작성한 코드를 동료 개발자가 리뷰를 해주는 것을 의미합니다. 이를 통해 대략 다음과 같은 목적을 이룰 수 있습니다.

• 코드 내의 결함을 미리 발견할 수 있다.
  • 필자가 심리학에 대해서는 하나도 모르지만 필자의 경우나 필자 주변의 개발자들을 보면 자신이 만든 코드에 대해서는 자신도 모르게 방어하려는 성향이 강하다. 다시 말해 무의식 중으로 코드의 나쁜 부분을 보려고 하지 않거나 무시하려고 하기 때문에 나쁜 부분이 잘 보이지 않는다.
  • 따라서 많은 경우 코드 작성자는 자신이 작성한 코드에 있는 문제를 잘 발견하지 못하는 경향이 있다.
• 코드를 예쁘게 만들려고 노력한다.
  • 코드 리뷰어가 의견을 달고 그 의견에 특별한 이견이 없으면 그 코드를 만든 개발자는 코드를 수정하고 다시 Push 하는 등의 작업을 추가로 해야 한다. 이런 작업은 개발자에게 번거로운 작업이다.
  • 리뷰어가 발견한 로직 상의 문제 등은 개발자가 미리 찾기는 어려울 수 있겠지만 변수명, 포맷, 코드의 중복, 유연성, 가독성 등에 대해서는 미리 조심하면 두번 작업을 하지 않아도 된다.
  • 따라서 리뷰어가 없을 때에 무의식적으로 잘못 정한 변수명이나 Copy &Paste 된 코드 등에 대해서 한번 더 생각하고 가능한 다시 작업하지 않도록 신경 쓴다.
• 코드 개선에 대한 토론을 할 수 있다.
  • 이 항목도 앞의 항목과 비슷한 사항이다. 더 좋은 구현 방법이 있음에도 불구하고 시간의 문제, 귀챠니즘 등으로 인해 수많은  if..else 절의 남발, 3 ~ 4 단계 이상의 블럭 구성 등으로 구현된 코드가 많다.
  • 리뷰어는 시간이 더 소요되고, 코드를 만드는데 있어 조금 더 번거롭기는 하지만 코드 개선에 대한 의견을 제시할 수 있다. 이런 의견을 달 수 있는 것은 생각해보면 내가 만들지 않지 때문이지 않을까 한다. 필자의 경우도 직접 만든 코드는 아주 지저분하지만, 지저분한 코드를 리뷰할 때는 좀 더 개선된 방법을 제안하는 경우가 많다.
• 다른 업무를 이해할 수 있다.
  • 다른 개발자가 작성한 코드를 보면서 리뷰어는 그 코드를 통해 어떤 기능을 수행하고자 하는 지를 간접적으로 알게 됨으로써 별도의 설명없이도 어느 정도는 자신의 메인 업무가 아닌 업무에 대해서도 알게 된다.
  • 이를 통해 자신이 만드는 코드가 다른 개발자 만든 코드에 어떤 영향을 미치는지도 알게되어 자연스럽게 업무 공유가 가능해진다.
• 다른 개발자의 코드를 통해 다른 개발 방법을 배울 수 있다.
  • 개발 기간과 개발자의 능력 사이는 비례 관계가 아닌 것은 확실하다. 지금 있는 개발팀만 하더라도 일년이상 동일한 개발 언어와 플랫폼으로 개발을 했음에도 불구하고 개발 능력은 크게 개선되지 않았다.
  • 물론 업무의 이해 능력과 대응 능력은 개선되어 전체적인 생산성은 증가하였지만, 그래도 개발 능력은 여전히 그대로이다.
  • 이것은 두가지 관점으로 볼 수 있는
    • 1. 별도의 코칭 과정이 없었고,
    • 2. 개발자 스스로도 학습하려는 의지가 없었다.
  • 이런 상황에서 2번의 경우 강제할 수 없기 때문에 1번에 초점을 맞추어야 하는데 코드 리뷰를 하게 되면 자연스럽게 코칭을 할 수 있게 된다. 또는 2번의 경우도 잘 만들어진 다른 개발자의 코드를 보면 자신도 모르는 사이에 학습을 하게 된다. 다만 팀 전체의 코드가 잘 만든 코드가 없고, 1번의 코칭이 없다면 더 좋지 않은 방향으로 흐를 수가 있다. 나쁜 코드가 계속 확산될 수 있다.

현재 팀의 코드 리뷰는 결함 발견도 있지만 "코드를 예쁘게 만들기"와 "학습"에 중점을 두고 있습니다. 이후 결함 파악 또는 개선을 위한 토론 등으로 진행할 예정입니다.

코드 리뷰를 꺼려하는 개발자

이렇게 좋은 측면이 많은 코드 리뷰이지만 이를 적용하는데 있어 개발자들이 꺼려하는 경우도 있습니다. 코드 리뷰를 재대로 적용하기 위해서는 왜 개발자들이 코드 리뷰를 꺼려하는지에 대해 생각해 보고, 그런 거부감이 발생하지 않도록 주의를 기울여야 합니다. 대략 다음과 같은 이유 때문이 아닐까 생각합니다.

• 개발 속도의 문제
  • 개발자는 코드 리뷰를 하면 지금 수행하는 개발 속도보다 느려질 것이라고 생각한다. 이 경우는 대부분 코드 리뷰의 문제가 아니라 현재 개발 프로세스 자체에 문제가 있는 경우가 많다. 코드 리뷰 때문에 개발 속도가 느려진다는 의미는 다음과 같은 경우가 대부분이다.
    • 개발자가 코드 수정 -> staging branch push -> staging test -> 배포
    • 이 경우 개발자는 코드 수정 후 바로 스테이징에 반영되어 테스트 해보기를 원하는데 이렇게 되면 스테이징에 배포된 다른 이슈와 합쳐지게 되고 이 이슈가 문제가 발생했을 때 스테이징에서 테스트 완료된 코드도 배포를 할 수 없게 된다. 이 경우 문제가 발생한 이슈를 해결하거나 그 이슈를 제거한 후 배포해야 한다.
  • 이슈 하나만 보면 운영까지 배포되는데 기존에 몇 시간이면 되는 작업이 코드 리뷰를 하게 되면 하루 이상 또는 몇 일이 걸릴수도 있다.  하지만 리뷰어가 코드를 리뷰하는 동안 개발자는 놀고 있는 것이 아니라 다른 이슈를 처리하고 있기 때문에 전체적인 시간은 리뷰어가 리뷰를 수행하고 이를 피드백 받아 코드를 수정하는 과정의 시간만 더 추가될 뿐이다. 피드백 받아 코드를 수정하는 과정은 테스트 과정에서 발생할 문제를 미리 해결한 것이기 때문에 추가된 시간이라 볼 수 없다. 따라서 추가된 시간은 리뷰어가 리뷰를 수행하는 시간 정도이다.
• 코드를 보는 행위가 얼핏 자신의 코드를 평가(또는 지적)하는 행위가 된다는 인식
  • 현재 조직에서도 이런 팀이 한 두개 있었는데 코드 리뷰를 하자고 하고 서로 동의도 했지만 Pull Request를 보내지 않았다. 몇번 확인해보니 자신들의 코드를 보여주는 것을 꺼려하고 있었다. 물론 소스 레포지토리가 모두 공개되어 있기 때문에 직접 들어가서 볼 수 있다. 전체 코드는 팀원 전체가 만든것이기 때문에 제가 보는 것에 별다른 느낌이 없지만 자신이 처리하고 있는 이슈 그 자체에 대해 자신이 만든 코드를 남이 본다는 것 자체가 코드에 대한 평가를 받는다는 느낌이 든다.
  • 어떤 경우에도 리뷰어의 코멘트나 리뷰 그 자체에 대해서는 이런 느낌을 받지 않도록 하는 것이 좋고 특히 코드 리뷰 도입 초기에는 이점을 계속 고려해야 한다.
• 리뷰어의 시간 부족
  • 리뷰어 입장에서도 자신의 코드를 만들고 있는 중에 다른 개발자의 코드를 리뷰함으로써 개발의 흐름이 끊기고 자신의 시간을 낭비한다고 생각할 수 있다. 따라서 코드 리뷰 자체가 부실해질 수 있다.
  • 아직 상호 리뷰를 하지 않고 필자 혼자 리뷰를 하는 주요 요인이기도 하다. 코드 리뷰에는 책임감이 따르는데 이 책임감을 강제할 방법은 많지 않고, 강제하는 것 자체가 제대로 된 코드 리뷰가 아니라고 생각한다.
• 잘 모르는 업무 또는 서비스에 대한 리뷰
  • 리뷰어가 해당 서비스에 관계가 없거나 같은 업무라 하더라도 서비스간의 연관성이 없는 경우 리뷰어는 모르는 업무에 대한 코드 리뷰를 봐야하고, 리뷰를 요청한 사용자도 리뷰 결과에 대해 신뢰를 할 수 없게 된다.

코드 리뷰어로서의 행동

코드 리뷰를 현재 팀에 적용하는데 있어 앞에서의 언급한 거부감 또는 꺼려함이 또는  몇가지 이슈들이 나오고 있지만 아직까지는 크게 문제되지 않는 상황입니다. 아직 본격적으로 Peer 리뷰를 하지 않고 저 혼자만 리뷰를 하고 있는 상황이라 그렇지 않나 생각합니다. 피어 리뷰를 적용하고 난 이후에 이 부분은 다시 한번 정리해보도록 하겠습니다. 다음은 제가 처음 코드 리뷰를 적용하면서 어떤 관점으로 리뷰를 하고 있는지 정리해 보았습니다.

• 가장 중요한 것은 코드 리뷰가 지적질이 아닌 본인에게 도움이 된다는 느낌을 주도록 노력 한다.
• 처음에는 리뷰어로는 코드 리뷰 경험이 많은 사람만 참여시킨다. 경험이 있는 사람이 부족한 경우 한두명 또는 한 그룹 정도만 코드 리뷰에 참여시킨다.
  • 현재는 저 혼자이고 각 그룹별로 그룹 리더에게 코드 리뷰를 맡겨 보려고 탐색 중
• 처음부터 강하게 코드 리뷰를 하기 보다는 하나의 PR에 대해 가장 눈에 띄는 한, 두개만 코멘트를 달아준다.
  • 예를 들어 중복된 코드의 중복을 없애거나, 잘못된 변수명 등에 대해 수정 요청을 하는 등
  • 이후 점차 비즈니스 로직, 리팩토링 등으로 늘려 나간다.
  • 즉, 많은 부분을 보더라도 크게 문제 되지 않는 부분은 초기에는 무시하고 넘어가기도 한다. 코드 리뷰 적용 초기에 하나의 이슈에 너무 많은 Comment를 달면 코드 작성자가 힘들어 할 수도 있다.
• 처음 코드 리뷰를 시작할 때에는 특별한 이슈가 없더라도 최소한 한개의 코멘트는 한다.
  • 이유는 리뷰어가 나의 코드를 봤구나 하는 느낌을 주기 위해서이다.
  • 이후 어느 정도 지난 시점이 되면 특별한 게 없으면 그냥 +1.
• 초기에는 Pull Request 가 등록되면 바로 리뷰를 진행하고 피드백을 준다.
  • 코드 리뷰로 인해 자신의 개발이 더디게 진행된다는 느낌을 주지 않게 하지 위해 빠르게 리뷰를 해준다.
  • 코드 리뷰가 본 궤도에 올라오면 리뷰어의 시간에 맞추어 진행해도 무관한 듯
• 개발자의 성향에 따른 리뷰를 다르게 한다.
  • 자신의 코드에 자부심이 많은 개발자
    • 코딩 스타일이나 코딩 기법 등과 같은 부분에 대한 코멘트 보다는 성능 저하 요소 등 명백하게 개선할 여지가 있는 부분에 대해 코멘트를 한다.
    • 코드 그 자체에 대해 코멘트를 할 경우 레퍼런스 링크를 추가하는 등 리뷰어의 의견이 아니라 일반적인 의견이라는 것을 강조
  • 뭔가를 가르쳐 주기를 원하는 개발자
    • 단순히 이렇게 수정하는 것이 어떠냐라는 의견보다 왜 이렇게 하는 것이 좋은지에 대한 설명을 추가한다.
    • 왜 코드가 이렇게 바뀌어야 하는지 또는 왜 이런 의견을 달았는지에 대한 이유를 설명한다. 레퍼런스가 되는 링크를 추가하기도 한다.
• 필요하면 코멘트를 등록하는 것 이외에 오프라인에서 직접 찾아가서 의견을 교환한다.
• 이번 Pull Request에서 수정하기에 내용이 많은 경우 새로운 이슈로 등록하고 이번 Pull request는 Merge 한다.
  • 이때에는 반드시 새로 만든 이슈를 Pull Request의 코멘트로 등록하도록 한다.

실제 리뷰 사례

현재 제가 하고 있는 코드 리뷰는 주로 코칭의 도구로 사용하고 있습니다. 따라서 내용도 코드 상의 버그나 문제점을 찾는 것 보다는 주로 코드의 개선에 대한 내용이 많습니다. 대부분은 코드 리팩토링에 나오는 내용들입니다.

코딩 컨벤션은 잘 키지고 있는가?

개발 팀 내에 별도의 컨벤션 정의는 하지 않고 있습니다. 다만 각 언어의 기본 컨벤션을 지키도록 가이드 하고 있습니다. 코딩 컨벤션은 기본중의 기본이지만 현재 팀에서 잘 지키는 개발자도 있지만 잘 지키지 않는 개발자도 여럿있어 이 부분에 대해서는 리뷰 시에 지속적으로 코멘트를 추가하여 수정할 것을 권고합니다.

코드가 이해하기 쉬운가?

일단 처음 딱 봤을때 어떤 처리를 위한 코드인지 알지 못하면 의문을 제기합니다. 어떤 부분때문에 이해하기 어려운지를 확인하고 수정 하는게 어떤가 하는 의견을 제시합니다. 최근에는 주로 함수를 파라미터로 전달하는 경우에 대한 내용이 많았습니다. 제가 옛날 사람이라서 그런지 이해하기 어려운 코드가 많습니다.

• 지금 보니 "~좋습니다." 와 같은 문구가 좋지는 않네요. 뭔가 강요하고 있는 느낌인데 "이렇게 하는 것은 어떤가요?" 정도가 더 좋은 느낌이 듭니다.

변수명 또는 함수명은 이해하기 쉬운가?

• 해석하기 어려운 약어 또는 한, 두 글자의 변수 사용
  • 현재 개발팀은 여러 가지 개발 언어를 사용하고 있지만 go를 가장 많이 사용하고 있습니다. go의 naming convention을 보면 다음과 같이 되어 있습니다.

    Variable names in Go should be short rather than long. This is especially true for local variables with limited scope. Prefer c to lineCount. Prefer i to sliceIndex.The basic rule: the further from its declaration that a name is used, the more descriptive the name must be. For a method receiver, one or two letters is sufficient. Common variables such as loop indices and readers can be a single letter (i, r). More unusual things and global variables need more descriptive names. (https://github.com/golang/go/wiki/CodeReviewComments#variable-names)

  • 이런 규칙에도 불구하고 현재 팀의 많은 개발자들이 의미있는 변수명을 사용해야 하는 경우임에도 불구하고 1, 2글자의 변수명을 채택하는 경우가 많습니다. 이런 경우 반드시 수정하도록 권고하고 있습니다. 이것은 위의 코딩 컨벤션 정의에서 "'c', 'i' 등을 더 선호한다 라는 문구" 때문이지 않을까 추측해 봅니다.
• 또한 변수명에 "1", "2" 이렇게 사용하는 경우와 오타 등도 수정을 권고하고 있습니다.
• 오타

• 변수명이 해당 변수가 가지고 있는 값과 동일하게 사용되었는지 확인
  • 사례: 로직이 수정되면서 원래는 분(minute)를 저장하는 변수였지만 시간으로 변경되었는데 변수명은 그래도 사용
• 비영어권 개발자라면 항상 겪는 적절한 영어 단어 선택의 어려움을 겪게 되는데 이럴때는 같이 고민해서 결정하려고 합니다.
  • 사례: perfectProfile
    • 처음에 이 변수가 의미하는 것을 파악하기 어려웠는데 실제 실행되는 화면을 보고 이해했습니다. 이 변수는 사용자의 Profile 중 optional 필드 까지 채워졌는지를 파악하는 변수였습니다.

의미가 애매 모호한 상수의 사용

• 예를 들어 if discount_rate > 0.5 에서 "0.5" 와 같이 상수에 대해서는 이 상수가 의미하는 바를 물어 보고 그 답변 대로 constant variable을 만드는 것이 어떠냐고 의견을 추가합니다.

사용하지 않는 주석 처리된 코드는 삭제

필자도 그렇지만 여러 개발자들은 한번 작성된 코드는 삭제하지 않고 주석 처리하는 경향이 있습니다. 사용하지 않는 코드는 특별한 경우가 아니라면 주석 처리를 하는 것보다 삭제하는 것이 좋습니다.

Logger 대신 표준 출력 함수를 사용하는가?

코드 중에 여러 부분에서 System.out.println 또는 fmt.Println 과 같이 단순히 표준 출력으로만 로그를 출력하고 있는데 이런 경우에는 반드시 logger 이용하도록 가이드 합니다. Logger도 상황에 맞게 로그 레벨(Debug, Info, Warn, Error 등)을 맞게 사용지 확인합니다.

코드가 중복되지는 않은가?

일부 개발자들 중에는 그냥 잘 돌아가면 그것으로 그만이지 중복된 코드를 만드는 것이 뭐가 문제인가? 라는 태도를 취하는 개발자들이 있습니다. 특히 모듈이 작고 몇가지 기능만 처리하는 마이크로 서비스의 경우 코드가 쉽고 들어오기 때문에 이런 반론을 제기하는 경우가 있습니다. 이런 개발자에게 다음 코드는 어떻게 해서 만들어지게 되었는지 묻고 싶습니다.

코드 중복의 경우 다음과 같이 대안을 제시하기도 합니다.

그리고 개발자들이 자신의 개발 역량을 높이는 가장 쉬운 방법 중의 하나가 "어떻게 하면 코드의 중복을 제거할 것인가?"를 계속 고민해보는 것이라고도 생각합니다.  코드에서 중복을 제거하기 위해서는 다양한 기법, 패턴들을 사용해야 하기 때문입니다.

함수가 너무 길지 않은가?

너무 긴 경우 여러 개의 함수로 다시 나눌 수 없는지 확인합니다. 하나의 함수가 너무 길면 코드 읽기도 어렵고 각 기능에 대해 테스트 코드를 만드는 것도 어렵습니다.

너무 많은 레벨의 if 문

if  { if { if { 와 같이 3 단계 이상으로 내려가는 if 문이 있는 경우 다른 방안이 없는지 검토합니다.

if, for, while 문에서 조건에 대한 처리

조건절의 경우 유사한 조건이 코드 내에 여러 군데 흩어져 있을 수도 있고, 조건문 자체가 너무 긴 경우도 있습니다. 또는 어떤 조건을 처리하려고 하는지 로직 자체만으로 파악하기 어려운 경우가 많습니다. 이 경우 함수로 만드는 것을 권장하고 있습니다.

Enhanced for loop(for-each) 사용 권장

for loop 작성 시 index를 이용하여 반복 조건을 명시하는 것보다 enhanced for loop 사용하는 것을 권장합니다.

테스트 코드가 있는 경우 테스트 코드 부터 먼저 확인

테스트에서 커버하는 상황이 적절하면 일단 기본은 만족한 것이라고 생각합니다. 하지만 여기의 대부분은 테스트 코드가 없습니다(이제 조금씩 만들어 지고 있지만).

테스트 코드도 읽기 쉽고 중복 등이 없는지 확인

테스트 코드 역시 실제 코드와 동일하게 잘 만들어 졌는지 리팩토링할 요소는 없는지 확인합니다.

기존 코드에 대한 수정인 경우 리팩토링을 할 수 있는 부분이 없는지 확인

앞의 몇개 글에서 현재의 개발 스타일이 일단 개발, 이후 개선이기 때문에 많은 부분이 리팩토링 대상이 됩니다. 이 부분은 개발자와 협의해서 다른 이슈로 만들어서 할 것인지, 이 이슈 내에서 수정할 것인지 결정합니다. 아직까지 많은  개발 팀원이 스스로 리팩토링에 대한 개념이나 실행할 수준이 안되기 때문에 시니어 개발자와 같이 진행하도록 권고하고 있습니다.

데이터베이스 처리와 관련된 로직의 경우

데이터베이스의 테이블에 데이터 처리를 하는 코드의 경우 다음 사항에 주의하여 코드를 리뷰합니다.

• UPDATE, DELETE 문에는 반드시  WHERE 조건이 있는지 확인
  • ORM 도구를 이용하여 객체를 로딩한 다음 해당 객체의 save, update 기능을 이용하는 경우에는 쉽게 파악할 수 있지만 객체 로딩 없이 where 조건을 이용하는 경우 더 세심하게 리뷰를 해야 함
• WHERE 조건이 있는 코드에서는 DB에 Index가 잡혀 있는지 확인 또는 질문
• 트렌젝션 처리가 되는지 확인
  • 트렌젝션 처리를 하고 있는지와 적절하게 처리하고 있는지 확인
• 문자열로 SQL을 만드는 경우 Space 가 잘 추가 되었는지 확인
  • sql = select_clause + where_clause 이렇게 된 경우 select_clause 변수와 where_clause 변수 사이에 space가 없으면 실행 시 에러가 발생
• 일련번호를 생성하는 로직에서 동시에 요청을 받는 경우 중복된 일련번호가 만들어질 가능성이 있는지 확인
• 하나의 SQL에 여러 개의 서로 다른 SQL 문을 실행하는 경우

시간 관련 처리 시

시간 관련 처리 시에도 몇가지 꼼꼼함이 요구됩니다.

• 시간은 UTC 기준인지 로컬 시간 기준인지 확인 후 해당 기준에 맞게 변경 요청
  • 기준을 UTC로 하는 것이 좋은지 로컬 시간으로 하는 것이 좋은지는 논쟁의 여지가 있지만 어떤 경우에도 기준에는 맞추어야 오류가 발생하지 않음
• Master/Detail의 경우 CreatedAt 필드는 동일한 시간을 사용하는 것이 좋다. Loop 내에서 시간을 각각 시간을 설정하는 일자가 바뀌는 경우도 발생할 수 있기 때문이다.

예외 상황에 대한 처리가 잘 되어 있는지 확인

go의 경우 예외 처리가 잘 안되어 있으면 해당 프로그램(daemon)이 panic 상황이 발생하고 해당 데몬 중지하게 됩니다. 물론 recover 기능을 추가해서 자동 재시작 하는 것도 가능하고, docker의 기능을 이용하여 자동으로 container 자체를 재시작할 수 있습니다. 하지만 예외 상황에 대한 처리 그 자체는 매우 중요하기 때문에 예외 사항에 대한 처리 부분을 주의 깊게 확인합니다.

자바와 같이 try..catch 문을 지원하는 언어의 경우 try...catch 에서 catch 절에서 아무런 처리를 하지 않는 경우 리뷰 시에 comment를 추가하고, 가끔 처리하지 않아도 되는 예외의 경우 주석을 추가하도록 유도 합니다.

비즈니스 로직에 대해서는

다른 개발자가 개발하는 비즈니스 로직의 경우 잘 알지 못하기 때문에 필요한 경우 다른 개발자에게 리뷰 요청을 하기도 합니다. 이 경우 제가 주로 코드 품질에 대해 리뷰를 보고 다른 분이 비즈니스 요건에 맞는지 리뷰를 봅니다.

어쩔 수 없이 제가 리뷰를 보는 경우에는 질문을 해서 개발자에게 다시 한번 확인하도록 합니다.

동시 접속 시 문제되는 상황은 없는가?

간혹 개발하면서 이 기능이 동시에 여러 사용자에 의해 호출될 수 있다는 생각을 하지 않고 개발하는 경우가 있습니다. 이런 부분에 대해서도 꼼꼼히 확인을 합니다. 특히 상태 정보를 가지고 있는 클래스 등에 대해서는 세심하게 확인합니다.

소스 코드 내에 보안 관련 문자열이 있는지 확인

소스 코드 내 또는 환경설정 파일에 다음과 같이 보안 관련 정보가 존재하는지 확인하고 이를 실행 시점에 시스템 환경 변수를 사용하도록 요청합니다.

• DB 패스워드
• 외부 API의 보안 토큰
• JWT 토큰 복호화 키 등

이런 내용 중 어떤 것들은 어쩔수 없이 그냥 두는 경우도 있지만 보안 관련 문자열 관련해서 가장 중요하게 보는 내용은 javascript 코드에 이런 문자열이 포함되어 있는지 반드시 확인합니다. 이것은 보안에 치명적이기 때문입니다.

Authentication & Authorization에 대한 확인

각 기능이 정해진 사용자에게만 노출되도록 구현되었는지 확인합니다. 아직 보안에 대한 인식이 많이 부족하여 이 부분은 꾸준히 지적하고 처리 방식을 공통 모듈화하도록 노력하고 있습니다.

커밋로그가 커밋의 의도를 잘 반영하고 있는지

아직 이 부분은 엄격하게 보고 있지는 않습니다. 그 이유는 서로 사용하는 언어도 다르기 때문에 영어, 중국어, 한국어 이렇게 섞여 있습니다. 애초에 사용하고 싶은 언어로 사용하고 대신에 잘 작성해달라고만 했습니다. 그렇지만 아직 git에 익숙하지 않아서 중간 중간 테스트를 위해 커밋한 로그를 지우지 않고 그냥 올리는 경우가 많습니다. 이 부분은 어느 정도 시간이 지나면 왜 커밋 로그가 중요한지 설명하고 커밋 로그를 잘 적어 달라고 부탁하면서 리뷰 시에도 엄격하게 볼 예정입니다.

참고로 이 글을 작성하고 있는 중에 운영 서버에 배포된 코드를 롤백해서 이전 버전으로 나가야 할 상황이 발생되었는데 제대로 작성되지 않은 커밋로그 때문에 어디부터 rebase 해야 할지 몰라서 하나 하나 찾아야 하는 경우도 발생하였습니다. 이렇게 보니 배포시 tagging 도 제대로 하는지도 검증해야겠네요.

커밋로그는 다음과 같은 규칙으로 적용하려고 합니다.

• 하나의 이슈는 하나의 커밋 로그만 있도록 한다.
• 커밋 로그의 타이틀은 <#이슈번호> <이슈 제목> 형태로 한다.
• 코드 리뷰를 통해 변경된 경우 추가로 커밋 로그를 작성할 수 있고 이때의 커밋 로그 타이들은 <#이슈번호> <이슈 제목> <코멘트 번호 또는 요약문> 형태로 한다.
• 커밋 로그에서 타이틀 이외에 본문은 자유롭게 작성한다.

PR이 충돌이 발생한 경우

이 경우 리뷰어가 리뷰를 보기 어려운 경우가 많습니다. 어떤 코드가 이번 PR로 인해 변경된 코드인지 찾기 어렵습니다. 이 경우 리뷰어는 " 충돌이 발생했다는 " 내용을 코멘트로 달아 해당 코드 개발자에게 Rebase를 요청합니다. 그렇지 않으면 개발자는 충돌이 발생했는지 알지 못하는 상태로 계속 기다릴 수 있기 때문입니다.

성능에 영향을 미치는 코드는 없는가?

현재 팀의 많은 개발자들은 시스템의 성능에 대한 고려를 하지 않고 있습니다. 개발자가 성능까지 고려하지 못할 수도 있고, 그냥 개발하기 편해서 그럴수도 있습니다. 여기 특성상 평소에는 잘 돌아가는 코드라도 11/11일 대규모 이벤트 날에는 서비스가 운영되지 않을 수도 있기 때문에 성능 관련 부분을 꼼꼼히 체크 합니다.

• 코드 중에 sleep 이 있는 경우
• 데이터 많은 상황에서 중첩된 For loop의 사용
• Page 처리 없이 모든 데이터를 로딩하는 경우
• for, while의 경우 무한반복되는 상황이 없는지 확인
• 이외에도 많은 상황이 있음

성능 관련해서는 코드 리뷰 뿐만 아니라 주기적으로 부하 테스트를 수행하여 성능에 문제 있는 부분을 지속적으로 개선하려는 활동을 병행합니다.

마치며

이번 글에서는 실제 코드 리뷰 진행된 사례를 중심으로 코드 리뷰를 할 때 구체적으로 어떤 내용을 중심으로 보고, 어떤 관점으로 보는지에 대해 살펴보았습니다. 대부분은 리팩토링 등에서 언급된 내용이고, 코드 작성 시 기본이 되는 내용이 많습니다. 실제 현장에서는 이런 기본 사항에 대해서도 잘 지켜지지 않는 경우도 많기 때문에 기초적인 내용도 모두 언급해 놓았습니다.

본문에서도 언급했지만 코드 리뷰는 강제하면 거의 효과가 없어지게 되는 것 같습니다. 강제하기 보다는 코드 리뷰가 개발자 자신에게 도움이 된다는 것을 인식 시키는 것이 가장 중요하다고 생각합니다. 그 다음은 리뷰어가 코드 리뷰를 꼼꼼히 할 수 있도록 환경을 조성하고, 리뷰의 품질을 높이는 활동을 하는 것입니다. 필자가 여러 상황에 대해 예제 상황까지 캡쳐를 하면서까지 수고스럽게 글을 쓴 것도 리뷰어의 리뷰 품질을 높이기 위함입니다. 이 글이 코드 리뷰를 시작하는 개발 팀 또는 개발자에게 조금이나마 도움이 되었으면 합니다.

이 글의 실제 내용은 계속 업데이트 될 예정입니다. 독자분들 중에서도 좋은 팁이 있으면 공유 부탁드립니다.