코드리뷰에 대하여

우리 개발팀에는 DoR, DoD,DoP 라는 것이 있다. 이전에는 겪어본적이 없는것들이다. 나중에 따로 정리를 하겠지만, 우선은 코드리뷰에 대해 정리를 해보려고 한다. 

이전 회사에서도 코드리뷰가 있었다. 처음부터 있었던건 아니고, 나중에 생겼다. 하지만 반드시 reviewer 가 accept 해야 commit 이 되는것도 일부 모듈에 대해서만 적용되었기 때문에 모든 code commit  에 대해 approval  이 필요하지 않았다. 때로는 빨리 검증 받고 배포해야 하는데 review 안되서 이게 걸림돌이라는 느낌이 강했고, 어떤 경우엔 담당자가 나 혼자여서 review 를 해줄 사람이 없었다. 모듈별로 주담당-백업이 나뉘어 있었는데, 백업 모듈에 대해 review 를 하기엔 잘 알지 못하고, 주담당인 모듈에 대해선 누군가 review  를 해줘야하는데 오래 걸리니 걸림돌이 되고. 백업이 없는 경우는 review 가 없는 상황. code reivew  에 대한 인식이 없다가 생긴것은 좋았으나, 그것이 개발에서 꼭 필요한 일부분이라기 보단 형식적인 통과의례로서 병목처럼 여겨졌다.

여기서는 아니다. review 를 받지 못하면 merge 되지 않는다. 우리 팀은 이제 7명. 옆 팀은 더 많다. 한 9명 되나? frontenf, backend 가 한 팀에 있고,  내 모듈, 네 모듈의 개념이 아닌, 우리 팀의 모듈이다. 그래서 특별히 어려운 티켓을 제외하면 누구나 develop, review, test 를 할 수 있게 되어있다. 

reviewer  를 찾는 문화도 있다. daily  에서, 혹은 팀 채널에서 티켓 개발이 끝났으니 reviewer  를 찾고 있다, 라든지, 지금 할거 없는데  도움 필요한 사람? 이런식으로 나서기도 한다. 

review 도 대충하지 않는다. 철저하다. 여러번 퇴짜를 맞았다. 나도 마찬가지로 적극적으로 의견을 내려고 한다. 아무리 생각해서 짠 코드고,  senior 의 코드라도. 인간이라면 실수가 있기 마련이고, 왠만한 복잡도를 가진 ticket  에는 적어도 한개의  argue 는 있기 마련이다. 그런데  review  를 했는데 아무것도 나온게 없다면, 정말 완벽하거나, 내가 review 를 할만큼 실력이 안된다는 말이다. 

찾다가, 어딘가에서, google 의  code review 에 관한 글이 있어 읽으면서 정리를 한다. 회사 내에 review 에 관한 정리는 없는것같아, 잘 다듬어서 회사 Confluence 페이지에도 올리면 좋을것 같다.

1.코드리뷰 표준

• 코드리뷰의 주된 목적은, 시간에 따라 codebase 가 점점 더 발전하는 것을 확실시 하는것이다.
• developer must be able to make progress, otherwise there's no improvement
• code reviewer make sure that each commit doesn't decrease the quality of codebase as time goes on
• 이런 경우는, 팀이 시간에 쫓기거나, 목표를 위해 지름길을 선택해야 하는 경우 발생한다.
•  code reviewer 는 그들이 review 하는 code 에 대해 책임과 ownership 이 따른다. 
• 일반저으로, reviewer 는 code 를 approve 해야한다, 언제 해야하냐면, code commit 이 완벽하지 않더라도,  codebase health 를 definitely improve 한다면 말이다.
• perfect code 는 없다. better code 가 있을 뿐이다.
• 따라서 우리가 추구해야 할 것은 continuous improvement 이다.
• 기술적 사실과 데이터가, 의견과 선호도 위에 존재한다.
• conflict 의 첫번째 해결 방법은, developer 와 reviewer 가 consensus 를 이루는 것이다.
• 이뤄진 consensus 는 comment 로 적어두는것이 future readers 에게 도움이 된다. 
• comment 가 안되면, face-to-face, 혹은  video call 을 통해 도달한다.
• 그래도 안되면, 가장 흔한 다음 방법은 escalation이다.
• Team discussion, Tech leader involving,  asking maintainer/ Engineering Manager 가 있겠다.
• 당사자간 agreement  에 도달하지 못한다고 해서 commit/MR  이 방치되어서는 안된다.

2. 코드리뷰 속도

• 기본적으로 코드리뷰 속도는 빨라야 한다.  개인의 개발속도보다 팀으로서 개발속도가 더 중요한데, 코드리뷰는 팀의 개발속도에 영향을 미치기 때문이다.
• 코드리뷰가 느려지면 팀 개발속도가 당연히 저하된다.
• 코드리뷰 프로세스가 늦어지면 개발자들은 코드리뷰에 저항하게 된다. 
• 늦어질수록 당연히 MR 에 압박을 느끼고, 이는 코드 health 에 영향을 끼친다.
• 이프로세스때문에  refactoring, clean code 등에 대한 의지가 저하된다.
• 얼마나 빨라야 하는가?
• 코드리뷰 요청이 오자마자 바로 하는것이 이상적이다.
• 가장 오래 걸리더라도 최대 하루안에 마무리 될 수 있어야 된다.
• 적당한 사이즈의 경우 하루 안에 여러번 핑퐁을 칠 수 있다.
• Interrupt 에 대한 생각
• 코드리뷰가 당연히 중요하지만, 개발하던걸 멈추고 할만큼인가는 고려해봐야한다.
• 개발하던걸 멈추고 리뷰 후 다시 개발에 집중하는데 걸리는 context switching  이 매우 크다.
• 티켓 개발 완료후, 미팅 후, 점심 식사 후 등에 하는것이 좋다.
• 빠른 대응의 중요성
• 코드리뷰도 두사람 이상의 핑퐁이다 보니, 코드 리뷰 전체가 완료되는 속도 보다는 서로간 개개인이 응답을 빨리빨리 하는것이 더 중요하다.
• 이는 전체 프로세스가 오래걸리더라고 개발자를 좌절하지 않도록 도와준다.
• Cross Time Zone
• 서로 시간대가 다른 경우, 둘다 online 인 시간대에 하는것이 당연히 좋다.
• 그렇지 않다면 상대가 다음날 출근시 확인 할 수있도록 그전에 마무리 하는게 좋다.
• LGTM (looks good to me)
• 개발자가 남은 리뷰 항목들을 resolve 할것이라고 확실하거나, minor  해서 꼭 해당 개발자가 해야될 필요가 없는 경우, 속도를 높이기 위해 아직 남은 threads 가 있더라도  LGTM/Approval 을 할 수 있다.
• Large CL
• 양이 너무 많아 혼자  다 할수 없을것 같은 경우, 일단 쪼개서 작은 MR 로 나눠줄 수 없는지 물어본다. 그게 안된다 하더라도, comment  를 남겨서, 개발자가 block 되지 않도록 한다.
• 기타
• 이런 과정이 반복되면, 코드리뷰의 주기가 점점 더 빨라 질 수 있다. 하지만 중요한것은 코드리뷰 기준을 지키는 것이다.
• 응급상황에는 물론 코드리뷰를 패스 할 수 있지만, 응급상황에 대한 정의가 확실히 있어야 한다.

3. 코드리뷰 커멘트는 어떻게 써야하는가

• 친절히, 사유를 논리적으로 적되, 직접적인 수정 방향과 개발자에게 맞기는것에 밸런스를 맞추고, 설명을 듣기보다는 코드를 clean 하게 바꾸거나, comment 를 달아서 이후 다른 개발자들도 알 수 있도록 한다.
• 개발자에 대한 피드백이 아니라, 코드 자체에 대한 피드백이어야한다.

• Bad: "Why did you use threads here when there's obviously no benefit to be gained from concurrency?"

• Good: "The concurrency model here is adding complexity to the system without any actual performance benefit that I can see. Because there's no performance benefit, it's best for this code to be single-threaded instead of using multiple threads.

• 왜, 피드백을 주는지에 대해 설명을 해야한다. 

• fix 하는건 개발자의 역할이지만, 도움이 된다면 가이드를 주는것이 도움이 될 때도 있다. 직접 detail 한 가이드를 주는것과, 개발자에게 알아서 맞기는것 사이에 밸런스를 잘 이루어야 한다.

• 리뷰중 좋은 점 혹은 배울점을 발견한다면 칭찬같은 코멘트를 남기는것도 좋다. 물론 이유와 함께.

• 대개 리뷰어에게 개발자가 어떤 설명이 필요하다면, 그것은 코드가 더 clean 하게 다시 써져야 한다는것을 말한다. 혹은 comment 를 코드 사이에 달 필요가 있다. 그래야 다른 개발자들에게도 추후 전달된다.

4. Handling Pushback

• Who is right
• 일단은 개발자의 설명이 맞는지, code health 측면에서 말이 되는지 살펴본다. 하지만 개발자가 항상 옳지는 않으므로, 정확한 설명과 이유를 공손히 설명한다. 
• let the developer know that you hear what they're saying, you just don't agree.
• Cleaning up later
• 개발자들은 another round 를 하고싶지 않아서 나중에 하겠다고, clean up ticket 을 만들어서 하겠다고 하는 경우가 많지만, 이 MR  이 끝나자마자 처리하지 않으면, 결코 하지 않게 된다는것이 증명 되었다. 
• 이는 codebase 의 degeneration 을 불러온다
• 당장 해결될 수가 없다면 버그 ticket  을 만들고 그 개발자를 assign 하여 get lost 되지 않도록 한다. 

5. What to look

• make sure review every line
• look at the context
• focus on improving code health
• Context
• 단 4줄이 변경되었더라도, 전체적으로 보면 4개의 메소드로 쪼개져야 하는 변경일수도 있다.
• MR 자체를 as a whole 로 보고, 이 변경이 code health 를 발전 시키는지 확인해야한다. 그렇지 않으면 accept 해서는 안된다. 
• 대부분 시스템이 작은 변경이 더해지고 쌓여서 복잡해진다. 작은 복잡도의 증가도 경계해야한다.
• Every Line
• 임포트된 파일이나, 자동 생성된 파일등은 scan over 할 수 있지만, 인간이 작성한 코드는 모두 한줄한줄 다 확인하고 이해해야한다.  어느 부분을 더 자세히 보고 확인할지는 리뷰어에 달렸지만, 모든 라인을 다 읽고 확인해야한다.
• 만일 이해가 안되는 부분이 있으면 개발자에게 clean code, refactoring 등을 요청한다. 리뷰어에게 이해하기가 힘들면, 다른 개발자들에게도 마찬가지일 것이기 때문이다. 
• qualify 되지 않는다고 하면, qualify 된 다른 리뷰어를 포함시킨다.  security, concurrency,accessibility, internalization 등과 관련해서.
• Documentation
• 개발 변경된 부분관 연관된 문서에 변경이 있는지도 확인해야 한다. 
• Consistency, Style
• 기존에 유지하고 있는 style guide 를 따르는것이 원칙이다.
• 더 나은 변경이라면 argue 의 가치가 있다.
• style guide 자체에 대한 개선은 별도의  MR 을 통해 이루어져야한다.
• Comments
• 주석이 clear하고, 꼭 필요한지 확인한다.
• what 이 아닌 why  에 대한 것이어야 한다.
• 코드 자체가 explain enough 하지 않는건 comment 가 아닌 코드 자체를 수정해야 하는 경우가 대부분이다.
• Naming
• 모든 네이밍은 중요하다. 너무 길지 않아야하며, 충분히 그것에 대해 표현되어야 한다.
• Tests
• 변경사항에 대해 모든 적절한 test 가 있는지 확인해야한다.
• 테스트가 제대로 되었는지, 코드가 broken 일때 실제로 테스트가 fail 하는지, assert 는 올바른지 확인한다.
• 테스트도 복잡도가 관리되어야 하는 코드의 일부이다.
• Complexity
• 각각의 라인의 복잡도에 대해, 메소드 단위의 복잡도에 대해, 클래스 단위의 복잡도에 대해 살펴본다.  복잡하다는건, 코드를 읽었을때 빠르게 이해하기 힘들다는 것이다. 혹은 이 메소드를 쓰거나 수정할때 버그를 생산할 가능성이 높은 것이다.
• 한편으론 over-engineering  은 아닌지도 살펴야 한다. 현재 풀어야 하는 문제를 풀어야지, 미래에 풀어야할 문제까지 풀 필요는 없다. 무리한 일반화, 무리한 기능의 추가는 지양되어야한다.
• Functionality
• 티켓이 말한대로 기능이 개발 되었는지 확인한다. 개발자에게, 그리고 서비스 end user 에게 좋은 변경인지 확인한다.
• edge case, concurrency, user perspective 관점에서도 확인한다
• 버그는 없는지, 코드를 읽는 것으로 확인한다
• UI change 같이, 크리티컬 하지만 코드만으로 확인이 어려운 경우 직접 빌드해서 테스트하거나, 그게 어려우면 개발자에게 데모를 보여달라고 해서 확인한다
• 또하나 중요한 경우는 parallen programming 이 있는 경우이다. deadlock, race condition 같은 문제는 코드만으로, 혹은 단순히 돌려보는것 만으로 발견하기 매우 어렵다. 
• Design
• 제일 먼저 보아야 할것은 overal design 이다. 기존 code base 에 잘 부합하는지 각각의 코드 조각들이 상호 잘 작용하는지 등이다.

참조

https://github.com/google/eng-practices/tree/master/review/reviewer

https://chromium.googlesource.com/chromium/src/+/master/docs/cr_respect.md