7

Suppose I have the following code:

function DoSomething:Boolean;
var obj : TMyObject;
i : Integer;
begin
  Result := False; //We haven't executed GetValue() correctly yet
  obj := TMyObject.Create();
  try
    //perform some code that may produce an exception        
    i := obj.GetValue();
    //Set the return to True as we executed GetValue() successfully
    Result := True;
  finally
    //do some cleanup
    obj.Free; 
  end;
end;

The Delphi compiler is complaining that the value assigned to Result is never used in the first line.

I'm probably missing something obvious, but i don't see why the compiler would optimize this out (if optimization's are on).

I have always been taught to explicitly set my variables so as no confusion what their values are. On top of that, should the GetValue() function generate an exception, the Result := True; line will never execute. So we are at the mercy of whatever Delphi initialized the variable to be.

So is this safe/acceptable code? Should i simply remove the first line of the method, which would make it a lot harder to read? Failing that I would have to turn the specific compiler warning off, but I'm reluctant to do that as this warning message can give useful information.

Simon
  • 9,197
  • 13
  • 72
  • 115
  • About your thoughts on what the compiler does with the `Result` if you don't initialize it, the answer is **nothing**. See [Is Result variable defined from first line in a function?](http://stackoverflow.com/q/10084043/576719). – LU RD Jun 13 '12 at 07:51

2 Answers2

16

The compiler is correct. The assignment of False to Result is futile and the only value your function can return is True.

The two possible execution paths are:

  1. The function does not raise an exception and returns True.
  2. The function does raise an exception, and therefore does not return a result value at all.

The solution is simple, remove the line of code that sets Result to False. At which point it becomes completely clear that the return value serves no purpose and you can simply turn the function into a procedure.

David Heffernan
  • 601,492
  • 42
  • 1,072
  • 1,490
5

Your function has only two outcomes. Either it returns True or it will raise an exception so you could turn it into a procedure instead to make the warning go away.

If you want the result of the function to be False when GetValue() raises an exception you have to capture that exception in DoSomething and set the return value to False. In that case you should start your function initializing the return value to True.

Something like this:

function DoSomething:Boolean;
var
  obj : TMyObject;
  i: Integer;
begin
  Result := True;
  obj := TMyObject.Create();
  try
    try
      i := obj.GetValue();
    except
      Result := False;
    end;
  finally
    obj.Free;
  end;
end;
Mikael Eriksson
  • 136,425
  • 22
  • 210
  • 281
  • So i should have a nested `try..except` in there :( – Simon Jun 13 '12 at 05:52
  • If that is what you want. You should not use the return value from the function `DoSomething` when it raises an exception to the caller so the initializing of Result to `False` should never be used. Either you handle the exception in `DoSomething` or you do it higher up.. – Mikael Eriksson Jun 13 '12 at 05:58
  • -1 This answer gets it badly wrong. The code in the question can raise an exception. The code in this answer swallows exceptions. These two pieces of code have wildly different behaviour. – David Heffernan Jun 13 '12 at 06:15
  • 3
    @DavidHeffernan I guess you missed this part of my answer "If you want the result of the function ....". And yes, it changes the behavior of the function. – Mikael Eriksson Jun 13 '12 at 06:18
  • Look at the comment from Simon. Simon now things that the best course of action is to swallow all exceptions. That's pretty unlikely. – David Heffernan Jun 13 '12 at 06:21
  • 2
    I understand not to swallow all exceptions. In my particular instance, it is ok for an exception to be thrown and the app can continue execution. – Simon Jun 13 '12 at 08:59