CODEWITHSUNDEEP
delphirefactoringcompiler-warnings
Mason Wheeler
Mason Wheeler

Reputation: 84540

Avoiding "variable might not have been initialized"

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?

Upvotes: 12

Views: 5214

Answers (5)

Christopher Chase
Christopher Chase

Reputation: 2890

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

Upvotes: 1

Deltics
Deltics

Reputation: 23036

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. :)

Upvotes: 6

sabri.arslan
sabri.arslan

Reputation: 558

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

Upvotes: 1

Chris Thornton
Chris Thornton

Reputation: 15817

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!)

Upvotes: 6

Neil
Neil

Reputation: 7437

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.

Upvotes: 12

Related Questions