[CLOSED] Result assignments removal in lcl/widgetset/wslclclasses.pp

[lagprogramming]

lcl/widgetset/wslclclasses.pp has a series of useless "Result := nil;" lines in:

Code: Pascal  [Select][+][-]

1. function FindClassNode(const AComponent: TComponentClass): PClassNode;
2. var
3.   idx: integer;
4. begin
5.   Result := nil;
6.   if WSClassesList.Search(AComponent, idx) then
7.     Exit(WSClassesList[idx]);
8.   Result := FindNodeParent(AComponent.ClassParent);
9. end;

No matter what, the nil value will be overwritten later.

Code: Pascal  [Select][+][-]

1. function FindParentWSClassNode(const ANode: PClassNode): PClassNode;
2. begin
3.   Result := ANode^.Parent;
4.   while Result <> nil do begin
5.     if Result^.WSClass <> nil then Exit;
6.     Result := Result^.Parent;
7.   end;
8.   Result := nil;
9. end;

There are two ways of leaving the "while Result <> nil do" loop:
- exiting with an assigned result at "if Result^.WSClass <> nil then Exit;" in which case the line "Result := nil;" won't be run;
- exiting when Result is nil, situation when the line "Result := nil;" is useless.

Code: Pascal  [Select][+][-]

1. function FindCommonAncestor(const AClass1, AClass2: TClass): TClass;
2. begin
3.   Result := AClass1;
4.   if AClass2.InheritsFrom(Result) then Exit;
5.   Result := AClass2;
6.   while Result <> nil do begin
7.     if AClass1.InheritsFrom(Result) then Exit;
8.     Result := Result.ClassParent;
9.   end;
10.   Result := nil;
11. end;

Similar to function FindParentWSClassNode(const ANode: PClassNode): PClassNode;
There are two ways of leaving the "while Result <> nil do" loop:
- exiting with an assigned result at "if AClass1.InheritsFrom(Result) then Exit;" in which case the line "Result := nil;" won't be run;
- exiting when Result is nil, situation when the line "Result := nil;" is useless.

Edit: Removed the patch because it become obsolete.

[Thaddy]

Well, less rude than usual:
You are mixing up Break with Exit. Exit really leaves the method/procedure/function and Break exits the loop. So your patch is useless, unless there is a real bug.
Which is not what I conclude. The result value seems always correct.
See:
https://www.freepascal.org/docs-html/rtl/system/break.html
and
https://www.freepascal.org/docs-html/rtl/system/exit.html

FWIW: I know the difference between Break and Exit can be confusing, but take note.
( It can be even more confusing when refactoring this using continue )

[wp]

I think the patches are correct. But I would not apply them because they sacrifice readability without having a noticeable advantage.

[lagprogramming]

In addition to removing the "Result := nil;" lines, I've changed the two exits inside the while loops with breaks. Also, at function FindClassNode I've removed the exit by adding an else.

Code: Pascal  [Select][+][-]

1. function FindClassNode(const AComponent: TComponentClass): PClassNode;
2. var
3.   idx: integer;
4. begin
5.   if WSClassesList.Search(AComponent, idx) then
6.     Result := WSClassesList[idx]
7.   else
8.     Result := FindNodeParent(AComponent.ClassParent);
9. end;

Code: Pascal  [Select][+][-]

1. function FindParentWSClassNode(const ANode: PClassNode): PClassNode;
2. begin
3.   Result := ANode^.Parent;
4.   while Result <> nil do begin
5.     if Result^.WSClass <> nil then Break;
6.     Result := Result^.Parent;
7.   end;
8. end;

Code: Pascal  [Select][+][-]

1. function FindCommonAncestor(const AClass1, AClass2: TClass): TClass;
2. begin
3.   Result := AClass1;
4.   if AClass2.InheritsFrom(Result) then Exit;
5.   Result := AClass2;
6.   while Result <> nil do begin
7.     if AClass1.InheritsFrom(Result) then Break;
8.     Result := Result.ClassParent;
9.   end;
10. end;

[wp]

I don't see a difference in using "break" or "exit" here.

[lagprogramming]

I find the modified code cleaner and easier to read and understand, but this is something based on each programmer's individual experience and coding style.
The modified code is a proposal(suggestion). It's not a big deal if it's ignored, it's not like a bug would be fixed. When I read the original code it took me a while to understand the purpose of those "Result := nil;" lines. The conclusion was that they are useless. By replacing the original code with the proposed one no other programmer will waste the time. Besides, looking at Thaddy's reply, those lines might be confusing for other experienced programmers, too.

[wp]

Applied the change in FindClassNode. Will not apply the others because I think the current code is clearer.