Monday, February 18, 2013

Legacy Code: Scoping

Hey when I change X why does it break Y when they "should" be completely unrelated.   It's a common problem in development where changing one bit of code has unintended side effects on a different section of code.    Proper varible scoping can help reduce these errors.  Unit testing is the other piece to solving this problem.

Lets start with the most common global variables.   They are are almost always bad, in fact it's really hard to come up with an example of where I feel like they should be used.   However, by default Delphi creates several of them for you.

Specifically, when you create a new form or datamodule there is a variable in that unit that represents that form or datamodule.     We have tried to remove these with varying degrees of success.  Over the years the visual designers have given us errors after removing them.   Not to mention if auto created then the variable is needed.    So now by default we ignore and don't use them.   However, there are exceptions and here is one story....

Our application uses a single database connection of type TSqlConnection.   It was placed on a TDataModule and let everyone use it.   Every demo you see with Delphi told you do it this way.    It made everything really easy, until...

We had new needs which turned into problems.
  1. We wanted to take a section of code, and multi-thread it.    You need to have a database connection per thread, to pull this off.    All of that code was immediately broken and had to be changed.   
  2. We wanted to use a separate set of connection parameters when doing different tests.
  3. We wanted to unit test without creating the whole data module, which over time became a place for developers who were lazy to place common code.
  4. We had non visual code that was used in more than one application.    The other applications had different requirements on how the connection needed to be setup.   So those had connections setup in a different unit than the original global version.
So what have we done to help resolve this problem...

Our code base really depends on TFrame we have several hundred them, actually we were lucky all our frames used a common TFrame Descendant that we created.   Each frame typically represents a new tab in our application.   Each frame is created in a common factory.   This factory has knowledge of the database connection.  So one of many things we did, to help us for the future was to add a Connection property to the common TFrame Descendant.   The connection property is set in the common factory.    Now frames have a connection they can access without directly accessing the global connection variable.    

Now one by one as we visit each screen we are able to remove the global references, and use the new Connection property.   We still have several frames that use the global connection.    But sometimes you have incrementally attack a problem, as you can't rewrite each time decide you hate the code your looking at, otherwise I would never make a deadline.

It is not just Global variables that are a problem, it's improperly scoped variables.    Here is one that I have seen done multiple times.

unit Unit1;

interface
uses SysUtils, Classes, Forms, SqlExpr;
type
  TExample1 = class(TForm)
    MyQuery : TSqlQuery;
  protected
  public
    procedure DoSomething;
  published
  end;

var
  MyExample1 : TMyExample1;

implementation

{$R *.dfm}

{ TExample1 }

procedure TExample1.DoSomething;
begin
  MyQuery.SQL.Clear;
  MyQuery.SQL.Add('SELECT * FROM SOMETABLE');
  MyQuery.Open;
  while not MyQuery.EOF do
  begin
     // Do Something with MyQuery
     MyQuery.Next;
  end;
  MyQuery.Close;
end;

end.

Every time I see code like this it's typically want to scream not again, although it is a quick and easy refactor.

First off let's explore what the problem is with this code.   MyQuery was a Component that was dropped on to the form.   When you drop a component on a form, anyone who has access to the form can change it because if you don't specify, private, protected or public the default behavior is published!   The DFM Streaming mechanism depends on this so there is not much you can do to change this.

However, dropping components on to a form or Datamodule that are only used in one method is like asking someone to abuse you.

Instead change your code to create the query like this.


unit Unit1;

interface
uses SysUtils, Classes, Forms, SqlExpr;
type
  TExample1 = class(TForm)
  protected
  public
    procedure DoSomething;
  published
  end;

implementation

{ TExample1 }

procedure TExample1.DoSomething;
var
 MyQuery : TSQLQuery;
begin
  MyQuery := TSqlQuery.Create(nil);
  try
    MyQuery.SQLConnection := SomeConnection; // Hopefully passed to you and not global
    MyQuery.SQL.Clear;
    MyQuery.SQL.Add('SELECT * FROM SOMETABLE');
    MyQuery.Open;
    while not MyQuery.EOF do
    begin
       // Do Something with MyQuery
       MyQuery.Next;
    end;
    MyQuery.Close;
  finally
    MyQuery.Free;
  end;
end;

end.


Notice the MyQuery is no longer declared in published section but is now local to DoSomething.

 The key point of all of this...

 When writing code you should scope the variable declarations to be as close as possible to the code that uses it.

Thursday, February 14, 2013

Legacy Code: Abstract to Interface

Say you where given the following legacy code snippet, and where told that you needed five new  packet types that need to be written.

   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?

  1. I was able to write a test for TPacketWriter.execute using a Mock Object that implemented the  Interface
  2. I was able to test both WriteFilesX and WriteFileY for each class independently 
  3. 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 virtualabstract; you should ask if an interface makes sense.


Legacy Code

Today, It has been 18 years since Delphi was released.      Some Delphi projects have been around for quite some time now.     I think we all look back on code we wrote a year or two ago that we would write in a completely different way today.   Sometimes is because a new feature of the language or a new library is available, and sometimes it is just that you have learned a better way to do something.

Where I work....
In 1998 there was a mainframe application, that was not going to make it through the year 2000 switch.   A decision was made to rewrite the application and Delphi was chosen.     We have had several other applications that were started after the year 2000 that were written in other technologies.   Many of the these have had to be completely rewritten.     However, our not so little Delphi application not only survives it thrives.  Business is always changing we are constantly updating our application to meet these ever changing needs.   

Much of the original code written in 1998 still exists in some form, with many more lines of code after that.   The team has evolved over that time as well, we only have one developer left who started the project.  I estimate that we have had 40+ unique developers who have worked on this code base.   Each unique,
each had there own coding style and skill sets.    

Some of the past code is very procedural with tons of global variables and is written in a such a way that it's very difficult to test and maintain.   Where new code is written using better practices, and with good unit test coverage.     But it's not uncommon to be asked to change an area of the application that has not been significantly changed since it was first written in 1998.   When faced with such a change, how do I do that in a safe way and not continue to keep the code looking like it was written in 1998.

I have been collecting various snippets of code and how I changed them for the better.   Today I will be starting a series of blog posts on dealing with legacy code.   I suspect this may generate a bit of debate and that conversation is welcome.  I have learned that there is always a better way.     I have also learned that what works from problem X may be the complete wrong solution for problem Y.      

My hope is that what I write will help others to think in new and different ways when dealing with code.


Thursday, February 7, 2013

AnyDAC - worries with some real excitement.

When I read Marco Cantu's Post titled "Embarcadero Buys AnyDAC"  I began to worry.

Where I work we spent a great deal of time converting from the BDE to dbExpress.    Granted we did it faster than we thought it would take, it's not something I am ready to do again.    Having to switch from dbExpress to another Database Layer was really not going to be an option for us.    So I spent some time really reading the documentation on AnyDAC.

From what I have read this has turned from worry to exciting news, with a one thing that may need to be done.

First off let me address my worry about dbExpress.
What is lacking that would need to developed.  (Maybe already done since I have no code to look at yet, and I am just reading docs)
  • Ability to have a TSQLConnection use an existing TADConnection class or vice versa.
  • I basically think it would be great if AnyDAC existing connection could become an dbExpress Driver.
  • I know enough about the dbExpress framework to know this is something that would be very possible to do.   
  • In the long run this would also allow Embarcadero to only have to support one set of drivers and not one for dbExpress and one for AnyDAC.       
  • If Embarcadero does something stupid and don't do this and just stop maintaining dbExpress Drivers without providing a bridge then I would have to stop upgrading Delphi versions, but I really don't see that happening.    
This would allow existing dbExpress applications to start using AnyDAC without having to have two connections to the database.   

Then I started looking at the features of AnyDAC and found several that are really appealing.  
In addition I noticed that AnyDAC really has a good cross platform story.   Which might be a huge thing for the Mobile editions of Delphi.   Granted I think that connecting directly to a remote database via a mobile application is a security risk.

Overall I think this will be a good thing, depending on the decisions made by Embarcadero that have yet to be made.