본문 바로가기
ETC

코드리뷰에 대하여

by devson 2021. 2. 24.

왜 우리는 코드리뷰를 하는가?에 대해 사내에 발표하기 위한 자료를 정리하여 올립니다.

 

이 포스팅은 정답이 아닌 개인적인 의견과 관점이고 팀마다 상황이 다르고 조직마다 지향하는 바가 다르기에 상황에 맞게 적절한 전략으로 코드리뷰를 진행하는 것이 좋다고 생각합니다. :)

 

 

Why?

우리는 왜 코드리뷰를 하는가?

  • 버그가 될 수 있는 부분을 사전에 파악하기 위해

  • 코드의 성능, 가독성을 증가시키기 위해

  • 팀 코드 스타일의 일관성 유지시키기 위해

  • 새로운 feature 코드를 공유하고 잠수함 패치를 방지하기 위해

  • 작업한 사람을 탓하지 않기 위해 (책임을 분배하기 위해)

  • ⭐️ 서로의 코드를 통해 배울 수 있음

    • Reviewer를 통해 배우기

      val inputStream = new FileInputStream("text.txt");

try { 
    loadDataFrom(inputStream) 
} catch (exception: Exception) { 
    logger.error(exception) 
} finally { 
    inputStream.close() 
}

// Reviewer: 
// kotlin `use`를 사용하면 finally에서 close를 하지 않아도 해당 리소스를 반환하여 
// 보다 간결하고 안정적인 코드가 됩니다.

    • Reviewee를 통해 배우기

      @Entity
@Inheritance(strategy = InheritanceType.SINGLE_TABLE)
public abstract class Product { 
    @Id private long productId; 
    @Column private String name;
} 

@Entity 
public class FoodProduct extends Product { 
    @Column private LocalDataTime expirationDate; 
} 

@Entity public class ElectronicProduct extends Product {
    @Column private Int power;
}

// Reviewer: 
// 문서를 봐도 정확히 이해가 안되는데 
// @Inheritance(strategy = InheritanceType.SINGLE_TABLE)의 역할이 무엇인가요? 

// Reviewee: 
// SINGLE_TABLE 전략을 사용하면 abstract Entity의 구현체들에 대한 DB table이
// 하나하나 생성되지 않아 table 관리 및 조회 측면에서 유리합니다.

 

코드리뷰가 단순히 코드를 체크하는 업무의 과정 중 하나가 아니라 좋은 개발자가 되기 위해, 더 좋은 소프트웨어를 개발하기 위한 서로의 노력과 소통의 과정이라고 생각하면 좋을 것 같습니다 :)

 

How?

Reviewee와 Reviewer의 입장에서 코드 리뷰를 어떻게 대해야할지?

Reviewee

코드리뷰 시 Reviewer 로서

 

Reviewer

코드리뷰 시 Reviewee 로서

  • 의견에 대해 논거가 충분해야한다

    • 객관적인 사실이 개인의 선호도보다 우선시 되어야한다.

      if (someCondition) 
  return doThis(); 
else if (otherCondition) 
  return doThat(); 
throw new Error();

// bad 👎
// Reviewer: 
// if 문에 왜 brace이 없죠? 넣는게 좋지않나요??

// good 👍 
// Reviewer: 
// if 문에 brace를 항상 넣어주세요. 
// 당장은 이슈가 없어도 위와 같은 코딩 스타일은 추후 크리티컬한 버그가 일어날 요지가 있습니다. 
// https://embeddedgurus.com/barr-code/2014/03/apples-gotofail-ssl-security-bug-was-easily-preventable/
      def getCode(condition: bool) -> int: 
    if condition: 
        return 1 
    else: 
        return 2

# bad 👎
# A: 삼항연산자로 대체해주세요.
# B: 특별한 이유가 있을까요??
# A: 코드가 한 줄이고 더 보기 편하잖아요. <= 주관적

# good 👍
# A: 우리 코딩 컨벤션에 맞춰서 삼항연산자로 대체해주세요.

  • 리뷰는 최대한 빨리 해주는 것이 좋다 (아무리 늦어도 당일)

  • 리뷰 시 해당 코멘트의 강조 여부를 알려주는 것이 좋다

 

How to make review process better?

Do

  • 리뷰 역시 communication이다

    • 코드 리뷰 이전에 대화에 있어 기본적인 에티켓을 지켜야한다

    • 동료로서 Reviewer의 좋은 코드에 대해서 칭찬을 해주는 것도 중요하다

    • 서로 생각과 지식의 차이를 인정하고 이에 대해 건강한 피드백과 좋은 타협점을 내도록 해야한다

      def square_numbers(numbers: List[int]) -> List[int]:
    squared_numbers = []
    for num in numbers: 
        squared_numbers.append(num * num)
    return squared_numbers

# bad 👎
# Reviewer: 왜 for 문을 사용한거죠? List Comprehension으로 하는게 더 나을거 같네요
# Reviewee: 왜죠? 결과가 같은데 굳이 변경할 필요가 있나요? 별 차이는 없으니 변경은 안할게요

# good 👍
# Reviewer: 
# for 문을 사용하는 것보다 List Comprehension이 Python에서 지향하는 바이기도 하고 성능면에서도 우세합니다.
# 이 영상(<[https://www.youtube.com/watch?v=Txz7K6Zc-_M](https://www.youtube.com/watch?v=Txz7K6Zc-_M)>)의 5분 20초 쯤 보면 성능에 대해 나와있습니다.
# Reviewee: # 좋은 아이디어주셔서 감사합니다만, 다른 분들이 List Comprehension에 대해 충분한 숙지가 되어있지가 않으니
# 쉽게 알아볼 수 있는 for 문을 사용하는 것이 현재 상황에서는 좋다고 생각합니다.

    • 참고

  • Notification for review request and review comments

    • Upsource, Codestream과 같은 IDE 통합 code review tool을 사용하면 실시간으로 리뷰가 이뤄지기 편함
    • Slack을 통해 리뷰 요청이나 코멘트 시 알림을 받도록 함
    • Scheduled PR reminder

  • Test code를 통해 기능을 수행했을 때 이에 대한 기대값을 보여주면 해당 코드에 대한 사용법을 알기 편하다.

Don't

  • Coding convention, style, test 코드 정상 작동 여부를 리뷰한다
    ⇒ git hooks, github actions, linter, static analysis tool을 통해 자동화

  • approve 후 추가 commit 및 잠수함 패치
    ⇒ 새로운 commit 시 merge 막기

  • ☠️ 교착상태

 

References

'ETC' 카테고리의 다른 글

Double Dispatch를 통해 유연하게 확장하기  (0) 2022.02.10
Arrow Core: Tutorials - Functional Error Handling (작성 중)  (0) 2021.12.31
WebSocket Server에서 Connection 'close' 이벤트를 받지 못할 때  (1) 2021.01.06
BCrypt로 Password Hashing 하기  (0) 2021.01.02
Sync & Async / Blocking & Non-Blocking  (0) 2021.01.02

관련글

댓글

티스토리툴바