|
아래의 코드도 지인으로 부터 얻은 것이다. 지인이 현재 다니는 회사에서, 전임자가 만들어 놓고 퇴사한 코드의 일부라고 하는데, 그 지인은 이 코드의 유지 보수를 맡아야 하기 때문에 무척 고민스럽다고 한다.
다음 코드를 보면 관록있는 개발자라면 기겁을 할 것이다. 구조와 코드의 흐름, 네이밍 원칙 등 모든 면에서 너무 엉망인 코드이다. 그런데, 의외로, 이 정도로 심한(?) 형태의 코드를 현업에서 자주 볼 수 있다. 이는 참으로 안타까운 일이다. (말은 이렇게 건방지게 하지만, 아주 오래전의 나의 소스 코드 역시 이런 심한 형태를 간직하고 있을지도 모르겠다.)
procedure TJ3_2.JIBGE_PR_Click(Sender: TObject);
var
rForm:TForm;
eGongsa : string;
begin
j3_file.DataSource_JIBGE.DataSet := nil;
j3_file.DataSource_ILLWE.DataSet := nil;
eGongsa := J3_FILE.Query_JIBGEC.fieldbyname('sgongsa').asstring;
rForm:= Tpr_jibge.Create(nil);
rForm.ShowModal;
rForm.Free;
if J3_FILE.Query_JIBGEC.Active then
J3_FILE.Query_JIBGEC.Locate('SGONGSA', eGongsa, [loPartialKey]);
j3_file.DataSource_ILLWE.DataSet := j3_file.Query_ILLWE;
j3_file.DataSource_JIBGE.DataSet := j3_file.Query_JIBGE;
end;
위 코드는 지적받아 마땅한 곳이 한두군데가 아니다. 델파이에서 금기시해야 마땅한 폼변수 사용이라든가. 파괴자 호출 부분을 try finally로 묶지 않았던 것이라던가, 쓸데없는 지역 변수 사용, 의미 모호한 코드, 대체 가능한 코드 등, 분량은 얼마 안되지만 너무나 많은 허점을 가진 코드이다.
여러분들은 이 코드를 어떻게 리팩토링 하시겠는가? 다 함께 리팩토링 해 보자...
--------------------------------------------------------------------------------------------
리팩토링 1차 버전
--------------------------------------------------------------------------------------------
타인의 코드를 리팩토링할 때 항상 고민스러운 점이 원 개발자의 코딩 의도이다. 원개발자가 바로 옆에 있다면 물어볼수나 있지만, 그렇지 못하다면 리팩토링이 상당히 애매해진다. 더우기 테스트할 환경이 없다면 더더욱 그러하다.
위 코드에서 원 개발자의 코딩 의도가 애매 모호한 부분이 두군데 있다. 먼저 첫번째로 애매한 부분은 처음과 마지막의 다음 코드들이다.
j3_file.DataSource_JIBGE.DataSet := nil;
j3_file.DataSource_ILLWE.DataSet := nil;
//... 중략
j3_file.DataSource_ILLWE.DataSet := j3_file.Query_ILLWE;
j3_file.DataSource_JIBGE.DataSet := j3_file.Query_JIBGE;
초반부와 후반부에서 짝을 이루는 저 코드들은, 내 짐작으로는 DisableControls와 EnableControls로 대체되어도 무방할 것 같다. 그러나 함부로 그렇게 수정할 수는 없다. 코드만 봐서는 원 개발자의 의도를 제대로 알기 힘들기 때문이다.
의도가 애매한 또 다른 부분은 다음 코드이다.
eGongsa := J3_FILE.Query_JIBGEC.fieldbyname('sgongsa').asstring;
// 중략
if J3_FILE.Query_JIBGEC.Active then
J3_FILE.Query_JIBGEC.Locate('SGONGSA', eGongsa, [loPartialKey]);
내 짐작에는 중략 부분의 코드 실행 이전에 Query_JIBGEC의 현재 레코드 포인터 위치를 보관해 두었다가 중략 부분 실행 후에 원래 레코드 포인터 위치로 복원하는 코드인것 같다. 아무리 좋게 봐주려해도 이 개발자는 델파이 기본 문법에 대해서 상당히 문외한인 것만은 틀림이 없다.
원 개발자의 의도를 알면 더 정교한 리팩토링이 가능하지만, 이는 현재로는 불가능하므로, 첫번째는 개발자 의도를 모른다고 가정하고, 두번째는 레코드 포인터 위치 복원으로 간주하자.
다음에 보이는 코드는 원래 함수에서 전역 변수격인 j3_file을 참조하는 코드를 앞부분으로 덜어내고, Create와 Free를 ttry finally로 묶고, 반복 문자열을 상수로 덜어낸 리팩토링 버전이다. 원 코드로 봐서는 j3_file은 폼 객체이거나 데이타 모듈 객체로 추정되는데, 전역 객체를 소스 여러군데서 마구잡이로 참조하면 후일 유지보수가 매우 힘들어진다. 따라서 전역 객체(변수) 참조는 최대한 줄이고, 그 참조가 불가피하다면 그 참조 위치를 제한적으로 함이 옳을 것이다.
// 모든 리팩토링은 반드시 컴파일될수 있어야 한다. 따라서
// 컴파일 가능하게 하기 위해서 구라 클래스와 객체를 정의한다.
type
Tx = class
DataSource_JIBGE, DataSource_ILLWE : TDataSource;
Query_JIBGEC : TQuery;
Query_ILLWE, Query_JIBGE : TQuery;
end;
Tpr_jibge = class(TForm)
end;
var
j3_file : Tx;
// 리팩토링 버전.
procedure JIBGE_PR_Click2(Sender: TObject);
var
eGongsa : string;
dataSet1, dataSet2 : TDataSet;
datasource1, datasource2 : TDataSource;
query1 : TQuery;
const
FIELDNAME = 'sgongsa';
begin
with j3_file do
begin
dataSource1 := DataSource_JIBGE;
dataSource2 := DataSource_ILLWE;
dataSet1 := dataSource1.DataSet;
dataSet2 := dataSource2.DataSet;
dataSource1.DataSet := nil;
dataSource2.DataSet := nil;
query1 := Query_JIBGEC;
eGongsa := query1.fieldbyname(FIELDNAME).asstring;
end;
with Tpr_jibge.Create(nil) do
try
ShowModal;
finally
Free;
end;
if query1.Active then
query1.Locate(FIELDNAME, eGongsa, [loPartialKey]);
dataSource1.DataSet := dataSet1;
dataSource2.DataSet := dataSet2;
end;
대부분의 경우 리팩토링 버전은 원래 버전보다 소스 길이가 간략해져야 하지만, 위 코드는 오히려 늘어 났다. 이는 리팩토링 버전에 에러 체크와 구조 조정 과정을 거쳤기 때문이다. 새버전은 소스길이가 다소 길어 졌지만 그 구조는 더욱 명확해 졌다.
--------------------------------------------------------------------------------------------
리팩토링 2차 버전
--------------------------------------------------------------------------------------------
리팩토링할때 중요하게 고려할 사항은 마구잡이로 새 명칭을 도입하지 말라는 것이다. 새 명칭이란 전역 변수나, 함수나 클래스를 새로이 정의하는 것이다. 할수만 있다면, 새로운 클래스와 새로운 함수를 만들지 않고도, 기존 소스를 정갈하게 하도록 해야 한다. 새 명칭을 도입하면 소스의 구조가 복잡해지게 되기 때문이다.
간혹 어떤 개발자들은 클래스를 사용하지 않아도 될 경우에도 과도하게 클래스를 만들거나, 굳이 함수로 덜어내지 않아도 될 코드를 함수로 덜어내기도 한다. 이는 별로 바람직하지 않다. 새명칭 도입은 개발자로 하여금 암기할 것을 많게 하고 도큐멘트할 것이 많아지게 하므로, 할수만 있다면 안하는 것이 좋다.
새로운 명칭을 도입할 때는 그 명칭을 도입해서 생기는 이익이 손실을 상회할 때, 즉 공들인 바에 비해서 앞으로 매우 유익할 것을 기대할 수 있을 때만 행해야 한다. 달리 말하면, 새 명칭 도입으로 인해서 발생하는 코드 복잡도 증가 손실이, 앞으로 추가 작성해야할 코드 복잡도 증가 손실에 비해서 훨씬 이득이 있을 경우라야 한다.
항상 새로운 명칭의 도입은 신중해야 한다. 그런데 첫번째 리팩토링에서 매우 중요한 범용적 함수들이 몇개 필요함을 깨달을 수 있다. 이 때는 새 명칭 도임이 필요한 시기이다.
만일 다음과 같은 함수를 만들 수 있다면..
procedure SavePropertyAndSetNil(AComp : TComponent; AProp:String);
begin
// 어떤 객체의 특정 속성을 보관하고 그 속성을 NIL로 세팅, 나중에 자동으로 복원
end;
procedure SaveBookMark(ADataSet : TDataSet);
begin
// 데이타셋의 현재 레코드 포인터 위치를 보관하고 나중에 자동으로 복원
end;
위 함수를 사용한 리팩토링 버전은 다음과 같이 될 것이다.
procedure JIBGE_PR_Click3(Sender: TObject);
const
FIELDNAME = 'sgongsa';
begin
with j3_file do
begin
SavePropertyAndSetNil(DataSource_JIBGE, 'DataSet');
SavePropertyAndSetNil(DataSource_ILLWE, 'DataSet');
SaveBookMark(Query_JIBGEC);
end;
with Tpr_jibge.Create(nil) do
try
ShowModal;
finally
Free;
end;
end;
SavePropertyAndSetNil과 SaveBookMark는 한번 만들어두면 앞으로 요긴하게 써 먹을 수 있는 함수임에 틀림없다. 관록있는 개발자들이란, 즉 생산성 뛰어난 개발자들이란, 한번 만들어두면 두고두고 쓰먹을 수 있는 함수나 클래스가 뭔지를 직관적으로 알고, 이들을 미리 만들어 두는 사람들이라고 해도 과언이 아니다.
예전의 내 강좌를 보면 SavePropertyAndSetNil과 SaveBookMark를 어떻게 구현해야 할지 답이 있으므로 굳이 여기서 그 함수를 만들지는 않도록 하겠다.
--------------------------------------------------------------------------------------------
리팩토링 3차 버전
--------------------------------------------------------------------------------------------
만일 어떤 객체를 함수 리턴시에 자동 파괴하도록 하는 AutoFree라는 함수를 만들었다면 위 코드는 더욱 간단하게 할 수 있을 것이다.
AutoFree(객체, 객체 생성자 호출) // 리턴시에 자동으로 객체.Free를 호출하게 하도록 하는 함수
procedure JIBGE_PR_Click3(Sender: TObject);
const
FIELDNAME = 'sgongsa';
var
frm : TForm;
begin
with j3_file do
begin
SavePropertyAndSetNil(DataSource_JIBGE, 'DataSet');
SavePropertyAndSetNil(DataSource_ILLWE, 'DataSet');
SaveBookMark(Query_JIBGEC);
end;
AutoFree(frm, Tpr_jibge.Create(nil) );
frm.ShowModal;
end;
|
