Tom Blocker wrote:
> Explicit or untested casts don't make sense since the language gives
> us an easy way to not lay such landmines. I never let this one get by
> in code reviews. It is just too dangerous.
>
> A last note, using interfaces can also provide an alternative to
> doing a bunch of such casts thereby improving coupling and reducing
> dependencies. GetInterface basically does this very thing, test then
> use...
>
> Rob, what am I missing here?
First off, we're starting with the premise that the function in question
is always supposed to receive an instance of TMyForm, right?
Whether the function is actually declared that way, and whether all
callers follow that rule, aren't certain, but *if* a caller passes
something that is not a TMyForm, then we should consider the program to
have a bug -- it should be fixed to either pass an object of an
appropriate type, or not call our function at all.
You seem to think that I was suggesting writing code like this:
procedure Foo(F: TForm);
begin
TMyForm(F).Test := 0;
end;
That's not what I suggested, though. It doesn't do anything to prevent
the bug condition. The caller can pass any TForm, and the function will
simply assume it's of the desired type. Finding that bug will take a
long time.
Ideally, I'd prefer this:
procedure Foo(F: TMyForm);
begin
F.Test := 0;
end;
That completely eliminates the need for testing the input's type at run
time. The compiler performs all the tests for you.
But suppose for some reason you can't write that ideal function. Maybe
the function's signature is dictated by another interface, which expects
a particular function-pointer type. There are ways of ensuring that the
function still always gets an instance of TMyForm.
One is to use an assertion:
procedure Foo(F: TForm);
begin
Assert(F is TMyForm);
TMyForm(F).Test := 0;
end;
Now the function will reject any incorrect input. The caller can still
pass objects of the wrong type, but the function will raise an exception
at run time, so we'll be able to see immediately where the problem is
the first time we test the program.
We can also use a checked type-cast:
procedure Foo(F: TForm);
begin
(F as TMyForm).Test := 0;
end;
This differs slightly from an assertion. Assertions can be disabled, but
checked type-casts can't, so this latest function will always check its
input, even in release builds that have assertions turned off. Also, the
exception type will differ. That's probably not an issue, though, since
neither EAssertionFailed nor EInvalidCast are exceptions that anyone
should have much reason to catch. Either way, these latest two functions
either set the Test member to zero, or fail with an exception, which can
be noticed during debugging.
Now we modify the function to make it look like your example:
procedure Foo(F: TForm);
begin
if F is TMyForm then
(F as TMyForm).Test := 0;
end;
This function will never fail with an exception. If its input is not an
instance of TMyForm, the function will simply return to its caller as
though nothing happened. I consider that a bad thing. The premise of
this exercise is that it is an error in the program if this function
gets called on non-TMyForm objects. But the way this function is
written, it provides no help in detecting that error. We need to perform
much more thorough testing to find it. We need to test the value of the
Test member on output to ensure it's been set. Once we notice that it's
not being set, we need to step through the code to determine why.
The code performs a redundant test. Like I wrote in my previous message,
the above function works identically to this:
procedure Foo(F: TForm);
begin
if F is TMyForm then begin
if F is TMyForm then
TMyForm(F).Test := 0
else
raise EInvalidCast.Create(...);
end;
end;
But as I just explained, that code will never actually raise an
exception. The only way it could raise an exception is if a test that
succeeds the first time fails the second time. But that can't happen, so
let's simplify the code by removing the "else" branch. (The compiler
can't remove it form the code it generates for the "as" operator, but we
can remove it from our manually written equivalent.)
procedure Foo(F: TForm);
begin
if F is TMyForm then begin
if F is TMyForm then
TMyForm(F).Test := 0;
end;
end;
See the redundancy? There's no reason to invoke two "is" tests with
identical arguments in sequence like that. So let's move the second one:
procedure Foo(F: TForm);
begin
if F is TMyForm then begin
TMyForm(F).Test := 0;
end;
end;
When I wrote, "The 'as' operator performs an 'is' test anyway," maybe
you thought I was suggesting removal of the "is" test, but that's not
what I meant. I was suggesting removal of the "as" cast since the result
of the test it performs is already known and doesn't need to be
performed again.
The code you posted performs unnecessary checks, and it fails silently.
I consider those good reasons not to use that style of code. I gave my
preferred styles earlier in this message.
--
Rob
_______________________________________________
Delphi mailing list -> [email protected]
http://www.elists.org/mailman/listinfo/delphi
