Bad smell in code review

코드 리뷰 시간에 코드 owner가 수정 사항의 설명을 마치면서 자신의 수정이 완벽하지 않다며 다음과 같이 말하더군요.

하나의 클래스안에서 특정 함수를 그 클래스안의 특정 함수들만 사용하게 하고 싶다. 즉, 클래스 내부의 메소드 사이에도 private같은 것이 있으면 좋겠다.

Refactoring의 bad smell이 생각이 나더군요. 좀더 자세한 내용을 들어보니 SRP(Single Responsibility Principle)가 지켜지지 않은 클래스였습니다. (low-cohesion, high-coupling)

이 내용이 이번 코드 리뷰의 결과로 수정되지는 않았으나 refactoring list의 Extract Class가 적용되어야 할 경우였습니다.

제가 배운 오늘의 교훈은.

"왜 xx 기능이 내가 사용하는 언어에선 지원되지 않나?"라는 생각이 들면 먼저 설계에 문제가 없는지를 검토해 보자

입니다. :-)

--

저희가 코드 리뷰를 시작한지 몇개월 되지 않았는데 대부분의 reviewer들이 review할 코드를 읽지 않고 들어옵니다. 따라서 별로 능동적으로 참여하지 않게 되고 시간 낭비라는 생각에 코드 리뷰 시간을 싫어하죠. :-)

Wikipedia를 보니 line-by-line 코드 리뷰가 defect를 제거하는데 별로 효율적이지 않다는 말도 있네요. 하느냐 마느냐... 그것이 문제군요. :-)

부록(?)으로 Code Review Starter Kit 에 있는 내용입니다.

1. Everyone knows which code to review


2. Reviewers are familiar with requirements and design


3. Purpose of review is clear


4. Reviewers review all the code by themselves first


5. Reviewers record comments in writing


6. Code author hears and accepts comments in person


7. Code author tracks comments through to fix


8. Everyone saves time-stamped records to see how they’re doing