Do I need to 'create'/initialize local variables if they are just used as pointers to form controls?

Question

I have this simple example of syncing Scrollboxes, where I control which side is synced by Radio button - Sync Left or Right side. When I compile, I get compiler messages:

[dcc32 Warning] Unit1.pas(51): W1036 Variable 'ScrlBox1' might not have been initialized

[dcc32 Warning] Unit1.pas(51): W1036 Variable 'ScrlBox2' might not have been initialized

This is the simple example:

procedure TForm1.Button1Click(Sender: TObject);
var
  ScrlBox1, ScrlBox2: TScrollBox;
begin

    if radiobtn_SyncLeftSides.Checked then // Snyc Left side
    begin
      ScrlBox1 := ScrollBoxLeft1;
      ScrlBox2 := ScrollBoxLeft2;
    end
    else if radiobtn_SyncrightSides.Checked then  // Snyc Right side
    begin
      ScrlBox2 := ScrollBoxRight1;
      ScrlBox1 := ScrollBoxRight2;
    end;

    // Sync scroll boxes
    ScrlBox2.VertScrollBar.Position := ScrlBox1.VertScrollBar.Position;
    ScrlBox2.HorzScrollBar.Position := ScrlBox1.HorzScrollBar.Position;

end;

What is the issue here?

If I add this at the beginning the message is gone:

ScrlBox1:= TScrollBox.Create(nil);
ScrlBox2:= TScrollBox.Create(nil);

but I don't think creating scroll box variables is necessary, right? These are just variable pointers to controls on form.

score 6 · Accepted Answer · answered Feb 25 '16 at 22:09

6

If both radiobtn_SyncLeftSides and radiobtn_SyncrightSides are unchecked, you are not initializing the ScrlBox1 and ScrlBox2 variables before using them. That is what the compiler is complaining about.

procedure TForm1.Button1Click(Sender: TObject);
var
  ScrlBox1, ScrlBox2: TScrollBox;
begin
  if radiobtn_SyncLeftSides.Checked then // Snyc Left side
  begin
    ScrlBox1 := ScrollBoxLeft1;
    ScrlBox2 := ScrollBoxLeft2;
  end
  else if radiobtn_SyncrightSides.Checked then  // Snyc Right side
  begin
    ScrlBox2 := ScrollBoxRight1;
    ScrlBox1 := ScrollBoxRight2;
  end else
  begin
    // NOT INITIALIZED HERE!!!!!
  end;

  // Sync scroll boxes
  ScrlBox2.VertScrollBar.Position := ScrlBox1.VertScrollBar.Position;
  ScrlBox2.HorzScrollBar.Position := ScrlBox1.HorzScrollBar.Position;
end;

If you don't want to sync the scrolling, you should just Exit the procedure:

procedure TForm1.Button1Click(Sender: TObject);
var
  ScrlBox1, ScrlBox2: TScrollBox;
begin
  if radiobtn_SyncLeftSides.Checked then // Snyc Left side
  begin
    ScrlBox1 := ScrollBoxLeft1;
    ScrlBox2 := ScrollBoxLeft2;
  end
  else if radiobtn_SyncrightSides.Checked then  // Snyc Right side
  begin
    ScrlBox2 := ScrollBoxRight1;
    ScrlBox1 := ScrollBoxRight2;
  end else
  begin
    Exit; // <-- HERE
  end;

  // Sync scroll boxes
  ScrlBox2.VertScrollBar.Position := ScrlBox1.VertScrollBar.Position;
  ScrlBox2.HorzScrollBar.Position := ScrlBox1.HorzScrollBar.Position;
end;

answered Feb 25 '16 at 22:09

Remy Lebeau

555,201
31
458
770

Interesting, didn't think about this because I make sure one of the radio buttons is always checked by default, so this should never happen. Is there a way to avoid third execution branch? – Mike Torrettinni Feb 25 '16 at 22:22
1

@Mike: Remy answered that - see the sentence between the two code blocks in his answer. – Ken White Feb 25 '16 at 22:44
OK, Exit seems to be the way to do it. I'm trying to avoid it as it looks odd, but maybe I'm just not used to using Exit - the reason for these compiler messages. – Mike Torrettinni Feb 25 '16 at 23:00
I just accepted your answer, in case it gets deleted, so you get the points. – Mike Torrettinni Feb 25 '16 at 23:09
5

If one of the two radio buttons is *always* checked, then you shouldn't need to inspect *both*. Replace the second `if` clause with a simple `else` instead: `if radiobtn_SyncLeftSides.Checked then {...} else {...}`. – Rob Kennedy Feb 25 '16 at 23:27
Thank you @RobKennedy, I like your suggestion. It fits better for this example, and Exit will fit better when I have more than just 2 options. – Mike Torrettinni Feb 25 '16 at 23:46
1

The alternative is to use a `TRadioGroup` instead of individual `TRadioButton`s, and then make decisions based on the `TRadioGroup.ItemIndex` property, such as with a `case` statement . – Remy Lebeau Feb 26 '16 at 01:05
4

If you can guarantee that one of the radiobuttons is checked then you should raise an exception instead of a simple exit. Once you add another option the exception is your reminder – Sir Rufo Feb 26 '16 at 01:22

Do I need to 'create'/initialize local variables if they are just used as pointers to form controls?

1 Answers1