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.