views:

254

answers:

3

Hi,

Found a piece of code today, that I find a little smelly...

TMyObject.LoadFromFile(const filename: String);
begin
  if fileExists(filename) then
    self := TSomeObjectStreamer.ReadObjectFromFile(filename);
end;

If this code works, it will at least leak some memory, but does it work?
Is OK to assign to self in this manner?

What if the streamed object is of a different subclass then the original self?
What if the streamed object is of a different class with no common ancestor to the original self?

+6  A: 

You can assign to Self, but it's only a local variable and you won't actually change anything outside the scope of that method. So that code is almost certainly not going to do what the original coder apparently thinks it's going to do.

Mason Wheeler
+1  A: 

yes, you can use self as a local temporary variable, even if it is useless here. But the streamed object must be the same class as the self (TMyObject) in this case, or the compiler will detect an error, because type are not compatible.

In your example, TSomeObjectStreamer.ReadObjectFromFile() should return a TMyObject or your compielr should warn you (or throw an error)

TridenT
+2  A: 

Consider that a method is equivalent to a free routine accepting the Object as its 1st parameter named Self:

TMyClass.MyRoutine({args})  <=>  MyRoutine(Self: TMyClass {; args})

With that in mind, you see that you can locally change the content of Self without damaging your original Object.

But you are right it is really smelly and very much prone to error.

I would not accept code like that without a very strong convincing case in a comment...

François
Somehow your usage of bold makes my ears ring.
dummzeuch
@dummzeuch, ... so it is a noisy warning for smelly code! ;-)
François