1000 개의 코드 리뷰를 통해 배운 점 (What I learned from doing 1000 code reviews)

링크드인(LinkedIn)에서 일하면서 내가 주로 한 일은 코드 리뷰를 하는 것이었다. 몇 가지 제안사항이 계속해서 제기 되어 왔기 때문에 팀과 공유 할 목록을 작성하기로 했다.
가장 일반적인 코드 리뷰 체크 사항은 3 가지이다 (+1 보너스).
presentation
제안 1: 무언가 이상하다면 예외(exception)를 던져라(throw)
내가 본 일반적인 패턴은 다음과 같다:
이 패턴은 실제로 내가 작업 한 모바일 앱 중 하나에서 방전을 야기했다. 우리가 사용했던 검색 백엔드에서 예외를 던지기(throwing exceptions) 시작했다. 그러나 앱의 API 서버에는 위와 비슷한 코드가 있었다. 따라서 앱의 관점에서 보자면 200 성공 응답을 받은 것이고 모든 검색어에 대해 빈 목록을 보여주게 된것이다.
만약에 이렇게 하는 대신에 API에서 예외를 발생시켰더라면 우리 모니터링 시스템에서 이를 즉시 잡아 냈을 것이고 수정되었을 것이다.
예외를 잡은(catch) 후에 빈 객체를 리턴하려는 경우가 많이 있다. Java에서 빈 객체의 예로는 Optional.empty(), null과 빈리스트(empty list)를 들수있다. 이 문제가 항상 발생하는 곳은 URL을 파싱 할 때다. URL에서 문자열을 파싱 할 수 없는 경우 null을 반환하는 대신 다음과 같이 질문해보자. "URL이 잘못된 이유가 뭐지? 이게 우리의 업스트림(upstream)에서 고쳐야 할 데이터 이슈인가? "
빈 객체는 이러한 것을 다루는 데 적합한 도구가 아니다. 뭔가 예외 상황이 발생 한다면 예외(exception)를 던져야(throw) 한다.
제안 2 : 가능한 가장 구체적인 타입(type)을 사용하라
이 제안은 기본적으로 stringly typed programming의 반대되는 것이다.
나는 빈번하게 이러한 예제와 같은 코드를 보게 된다:
가능한 가장 구체적인 타입을 사용하면 전체 클래스의 버그를 피할 수 있으며, 기본적으로 Java와 같이 강 타입 언어를 선택하는 이유이다.
여기서 질문은: 어떻게 선의의(well-intentioned) 프로그래머가 나쁜 stringly typed code를 작성하게 되는 걸까? 그 대답은: 외부에선 강한 타입을 사용하지 않기 때문이다. 문자열이 들어오는 위치는 다음과 같이 다양하다:
  • URL의 쿼리와 path 파라미터
  • JSON
  • enum 타입을 지원하지 않는 데이터베이스
  • 잘못 작성된 라이브러리
이러한 모든 경우에 다음과 같은 전략을 사용하여 이 문제를 피해야 한다: 문자열 파싱 및 직렬화(serialization)를 프로그램의 경계(fringe)에 유지하자. 다음은 예제이다:
이러한 접근 방법에는 많은 장점이 있다. 먼저, 잘못된 데이터가 바로 발견된다. 문제가 있으면 어플리케이션이 바로 에러를 내뱉는다(fails early). 또한 데이터의 유효성을 한 번 확인한 후에는 전체 어플리케이션에서 파싱 예외를 잡을 필요가 없다. 또한 강력한 타입을 사용하면 보다 서술적인(descriptive) 메서드를 표현 하게 된다; 모든 메소드에 많은 javadoc를 작성할 필요가 없게 된다.
제안 3: null 대신에 Optionals를 사용 하라
Java 8에서 제공하는 가장 좋은 기능 중 하나는 존재할 수도 있고 존재하지 않을 수도 있는 엔티티(entity)를 나타내는 Optional 클래스다.
사소한 질문: 자체 약어를 갖는 유일한 예외는 무엇일까? 답: NPE(Null Pointer Exception). 그것은 자바에서 가장 흔한 예외이며 10억 달러짜리 실수라고 한다.
Optional을 사용하면 프로그램에서 NPE를 완전히 제거 할 수 있다. 그러나 올바르게 사용해야한다. Optional 사용에 있어 몇 가지 조언이 있다:
  • 당신이 그것을 사용하기 위해서 Optional을 가질 때마다 단순히 .get()을 호출하면 안된다. 그 대신에 Optional이 없는 경우에 대해 신중하게 생각하고 기본값을 명시 해준다.
  • 합리적인 기본값을 아직 갖고 있지 않으면 .map().flatMap()과 같은 메서드를 사용하여 나중에 이 결정을 연기 할 수 있다.
  • 외부 라이브러리가 null을 리턴한다면 바로 즉시 Optional.ofNullable()을 사용하여 값을 래핑한다. 나를 믿어 봐라. 나중에 감사 해 할 것이다. 널(NULL)은 프로그램 내부에서 "버블 업(bubble up)"하는 경향이 있으므로 소스에서 멈추는 것이 가장 좋다.
  • 메서드의 리턴 타입으로 Optional을 사용한다. 이렇게 하면 값이 존재하지 않는 것이 가능한지 알아 내기 위해 javadoc을 읽을 필요가 없기 때문에 좋다.
보너스 제안: 가능한 한 메소드를 "Unlift" 하라
다음과 같은 메서드는 사용하지 않는 것이 좋다:
위에 예시로 든 모든 메서드에 공통점이 있을까? 그들은 컨테이너 객체인 Optional, List, 또는 Task를 메서드 파라미터로 사용하고 있다. 리턴 타입이 동일한 종류의 컨테이너 인 경우 (예: 하나의 param 메소드가 Optional을 취하여 Optional을 반환하는 경우) 더욱 심각한 문제가 생긴다.
왜일까? 1) Promise<A> method(Promise<B> param)은 아래의 경우보다 유연(flexible) 하지 못하다.
2) A method(B param).
Promise<B>가 있다면 1)을 사용하거나 2) .map을 사용하여 함수를 "리프팅(lifting)" 할 수 있다. (예: promise.map(method)).
그러나 B만 있는 경우 2)는 쉽게 사용할 수 있지만 1)은 사용할 수 없으므로 2)가 훨씬 더 유연하다고 볼 수 있다.
나는 이것을 "unlifting"이라고 부르고 싶다. 왜냐하면, 일반적인 유틸리티 메서드 인 "리프트 (lift)"의 반대이기 때문에 이것을 "unlifting"이라고 부른다. 이러한 방법을 적용하면 유연하고 사용하기 쉬운 메서드를 만들게 되고 호출자(caller)가 보다 쉽게 메서드를 사용할 수 있다.
참고
“Practical Functional Programming”의 다른 글도 참고 하자: https://hackernoon.com/practical-functional-programming-6d7932abc58b


원문
이 글은 번역 글 입니다. 원문은 아래에서 확인 할 수 있습니다. 혹시 잘못된 번역을 알려주시면 수정하도록 하겠습니다.
https://hackernoon.com/what-i-learned-from-doing-1000-code-reviews-fe28d4d11c71