CODEWITHSUNDEEP
delphirefactoringdelphi-5
Hugh Jones
Hugh Jones

Reputation: 2694

Advice on refactoring some poor code

Consider the following

constructor TSettlement.Assign( const OldInst : TSettlement; const ResetFsToo: Boolean );
begin
  //inherited
  Create;
  if OldInst = nil then
    exit;

  Self.Acceptancedate := OldInst.Acceptancedate;
  // etc etc
end;

and please consider also these invocations elsewhere in the code

SettInst.Assign(DisplaySett, False);

DisplaySett := TSettlement.Assign(nil, False);

NewInst := TSettlement.Assign( Displaysett, False );

and (perhaps worst of all)

  if OldList.Count > 0 then
    for loop := 0 to OldList.Count -1 do  
      Self.Add(TSettlement.Assign(OldList.Data[loop], True));

This is leaky code and I object to the use of the method name 'Assign' for a constructor for reasons which I take to be obvious, but I am not obliged to fix it.

I want to improve it because I have pride in my work.

I am proposing to change the Assign method from a constructor to a procedure and remove the call to Create(). This will require me to change code in many places in the application. Obviously there is risk in doing this.

Before I dive in and get cracking can anyone suggest any alternative approach I should consider?

Are there possible pitfalls I have maybe not thought of that I should be aware of?

Upvotes: 1

Views: 201

Answers (1)

David Heffernan
David Heffernan

Reputation: 612914

That constructor does not necessarily leak. It's plausible that the code actually works. However, it is inpenetrable and leads to calling code whose semantics are hard to discern from the outside. You should refactor.

There's an easy way to refactor. The key is that you break up the two modes of operation into separate functions:

  1. A constructor named Create that acts like a normal Delphi constructor. You appear already to have this.
  2. A procedure that copies the contents of one instance to another. That could well be named Assign.

So, here's the plan of campaign:

  1. Change Assign into a procedure rather than a constructor.
  2. Deal with the call to Create that can be found in the current Assign. You need to remove that from Assign, but make sure that everything that it did still happens when Assign runs.
  3. Now all constructor mode calls to Assign fail to compile. So we fix them.
  4. The ones that passed nil, convert into constructor calls of Create.
  5. The other ones need to call the Create constructor and then Assign.

You might want an overloaded Create that receives an existing instance and calls Assign. That could be convenient.

That would leave you able to do all that you do today, but would avoid calling a constructor on an instance which is always a bad idea.

Upvotes: 4

Related Questions