(리팩터링 2판) 3장 Bad Smells in Code

Posted Oct 14, 2021 Updated Nov 20, 2024

By umbum 36 min read

이번 챕터는 언제 리팩터링 해야 하는가? 어떤 코드가 리팩터링이 필요한 코드인가?에 대해서 다룬다.
(적어도 나는)코드가 구조적으로 예쁘지 않아서, 보고 있자니 뭔가 마음 한켠이 불편해서, 같은 ‘느낌’을 감지하고 리팩터링을 하게 되는데
이런 모호한 기준을 끄집어 내서 문장으로 구체화한 챕터이다. 리팩터링이 필요한 코드가 가지고 있는 공통점, bad smell에 대해서 얘기한다.
무엇이 문제인지 알아야, 그에 따른 해결 방안도 떠올릴 수 있다.

부록B에 보면 bad smell case와 해법이 잘 정리되어 있음.
정리하면서 평소 개발하면서 정립했던 나의 개인적인 의견도 같이 적어 두었음. (인용구로 되어 있는 부분)

3.1 기이한 이름

함수든, 변수든, 클래스든 이름만 보고도 각각이 무슨 일을 하고 어떻게 사용해야 하는지 명확히 알 수 있도록 신경써서 지어야 함.

자바 컨벤션에서는 길더라도 명확하고 서술적인 이름을 선호하는 경향이 있다.
하지만 해당 이름이 사용되는 바운더리에 따라 일정 부분 생략하고 짓는 것도 괜찮아 보임.
e.g., ‘어떤 클래스 내에서만 사용되는 USD 포매팅 역할의 private 함수’ 라면, 꼭 formatAsUSD() 가 아니라 그냥 usd() 정도로 네이밍 (챕터1 예제)

3.2 중복 코드

DRY 하게 작성합시다. 당연한 얘기라서 생략

3.3 긴 함수

경험적으로, 오랜 기간 잘 활용되는 프로그램들은 하나같이 짧은 함수로 구성된 프로그램.
짧은 함수로 구성된 코드베이스를 훑으면 연산하는 부분이 없고, 다 함수 호출(위임) 하는 식으로만 진행되는 것 처럼 보인다. (연산은 말단에서 수행하니까)
그리고 저자는 이러한 짧은 함수 간접 호출로 구성된 프로그램이 좋은 프로그램이라고 생각함.

왜 긴 함수는 나쁘고 짧은 함수가 좋은가?

함수가 길 수록 이해하기 어렵다.
짧은 함수는 코드를 이해하고, 공유하고, 선택하기 쉬워진다.
- 짧은 함수를 쓰게 되면 함수 호출부와 선언부를 왔다갔다 하며 봐야해서 코스트가 더 드는 것 아닌가?
- ⇒ 짧은 함수로 구성된 코드를 이해하기 쉽게 만드는 가장 확실한 방법은 좋은 이름이다. 함수 이름을 잘 지어두면 본문 코드를 볼 이유가 사라진다.

이해 - 가독성 / 공유,선택 - 재사용성

그래서 저자는 적극적으로 함수 쪼개기를 권장하고 있음. 분리한 함수가 단 한 줄 이라도, 심지어 원래 코드보다 길어지더라도 함수로 뽑는다. 여기서 핵심은 함수의 길이가 아닌 , 함수의 이름(목적,의도)과 구현 코드의 괴리가 얼마나 큰가다. ‘무엇을 하는지’를 코드가 잘 설명해주지 못할수록 함수로 만드는 게 유리하다.

언제 함수를 분리해야 하는가?

1. 코드 만으로는 의도가 잘 드러나지 않을 때.

특히 주석을 달아야만 하는 경우가 좋은 시그널이다. 주석을 다는 것 보다, 해당 부분을 함수로 분리하고 그 것의 동작 방식을 코드로 나타낸 후에 함수 이름을 동작 방식이 아닌 의도가 드러나게 짓는다.

주석이 필요 없는 코드가 좋은 코드.
의도는 이름에, 동작 방식은 코드에.

2. 조건문이나 반복문

조건문이나 반복문도 분리 필요성을 나타내는 좋은 시그널일 수 있다.

개인 의견 정리

이 항목의 제목이 “긴 함수” 이긴 하지만, 저자가 얘기했듯, 중요한 것은 함수의 길이가 아니라

의도(‘무엇을 하는지’)를 한눈에 알아볼 수 있는 코드인지 라고 생각한다.

코드를 작성하다, 함수 분리를 두고 고뇌하게 되는 상황은 아래와 같다.

분리된 함수 호출처가 둘 이상인 경우, 재사용성이라는 중대한 사유가 있으니 기꺼이 분리하게 되는 반면
호출처가 1개인 경우(앞으로도 1개일 것 같은 경우), 분리해서 얻을 수 있는 이점이 가독성 뿐인데, 이 가독성이란게 상당히 주관적인 기준이다 보니 고민에 빠지게 된다. “괜히 분리했다가 더 알아보기 힘든 것 아닌가?” “너무 잘게 쪼개는 것 아닌가?” 사실 어설프게 분리된 로직은 다시 합치는게 맞기 때문에, 분리에 명확한 기준이 있다면 도움이 된다.

이 때, 의도(‘무엇을 하는지’)를 한눈에 알아볼 수 있는 코드인지 를 기준으로 사용할 수 있겠다.

의도를 한눈에 알아볼 수 없다면, 분리한 함수가 단 한 줄 이라도, 심지어 원래 코드보다 길어지더라도

함수를 분리하여 의도를 이름에 나타내고 / 동작 방식은 코드에 나타낸다.

3.4 긴 매개변수 목록

매개변수 줄이는 방법은 다양한데, 그 중 생각해볼 만한 주제가 있어 별도 정리

객체 통째로 넘기기 vs 풀어서 넘기기

객체 통째로 넘기기 pros

파라미터를 풀어서 받다 보면 매개변수가 길어지므로, 객체를 통째로 넘기면 매개변수 수 줄일 수 있음
객체 전체를 넘기면 함수 시그니처 변경 없이 내부에서 해당 필드를 사용하는 부분만 추가해주면 됨. 그러나 객체를 풀어서 넘기면 추가 파라미터를 받기 위해 함수 시그니처 변경 필요. ⇒ overloading 하는 등 하위호환성 고려해야 함.
“가독성” 파라미터가 너무 많은 경우 풀어서 넘기면 보기 싫은 코드가 된다.

풀어서 넘기기 pros

함수의 응집도를 생각하면, input은 명확한 편이 좋으니 딱 필요한 매개변수만 풀어서 받는게 낫다.
객체 전체를 넘겨버려서 필요 없는 필드까지 함께 받게 되면 그 함수 내부에서 객체 내부의 어떤 데이터를 사용할지가 모호해지니까, 파라미터를 이용한 표현력이 떨어지게 된다는 단점이 있음.

이 주제에 대한 자세한 내용은 11.4절에서 다룬다.
6.5절에서도 얘기하고 있다. 인용하자면, “이 문제의 정답은 바로 정답이 없다는 것이다.”
= 상황에 따라 개발자가 직접 판단해야 한다.

참고) MobX에서는, re-render 때문에 객체 통째로 넘기기가 권장됨.

React, MobX - ‘역참조는 최대한 늦게 해라’ 항목

3.5 전역 데이터

전역 데이터는 코드베이스 어디에서든 건드릴 수 있고 값을 누가 바꿨는지 찾아낼 메커니즘이 없다는 게 문제다. 사용하는 시점에 이 변수의 값이 어떤 값이리라 예상한 것이 틀릴 가능성이 높고, 이는 버그로 이어진다. 결국 “ 어디서든 건드릴 수 있고 변경 추적이 어렵다 “ 라는게 핵심이므로, 꼭전역 변수가 아니더라도 접근 바운더리가 넓은 클래스 변수와 싱글턴에서도 같은 문제가 발생할 수 있다 .

3.6 가변 데이터

전역 데이터 쓰지 말아야 하는 이유를 먼저 생각해보면, 불시에 변경될 수 있고 어디서 변경되었는지 추적하기도 어렵다. 라는 점임.
가변 데이터도 똑같다. 전역 데이터 보다 상황은 낫지만, 어쨌든 변경 가능한 데이터 보다는 불변 데이터가 이러한 관점에서는 더 안전함.
- set이 가능하다면, 이 필드가 이전 과정의 어디선가 set 해서 바뀌지는 않았는지?를 따라가면서 다 확인해야 한다.
- 반면 final이면 불변이므로 최초 초기화만 확인하면 된다.
가변 데이터를 써야만 하는 상황이라면 무분별한 데이터 수정에 따른 위험을 낮추기.
- 캡슐화를 통해서 변수 수정에 대한 end-point 제어
- 변경 가능 유효범위 줄이기 (변수의 유효범위가 단 몇 줄뿐이라면 가변 데이터라 해도 문제를 일으킬 일이 별로 없다.)
- CQRS 분리
- 그 밖에 본능적으로 수행하게 되는 다양한 방법들

보통은

final이 제일 낫고,
불가하다면 private setter 처리해서 class 내부로 변경 가능 바운더리를 제한하는 방법도 괜찮다.
너무 타이트하게 불변을 지키려다 보면 곤란한 경우가 종종 있어 1, 2를 적절히 사용하는게 좋다.

의견 추가

mutable 객체가 가장 짜증나는 상황은, 객체가 method parameter로 전달되었을 때, “이 메서드 안에서 객체가 혹시 수정되지 않았을까?” 불안에 떨고 혹시 변경지점이 없는지 샅샅이 뒤져봐야 한다는 점.
method 안쪽을 따라가며 모두 뒤져보거나, MyClass의 필드 하나 하나 IDE의 도움을 받아 어디서 참조되고 있는지 값을 바꾸는 곳은 있는지를 뒤져보아야 하는데 추적하기 상당히 까다로워 유지보수가 힘들다.

반면 immutable 객체는, 메서드 안에 전달하더라도, 변수 참조 자체가 바뀌지 않는 이상 그 객체의 상태가 그대로라는 것을 확신 할 수 있다는 것이다. (안심되고 수정하기 편한 코드)

  
MyClass obj = new MyClass(1, 2, 3);
method(obj);    // 혹시 method 안에서 obj의 상태를 변경하지 않았을까?
obj // <- 이 시점에 obj의 상태가 변경되지 않았을거라고 확신 할 수 없다.

반면 불변 처리하고 copy()를 쓴다면 어떨까?

  
MyClass obj = new MyClass(1, 2, 3);
method(obj);
obj // <- obj는 불변이므로 바뀌지 않았을거라 확신 할 수 있다.
---
MyClass newObj = method(obj);
newObj // <- newObj는 뭔가 바뀌었을 가능성을 나타낸다. 바뀔 가능성이 없다면 리턴도 하지 않는다.

즉, 현재 읽고 있는 call stack depth에서 아래로 더 들어가보지 않아도 변경 되었는지 아닌지 알 수 있다.

[!info] 경험 상 불변 데이터의 장점은 시스템의 규모가 작을 때는 잘 나타나지 않는다. 코드베이스가 어느 정도 규모를 갖추었을 때 비로소 나타난다.

3.7 뒤엉킨 변경

책에서는 모듈 이라는 용어를 사용했지만, 객체지향에서는 class로 바꿔 생각해도 무방해 보인다.

단일 책임 원칙(SRP) 이 지켜지지 않았을 때 발생 [객체 지향 5대 원칙 : SOLID ]
- SRP에 따르면, 어떤 모듈 A는, 그 모듈이 담당하는 기능 x에 관한 변경이 있을 때만 변경이 발생해야 한다.
- 그러나 모듈 A가 담당하는 기능이 x, y 두가지라면, x에 관한 변경이 있을 때에도 A를 변경해야 하고, y에 관한 변경이 있을 때에도 A를 변경해야 한다. = SRP 위반 = 뒤엉킨 변경
e.g., DB를 추가해야 할 때도 모듈 A를 변경해야 하고, 금융 상품을 추가해야 할 때도 모듈 A를 변경해야 한다면 ‘뒤엉킨 변경’이다.
- 이런 상황이라면 모듈을 쪼개야 한다.
- 물론, DB와 금융 상품 여러 개를 추가하고 나서야 이 악취가 느껴지는 경우도 많다. 개발 초기에는 맥락 사이의 경계를 명확히 나누기가 어렵고 소프트웨어 시스템의 기능이 변경되면서 이 경계도 끊임없이 움직이기 때문이다.

개발 초기에는 맥락 사이의 경계를 명확히 나누기가 어렵다는 것이 중요.
그래서 처음 부터 완벽한 코드를 작성할 수 있다는 믿음은 미신이다. 라는 말이.

#####

3.8 산탄총 수술

뒤엉킨 변경과 비슷해 보이지만 정반대다.
산탄총 수술은, 변경해야 하는 이유는 1개인데 자잘하게 수정해야 하는 클래스가 너무 많은 경우 를 의미한다.
이는 즉, 기능 x를 담당하는 부분이 코드베이스 전반에 퍼져 있다고 볼 수 있으므로 이를 단일 모듈로 모아주는 것이 해결책 (응집도 up)


	뒤엉킨 변경	산탄총 수술
원인	여러 맥락이 한 코드에 섞여 들어감	한 맥락이 여러 코드에 흩뿌려짐
해결	맥락별로 모듈 분리	하나의 맥락은 하나의 모듈로 모음

3.9 기능 편애

기능 편애는 흔히 어떤 함수가 자기가 속한 모듈의 함수나 데이터보다 다른 모듈의 함수나 데이터와 상호작용 할 일이 더 많을 때 발생한다.

응집도는 높이고, 결합도는 낮춰야 좋은 프로그램.이라는 관점에서 바라보면,
자기가 속한 모듈과 상호작용이 적다는 것은 응집도가 낮은 상태, 다른 모듈과 상호작용이 많다는 것은 결합도가 높은 상태 라고 볼 수 있다.

단순히 타 모듈의 멤버와 상호작용이 많다고 모두 기능 편애인 것은 아니므로 상황에 따라 판단해야 할 것. ( Strategy Pattern, Visitor Pattern 등 예외적인 상황 )
아무튼 가장 기본이 되는 원칙은 ‘함께 변경할 대상을 한데 모으는 것’ 이다.

3.10 데이터 뭉치

데이터 서너 개가 항상 함께 다닌다면, 데이터 클래스로 묶어준다. (클래스의 필드에서, 메서드의 시그니처에서 발견할 수 있다.)
추후 로직이 붙으면서 일반 클래스로 단계적 개선도 노려볼 수 있다.

3.11 기본형 집착

기본으로 제공하는 primitive type 타입 말고, 새로운 기본타입을 정의하는 것을 꺼리는 경우가 많지만
화폐, 좌표, 전화번호 등 필요하다면 기초 타입을 직접 정의해서 사용할 것.
e.g., 특히 전화번호 같은 타입을 그냥 문자열로 다루는 경우. 또는 legacy system에서 시간을 문자열로 다루는 경우

3.12 반복되는 switch문

switch문은 다형성을 이용해 없애버릴 수 있는데, 모든 switch문이 나쁜가? 그렇다면 if까지도?
저자는 모든 switch if가 나쁘다고는 생각하지 않는다. 하지만 똑같은 분기 조건을 가진 switch, if가 여러 곳에서 반복해 등장하는 코드 에 집중하자고 말함.

중복된 switch의 단점은 분기 case가 하나 추가될 때 마다 다른 switch도 모두 찾아서 함께 수정해야 한다는 점이다. (산탄총 수술)

/*** 예시 추가 ***/
switch1 (obj.type) {
  case... logic-x
}
...
switch2 (obj.type) {
  case... logic-y
}
=> obj.type을 기준으로 class 분리

반복되는 switch 자체의 단점도 물론 있지만, 그 보다 중요한 것은 반복되는 같은 조건의 switch가 ‘이 부분을 클래스로 추출하는 것이 적절하다’는 시그널인 경우가 많다는 것이다. if 분기에 따른 body(로직)이 객체 외부에 존재하는 상황은 객체지향의 책임, 관심 분리를 위반했을 가능성이 있다.
10.4 조건부 로직을 다형성으로 바꾸기 참조.
factory에서만 분기해서 구체 타입을 지정하고, factory 바깥에서는 구체 타입 상관 없이 인터페이스로 참조 할 수 있다면 훌륭하다.

다형성 뿐만 아니라, java에서는 enum에 functional interface를 사용하여 로직을 enum에 넣어서 switch를 제거하는 방법도 있다. 자주 쓰는 방법. 저자가 얘기한 조건이 추가될 때 누락되는 상황도 방지할 수 있다.
[Effective Java] 6장 enum 타입과 애너테이션 (Annotation)

3.13 반복문

반복문 자체를 올드하다고 보는 듯.
최근에는 일급 함수를 지원하는 언어가 많아졌기 때문에 반복문을 파이프라인으로 바꾸기 (e.g., stream or 코틀린의 고차함수)를 적용해서 시대에 걸맞지 않은 반복문을 제거할 수 있게 됐다.

하지만 모든 상황에서 filter map 같은 함수형 요소들이 더 유리한 것은 아니라고 생각.
for를 돌아야 더 보기 좋을 때가 있음. 예를 들어, 아래와 같은 경우.

  
case 1:
listA.stream() // A{x, y}
.filter()
.map() // x 필요
.map() // x' 필요
.map() // x'' & y 필요
// y를 마지막 map까지 끌고가야 한다. Pair<T, R> ?


case 2:
Map<T, A> a와 Map<T, B> b를 같이 순회해야 하는 경우
a.key intersect b.key 해서 공통 key를 얻고 stream()을 돌리는 것도 가능은 하지만...

3.14 성의 없는 요소

의미 없는 한줄짜리 함수, 메서드가 하나 뿐인 클래스 등이 이에 해당된다.
함수 인라인하기, 클래스 인라인하기 등으로 제거 검토

3.15 추측성 일반화

‘나중에 필요할 거야 ‘ 라는 생각으로 당장은 필요 없는 모든 종류의 후킹 포인트와 특이 케이스 처리 로직을 작성해두는 경우.
미래를 대비하는 것은 좋으나 사용하지 않는다면 쓸데없는 낭비일 뿐이다. 당장 걸리적거리는 코드는 눈앞에서 치워버리자.
하는 일이 거의 없는 추상 클래스, 쓸데없이 위임하는 코드, 본문에서 사용되지 않는 매개변수 등이 이에 해당된다.

어차피 git commit log 남으니까. 지웠다가 나중에 다시 복구하면 된다.
지우기 전에 책임 등의 원칙에 따라 정당한 사유로 작성된 코드인지, 과도한 대비책으로 작성한 코드인지 검토는 꼭 필요할 듯.

3.16 임시 필드

보통 객체를 가져올 때 당연히 모든 필드가 채워져 있으리라 기대하는 게 보통이라, 특정 상황에서만 값이 존재하는 임시 필드는 좋지 않다.
클래스 추출하기로 임시 필드 제거해준다.
널객체(e.g., 미확인 고객) 때문에 임시 필드가 발생하는 경우라면 널객체를 따로 정의해준다.

경험 상 클래스를 너무 다목적으로 사용하다 보면 종종 발생한다. 목적에 따른 클래스 분리 검토가 필요한 시점.
시스템에 항상 완벽하게 초기화 된 상태의 객체만 존재하도록 하는 것은 현실적으로 불가능하다. (그래서 kotlin에서 copy 같은 메서드도 지원하는 것이고.)
그러나 [use-case에 따라 일부 필드가 영영 초기화되지 않는 객체]와 [초기화 되기 전에 잠시 불완전한 상태를 가지며 초기화 되면 완전해지는 객체]는 구분해야 한다.

3.17 메시지 체인

  
managerName = aPerson.department.manager.name
// 문제점은 네비게이션 중간 단계를 수정하면 클라이언트 코드도 수정해야 한다는 것.
// 개선책은 위임 숨기기
managerName = aPerson.department.managerName // manager 객체의 존재를 숨김(위임 숨기기)
managerName = aPerson.manager.name // department 객체의 존재를 숨김
managerName = aPerson.managerName // manager, department 둘 다 숨김
// 또는 목적에 따라서 아예 함수 분리

메시지 체인을 무조건 나쁘다고, 반드시 개선해야 하는 사항으로 볼 필요는 없다.

3.18 중개자

객체 지향의 대표적인 장점 encapsulation
보통 encapsulation 하기 위해 delegation이 자주 사용된다.

즉, 관심 없는 세부 구현 사항을 숨기고 캡슐화 하기 위한 수단 중 하나로, 세부 구현을 다른 클래스에 위임하는 방법을 종종 사용하게 된다.

하지만 클래스가 제공하는 메서드 중 절반이 다른 클래스에 구현을 위임하고 있다면?
이럴 때는 중개자를 거칠 필요 없이, 실제로 일을 하는 객체와 직접 소통하도록 한다.
중개자에서 위임 메서드를 제거한 후, 중개자에 남은 일이 거의 없다면 호출하는 쪽으로 인라인 한다.

3.19 내부자 거래

사실 이 항목에서는 무엇을 말하고자 하는지 잘 모르겠다. 결합도 줄이자.가 끝인가?? 당연한 얘기들이라서..
부모-자식 상속 구조에서 더 이상 수용이 안되는 경우, 서브클래스를 위임으로 바꾸기나 슈퍼클래스를 위임으로 바꾸기를 활용하자.

수용이 안되는 상황이 발생했다는 것 자체가 상속 보다 컴포지션을 사용하는게 더 올바른 구조임을 알리는 시그널 일 수 있다.
상속 vs 컴포지션 구분 : delegation, decorator, wrapper

3.20 거대한 클래스

필드 수가 너무 많아진 클래스
- 클래스 추출하기, 슈퍼클래스 추출하기, 서브클래스 추출하기
코드량이 너무 많은 클래스
- 중복 제거
클라이언트 호출 패턴에서 기능 그룹을 묶을 단서를 얻은 뒤, 클래스 분리

3.21 서로 다른 인터페이스의 대안 클래스들

인터페이스가 같아야 클래스를 갈아끼울 수 있으니, 메서드 시그니처 일치 시켜준다.
대안 클래스(구현 클래스)들 사이에 중복이 발생하면, 슈퍼클래스 추출하기를 적용할지 고려.

3.22 데이터 클래스

public 필드는 캡슐화 해주고, 변경하면 안되는 필드는 세터 제거하기.
불변 필드는 굳이 캡슐화 할 필요가 없고, 게터를 통하지 않고 그냥 필드 자체를 공개해도 된다.
한편, 데이터 클래스는 필요한 동작이 엉뚱한 곳에 정의돼 있다는 신호일 수 있다.

https://martinfowler.com/bliki/AnemicDomainModel.html
Domain Model 클래스에 field, Getter, Setter만 두고 단순히 DTO 처럼 사용하며 로직은 다 Service에 넣어버리는 것은 안티패턴이라는 의견.
습관 처럼 data class를 만들지 않도록 유의해야 함.

3.23 상속 포기

서브클래스에서, 상속 받은 멤버가 필요 없다면???
예전에는 계층 구조를 잘못 설계했기 때문으로 봤다. ⇒ 공통되지 않은 항목은 모두 서브클래스로 내려준다.
하지만 지금은 무조건 위와 같이 해야한다고 생각하지는 않는다. 일부 동작을 재활용하기 위한 목적으로 상속을 활용 하는 것은 실무 관점에서 유용한 방식이기 때문. 찝찝하긴 하지만.
상속을 위임으로 바꾸기 를 고려해볼 수 있다.상속 vs 컴포지션 구분 : delegation, decorator, wrapper

3.24 주석

주석을 남겨야겠다는 생각이 들면, 가장 먼저 주석이 필요 없는 코드로 리팩터링해본다.
코드 동작을 위한 선행 조건은 assert 를 활용한다. [assert 에 대해서 ]
그 밖에 주석이 필요한 경우
- 현재 진행 상황 뿐만 아니라 확실하지 않은 부분에 대해서
- 코드를 지금처럼 작성한 이유에 대해서

0Engineering, refactoring

리팩터링

This post is licensed under CC BY 4.0 by the author.