주문의 status 로 표시 상태를 계산하는 로직이 여러 화면에서 쓰이고 있었다. grep 으로 찾으니 비슷한 코드가 다섯 군데 있었고 완전히 같지는 않았다.
Table of contents
Open Table of contents
어느 게 맞는지 몰랐다
다섯을 나란히 놓고 비교했다.
세 곳은 상태를 여섯 가지로 나눈다
한 곳은 다섯 가지다 (하나가 빠졌다)
한 곳은 순서가 다르다어느 것이 맞는지 코드만으로는 알 수 없었다. 다섯이 다 쓰이고 있고 각각 다른 화면에서 다른 status 를 보여 준다.
svn log 를 보니 원본이 하나 있었고 새 화면마다 복사해 조금씩 고친 것이었다. 그 뒤 원본에 새 상태가 추가됐는데 복사본 중 하나만 따라갔다.
이 상태에서 규칙 하나를 고치려면 grep 으로 다섯 곳을 다시 찾아야 한다. 그런데 다섯이 서로 다르니 같은 수정을 그대로 적용할 수도 없었다.
원인 — 복사가 합리적인 이유
탓할 일은 아니고 그때마다 이유가 있었다. 공통화하면 한 곳을 고칠 때 다섯 화면이 다 바뀐다.
그 calcStatus 하나가 무서우면 복사가 안전해 보인다. 새로 만드는 화면만 생각하면 기존 것을 건드리지 않는 쪽이 위험이 적다.
완전히 같으면 공통화가 자명한데 화면마다 미묘하게 다르면 조건 분기를 넣어야 한다. 그러면 공통 함수가 복잡해져서 그것대로 부담이 된다.
새 화면을 이틀 안에 만들어야 하면 .php 하나를 복사하는 쪽이 빠르다. 셋 다 그 순간에는 합리적이었고 문제는 그다음에 온다.
복사한 순간이 아니라 그 뒤가 문제다
복사 자체는 손해가 없고 손해는 calcStatus 원본이 바뀔 때 생긴다.
새 상태가 추가된다 → 복사본은 모른다
버그가 고쳐진다 → 복사본에는 버그가 남는다
조건이 바뀐다 → 화면마다 다른 답이 나온다그리고 바뀐 것을 아무도 모른다. 복사본이 어디 있는지 목록이 없기 때문이다.
이번 조사도 같은 주문인데 화면마다 status 가 다르다는 문의에서 시작됐다. 복사한 지 한참 지난 뒤에 그것이 문의로 돌아온 것이다.
판단 기준 — 의도된 차이와 방치된 차이
다섯을 하나로 합치기로 했는데 차이를 어떻게 다룰지가 문제였다. 기준을 하나로 잡았다.
화면마다 다르게 보여야 할 이유가 있으면 인자로 받고 그냥 안 따라간 것이면 없앤다. 의도된 차이와 방치된 차이를 가르는 것이다.
diff 로 다섯을 하나씩 확인하니 넷은 방치였다. 하나는 진짜 다른 요구였는데 관리자 화면에서만 내부 상태를 더 보여 주는 것이었다.
function calcStatus(Order $o, bool $detailed = false): string그래서 하나로 합치고 $detailed 를 기본값 false 로 뒀다. 이 구분을 안 하고 합치면 의도된 차이까지 뭉개진다.
한 번에 안 바꿨다
다섯 곳을 동시에 바꾸면 무엇이 깨졌는지 모른다. 그래서 순서를 뒀다.
1. 공통 함수를 새로 만든다 — 기존 코드는 안 건드린다
2. 기존 다섯과 결과를 비교한다 — 실제 데이터로 같은 답이 나오는지 본다
3. 로그가 안 쌓이면 하나씩 교체한다
4. 다 바꾼 뒤 비교 코드를 뺀다둘째가 핵심이었는데 calcStatus 와 기존 것을 다 계산해 다르면 로그를 남긴다.
// 임시로 양쪽 다 계산해서 다르면 로그
$old = $this->calcStatusInline($order);
$new = calcStatus($order);
if ($old !== $new) {
Log::warning("status mismatch", compact('order', 'old', 'new'));
}$old 와 $new 가 다르면 그 주문 번호와 양쪽 값이 남는다. 며칠 지켜보면서 Log::warning 이 안 쌓이는 것을 확인하고 나서 하나씩 교체했다.
이 단계 덕분에 복사본 하나가 실제로 다른 답을 낸다는 것을 배포 전에 알았다. 상태 하나가 빠져 있던 그 건이다.
재발 방지 — 다시 복사되지 않게
합쳐 놓고 시간이 지나면 또 복사된다. 그래서 몇 가지를 남겼다.
/**
* 주문 상태 계산. 이 로직의 유일한 정의.
* 새 화면에서 상태가 필요하면 이 함수를 부른다. 복사하지 않는다.
* 화면별 차이가 필요하면 인자를 추가한다.
*/calcStatus 위에 여기가 유일한 정의라는 것을 적었고 grep 으로 찾아온 사람이 그 주석을 먼저 본다.
비슷한 코드를 찾는 검사도 정기적으로 돌렸는데 status 문자열 상수를 여러 곳에서 쓰는지 grep 하는 정도다.
완벽하지는 않다. 다만 유일한 정의라고 적혀 있으면 복사하기 전에 한 번 멈춘다.
정리
- 같은 로직이 여러 곳이면 언젠가 일부만 고쳐진다
- 복사하는 순간의 판단은 대개 합리적이고 손해는 원본이 바뀔 때 난다
- 복사본이 어디 있는지 목록이 없으니 바뀐 것을 아무도 모른다
- 차이를 의도된 것과 방치된 것으로 가른다
- 앞은
$detailed같은 인자로 받고 뒤는 실제 값으로 가려 없앤다 - 이 구분을 안 하면 의도된 차이까지 뭉개진다
- 다섯 곳을 동시에 바꾸면 무엇이 깨졌는지 모른다
- 양쪽을 다 계산해 다르면 로그를 남기면 배포 전에 차이가 드러난다
- 로그가 안 쌓이는 것을 확인하고 하나씩 교체한다
- 합친 뒤 유일한 정의라는 주석을 남겨 복사 전에 멈추게 한다