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:
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 …