Should I subtract 1 from the upper bound of my "for" loops?

Question

I'm having a bit of trouble figuring out what's going wrong with this function. I'm not sure if I should be using -1 or not anymore, and no matter how I try to arrange the code it seems to return nil even when it shouldn't. Could someone with fresh eyes take a look? Also, I'm not sure my result := nil is in the proper place.

function TFrmMain.FindQueryFrm(Server, Nickname: String): TFrmMessage;
var
  I,M: Integer;
begin
  ///  No -1 in the I loop - why? Because the first childform will not always be
  ///  of type TFrmMessage, which is what we're looking for.
  ///
  ///  Is this approach wrong?
  for I := 0 to MDIChildCount do
  begin
    if Screen.Forms[I] is TFrmMessage then
    begin
      ///  Same concept with -1 here (M Loop)... I need to check all forms
      ///  stored by QueryManager to see if their .MyServer and .QueryWith's match
      ///
      ///  Is the M Loop wrong?
      for M := 0 to QueryManager.Count do
      begin
        if UpperCase((QueryManager[M] as TFrmMessage).MyServer) = UpperCase(Server) then
        begin
          if UpperCase((QueryManager[M] as TFrmMessage).QueryWith) = UpperCase(NickName) then
          begin // BINGO!
            Result := (QueryManager[M] as TFrmMessage);
            exit;
          end;
        end; // HOST COMPARE
      end; // M Loop
    end; // Is TFrmMessage
  end; // I Loop
  Result := nil; // None Found
end;

Answer 1

If I'm not mistaken, since you use Screen.Forms you should also use Screen.FormsCount.
If the array has N elements and it starts at index 0, we enumerate it from 0 .. N-1.
Array[N] would be wrong.

Check out if the below code works:

//for I := 0 to MDIChildCount do
for I := 0 to Screen.FormsCount-1 do
begin
  if Screen.Forms[I] is TFrmMessage then
  begin
     for M := 0 to QueryManager.Count-1
     ...

Answer 2

If you're only interested in the MDI children, as you seem to be, given you're using the form's MDIChildCount, then use the form's MDIChildren property. Those two properties go together, just like the screen's FormCount and Forms properties are a pair. Your code is mixing a form property with a screen property.

begin
  for I := 0 to MDIChildCount - 1 do
  begin
    if MDIChildren[I] is TFrmMessage then
    begin

Furthermore, you should definitely subtract 1 from the number of query managers, or else it means you're not properly keeping track of how many query managers you have in the first place. The "-1" you see in most code is there because the upper bound of a Delphi "for" loop is inclusive. The loop variable will start at the lower bound and the loop will continue running until the variable passes the upper bound. It may help you to reason about what happens in the base case, when there are no items in the list. In that case, the loop shouldn't run at all, right? Because there's nothing there to find. A loop set to run from "0 to 0" will execute one time, so the upper bound would need to be negative to prevent the loop from running. (This is all described in the documentation.)

As for why your function returns a null reference even when you think it shouldn't, I can only assume that it's due to the other problems in your code. Perhaps you're not looping over as many forms as you thought you were, or perhaps you're going beyond the end of the query-manager list and getting some undefined value. The placement of your Result assignment is correct, although it doesn't really matter where you put it since the only other place it gets assigned is right before the function exits.

---

I see you asked about MDI children on About.com. There, Zarco Gajic answered your question by giving you code like this:

for cnt := 0 to -1 + MDIChildCount do

Although it's valid code, it is not idiomatic. I have never seen anyone else write code like that before, so you would be wise not to pick up that habit. When we want one less than something, we do not add a literal negative one to the value. Rather, we subtract positive one:

for cnt := 0 to MDIChildCount - 1 do

Alternatively, I sometimes use the Pred standard function:

for cnt := 0 to Pred(MDIChildCount) do

Answer 3

In short: yes! Arrays in Delphi are 0 based, thus using the count of something gives you one too many.

MyArray[0] := 'first item';
MyArray[1] := 'second item';
for i:=0 to MyArray.count-1 do
begin
  ...
end;

If you're using the count (being 2) without the minus 1, then the last time you go through the loop would ask for myarray[2] which isn't there.

In short you should use a (cleaner) syntax like this:

for M := 0 to Pred(QueryManager.Count) do

Answer 4

It's returning nil because you're not actually visiting every form in the application in your loop, and that has nothing to do with whether or not use use -1 in the loop (which, in a properly written version, you must do).

You're not visiting every form because you're looping through the applications list of forms, which contains the MDIChildren and the non-MDI forms. But the value you're using to decide how many of those forms to look at is MDIChildCount. That number includes only the MDIChildren. So, if your application has 7 forms, of which 4 are MDI children, you're only looking at the first 4 out of the 7 forms (well, 5 because of the loop problem). If the form you want is number 6 or 7 (which is likely) you'll never reach it.

Also, I don't see where, inside the loop on the forms, you're actually referring to any property of the form.