The With statement … some people lThe with statement in Delphi … it has been the source of many discussions … Some people swear by it, other people hate it. I’m mostly in the latter camp. My personal opinion is dat the use of with should be limited as much as possible and that the use of the with statement is in most cases used to camouflage ‘bad coding’. Let me explain myself …
What is the With statement in Delphi ?
Before we start, let me try to explain what the With statement is. It’s actually just a convenience statement in Delphi wihch can be used to reference elements of a complex variable. In theory it simplifies code by removing the need to repeat the complex variable name when referencing it’s elements. For example …
oCustomer.Name := 'Devia';
oCustomer.Type := ctCompany;
oCustomer.Country := 'Belgium';
This code can also be written as :
with oCustomer do
begin
Name := 'Devia';
Type := ctCompany;
Country := 'Belgium';
end;
Another example might be when creating an instance of an object and setting some of it’s properties like :
with TCustomerReport.Create( Application ) do
begin
Try
Caption := 'Customer Report';
PreviewReport;
Finally
Free;
end;
end;
In such simple cases, I do not mind the use of the with statement all that much. Although personally I don’t really use the last structure frequently. I would actually use something like :
var
oReport : TCustomerReport;
begin
oReport := TCustomerReport.Create( Application );
try
oReport.Preview;
finally
FreeAndNil( oReport );
end;
end;
This is just my personal preference, but I have found this last one to be a little bit easier to debug.
The With statement can easily make code more confusing !
Yes, things can quickly get more confusing. Not only can it get more confusing for you to read, but also confusing for the compiler. This is especially true for some nested with statements. One example I have seen looked like this :
procedure TMyForm.ButtonClick( Sender : TObject );
begin
with FCustomer, FCompany do
begin
Name := 'Devia';
Country := 'Belgium';
DoSomething;
end;
end;
Now the question is raised which object will get it’s name set ? Will it change the name of FCompany, FCustomer or maybe even the name of the form ? To show that it can get confusing, simply put a breakpoint on the Country line and hover with your mouse over the Name := ‘Devia’ line. Even the debugger isn’t 100% sure which name it should show. In my case (Delphi 10.4) it shows the contents of the Form’s name property.
Using the debugger windows to Evaluate / Modify the Name value does the same thing. So does the Watch List actually … There you could add a watch for Name, one for FCusotmer.Name and one for FCompany.Name and you will see that it changes the Name of the company.
How delphi actually resolves the with statement is using the following set of rules.
- First it checks the members of the references in the With statement starting from the innermost with statement.
- Local variables or parameters, including implicitely defined ones such as Result and Self
- Members of Self.
- Global Variables in the same unit
- Global Variables in other units. Here too it will start with the last unit in the Uses list and work it’s way up.
In our example simply changing the order of the 2 references in the with statement has a completely different result …
procedure TMyForm.ButtonClick( Sender : TObject );
begin
with FCompany, FCustomer do
begin
Name := 'Devia';
Country := 'Belgium';
DoSomething;
end;
end;
In this case the name of the Cusotmer will be set, since using the rules mentioned above the FCustomer is the first reference which matches the given rules. This is one of the reasons why I don’t really use the with statement all that much myself. Personally I would write the code like this :
procedure TMyForm.ButtonClick( Sender : TObject );
begin
FCompany.Name := 'Devia';
FCompany.Country := 'Belgium';
FCusotmer.DoSomething;
end;
To me, this is easier to read. Yes … I might have some repetitive things in there, but even debugging is a bit easier now. Simply hovering over the name shows me the correct thing. Logic because there is absolutely no doubt as to what you are refering to.
The With statement is often used to camouflage bad code !
I know … people will hate me for this statement (and some people who worked with me probably already do), but personally I do believe that in a lot of cases the use of the with statement is simply an easy way to camouflage bad coding. I will try to explain this using an anonimized / shortened piece of code I found in a project I had been given :
procedure TfrmTest.RefreshData();
begin
with dtnTest do begin
// Adding Info data for this Order ID
cdsInfo.EmptyDataSet;
cdsInfo.DisableControls;
try
with qryList do begin
Close();
Parameters.ParamByName('id').Value := _i_Current_Id;
Open();
First();
while not eof do begin
cdsInfo.Insert;
cdsInfo.FieldByName('id').AsInteger := FieldByName('id').AsInteger;
cdsInfo.FieldByName('Reference').AsInteger := FieldByName('Reference').AsInteger;
cdsInfo.FieldByName('Qty').AsInteger := FieldByName('Qty').AsInteger;
cdsInfo.FieldByName('Desc').AsString := FieldByName('Desc').AsString;
cdsInfo.FieldByName('WeightBrut').AsFloat := FieldByName('WeightBrut').AsFloat;
cdsInfo.FieldByName('WeightNet').AsFloat := FieldByName('WeightNet').AsFloat;
cdsInfo.FieldByName('Height').AsFloat := FieldByName('Height').AsFloat;
cdsInfo.FieldByName('Length').AsFloat := FieldByName('Length').AsFloat;
cdsInfo.FieldByName('Widht').AsFloat := FieldByName('Widht').AsFloat;
cdsInfo.FieldByName('Color').AsString := FieldByName('Color').AsString;
cdsInfo.FieldByName('PackageType').AsString := FieldByName('PackageType').AsString;
cdsInfo.FieldByName('Fragile').AsBoolean := FieldByName('Fragile').AsBoolean;
cdsInfo.Post;
Next;
end;
end;
cdsInfo.IndexFieldNames := 'id';
cdsInfo.First;
finally
cdsInfo.EnableControls;
end;
end;
end;
This is only a short piece of that method, and component / field names have been modified to anonimize the code. But this is code I see on a daily basis. The use of the With statement here is done to remove some repetitive object references. Otherwise the code would look like this :
procedure TfrmTest.RefreshData();
begin
// Adding Info data for this Order ID
dtmTest.cdsInfo.EmptyDataSet;
dtmTest.cdsInfo.DisableControls;
try
dtmTest.qryList.Close();
dtmTest.qryList.Parameters.ParamByName('id').Value := _i_Current_Id;
dtmTest.qryList.Open();
dtmTest.qryList.First();
while not dtmTest.qryList.eof do
begin
dtmTest.cdsInfo.Insert;
dtmTest.cdsInfo.FieldByName('id').AsInteger := dtmTest.qryList.FieldByName('id').AsInteger;
dtmTest.cdsInfo.FieldByName('Reference').AsInteger := dtmTest.qryList.FieldByName('Reference').AsInteger;
dtmTest.cdsInfo.FieldByName('Qty').AsInteger := dtmTest.qryList.FieldByName('Qty').AsInteger;
dtmTest.cdsInfo.FieldByName('Desc').AsString := dtmTest.qryList.FieldByName('Desc').AsString;
dtmTest.cdsInfo.FieldByName('WeightBrut').AsFloat := dtmTest.qryList.FieldByName('WeightBrut').AsFloat;
dtmTest.cdsInfo.FieldByName('WeightNet').AsFloat := dtmTest.qryList.FieldByName('WeightNet').AsFloat;
dtmTest.cdsInfo.FieldByName('Height').AsFloat := dtmTest.qryList.FieldByName('Height').AsFloat;
dtmTest.cdsInfo.FieldByName('Length').AsFloat := dtmTest.qryList.FieldByName('Length').AsFloat;
dtmTest.cdsInfo.FieldByName('Widht').AsFloat := dtmTest.qryList.FieldByName('Widht').AsFloat;
dtmTest.cdsInfo.FieldByName('Color').AsString := dtmTest.qryList.FieldByName('Color').AsString;
dtmTest.cdsInfo.FieldByName('PackageType').AsString := dtmTest.qryList.FieldByName('PackageType').AsString;
dtmTest.cdsInfo.FieldByName('Fragile').AsBoolean := dtmTest.qryList.FieldByName('Fragile').AsBoolean;
dtmTest.cdsInfo.Post;
dtmTest.qryList.Next;
end;
dtmTest.cdsInfo.IndexFieldNames := 'id';
dtmTest.cdsInfo.First;
finally
dtmTest.cdsInfo.EnableControls;
end;
end;
So yes … the use of the With statement removed all the repetetive references to the dtmTest datamodule and the QryList query on that datamodule.
Did it make the code more readable or easier to debug ? In my humble opinion … not at all. Did it camouflage bad code or bad coding principles ? YES !!
Let me give a simple abstract analogy which might look stupd at first glance, but it will help explain what I mean with camouflaging bad code … What if the code / logic was the process of preparing a meal or something similar, then the code above could look something like this :
procedure Customer.OrderSteakFrietInRestaurant();
begin
Kitchen.CleanUp;
Kitchen.TakePicture;
try
Kitchen.TakePanAndFrier;
Kitchen.Recipebook.Open;
Kitchen.Recipebook.FindRecipe('Steak Friet');
Kitchen.RecipeBook.Ingredients.First;
while not Kitchen.RecipeBook.Ingredients.IsLastIngredient do
begin
Kitchen.RecipeBook.Ingredients.Ingredient.TakePackage;
if ( Kitchen.RecipeBook.Ingredients.Ingredient.BelgianFries ) then /// Yes BELGIAN FRIES :-)
begin
Kitchen.RecipeBook.Ingredients.Ingredient.PutInFrier;
end
else
begin
Kitchen.RecipeBook.Ingredients.Ingredient.PutInPan;
end;
Kitchen.RecipeBook.Ingredients.NextIngredient;
end;
Kitchen.Frier.TurnOn;
Kitchen.Frier.TurnTo140Degrees;
Kitchen.Frier.FryFriesForTheFirstTime;
Kitchen.Frier.TurnTo180Degreesl
Kitchen.Frier.FryFriesForTheSecondTime;
Kitchen.Pan.PutOnFire;
Kitchen.Pan.LetPanHeat;
Kitchen.Pan.AddButterOrOil;
Kitchen.Pan.AddSteak;
Kitchen.Pan.LetSteakCook( 2 ); // 2 minutes for rare, maybe 4 for medium
Kitchen.Pan.TurnSteak;
Kitchen.Pan.LetSteakCook( 2 ); // 2 minutes for rare, maybe 4 for medium
....
finally
Kitchen.CleanAsOnPictureYouTookAtTheStart;
end;
end;
I know … very abstract, a bit silly, but that’s actually what a perfect translation of what the logic might be which is coded in here. Yes … you can remove a lot of the repetitive object references by using With statements, but that is actually besides the point here.
The point is … this logic should never have been here in the first place. If a customer wants to order a Steak Friet in a restaurant, he should not be responsible for the whole process. Heck … he shouldn’t even know about the recipe of how to cook the steak. The only thing he wants is to order a steak!
In our example the actual logic of how to prepare the meal should be moved to the object which is responsible for preparing the meal. And please don’t make me responsible for that or you will end up with somehting completely different on your plate.
Going back to our initial piece of code :
procedure TfrmTest.RefreshData();
begin
with dtnTest do begin
// Adding Info data for this Order ID
cdsInfo.EmptyDataSet;
cdsInfo.DisableControls;
try
with qryList do begin
Close();
Parameters.ParamByName('id').Value := _i_Current_Id;
Open();
First();
while not eof do begin
cdsInfo.Insert;
cdsInfo.FieldByName('id').AsInteger := FieldByName('id').AsInteger;
cdsInfo.FieldByName('Reference').AsInteger := FieldByName('Reference').AsInteger;
cdsInfo.FieldByName('Qty').AsInteger := FieldByName('Qty').AsInteger;
cdsInfo.FieldByName('Desc').AsString := FieldByName('Desc').AsString;
cdsInfo.FieldByName('WeightBrut').AsFloat := FieldByName('WeightBrut').AsFloat;
cdsInfo.FieldByName('WeightNet').AsFloat := FieldByName('WeightNet').AsFloat;
cdsInfo.FieldByName('Height').AsFloat := FieldByName('Height').AsFloat;
cdsInfo.FieldByName('Length').AsFloat := FieldByName('Length').AsFloat;
cdsInfo.FieldByName('Widht').AsFloat := FieldByName('Widht').AsFloat;
cdsInfo.FieldByName('Color').AsString := FieldByName('Color').AsString;
cdsInfo.FieldByName('PackageType').AsString := FieldByName('PackageType').AsString;
cdsInfo.FieldByName('Fragile').AsBoolean := FieldByName('Fragile').AsBoolean;
cdsInfo.Post;
Next;
end;
end;
cdsInfo.IndexFieldNames := 'id';
cdsInfo.First;
finally
cdsInfo.EnableControls;
end;
end;
end;
Should actually be nothing more than :
procedure TfrmTest.RefreshData();
begin
dtmTest.RefreshDataForTestScreen;
end;
All use of with statements on the form are gone, because we moved the actual logic somewhere else and are only calling the method which will take care of the logic. The actual logic should be moved to the dtmTest :
procedure TdtmTest.RefreshDataForTestScreen;
begin
cdsInfo.EmptyDataSet;
cdsInfo.DisableControls;
try
qryList.Close();
qryList.Parameters.ParamByName('id').Value := _i_Current_Id;
qryList.Open();
qryList.First();
while not qryList.eof do
begin
cdsInfo.Insert;
cdsInfo.FieldByName('id').AsInteger := qryList.FieldByName('id').AsInteger;
cdsInfo.FieldByName('Reference').AsInteger := qryList.FieldByName('Reference').AsInteger;
cdsInfo.FieldByName('Qty').AsInteger := qryList.FieldByName('Qty').AsInteger;
cdsInfo.FieldByName('Desc').AsString := qryList.FieldByName('Desc').AsString;
cdsInfo.FieldByName('WeightBrut').AsFloat := qryList.FieldByName('WeightBrut').AsFloat;
cdsInfo.FieldByName('WeightNet').AsFloat := qryList.FieldByName('WeightNet').AsFloat;
cdsInfo.FieldByName('Height').AsFloat := qryList.FieldByName('Height').AsFloat;
cdsInfo.FieldByName('Length').AsFloat := qryList.FieldByName('Length').AsFloat;
cdsInfo.FieldByName('Widht').AsFloat := qryList.FieldByName('Widht').AsFloat;
cdsInfo.FieldByName('Color').AsString := qryList.FieldByName('Color').AsString;
cdsInfo.FieldByName('PackageType').AsString := qryList.FieldByName('PackageType').AsString;
cdsInfo.FieldByName('Fragile').AsBoolean := qryList.FieldByName('Fragile').AsBoolean;
cdsInfo.Post;
qryList.Next;
end;
cdsInfo.IndexFieldNames := 'id';
cdsInfo.First;
finally
cdsInfo.EnableControls;
end;
end;
And for me, this would even be the first step to refactoring the code. Next up I would even split up this code in smaller blocks so it’s even easier to maintian and reuse. For esample … the opening of the qryList could be moved to it’s own method.
procedure TdtmTest.GetInfoForId( const aId : Integer );
begin
qryList.Close();
qryList.Parameters.ParamByName('id').Value := aId;
qryList.Open();
end;
procedure TdtmTest.RefreshDataForTestScreen;
begin
cdsInfo.EmptyDataSet;
cdsInfo.DisableControls;
try
GetInfoForId( _i_Current_Id );
qryList.First();
while not qryList.eof do
begin
cdsInfo.Insert;
cdsInfo.FieldByName('id').AsInteger := qryList.FieldByName('id').AsInteger;
cdsInfo.FieldByName('Reference').AsInteger := qryList.FieldByName('Reference').AsInteger;
cdsInfo.FieldByName('Qty').AsInteger := qryList.FieldByName('Qty').AsInteger;
cdsInfo.FieldByName('Desc').AsString := qryList.FieldByName('Desc').AsString;
cdsInfo.FieldByName('WeightBrut').AsFloat := qryList.FieldByName('WeightBrut').AsFloat;
cdsInfo.FieldByName('WeightNet').AsFloat := qryList.FieldByName('WeightNet').AsFloat;
cdsInfo.FieldByName('Height').AsFloat := qryList.FieldByName('Height').AsFloat;
cdsInfo.FieldByName('Length').AsFloat := qryList.FieldByName('Length').AsFloat;
cdsInfo.FieldByName('Widht').AsFloat := qryList.FieldByName('Widht').AsFloat;
cdsInfo.FieldByName('Color').AsString := qryList.FieldByName('Color').AsString;
cdsInfo.FieldByName('PackageType').AsString := qryList.FieldByName('PackageType').AsString;
cdsInfo.FieldByName('Fragile').AsBoolean := qryList.FieldByName('Fragile').AsBoolean;
cdsInfo.Post;
qryList.Next;
end;
cdsInfo.IndexFieldNames := 'id';
cdsInfo.First;
finally
cdsInfo.EnableControls;
end;
end;
Next up … it seems as if they copy all the fields from the QryList into or Info DataSet,so I would probably create a rountine which copies all the fields from aSource dataset to aDestinaiton dataset and use that instead. If only a given set of fields need to be copied, then I would add a routine which copies only the given fields, …
Heck … why not create those routines on a TDataSet class helper, that way it’s even easier to reuse.
The Result
So … I hope that using that silly example helped to show you that I was correct in saying the With statemnt is too often used to camouflage bad code or bad coding principles. By moving my code to the class who should be responsible for that logic, I have removed a lot of with statements.
The code is now also easier to reuse and maintain. I personally find it easier to read too, although some people will argue about that. Now … what was the result of my refactoring ? Well … something like this :
procedure TdtmTest.RefreshDataForTestScreen;
begin
cdsInfo.EmptyDataSet;
cdsInfo.DisableControls;
try
GetInfoForId( _i_Current_Id );
qryList.ForEachRecord
(
procedure ( aDataSet : TDataSet )
begin
cdsInfo.Insert;
cdsInfo.CopyFieldsValuesFromDataSet( aDataSet );
cdsInfo.Post;
end
);
cdsInfo.IndexFieldNames := 'id';
cdsInfo.First;
finally
cdsInfo.EnableControls;
end;
end;
In short … all repetitive logic have been moved to the class which should be responsible for that logic.
Looping over every record in a DataSet is something we do a lot. In short, we will disable the contorls, go to the first record, loop until we are at the last record. For each record we will do something, and once we are at the end of the dataset, or something happens, we will make sure the controls are enabled again. This logic was moved to a method on a TDataSet Class Helper.
Something similar was done for copying all field values from one dataset to antoher. In short, we will first check if the destination dataset is in Edit or Insert mode. Next, for every field found in the source dataset, we will check if a field with the same name can be found in de destination dataset. If that is the case, we will check if the destination field can be modified. If it isn’t ReadOnly we will fill it with the value of the source field. Again an ideal candidate for a method on a TDataSet Class Helper.
So … What do you think about this ?
I’m quite curious on what you folks think about all this. Do you agree with me that the use of the with statment doesn’t really make thinkgs easier to maintian ? Do you also think that it’s too often used to hide bad coding principles ? Would there be other things you would improve ? Feel free to let me know in the comment section…
