TDataObject = class(TObject) // Some Shared Data here. end; TPacketWriter = class(TObject) private function CreateObjectX : TDataObject; procedure WriteFileZ(Data : TDataObject); protected procedure WriteFileX(Data : TDataObject); virtual; abstract; procedure WriteFileY(Data : TDataObject); virtual; abstract; public procedure Execute; end; TPacketWriterTypeA = class(TPacketWriter) protected procedure WriteFileX(Data : TDataObject); override; procedure WriteFileY(Data : TDataObject); override; end; TPacketWriterTypeB = class(TPacketWriter) protected procedure WriteFileX(Data : TDataObject); override; procedure WriteFileY(Data : TDataObject); override; end; ... procedure TPacketWriter.Execute; var lObj : TDataObject; begin lObj := CreateObjectX; try WriteFileX(lObj); WriteFileY(lObj); WriteFileZ(lObj); finally lObj.Free; end; end; ... // where it was used var PWA : TPacketWriterTypeA; PWB : TPacketWriterTypeB; begin PWA := TPacketWriterTypeA.create; try PWA.Execute; finally PWA.Free; end; PWB := TPacketWriterTypeA.create; try PWB.Execute; finally PWB.Free; end; end;Well you might say that is easy I can quickly add the following.
TPacketWriterNewType1 = class(TPacketWriter) protected procedure WriteFileX(Data : TDataObject); override; procedure WriteFileY(Data : TDataObject); override; end; TPacketWriterNewType2 = class(TPacketWriter) protected procedure WriteFileX(Data : TDataObject); override; procedure WriteFileY(Data : TDataObject); override; end; TPacketWriterNewType3 = class(TPacketWriter) protected procedure WriteFileX(Data : TDataObject); override; procedure WriteFileY(Data : TDataObject); override; end; TPacketWriterNewType4 = class(TPacketWriter) protected procedure WriteFileX(Data : TDataObject); override; procedure WriteFileY(Data : TDataObject); override; end; TPacketWriterNewType5 = class(TPacketWriter) protected procedure WriteFileX(Data : TDataObject); override; procedure WriteFileY(Data : TDataObject); override; end;
But I have goal that all new code should be unit tested. Let just say that WriteFileX and WriteFileY are unique in each case and they are complex pieces of code. In the real world example The Execute Method was even more complex. Writing a test on just the execute method would involve a lot of cut and paste of test code for each instance. Not something that smelled correct to me.
The key here that I want to point out is that when you see virtual; abstract; you should ask if an interface makes sense. In this case the answer I determined that an interface made sense. I refactored the TPacketWriter to implement an interface with the resulting code looking like this:
TDataObject = class(TObject) // Some Shared Data here. end; IPacketFileWriter = interface ['{9BE43D5A-5566-4218-A83A-A54D567AD986}'] procedure WriteFileX(Data : TDataObject); procedure WriteFileY(Data : TDataObject); end; TPacketWriter = class(TObject) private function CreateObjectX : TDataObject; procedure WriteFileZ(Data : TDataObject); public procedure Execute(aFileWriter : IPacketFileWriter); end; TPacketWriterTypeA = class(TInterfacedObject,IPacketFileWriter) protected procedure WriteFileX(Data : TDataObject); procedure WriteFileY(Data : TDataObject); end; TPacketWriterTypeB = class(TInterfacedObject,IPacketFileWriter) protected procedure WriteFileX(Data : TDataObject); procedure WriteFileY(Data : TDataObject); end; TPacketWriterNewType1 = class(TInterfacedObject,IPacketFileWriter) protected procedure WriteFileX(Data : TDataObject); procedure WriteFileY(Data : TDataObject); end; TPacketWriterNewType2 = class(TInterfacedObject,IPacketFileWriter) protected procedure WriteFileX(Data : TDataObject); procedure WriteFileY(Data : TDataObject); end; TPacketWriterNewType3 = class(TInterfacedObject,IPacketFileWriter) protected procedure WriteFileX(Data : TDataObject); procedure WriteFileY(Data : TDataObject); end; TPacketWriterNewType4 = class(TInterfacedObject,IPacketFileWriter) protected procedure WriteFileX(Data : TDataObject); procedure WriteFileY(Data : TDataObject); end; TPacketWriterNewType5 = class(TInterfacedObject,IPacketFileWriter) protected procedure WriteFileX(Data : TDataObject); procedure WriteFileY(Data : TDataObject); end; ... procedure TPacketWriter.Execute(aFileWriter : IPacketFileWriter); var lObj : TDataObject; begin lObj := CreateObjectX; try aFileWriter.WriteFileX(lObj); aFileWriter.WriteFileY(lObj); WriteFileZ(lObj); finally lObj.Free; end; end; ... // Where it was used var PW : TPacketWriter; begin PW := TPacketWriter.Create; try PW.Execute(TPacketWriterTypeA.Create); PW.Execute(TPacketWriterTypeB.Create); PW.Execute(TPacketWriterNewType1.Create); PW.Execute(TPacketWriterNewType2.Create); PW.Execute(TPacketWriterNewType3.Create); PW.Execute(TPacketWriterNewType4.Create); PW.Execute(TPacketWriterNewType5.Create); finally PW.Free; end; end;So why do this?
- I was able to write a test for TPacketWriter.execute using a Mock Object that implemented the Interface
- I was able to test both WriteFilesX and WriteFileY for each class independently
- Implementing new supporting types is easy and I don't have to worry about TPacketWriter.Execute when I am writing my test.
I am sure there are other reasons, but writing testable code was my primary motivation.
There are many other things I can change with this code. But the point I want to repeat.
When you see virtual; abstract; you should ask if an interface makes sense.
Great post Robert. I enjoy learning and seeing techniques to make producing our code more maintainable and easier to manage. Keep up with the good work.
ReplyDeleteBTW I also enjoyed reading your opinion on the AnyDAC buyout - well balanced and well reasoned.
Cheers,
Colin
A big problem with your example: TInterfacedObject and interface reference counting! Your code change completely alters object ownership and lifetime, which is rarely a good idea with legacy code.
ReplyDeleteA solution is to create a variant of TInterfacedObject that doesn't do reference counting - you'll still gain most of the benefits of interfaces, but your changes will have much less impact on the rest of the legacy codebase.