function and result

[superc]

Hello,
I'm question me a thing about programming style in lazarus;

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

1.  
2. function TEvarilBuffClass.checkPNSNalreadyInserted(dirPathOutputFile: string; PNSN: string): Boolean;
3. var
4.   ts : TStringList;
5.   foundIt: Boolean;
6.   cnt: Integer;
7. begin
8.   Result := false;
9.   foundIt:= false;
10.   cnt := 0;
11.   try
12.     try
13.       ts := TStringList.Create;
14.       if (FileExists(dirPathOutputFile)) then
15.       begin
16.         ts.LoadFromFile(dirPathOutputFile);
17.  
18.         for cnt:= 0 to ts.Count-1 do
19.         begin
20.           if (AnsiPos(PNSN, ts.Strings[cnt]) > 0) then
21.           begin
22.             foundIt:=true;
23.             writeToMemoParent(PNSN+' already inserted... ');
24.             break;
25.           end;
26.         end;
27.         Result := foundIt;
28.       end;
29.     finally
30.       ts.Free;
31.     end;
32.   Except
33.     on e:Exception do
34.     begin
35.       writeToMemoParent('checkPNSNalreadyInserted Error...'+e.Message);
36.     end;
37.   end;
38. end;  
39.  

well, in this example I define immediately a return false, and if a function return true i change it, as a normal variable,
is it correct for you?

Thanks in advance.
 

[mse]

well, in this example I define immediately a return false, and if a function return true i change it, as a normal variable,
is it correct for you?

Yes. Instead to use "foundIt" you can set "result" directly.

[superc]

yes, but the question is: is a good pratice in lazarus, set always a boolean return value in a function that request it?

[Thaddy]

Well, it is actually not only good practice but more or less required. The compiler will issue a warning when the state of a result variable may be undefined.
A boolean function must be true or false, so initialize it to false and return true when a condition is met. Or make sure there is a mutually exclusive codepath for both.

I mostly compile my own code with -Sew (treat all warnings as errors) to make sure I don't miss such things.

[superc]

 ,
thanks

[Blaazen]

If you meant "how to set result" then you can set it directly in short functions (example from ComCtrls):

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

1. function TStatusBar.UpdatingStatusBar: boolean;
2. begin
3.   Result:=FUpdateLock>0;
4. end;

In more complex functions (like your example) is IMO better set Result:=False; and later change it, so the result is always defined. In other words, your code is OK.

[wp]

Not your question, but I would reverse the order of try-except and try-finally to make sure that allocated resources are released even if another exception occurs during the handling of a first exception.

BTW, you could shorten the code a bit if you avoid the temporary variable foundIt, use "Result" directly and exit when the result is known. Note that "exit" inside a try-finally block still always executes the finally part.

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

1. function TEvarilBuffClass.checkPNSNalreadyInserted(dirPathOutputFile: string; PNSN: string): Boolean;
2. var
3.   ts : TStringList;
4. //  foundIt: Boolean;
5.   cnt: Integer;
6. begin
7.   Result := false;
8.   //foundIt:= false;
9.   cnt := 0;
10.   ts := TStringList.Create;
11.   try
12.     try
13.       if (FileExists(dirPathOutputFile)) then
14.       begin
15.         ts.LoadFromFile(dirPathOutputFile);
16.  
17.         for cnt:= 0 to ts.Count-1 do
18.         begin
19.           if (AnsiPos(PNSN, ts.Strings[cnt]) > 0) then
20.           begin
21.             Result := true;   //foundIt:=true;
22.             writeToMemoParent(PNSN+' already inserted... ');
23.             exit;  //break;
24.           end;
25.         end;
26.         //Result := foundIt;
27.       end;
28.     Except
29.       on e:Exception do
30.       begin
31.         writeToMemoParent('checkPNSNalreadyInserted Error...'+e.Message);
32.       end;
33.     end;
34.   finally
35.     ts.Free;
36.   end;
37. end;  

[Thaddy]

Well spotted.

[440bx]

yes, but the question is: is a good pratice in lazarus, set always a boolean return value in a function that request it?

Not just in Lazarus.  In any programming language, it is good programming to initialize return values - such as function results and/or parameters passed by reference (var parameters).   Not only this prevents the programmer from "forgetting" to set a value or, having variables left with garbage because a particular execution path didn't set a variable/field to a specific value, when the function/procedure returns, it is usually very easy to determine which values/fields were set to values other than the defaults (initialization) which aids when debugging.

if you are concerned about good habits, initialization upon entry of a function/procedure is a very good one.  Another one, that is very good and too often missing, is a one or two line description of what a procedure does, immediately below the function/procedure declaration.  If the description takes more than 2 lines or if it cannot be made clear in 2 lines then odds are very good the program logic isn't quite everything it should be.

HTH.

[superc]

The question is about a warning that fpc raise when function not have a setted return value; why alert me if not important???

[Handoko]

To me it is very important and I always set the result at the beginning of the function. For example:

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

1. function CalculateSomething(a, b, c: Integer) : Integer;
2. begin
3.  
4.   Result := 0;
5.   if something_bad then Exit;
6.  
7.   // do the calculation
8.  
9. end;

The question is about a warning that fpc raise when function not have a setted return value; why alert me if not important???

Did you mean you created a function without a return value?

[superc]

Did you mean you created a function without a return value?

If I posted this question is possible for you? A function without return value is a procedure.
My question is relative to all code i see and often not have a return value for the boolean, unlike the code I posted
You have already answered me, and I thank you......

[Thaddy]

Some languages guarantee that the function result gets initialized to a valid default. In the case of a boolean, to false.
You must not rely on that. Ever.

[Zoran]

Not your question, but I would reverse the order of try-except and try-finally to make sure that allocated resources are released even if another exception occurs during the handling of a first exception.

BTW, you could shorten the code a bit if you avoid the temporary variable foundIt, use "Result" directly and exit when the result is known. Note that "exit" inside a try-finally block still always executes the finally part.

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

1. function TEvarilBuffClass.checkPNSNalreadyInserted(dirPathOutputFile: string; PNSN: string): Boolean;
2. var
3.   ts : TStringList;
4. //  foundIt: Boolean;
5.   cnt: Integer;
6. begin
7.   Result := false;
8.   //foundIt:= false;
9.   cnt := 0;
10.   ts := TStringList.Create;
11.   try
12.     try
13.       if (FileExists(dirPathOutputFile)) then
14.       begin
15.         ts.LoadFromFile(dirPathOutputFile);
16.  
17.         for cnt:= 0 to ts.Count-1 do
18.         begin
19.           if (AnsiPos(PNSN, ts.Strings[cnt]) > 0) then
20.           begin
21.             Result := true;   //foundIt:=true;
22.             writeToMemoParent(PNSN+' already inserted... ');
23.             exit;  //break;
24.           end;
25.         end;
26.         //Result := foundIt;
27.       end;
28.     Except
29.       on e:Exception do
30.       begin
31.         writeToMemoParent('checkPNSNalreadyInserted Error...'+e.Message);
32.       end;
33.     end;
34.   finally
35.     ts.Free;
36.   end;
37. end;  

Just to make more clear -- in this example, with exit, you can safely move assigning False to Result from top to the bottom of the function:

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

1. function TEvarilBuffClass.checkPNSNalreadyInserted(dirPathOutputFile: string; PNSN: string): Boolean;
2. var
3.   ts : TStringList;
4. //  foundIt: Boolean;
5.   cnt: Integer;
6. begin
7.   //Result := false; Not needed here any more!
8.   //foundIt:= false;
9.   cnt := 0;
10.   ts := TStringList.Create;
11.   try
12.     try
13.       if (FileExists(dirPathOutputFile)) then
14.       begin
15.         ts.LoadFromFile(dirPathOutputFile);
16.  
17.         for cnt:= 0 to ts.Count-1 do
18.         begin
19.           if (AnsiPos(PNSN, ts.Strings[cnt]) > 0) then
20.           begin
21.             Result := true;   //foundIt:=true;
22.             writeToMemoParent(PNSN+' already inserted... ');
23.             exit;  //break;
24.           end;
25.         end;
26.         //Result := foundIt;
27.       end;
28.     Except
29.       on e:Exception do
30.       begin
31.         writeToMemoParent('checkPNSNalreadyInserted Error...'+e.Message);
32.       end;
33.     end;
34.   finally
35.     ts.Free;
36.   end;
37.   Result := False;
38. end;  

[wp]

But I think that the "Result := false" should be inside the "finally". Code after the "finally" will not be executed because of the "exit".