Delphi instance methods and value types

Delphi 3 Minutes

David Heffernan commented on Girish Patil’s post on G+

But you should never make instance methods on value types that mutate the value.

Otherwise you can call such a method on an instance passed to a function as a const parameter.

Where “value types” in this case is meant to be an advanced record in Delphi.

I must admit that I am guilty of doing this and have of course lived to regret it. Here is an example:

type
  TDistance = record
  private
    FMillimetres: Int64;
  public
    function InMillimetres: Int64;
    function InMetres: Double;
    function InKm: Double;
    procedure AssignMetres(_Value: Double); // don't do this!
  end;

In case it isn’t obvious: This is an advanced record for storing a distance with a fixed resolution on 1 mm. The idea is to prevent cases where you have got a variable containing a distance but it isn’t immediately clear what unit is it using.

There are functions that return the value converted to metres and kilometres. And then there is a procedure AssignMetres which violates the rule that David stated:

procedure TDistance.AssignMetres(_Value: double);
begin
  FMillimetres := Round(_Value * 1000);
end;

On first glance, there is nothing wrong with this method. It simply assigns a new value to the record.

But now, consider this procedure that takes a const TDistance parameter:

procedure doSomething(const _Dist: TDistance);
begin
  // ...
  Dist.AssignMetres(5);
  // ...
end;

// ...
var
  SomeDist: TDistance;
begin
  SomeDist.AssignMetres(3);
  doSomething(SomeDist);
  // ...

The compiler won’t complain because calling methods of value types passed as const is allowed. But since the method has a side effect, setting the value of _Dist, it will not just change the value of _Dist inside the procedure but also the value of the variable passed to doSomething. The caller of course relies on that value to remain unchanged, because this is a const parameter after all. But, after the call to doSomething, the value of SomeDist now is 5m rather than 3m as originally assigned.

That’s what David meant with this comment.

This was bad enough, but there is another trap which I fell for. Consider this class that has a property Length of the type TDistance:

 type
  TCar = class
  private
    FHasChanged: boolean;
    FLength: TDistance;
    procedure SetLength;
  public
    property Length: TDistance read GetLength write SetLength;
    property HasChanged: read FHasChanged;
  end;
// ...
function TCar.GetLength: TDistance;
begin
  Result := FLength;
end;

procedure TCar.SetLength(const _Value: TDistance);
begin
  FHasChanged := True;
  FLength := _Value;
end;

Again, nothing seems to be wrong here. The SetLength property setter, in additon to setting the FLength field also sets the FHasChanged field to True which supposedly is read later on to determine whether any changes have to be saved somewhere. You could probably argue that the property getter for Length is not necessary, but who knows, it might become necessary later on.

It works fine too:

var
  MyCar: TCar;
  Dist: TDistance;
begin
  Dist.AssignMetres(4.5);
  MyCar := TCar.Create;
  try
    MyCar.Length := Dist;
    if MyCar.HasChanged then
      MyCar.SaveChanges;
  finally
    FreeAndNil(MyCar)
  end;

And then, probably years later, somebody thinks that the code above is unnecessarily complex and optimizes it like this:

var
  MyCar: TCar;
begin
  MyCar := TCar.Create;
  try
    MyCar.Length.AssignMetres(4.5);
    if MyCar.HasChanged then
      MyCar.SaveChanges;
  finally
    FreeAndNil(MyCar)
  end;

That’s a reasonable optimization. It looks much cleaner and gets rid of an unnecessary variable declaration. But it does not work!

Why? I’m glad you asked. Let’s have a look at what happens in this line:

MyCar.Length.AssignMetres(4.5);

It first calls the getter for the Length property, returning a copy of the FLenght field. Then it calls the AssignMetres method of that record, which changes the FMillimetres field of the record. Now, the question: What will be the value of the field MyCar.FLength? It will still be 0 (assuming the constructor of TMyCar does not initialize it otherwise), because we changed the value of the copy, not the value of the field. Also, MyCar.HasChanged will still be False, because the setter for Length has never been called.

So, advanced records should not have methods that change the record’s values. In the example, the solution would be something like this:

type
  TDistance = record
  private
    FMillimetres: Int64;
  public
    class function FromMetres(_Value: Double): TDistance; static;
    function InMillimetres: Int64;
    function InMetres: Double;
    function InKm: Double;
  end;

/// ...
class function TDistance.FromMetres(_Value: double): TDistance;
begin
  Result.FMillimetres := Round(_Value * 1000);
end;

So, a class function FromMetres would be used to return a new TDistance variable initialized to the given metres value.

And it would be used like this:

var
  MyCar: TCar;
begin
  MyCar := TCar.Create;
  try
    MyCar.Length := TDistance.FromMetres(4.5);
    if MyCar.HasChanged then
      MyCar.SaveChanges;
  finally
    FreeAndNil(MyCar)
  end;

The setter method for Length gets called does its thing. Everybody is happy. Until, of course, some smart ass like me thinks that an TDistance.AssignMetres would be a great idea …

Published