[코드 리뷰 이야기] 단수형을 단수형이라 부르지 못하고, 복수형을 복수형이라 부르지 못하니

최근 담당 업무가 바뀌어 개발 업무보다는

코드 리뷰어 역할을 하게 되었습니다.

(경로 우대라는 이름의 루키 보호를 위한 꼰대 격리 전략이라 의심하고 있음)

다행히 그간 개발해온 가락이 있다보니

견마지로(犬馬之勞)라고 남이 짠 코드에 오탈자 정도는 잡아줄 재주는 있는지라

리뷰 과정 중 개발자가 흔히 실수하거나

알려주면 도움이 되겠다고 생각되는 내용들을 정리해서

분량이 나오는대로 종종 포스팅을 하려 합니다.

이 글은 최소한 코드 리뷰가 어떠한 활동인지는 개념적으로라도 알고 있다고 가정합니다.

또한 인용된 소스 코드는 이야기 전개를 위해 변형하거나 첨삭될 수 있으며

민감 정보나 보호가 필요한 처리 로직 등은 담고 있지 않습니다.

내용이 딱딱하게 전개되지 않도록 B급 감성을 살짝 얹었으며

다소 무례하거나 진지하지 못한 표현은 의도된 것이오니

행여 수위 조절이 필요하다 판단되는 부분에 대해서는 피드백 부탁드립니다.

참고로 이 글은 읽는 호흡에 맞춰 개행이 되어 있습니다.

화면 폭이 좁은 모바일 기기에서는 개행이 부자연스러울 수 있으니 양해 부탁드립니다.

  • 개발 언어: Java
  • 버전: 무관
  • 의존 관계: Apache Commons Lang3 라이브러리 포함

[themify_hr color="dark-gray" width="100%"]

부제: 세상에서 가장 짧은 저주, 이름

오늘은 코드 리뷰 첫 번째 이야기입니다.

우선은 몸을 좀 풀기 위해 아주 사소하지만

뒤통수를 칠 수 있는 가벼운 주제로 시작합니다.

침소봉대(針小棒大)하는 과포장 포스트입니다만 (질소를 샀더니 과자가 들었어요)

코드 리뷰의 묘미를 맛보시는데 조금이나 도움이 되면 좋겠습니다.

자, 그럼 단도직입적으로 실제 소스 코드를 한번 살펴보도록 하죠.

알고리즘 시험을 보듯이 내용을 찬찬히 살펴보신 후,

자신만의 판단으로 이 코드에 대한 감상을 머리 속에 그려보시기 바랍니다.

우선, 냉장고의 재료를 확인하겠습니다.

자, 아래가 오늘 여러분이 요리할 식재료입니다.

원본 소스

1
2
3
if (xxxId == null && yyyId== null || "".equals(xxxId) && "".equals(yyyId)) {
    ...
}

이 코드를 놓고 코드 리뷰를 해보면 여러가지 다양한 의견들이 나올 수 있습니다.

몇 가지 가능한 의견들을 나열하면 다음과 같습니다.

  • '&&'와 '||'의 우선 순위가 헛갈릴 수도 있으니 괄호를 더 추가하는 것은 어떨까요?
  • 변수명 이름은 의미 파악이 쉬워서 잘 지어진 것 같습니다.
  • null 체크는 좀 더 일찍 하고 들어오는 것은 어떨까요?
  • isEmpty()나 isBlank() 같은 StringUtils를 활용하는 것이 쉽게 이해될 것 같습니다.
  • "".equals(xxxId) null safe하게 잘 처리한 것 같습니다.
  • 만약 xxxId.equals("") 했으면 NullPointerException이 나왔을 테니까요.
  • 아닙니다. 이미 xxxId에서 null 체크를 했잖습니까?
  •  그러니까 그 로직을 빼고 "".equals(xxxId)만 해도 된다는 말입니다.
  • 아니, 왜 좀 더 일찍 null 여부를 미리 하지 않습니까? 이거 좀 위험한 코드 아닙니까?

등등...

이렇게 같은 코드를 놓고도 다양한 의견들이 오고 갈 수 있습니다.

그리고 이 과정을 개발팀이 함께 함으로써

나의 코드가 아니라 우리의 코드라는 생각을 함양하게되고

같은 코드를 보더라도

어떤 부분에서 팀원과 생각이 같은지

어떤 부분에서 팀원과 생각이 다른지

견해와 관점 차이를 확인하면서 커뮤니케이션을 원활하게 할 수 있습니다.

참고로 최근 위 코드에서 나온 "".equals(xxx)와 xxx.equals("") 패턴에 대해서

토비 이일민님과 저를 포함한 많은 분들이 다양한 견해를 페이스북에서 주고 받았습니다만

공교롭게도 그 전에 리뷰한 코드에 이와 같은 패턴이 나왔을 뿐

최근 있었던 여러 의견들을 종합하는 내용은 아닙니다.

암튼 위와 같은 의견을 보면서

  • 아.. 나랑 같은 생각이네.
  • 움? 난 좀 다르게 생각해.

이런 반응이 있을 것이라 생각합니다.

만약 개발 팀원 내에서 여러 이견이 발생한다면

자유롭게 코멘트를 달고 의견을 개진하면서

가끔은 사이좋게 멱살 잡이도 하시고,

서로의 지적 욕구를 충족하거나 카타르시스를 느낄 수 있었다면

이후, 팀 코드에 대한 합의를 보시고

서로 다독이며 끈끈한 팀원의 브로맨스를 만끽하면 되겠습니다.

수수께끼는 풀렸어, 범인은 이 안에 있다!

대략 이 정도의 의견만 끌어낼 수 있어도 코드 리뷰는 일단 성공한 셈입니다.

결국 코드는 사람이 보는 것이기 때문에

코드가 정상 동작한다는 것만 전제된다면

제3자가 보더라도 쉽게 이해할 수 있고

유지보수하기 용이하면 잘 만든 코드인 셈이죠.

하지만 이렇게 끝낼 거라면 제가 포스트를 쓰지도 않았습니다.

이 코드는 육안으로 하는 코드 리뷰의 한계점을 보여줍니다.

왜냐하면 바로 이 부분이 있기 때문이죠.

보완할 코드

1
"".equals(xxxId)
  • 움? 저 코드가 어때서!

네, 별 이상없어 보입니다. 적어도 눈으로 볼 때는 그렇습니다.

이 쯤에서 우리는 이 포스트의 제목을 한 번 살펴봅시다.

뭔가 짚이는 것이 있습니까?

사실은 이 코드...

우리가 볼 때는 xxxId가 String으로 보입니다만

IDE와 같은 개발 툴에서 볼 때는 실제 타입이 String[] 였습니다. (-ㅁ-)

"".equals(xxxId)는 문자열 비교라면 문제가 없지만

빈 문자열과 String[]는 비교할 수가 없는 것이죠.

이런 패턴은 IDE가 아니라 일반적인 텍스트의 차이를 비교하는

온라인 코드 리뷰 시스템 등에서는 식별이 사실상 어려운 코드입니다.

좋은 목수는 연장 탓을 하지 않는다.

하지만 좋은 연장을 쓰면 더 좋은 제품을 더 쉽게 만든다.

  • 뭐야! 그걸 저것만 보고 어떻게 알아!

네, 그래서 코드 리뷰를 할 때는 텍스트를 눈으로 훑어보는 휴리스틱한 방법과

개발 IDE의 Plugin 형태로 제공되는 정적 분석 툴을 병행해서 써야 합니다.

오늘 이야기의 함정이기도 합니다. (제 뒤에 악마가 웃고 있습니다. 움훼훼)

CI/CD 환경에 연동되는 정적 분석 Plugin은 전체 통합 점검 차원이므로

우선은 개발자 IDE에서 1차 방어 하셔야 합니다.

1차 방어에서 걸러졌다면 CI/CD 환경에서의 점검이 부담이 적어지죠.

이 때는 2차 방어라기 보다는

점검 결과를 대시보드에 표시하는 정도로 활용할 수 있을 것입니다.

사실상, 많은 관리자들이 이 대시보드를 점심 시간 뉴스 보듯하기 때문에

여기에 빨간 불 팍팍 뜨는 건 누구도 원하지 않을 겁니다.

(경고하는데, 밥 먹을 때 건들지마라...)

팀원의 사기 저하를 막기 위해

형상관리를 할 때는 소스 코드의 오류를 가지고

범인 찾기를 하지 말라고는 합니다만

간혹 컴파일 안되는 소스를 커밋하고 가는 개발자가 있는 것처럼

점검 툴 안돌린 소스 코드를 커밋하고 가는 분들에겐

개발팀에서 규칙을 정하시어 사람을 해치지 않는 수준의 페널티를 물리는 것도 좋습니다.

원래 가볍게 싸대기 한대 쯤은 우정의 상징으로 볼 수 있잖습니까?

아구지 한대도 아니고.. (농담입니다. 정색)

이건 실수로 오류를 남긴 것이 문제가 아니라 그냥 확인을 안한 것이라면

팀원에 대한 최소한의 성의나 배려 문제로 봐도 되는 내용일 것 같습니다.

서로가 함께 만드는 제품의 품질을 위협하는 만큼

내가 야근할 잠재 가능성을 높이는 일이기도 할테니까요.

(내가 주말에 출근하는 날이 네 제삿날이다...)

한편, 실제로 이 코드는 Eclipse Plugin인 FindBugs를 통해 찾아내었습니다.

큰일 날 뻔 했네요. (너만 툴 쓴건 반칙이 아니냐?)

그럼 올바른 코드는 어떻게 고쳐져야 할까요?

일단 올바른 코드를 확인해보겠습니다.

1차 개선한 코드

1
ArrayUtils.isEmpty(xxxId)
  • 오오... 안구가 정화 된다.

네, 의미도 명확히 전달되고 불필요한 사제 코드 만들지 않아도 되는

강아지도 송아지도 다 쓴다는 Apache Commons Lang3 라이브러리를 활용했습니다.

만약, ArrayUtils와 같은 Apache Commons 계열의 라이브러리를 처음 보신다면

내가 안짜도 되는 코드를 만들고 있을 가능성이 많으므로

꼭 한번 API를 훑어보고 내 코드에 유사 코드가 있다면 교체하여

직접 개발한 인하우스 소스 코드의 양을 줄이기를 권합니다.

혹자는 Apache Commons가 너무 오래되서

버그도 있고 성능에도 문제가 있다고는 하지만

딱 부러지게 다른 대안이 있어서 교체가능하거나

프로파일링을 해보니 내가 짠 소스가 더 우수하다고 확신이 서지 않는다면

못이긴 척 살짝 기대어 가시는 것도 나름 경제적이고 합리적입니다.

참고로 FindBugs에서 나오는 버그 안내문에는

java.util.Arrays.equals(Object[], Object[])를 권하고 있습니다.

기회가 된다면 둘 중 누가누가 잘하나 프로파일링을 해보고 싶습니다만

오늘은 버그를 잡는데만 집중하기로 하죠.

버그 안내문은 참고하실 수 있도록 본 기사 하단에 인용해 두었습니다.

한편, 위와 같이 수정한 형태가 반드시 정답이라고는 할 수 없고

미처 생각치 못한  다양한 해법이 있을 수도 있겠지만

일단 이 형태도 그리 나빠 보이지는 않습니다.

(더 좋은 아이디어 등은 언제든지 코멘트로 공유를...)

끝판왕은 아직 나타나지 않았다.

일단 평화는 찾아왔습니다만 아직 불만은 있습니다.

  • 저러면 툴 돌리기 전까진 모르잖아, 저걸 어떻게 찾아?

과연 툴을 돌려야만 알 수 있었을까요?

그 전에 저 잠재 위험을 제거할 순 없었을까요?

아닙니다. 다시 한번 코드를 살펴보죠.

다시보는 1차 개선한 코드

1
ArrayUtils.isEmpty(xxxId)

고쳐진 코드지만 아직 좀 더 손 보고 싶은 부분이 있습니다.

아직 눈치채지 못한 분은 이 포스트의 부제를 다시 읽어보십시오.

네, 이름입니다. 단수형 이름

만화 음양사에서 아베노 세이메이는

이름이야 말로 세상에서 가장 짧은 저주라고 합니다.

볼드모트는 이름을 알면서 못부르는 것이지만

이 경우는 이름 자체를 잘못 부른 셈이죠.

잘못된 걸 모를 수 있습니다. 더 심각한 상황입니다.

우린 xxxId라는 이름의 저주에 걸렸습니다..

이 저주에 걸려 우리는 저 변수의 타입이 String이라고 생각했고 문제를 발견하지 못했습니다.

그러면 우린 이 저주에 걸리지 않으려면 어떻게 해야할까요?

네, 단수형은 단수형이라고 부르고 복수형은 복수형이라고 부르면 됩니다.

너무나 당연한 이야기입니다만 우린 야근과 격무에 지친 나머지 코드의 행, 단어의 의미를 되씹지 않고

그저 기계적인 방법으로 기계식 키보드 타격감만 느끼면서 타이핑해 나갈 수 있습니다.

(그래서 저는 노동요로 가사 있는 노래를 듣지 않습니다.)

그래서 이런 명명법을 처방합니다.

  • 단수형 변수는 단수형 단어로 표현
  • Array, List 등의 타입의 변수는 복수형 s를 단어 뒤에 붙여 복수형임을 알게 할 것
  • 혹은 해당 타입을 알 수 있도록 xxxArray, xxxList, xxxMap과 같이 타입을 알 수 있게 할 것
  • 단, 접미어로 타입을 붙일 때는 역할을 확인하기 위한 것이니, 인터페이스명으로 붙이되 구현 클래스명은 붙이지 말 것
  • 예를 들어 xxxList는 괜찮지만 xxxLinkedList는 좋지 않음
  • 구현 방법은 전혀 관심 밖의 내용임

자.. 이런 맥락이면 어떻게 하면 될까요?

저는 이렇게 처방해 봅니다.

2차 개선한 코드

1
ArrayUtils.isEmpty(xxxIdArray)

사실, 저는 개인적으로 Array 타입보다는 List타입을 선호합니다만 (예: xxxList)

해당 코드의 변화를 최소화하면서 1차적인 대응을 한다면 위와 같이 할 것 같습니다.

이렇게 rename을 할 때 주의하실 것은 그냥 rename 하시면 안되죠.

간단한 rename도 훌륭한 refactoring기법의 하나입니다.

난이도로 치자면 refactoring move만큼 심오하고

시한폭탄의 빨간 선을 끊어야하는지, 파란 선을 끊어야 할지

선택해야 할 만큼 위험한 작업이죠. (농담입니다. 정색)

vim, emacs 잘 쓰시는 분은 정규 표현식 replace 신공을 펼치시겠지만

그래도 java refactoring 만큼은

변경 사항을 미리 보기도 가능한 IDE의 refactoring 기능으로 rename하시면서

변경 범위를 preview로 확인하면서 수정하시길 권합니다.

(탐색 대상 조건 설정하기에 따라 텍스트 파일이나 프로퍼티 파일의 내용도 치환 가능)

코드 리뷰는 잘못된 습관을 고치는 거울

오늘 이야기 어떠셨나요?

이제까지 품질 담당자에게 쪼임 당하면서 수정했던 코드와는 사뭇 느낌이 다르셨을 것 같습니다.

뭐랄까... 광화문에 나갔는데 핫팩 하나 얻은 기분?

혹은 여전히 번거롭고 귀찮아서 독일에서 누가 대신 좀 해줬으면 하는 기분?

코드 리뷰는 서로의 얼굴과 옷매무새를 보면서

  • 얘, 너 화장 떴어, 이게 뭐니?
  • 김대리, 남대문 열렸다.

와 같이 서로의 작지만 고쳐지면 좋을 것을

팀원끼리 확인하는 과정입니다.

코드 리뷰는 맹목적인 믿음도 아니고 생활 습관이 되어야 합니다.

자매님, 형제님도 평소 코드 리뷰하시어 평화를 찾으시고 성불하세요.

참고: 상기 Bug에 대한 FindBugs의 안내문

[themify_box style="gray"]

This method invokes the .equals(Object o) to compare an array and a

reference that doesn't seem to be an array. If things being compared are

of different types, they are guaranteed to be unequal and the comparison

is almost certainly an error. Even if they are both arrays, the equals

method on arrays only determines of the two arrays are the same object.

To compare the contents of the arrays, use

java.util.Arrays.equals(Object[], Object[]).

Rank: Scariest (1), confidence: High

Pattern: EC_ARRAY_AND_NONARRAY

Type: EC, Category: CORRECTNESS (Correctness)

[/themify_box]

Popit은 페이스북 댓글만 사용하고 있습니다. 페이스북 로그인 후 글을 보시면 댓글이 나타납니다.