[CLOSED] Result assignments removal in lcl/widgetset/wslclclasses.pp
lazarus / 7-LCL

----------------------------------------------------------------------
lagprogramming · 2023-04-12 10:45

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

Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i function
FindClassNode(const AComponent: TComponentClass): PClassNode;var  idx:
integer;begin  Result := nil;  if WSClassesList.Search(AComponent,
idx) then    Exit(WSClassesList[idx]);  Result :=
FindNodeParent(AComponent.ClassParent);end;No matter what, the nil
value will be overwritten later.

Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i function
FindParentWSClassNode(const ANode: PClassNode): PClassNode;begin 
Result := ANode^.Parent;  while Result <> nil do begin    if
Result^.WSClass <> nil then Exit;    Result := Result^.Parent;  end; 
Result := nil;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][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i function
FindCommonAncestor(const AClass1, AClass2: TClass): TClass;begin 
Result := AClass1;  if AClass2.InheritsFrom(Result) then Exit;  Result
:= AClass2;  while Result <> nil do begin    if
AClass1.InheritsFrom(Result) then Exit;    Result :=
Result.ClassParent;  end;  Result := nil;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 · 2023-04-12 12:31

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 · 2023-04-12 15:06

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

----------------------------------------------------------------------
lagprogramming · 2023-04-12 15:41

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][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i function
FindClassNode(const AComponent: TComponentClass): PClassNode;var  idx:
integer;begin  if WSClassesList.Search(AComponent, idx) then    Result
:= WSClassesList[idx]  else    Result :=
FindNodeParent(AComponent.ClassParent);end;Code: Pascal
 [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i function
FindParentWSClassNode(const ANode: PClassNode): PClassNode;begin 
Result := ANode^.Parent;  while Result <> nil do begin    if
Result^.WSClass <> nil then Break;    Result := Result^.Parent; 
end;end;Code: Pascal  [Select][+][-]window.onload = function(){var x1
= document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i function
FindCommonAncestor(const AClass1, AClass2: TClass): TClass;begin 
Result := AClass1;  if AClass2.InheritsFrom(Result) then Exit;  Result
:= AClass2;  while Result <> nil do begin    if
AClass1.InheritsFrom(Result) then Break;    Result :=
Result.ClassParent;  end;end;

----------------------------------------------------------------------
wp · 2023-04-12 16:02

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

----------------------------------------------------------------------
lagprogramming · 2023-04-13 10:09

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 · 2023-04-13 11:01

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