Avoiding “variable might not have been initialized”

12
votes

I recently ran across a routine that looks something like this:

procedure TMyForm.DoSomething(list: TList<TMyObject>; const flag: boolean);
var
  local: integer;
begin
  if flag then
    //do something
  else local := ExpensiveFunctionCallThatCalculatesSomething;

  //do something else
  for i := 0 to list.Count do
    if flag then
      //do something
    else if list[i].IntValue > local then //WARNING HERE
        //do something else
end;

This gives Variable 'local' might not have been initialized even though you can tell by reading the code that you won't hit that line unless the code branch that initializes it has run.

Now, I could get rid of this warning by adding a useless local := 0; at the top of the procedure, but I wonder if there might not be a better way to structure this to avoid the issue. Anyone have any ideas?

5
delphirefactoringcompiler-warnings
I thought pre-initializing variables was good practise considering they otherwise point to random data in memory and could lead to unexpected behavior in your application if you don't pay attention to it :) C# I think wont even allow you to run that code because local isn't initialized. - Jonas B
Oh, I know it's a good practice. I just wonder if there isn't a better way to write the code where I don't need to initialize it in order to not get that warning. (And turning off warnings is cheating.) - Mason Wheeler
I guess the compiler just isn't that sophisticated. It probably only goes as far as seeing that local might be initialized. - Igby Largeman
"even though you can tell by reading the code that you won't hit that line unless the code branch that initializes it has run" -- The compiler is smart, but not that smart. It's a subtle thing you are doing to make sure that local is never accessed if it isn't initialized, and it errs on the side of the warning. - Nick Hodges
Somebody really should also point out the index over-run on the loop counter. ;) - Deltics

5 Answers

12
votes

I would separate it into two for-loops--one for when flag is true and one for when flag is false. As an added benefit, you won't have to execute the if-statement on every iteration.

6
votes

IMO, the assignment to 0 isn't useless here - it's beneficial to maintanability. So you'll save someone (perhaps your future self) from having to spend a minute or two to determine that the code works. And the design cleverness will likely be lost on them (even if it's you!)

6
votes

Refactor the code to contain two separate flows based on the flag parameter:

procedure TMyForm.DoSomething(list: TList<TMyObject>; const flag: boolean);
var
  local: integer;
begin
  if flag then
  begin
    //do something
    //do something else
    for i := 0 to Pred(list.Count) do
      //do something
  end
  else
  begin
    local := ExpensiveFunctionCallThatCalculatesSomething;

    //do something else
    for i := 0 to Pred(list.Count) do
      if list[i].IntValue > local then
        //do something else
  end;
end;

This essentially restates the answer given by neilwhitaker1, but also makes it clear that the initialisation of the local variable is to be brought inside the conditional branch, which is what addresses the compiler warning (which is only emitted if the varialbe used in a branch where it may not be initialised - in the branch that does not use it at all no such warning shall be emitted, and in the branch where it is used it is certain to be initialised, and since it is used in one branch you will not get a "may not be used" hint either.

NOTE: If any of the "// something else"'s are common to each branch, these could of course be refactored as local, nested procedures to avoid duplication.

ALSO NOTE: In the code above I have corrected the loop index over-run on the for loop. :)

1
votes

adding local:=0 good solution.

as i know the hints because can be uninitialized variables. if you always assign variable values for initing and using try finally blocks for error checking you can't live any problems.

as i know compiler give hints because even if you check flag at runtime your variable can be unassigned and your code can't run as expected.

sorry for my bad english :)

1
votes

i would change the top of your procedure to

procedure TMyForm.DoSomething(list: TList<TMyObject>; const flag: boolean);
var
  local: integer;
begin
  if flag then
   begin
     local := 0;
     //do something
   end
  else local := ExpensiveFunctionCallThatCalculatesSomething;

etc...

That way Local is set no matter what flag is, more so it's not being set twice if flag is false