JavaScript 코드 리뷰 - 코드 리뷰 문화

여러분의 팀에서는 코드 리뷰(code review)를 하시나요?

코드 리뷰 도구나 현장 사례 공유가 많아지면서 코드 리뷰를 수행하는 개발팀들이 늘어나고 있는 것 같습니다. 좋은 현상이죠. :)

제가 일하는 팀에서도 JavaScript로 웹 개발을 하면서 코드 리뷰를 상시 수행해 왔는데요, 그간 코드 리뷰를 하면서 다양하게 제안된 JavaScript의 코드 개선 사례나 팁을 공유하려고 합니다.

우선 오늘은 코드 리뷰 자체에 대해 리뷰를 수행하면서 느낀 점들 위주로 정리해 보도록 하겠습니다.

• 코드 리뷰
• 코드 리뷰는 문화다
• 코드 리뷰의 효과
  • 빨리 가려면 혼자 가고, 멀리 가려면 함께 가라
  • 인간에게는 모두의 장점이 있다
  • 조직은 개발자의 성장을 바란다
• 코드 리뷰하기
  • 겸손하고 정중하게 리뷰하기
  • 적극적으로 리뷰하기
  • 논리적인 근거로 리뷰하기
  • 도구 선정
  • 작게 자주 올리는 것이 좋다
  • 리뷰 정책
  • 컨벤션 체크
  • 리뷰 언어
• Wrap-Up
• References

코드 리뷰

코드 리뷰란 사람의 검토를 통해 코드상의 잠재적인 결함을 찾고 개선해 나가는 과정이라 정의할 수 있겠습니다(참고로 요즘은 사람이 놓칠 수도 있고 또 사람이 굳이 검토할 필요가 없는 코드 컨벤션 성격의 결함들이 있으므로 리뷰 과정에 eslint 같은 linter를 통한 자동 검수가 들어가는 것이 추세입니다).

코드 리뷰에는 다양한 기법들이 존재하는데, 여기에서는 팀 리뷰(team review) 중 온라인 코드 리뷰에 초점을 맞춰서 온라인 코드 리뷰 수행 시 고려할 사항들에 관해 얘기해 보겠습니다.

아래부터는 코드를 작성하고 리뷰를 요청하는 사람(author)을 리뷰 요청자, 리뷰하는 사람(reviewer)을 리뷰어라 칭합니다.

코드 리뷰는 문화다

팀에서 코드 리뷰를 수행하기 위해 가장 먼저 해야 할 일은 코드 리뷰를 ‘문화’로 받아들이는 것으로 생각합니다.

문화란 무엇인가요?

최근에 읽은 <스페이스 크로니클>이란 책에서는 문화란 어떤 집단이 더는 관심을 두지 않는 일을 행하는 것이라고 합니다. 즉 ‘누구나 당연하게 생각하는 것’이죠.

예를 들어 이탈리아의 슈퍼마켓에 가면 파스타 코너가 굉장히 다양하고 커서 여행객들이 놀라는데 막상 현지인들의 반응은 이렇다네요.

“파스타 코너인데 당연하죠.”

마찬가지로 코드 리뷰도 개발팀이라면 당연히 해야 하는 활동이라는 점에서 팀의 문화에 속합니다.

“개발팀인데 코드 리뷰는 당연하죠.”

코드 리뷰에 대해 거부감을 보이는 개발자가 있을 수 있는데, 제 생각에는 이걸 설득해야 한다는 것 자체가 사실 낭비인 것 같습니다. 애초에 서로의 문화가 맞지 않는다는 뜻이므로 처음부터 팀에서 배제하거나 코드 리뷰 활동을 정식 지표(KPI)로 삼아 기본 업무로 포함하는 게 좋을 것입니다.

코드 리뷰의 효과

그렇다면 코드 리뷰는 왜 당연하게 해야 할까요?

사람이 해야 하는 일이니 일정한 시간과 노력이 들게 되는 데 그만큼의 장점이 있어야 할 겁니다. 코드 리뷰로 얻을 수 있는 효과를 세 가지로 정리해 보겠습니다.

빨리 가려면 혼자 가고, 멀리 가려면 함께 가라

리뷰를 거치면 리뷰어의 확인을 기다려야 하므로 리뷰를 거치지 않을 때보다 코드 머지가 늦어집니다. 그래서 일견 개발 속도가 느려지는 것처럼 보이는데 코드 품질이 좋아지기 때문에 장기적으로는 개발 시간이 감소하고 배포 속도는 빨라진다고 볼 수 있습니다.

리뷰 요청자가 놓친 부분을 리뷰어가 짚어줄 수 있고,

• 다른 모듈과의 인터페이스적인 측면 (“이렇게 수정하면 제 모듈이랑 깨질 것 같은데요.”)

• 더 간단한 알고리즘 구현 (“바퀴를 재발명할 필요가 있을까요? 하물며 버그까지 있을 것 같은데요.”)

    function getLast(project) {
        if (project && project.analyses && project.analyses.length >= 0) {
            return project.analyses[project.analyses.length - 1];
        }

        return null;
    }

project.analyses[-1]인 경우가 생길 것 같네요.

_.last(project.analyses)를 쓰면 깔끔하지 않을까요?

리뷰 요청자 입장에서는 리뷰를 받아야 하므로 아무래도 좀 더 신경을 쓰게 되죠.

• 주석은 충분한가?

• 로직 흐름은 명확한가?

• 라이브러리를 추가했는데 라이센스 파일은 챙겼나?

• 우리 팀장님은 같은 로직 두 번 쓰는 거 완전 싫어하는데 리팩토링해야 하나? 다른 데 이미 있나 찾아보자.

팀 리뷰를 거쳐서 머지되었다는 심리적 안정감도 있죠, 나 혼자만의 잘못이 아니라는. (응?)

하지만 리뷰가 비동기로 진행되므로 리뷰 요청자가 프로세스를 진행하지 못하는 경우가 생길 수 있습니다. 로컬에서 feature 브랜치로 따로 작업하고 Git rebase를 활용해서 리뷰된 커밋과 머지하면 굳이 리뷰 때문에 작업이 중단될 일은 없다고 생각하지만 실제로는 데모 일정의 압박, 브랜치 관리의 귀찮음, 일을 순서대로 처리하고 싶은 강박(“이 커밋 깔끔하게 머지시킨 후에 다른 일 하자”)이 있어서 리뷰가 병목이 되기도 합니다.

인간에게는 모두의 장점이 있다

리뷰를 해 보니 이게 결국 사람의 일이라 리뷰어 각자의 특징이 드러납니다. :)

가령 어떤 리뷰어는 컨벤션을 중점 체크하고, 어떤 리뷰어는 Promise 객체의 잘못된 사용을 잘 짚어 내며, 어떤 리뷰어는 웬 듣지도 못한 유용한 라이브러리를 불쑥 추천합니다. 중요하게 생각하는 그리고 코드를 볼 때 잘 걸러지는 부분이 각자 다르다는 것이죠.

그만큼 다양한 방향에서 코드가 검토되니 코드가 계속 개선됩니다.

제 장점은 뭐였을까요? 저는 아무래도 제품 관리의 역할이 있다 보니 UI나 사용자에게 보이는 레이블, 에러 메시지에 대해 주로 리뷰했었습니다. 에러 메시지는 4H를 담고 있어야 하는데 이건 그냥 구색만 맞춘 개발자 영어잖아요!

조직은 개발자의 성장을 바란다

개발자의 성장도 코드 리뷰나 페어 프로그래밍(pair programming)의 효과 중 하나인 것 같습니다.

교육, 학회나 세미나 참여 등 성장의 기회에 대한 얘기를 보통 많이 하는데 코드 리뷰를 통해 다양한 리뷰어의 지식을 공유 받는 것, 다른 의견을 듣고 논리에 기반을 둔 토론으로 더 나은 결정을 끌어내는 경험을 쌓는 것, 같은 분야를 고민하는 사람들에게 받는 피드백을 통해 시간을 절약하는 것도 개발 조직이 제공할 수 있는 좋은 성장 기회라고 생각합니다.

자신보다 더 낫거나 경험이 많은 개발자로부터 바로 배울 수 있고, 그렇게 코드에 대해 열린 마음으로 일하는 습관과 경험이 오픈 소스 기여의 시작이기도 할 테니까요.

코드 리뷰하기

코드 리뷰는 팀 활동이기 때문에 코드 리뷰를 수행하기에 앞서 팀원 전체가 동의하는 리뷰 규칙을 세우고 진행하는 것이 좋습니다. 리뷰 규칙을 정할 때 고려할 것들에 대해 알아봅니다.

겸손하고 정중하게 리뷰하기

이런 규칙을 정하고도 실제로 진행해 보면 결국 사람에게는 감정이라는 게 있어서 리뷰가 계속되고 오랫동안 머지되지 않으면(pending) 아무래도 심적으로 불편해지기 마련입니다. 어떤 사소한 꼬투리로 촉발되어 감정 소모적인 댓글들이 달리기도 하고요(이런 경우 감정이 없는 도구로써 컨벤션 체크를 자동화하거나 중재자가 리뷰어/요청자를 중재해서 불을 끌 필요가 있습니다).

그래서 더욱 공격적이지 않은 겸손한 태도를 가지는 것이 중요합니다. 상대방을 존중하면서 ‘이렇게 해보면 어떨까요’, ‘이런 문제가 생기지 않을까요’라는 식으로 리뷰가 이루어져야 합니다.

아무래도 감정적인 사람에 의해 진행되는 활동이다 보니 말이 참 아 다르고 어 다르더라고요. 가령 라이브러리를 사용하자고 아래와 같이 코멘트를 달았는데,

그리고 url을 다룰 때는 항상 URIjs 모듈을 이용하세요.

요청자의 피드백이 다음과 같았습니다.

URIjs가 언제 들어왔나요? 사용하는 곳이 리뷰어의 모듈뿐이군요.

코멘트를 단정적이지 않게 아래처럼 했으면 요청자 피드백이 더 긍정적이지 않았을까 하는 생각이 듭니다.

그리고 url을 다룰 때는 URIjs 모듈을 이용하시면 좋을 것 같습니다. 가령 이 코드는 URIjs를 사용하면 아래처럼 쓸 수 있습니다.

또, 후배 개발자가 선배 개발자의 코드에 대해 말하는 것을 어려워할 수 있는데 이것 역시 모두가 동등한 리뷰어이자 리뷰 요청자라는 인식을 하고 시작되어야 합니다. 팀장 시절에 리뷰 코멘트를 수십 개 받고 ‘이것들이..’ 했던 기억이 납니다만^^; 지금은 그렇게 적극적으로 리뷰에 참여해줬던 팀원들에 대한 고마움과 팀 문화에 대한 자부심이 더 크네요.

비자아적 프로그래밍(Egoless Programming)

• 당신이 실수하리라는 것을 받아들여라.
  
• 당신이 만든 코드는 당신이 아니다.
  
• 당신이 얼마나 많이 알고 있다고 해도, 항상 누군가는 더 많은 것을 알고 있다.
  
• 권위는 지위가 아니라 지식으로부터 나온다.
  
• 자신보다 많이 알지 못하는 사람이라 해도 존경과 인내로 대하라.
  
• 사람이 아니라 코드 그 자체를 비판하라.

적극적으로 리뷰하기

리뷰는 어려운 것으로 생각하는 개발자들도 있는데, 아래처럼 가볍게 생각하는 것이 좋습니다.

1. 코드를 읽다가 뭔가 맘에 걸린다, 찜찜하다.
2. 참지 말고 거리낌 없이 이야기한다.

그래야 다양한 의견과 아이디어가 나오면서 코드가 개선되고 지식이 공유됩니다. 저도 “아 이거 쫌스러워 보일 텐데” 싶어 드래프트를 버린 경우도 꽤 있지만 대부분은 그냥 드르륵 적는데요, 요청자가 선별해서 잘 받아들였던 것 같습니다.

논리적인 근거로 리뷰하기

적극적으로 하면서도 자신의 리뷰에 대해 논리적인 근거나 참조 레퍼런스가 있으면 더 좋습니다. 즉, 아래처럼 되는 거죠.

1. 코드를 읽다가 뭔가 맘에 걸린다, 찜찜하다.
2. 왜 찜찜하지?
3. 내가 생각한 게 맞는지 찾아보고 돌려보고 증명한다.
4. 참지 말고 거리낌 없이 이야기한다.

내가 알던 것이 맞는지 이렇게 코멘트를 남겨도 되는지 한 번 더 생각해 보는 과정에서 리뷰어도 자신의 지식을 점검하는 계기가 됩니다. Q&A 관계에서 질문하는 사람보다 답변하는 사람이 더 많이 배우게 된다는 그런 거죠.

특히 웹 개발에 도움을 주는 온라인 서비스가 많으므로 이를 활용하는 것도 좋습니다.

httpbin.org나 RequestBin과 같이 웹 서버를 따로 구동하지 않아도 HTTP 클라이언트의 테스트가 가능한 서비스:

JSFiddle이나 Plunker와 같이 HTML/CSS/JavaScript 코드를 실행할 수 있는 서비스:

도구 선정

코드 리뷰를 위한 도구나 서비스는 매우 다양합니다.

• Gerrit이나 Phabricator 같은 오픈 소스 도구

• 상용 도구

  Collaborator나 Crucible 같은 상용 도구입니다. 커밋 없이 리뷰할 수 있는 pre-review, IDE 연동, 자사 제품과의 연동(가령 Crucible은 Atlassian 사의 제품이어서 JIRA와 잘 연동된다고 합니다) 등 많은 편의 기능을 제공합니다.

• GitHub의 Pull Request

  GitHub의 Pull Request(이하 PR)를 코드 리뷰 도구로 활용하는 방법입니다. 코드를 바로 머지하는 것이 아니라 fork 된 리파지토리 혹은 해당 리파지토리의 별도 브랜치에서 PR을 생성하고 이를 리뷰한 후에 머지합니다.

  GitHub과 연계된 reviewable.io나 ReviewNinja 같은 코드 리뷰 서비스도 있고요.

제 경우에는 Git과 Gerrit이 전사 및 프로젝트 표준이어서 고민 없이 Gerrit을 통해 리뷰를 진행했는데 각 팀의 상황에 맞게 선정하시면 되겠습니다.

작게 자주 올리는 것이 좋다

<클린 코드>에서 말하는 좋은 코드의 조건이 있습니다.

코드는 작을수록 좋다. 함수나 클래스는 한 가지 일만 잘해야 한다.

이와 마찬가지로 좋은 리뷰 요청의 조건은 작은 커밋을 자주 하는 것입니다.

Frequent feedback on small ships

대량의 커밋을 한번 딱 올리지 말고 작은 커밋을 자주 올려야 리뷰어가 편하게 집중적으로 리뷰하고 피드백을 자주 줄 수 있다는 것이죠.

아래는 팀에서 실제로 올라왔던 커밋과 그에 달린 리뷰입니다.

하나로 커밋된 너무 많은 변화

용감한 리뷰어들

+3720, -2374 코드!!! 도전!!!

우려하던 대로 거의 리뷰가 불가능한 수준의 커밋인데 리뷰가 됐네요. 최소한 파일 이름 변경은 따로 커밋되어야 했고 다른 부분들도 충분히 쪼개질 수 있을 것 같은데 한꺼번에 올라왔네요. 다음부터는 이런 커밋은 깔끔하게 -2 하겠습니다.

저는 솔직히 리뷰를 포기했는데 팀원들이 그 어려운 걸 해냈네요. ㅠ

위 커밋의 경우 기능 추가, 로직 변경, 파일 이름 변경이 다 섞여서 올라왔었는데 별개 커밋으로 올렸어야 합니다. 실제로 커밋 목적과 관계없다면 스타일이나 포맷 변경도 나중에 code cleanup 같은 별도 목적의 커밋으로 올리는 것이 좋은 습관일 것입니다. 물론 리뷰어도 요청자가 수정한 내용에 대해서만 리뷰하는 것이 좋습니다, 다른 부분의 버그나 스타일을 고치고 싶더라도 말이죠.

대안들

• 코드를 다 수정한 후에 커밋을 올릴 것이 아니라 일정 부분 수정할 때마다 커밋(patch set)을 올린다

  완료된 최종 코드를 한번 커밋하는 것이 아니고 일정 부분 수정된 코드를 그날그날의 커밋으로 올립니다. 완료된 상태가 아니므로 리뷰를 요청하지(리뷰어를 추가하지) 않아도, 리뷰어가 시간 날 때 그나마 작은 단위로 변경을 확인할 수 있습니다.

  명시적인 리뷰 요청이 없어도 리뷰어가 코드 커밋 상황을 알 수 있기 때문인데요, Gerrit의 경우 Watched Projects에 관심 있는 프로젝트를 등록하면 커밋에 대한 메일을 받을 수 있고 또 Slack과 CI를 연동해서 Slack을 통해 커밋 상황을 메시지로 받을 수도 있습니다.

Watched Projects

Bot message in the channel

이 글에서도 PR을 먼저 올리고 “WIP”와 “DONE”이라는 레이블을 추가하도록 해서 중간중간 점진적인 리뷰로 부담이 줄었다고 합니다.

• 오프라인 리뷰의 요청

  대량의 코드를 커밋할 수밖에 없을 때는 요청자가 오프라인 리뷰를 진행하는 것이 좋습니다. 오프라인 리뷰를 통해 코드 설계 목적, 주요 실행 흐름 등을 설명한 이후에 온라인 리뷰를 진행하면 리뷰가 수월할 수 있습니다.

  특히 어떤 주요 기능의 최초 커밋일 경우 대량일 확률이 높아 이때는 오프라인으로 진행하는 것을 권장했습니다. 오프라인 진행 시 해당 모듈 개발자가 발표자(Author)가 되어 다음의 일들을 수행합니다:

  • 발표할 내용을 간략하게 요약하여 리뷰 페이지 생성
  • 리뷰 진행
  • 작성 중인 코드를 IDE, vi 등을 통해 직접 리뷰
    • 동료들의 이해를 위해 code folding 혹은 다이어그램 등을 활용해 주요 실행 흐름을 우선 설명한다.
    • 그 이후 각 함수에 대한 세부 사항을 얘기한다.
  • 리뷰/피드백에 따른 액션 아이템 정리 및 수행/할당(JIRA 이슈 등록, 위키 정리 등)

  오프라인 리뷰는 준비, 참석, 수정 등 시간이 생각 외로 많이 걸려서(준비에 하루, 리뷰에서 도출된 개선 아이템 수정에 일주일 정도 걸렸던 적도..) 자주 수행되지는 못했는데, 정기적으로 하기보다는 스프린트 리뷰 이후 여분의 시간(slack day라고 불렀던) 등을 활용해 부정기 진행하는 게 현실적인 것 같습니다.

그래도 역시 리뷰 요청자가 “Frequent feedback on small ships” 원칙을 항상 새기고 커밋을 잘게 쪼개서 올리는 것이 중요합니다. 리뷰어와 그리고 미래의 나를 위해.

리뷰 정책

머지(merge 혹은 submit), 리뷰 시간 등의 정책을 합의해서 팀의 규칙으로 정할 필요가 있습니다.

역할

• 리뷰 마스터(최종 점수를 주고 머지할 수 있는 사람)를 정한다.
• 리뷰 코멘트에 대한 최종 수정 결정은 요청자가 한다.
• 나는 언제든 리뷰어이거나 리뷰 요청자가 될 수 있으므로 상대방 관점에서 생각하자.
  • 리뷰 요청자: 리뷰어가 쉽게 리뷰할 수 있도록 작은 커밋을 올리려고 노력한다. 리뷰 요청 전에 리뷰 시스템에서 자신이 커밋한 코드의 diff를 확인한다. 자신의 에디터에서는 보이지 않았던 문제- 가령 불필요한 공백 추가, 모듈의 require 순서, if (true) 같이 테스트를 위해 임의로 삽입한 코드 등 -가 보일 것이다.
  • 리뷰어: 리뷰 요청자가 변경한 부분에 대해서만 리뷰하고 무리한 강요를 하지 않는다.

  리뷰어의 의도 표시
  정식 규칙은 아니었고 팀 내에서 암묵적으로 사용된 것인데, 코멘트를 달 때 리뷰어의 의도나 의지를 표시하는 다음 말들을 붙이는 것도 좋습니다.
  

  • (옵션): 꼭 수정할 필요는 없고 코멘트를 참고만 하세요
    
  • (향후): 이번 커밋에는 수정하지 말고 다음 커밋이나 향후 리팩토링할 때 참고하세요
    
  • (궁금): 꼭 코드와 관련된 것이 아니지만 궁금한 것이 있습니다
    고쳐야 한다는 강제가 없어 요청자/리뷰어 모두의 부담이 덜어 지면서 활발한 리뷰에 도움이 되었던 것 같습니다.

리뷰 시간

• 리뷰어는 코드 커밋 후 1일 이내에 리뷰한다. 리뷰 요청자는 기한 내에 리뷰되지 않으면 Slack을 통해 리뷰를 요청할 수 있다. 바쁜데 리뷰 요청해서 미안한 것이 아닌 리뷰 늦게 해 줘서 미안하게 되는 문화를 만들자.
• 온라인으로 커뮤니케이션이 여의치 않으면 온라인에서 길게 논쟁하지 말고 리뷰 마스터나 PL이 중재하여 당사자들이 문제를 해결할 수 있게 도와준다.

리뷰 점수 및 머지 조건

• 리뷰어가 X명 추가되어야 하며, 몇 점 이상을 획득해야 머지된다.
  • 예를 들어, +2 이상일 때 머지 가능하도록 Gerrit을 설정하고 2명 이상의 리뷰 & 합계 2점을 머지 조건으로 채택
  • 새로운 패치 셋이 올라오면 이전에 획득한 점수를 무효로 할 것인지 고려할 것인지
• 명확한 버그나 수정 사항이 발견되어 머지할 수 없는 코드로 판단되면 -1점을 준다.
• jshint 자동 체크에서 실패하면 -1을 획득하며 리뷰어는 리뷰하지 않아도 된다.
• 모든 코드를 리뷰할 필요는 없고 모든 코드가 머지 조건을 만족할 필요도 없다.
  • 단순한 UI 문자열 변경, 아이콘 파일 등의 리소스 추가, 버그 수정(hot fix) 등은 리뷰 마스터가 바로 머지할 수 있다.
  • 실험적이거나 버려질 코드. 예를 들어, REST 서비스를 제공하는 서버가 있었는데 실시간 동작을 위해 곧 웹소켓으로 변경 예정이었습니다. 이 시점에 REST로 인터페이스 하는 클라이언트 코드의 리팩토링 커밋이 올라왔었는데, 굳이 리뷰할 필요가 없다는 의견이었죠.

  단, 리뷰를 느슨하게 하는 이 경우에도 컨벤션은 따라야 합니다. 별도 브랜치에서 실험적으로 진행하는 코드인데 굳이 따라야 하냐는 반박이 다른 팀에서 있었다고 하던데, 컨벤션은 따르라고 만든 것이기 때문에 전 솔직히 논쟁거리도 안 되는 문제라고 생각합니다.

• 코드 수정에 대한 최종 결정은 리뷰 요청자가 한다.
• 리뷰 요청자는 커밋 코드의 동작을 본인 로컬 환경이나 개발 서버에서 확인한 후 Verified 점수 1점을 주는 것이 좋다. 패치 셋(patch set)을 올리다 보면 개발자가 확인하지 않는 경우도 많고 컴파일 에러가 없는 JavaScript 특성상 오류를 알기가 어렵더군요.

또, 모듈별 단위 테스트에 실패하면 리뷰하지 않는다 혹은 단위 테스트가 같이 올라오지 않으면 리뷰하지 않는다는 규칙을 세울 수 있습니다. 해보고 싶었는데 빡빡한 일정을 이유로 실행하지 못했었네요.

리뷰 내용

• 온라인으로 커뮤니케이션이 여의치 않으면 말이 글보다 효율적이므로 상대방의 자리로 가서 물어본다. 단, 어떤 합의를 했는지 코멘트로 남겨 두는 것이 좋다.

• 리뷰어는 코드 내용이 잘 이해되지 않을 때 리뷰 요청자에게 찾아가 코드에 대한 설명을 듣는다. 이후 개발자가 이 설명을 주석으로 추가해 패치 셋을 올리는 것이 향후 유지보수에 도움이 된다.

  • 
  • 

• 주석 처리할 때는 TODO, FIXME 같은 레이블을 추가하는 게 좋다.

  • 

• 외부에서 가져온 오픈 소스를 수정할 때는 버그에 관계된 것만 고친다.

  버그 수정을 위해 외부에서 가져온(upstream) 오픈 소스를 고칠 때는 해당 버그에 관계된 부분만 고치는 것이 좋습니다. 간혹 버그와 관계없는 수정을 하는 경우(포맷 맞추기)가 있는데, 커밋과도 관계가 없고 오픈 소스에 패치를 보낼 때도 문제 될 수 있으므로 지양해야 합니다.

  버그 fix와 관계된 것이 아니라면 굳이 포맷팅/jshint 등을 위해서 원 upstream과 다르게 수정할 필요는 없을 것 같네요.

  기왕 한 거 다시 돌리기도 그렇네요. 큰 문제 없을 것으로 보입니다.

  jshint는 무시하시고, bugfix와 관련 없는 코드는 수고스럽더라도 되돌려놓는 게 좋겠습니다. upstream에 bug patch를 보낼 때도 문제가 될 것입니다.

  Done

컨벤션 체크

리뷰어가 컨벤션(code convention)을 지적하는 경우가 있는데 그렇게 좋은 신호는 아닙니다.

• 리뷰어가 전반적인 로직/기능을 이해하지 못해서 컨벤션이나 일반적인 로직 리뷰만 함

• 명확히 수립된 컨벤션이 팀 내에 전파되지 않음

• 컨벤션에 없는 스타일에 대한 지적은 매우 민감한 문제. 가이드에 있다면 따라야 하지만, 그렇지 않은 경우는 개발자마다 스타일이 다르므로 코멘트하지 않는 게 좋습니다. 정 스타일을 용납(?)할 수 없으면 공론화해서 가이드에 포함해야 합니다. Promise-then의 indentation이 굉장히 민감하게 대두한 기억이 나네요^^;

function f() {
    function inner() {
        return getPerson1()
        .then(getPerson2)
        .then(getPerson3);
    }
}

function f() {
    function inner() {
        return getPerson1()
                   .then(getPerson2)
                   .then(getPerson3);
    }
}

사람에 의한 컨벤션 리뷰는 감정이 상하기 쉽고 주요 로직 리뷰의 시간을 빼앗으므로 기계가 하도록 자동화하는 게 좋습니다.

1. 개발자가 Gerrit에 코드를 커밋한다.
2. 코드 커밋 상황이 트리거(Gerrit Trigger)가 되어 CI에서 빌드가 수행된다.
3. 빌드에서 jshint나 eslint에 의한 체크를 수행한다.
4. 체크 결과를 Gerrit의 코멘트로 등록한다. (Gerrit REST API 이용)

Gerrit comment for convention checking

Details for convention checking

컨벤션을 초기에 명확하게 수립하고 다듬어 가면서 해당 규칙을 linter 같은 도구의 룰로 만들고 코드 커밋 시마다 체크하도록 하는 것이 좋습니다.

자동화하면 사람이 할 때처럼 놓치는 것도 없고 사람한테 지적당했을 때의 감정 상함이 없으며 리뷰어들의 시간을 아끼게 됩니다.

• 도도와 파이썬: 좋은 선택과 나쁜 선택
• Git: Pull Request 가 들어오면 젠킨스로 테스트 돌리기
• How to integrate Eslint with jenkins?

리뷰 언어

리뷰 코멘트(comment) 작성을 한글로 할지 영어로 할지도 미리 정하는 것이 좋습니다.

제 경우에는 커밋 메시지, 코드 주석은 영어로 하고 리뷰 코멘트는 한글로 하는 것을 선호합니다. 물론 리뷰 대상 코드가 외국 개발자와 협업 중인 것이라면 당연히 영어로 코멘트를 달아야겠지만, 그 외의 경우에는 한글로 작성하는 것이 좋다고 생각합니다.

내부 개발에서 한글 코멘트를 사용하다가 영어로의 전환 얘기가 실제로 나왔었을 때 개발자들 사이에서 오갔던 의견들을 정리해 보았습니다.

• 영어 코멘트 선호
  • 해외 개발자 협업이나 추후 오픈 소스화 가능성을 생각한다면 영문으로 작성해야 한다.
• 한글 코멘트 선호
  • 한글 코멘트를 허용해보니 코멘트의 질이 달라지더라. 영어만 허용했을 때는 영어로 간단히 작성하기 어려운 내용의 경우 그냥 오프라인에서 직접 물어보게 되고 이력이 남지 않았는데 한글 허용이 되니 구체적으로 상세히 적을 수 있게 되어 온라인으로 의사소통이 잘 되고 이력도 잘 남아 다른 사람이 보고 참고하기에도 좋았다.
  • 더 편하게 코멘트를 달게 되면서 개발자들에게 긍정적인 영향(리뷰의 선순환)을 미치고 있어 보인다.
  • 개발자들의 영어 실력이 천차만별이라 영어만 허용 시 추가되는 비용이 생각보다 크다. 개발자들이 리뷰 코멘트나 답변을 영문으로 쓰면 문맥을 의도한 대로 전달하기 어렵고 머릿속에서 번역해서 써야 하므로 리뷰 시간이 더 걸린다.
  • 향후 오픈 소스가 되더라도 외부 개발자들이 커밋 로그를 참고하지 예전의 리뷰 코멘트까지 보지는 않을 것이다.

한글로 계속 작성해 오던 개발자들의 관성일 수도 있겠지만^^; 개발자들이 코드에 대해 편하게 얘기할 수 있다는 장점이 큰 것 같습니다.

아 그리고 커밋 메시지는 제목과 내용을 통해 무엇을 수정하기 위한 커밋인지 잘 드러내야 합니다. 바람직한 커밋 메시지에 대해서는 이 글을 참고하세요.

Wrap-Up

1년여간 코드 리뷰를 하면서 느꼈던 코드 리뷰 문화에 대해 정리해 보았습니다.

여러분은 어떻게 리뷰하고 계신가요? 코드 리뷰는 ‘문화’이기 때문에 동료 개발자들을 존중하면서 시행착오도 거쳐 가며 팀 상황에 맞는 절차와 방법을 찾아 자연스럽게 정착시켜가야 하겠습니다.

이 글에도 있지만, 코드 리뷰를 배우는 가장 좋은 방법은 코드 리뷰를 잘하는 사람에게 리뷰를 받는 것이더라고요. 저도 팀원들의 다양한 리뷰를 받고 보고 또 동료의 코드를 리뷰하면서 많이 배웠고 큰 기술 기업에서나 하는 거라고 어렵게 생각했던 코드 리뷰에 대한 두려움이 없어진 것 같습니다.

다음에는 코드 리뷰 동안에 수집된 JavaScript의 코드 개선 사례와 팁을 공유하겠습니다.

Code wins arguments!

References

• 드라마의 Pair Programming과 Code Review 도입 후기
• 카카오스토리 팀의 코드 리뷰 도입 사례 - 코드 리뷰, 어디까지 해봤니?
• 코드 리뷰는 어떻게 하나요?
• GitHub의 Pull Request를 통한 코드리뷰
• Pull Request를 이용한 개발 흐름을 적용해 보고 나서…

Related Posts

• JavaScript 코드 리뷰 - 코드 리뷰 사례로 알아보는 좋은 JavaScript 코드