function and result

[Thaddy]

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

1.  
2.   result :=true;
3.   try
4.      result := true; // again, just for fun
5.   finally
6.     result :=false;
7.   end;

Not such a good idea?

[440bx]

Anything other than setting the function result upfront is a bad idea.

The reason is obvious: until the function ends successfully, it hasn't ended successfully (as Yogi Berra would point out too) and its result should reflect that. The result should change _only_ when it ends successfully to indicate it did.

Anything else is simply an exercise in turning the trivially simple into gratuitous complexity.

[creaothceann]

There may be a race condition between FileExists and LoadFromFile, so...

Code: [Select]

function TEvarilBuffClass.is_PNSN_already_inserted(const FileName, PNSN : string) : boolean;
var
        i    : integer;
        List : TStringList;
begin
List := TStringList.Create;
try
        try
                List.LoadFromFile(OutputFile);
                for i := 0 to (List.Count - 1) do  if (AnsiPos(PNSN, List.Strings[i]) > 0) then begin
                        WriteToMemoParent(PNSN + ' already inserted');
                        exit(True);
                end;
                exit(False);
        except
                exit(False);
        end;
finally
        List.Free;
end;
end;

[jamie]

Nice leaky code there 

[lucamar]

I would do it like this:

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

1. function TEvarilBuffClass.checkPNSNalreadyInserted(dirPathOutputFile: string; PNSN: string): Boolean;
2. var
3.   ts : TStringList;
4. begin
5.   Result := false;
6.   if (FileExists(dirPathOutputFile)) then begin
7.     ts := TStringList.Create;
8.     try
9.       try
10.         ts.LoadFromFile(dirPathOutputFile);
11.         Result := (AnsiPos(PNSN, ts.Text) > 0);
12.         if Result then
13.           writeToMemoParent(PNSN+' already inserted... ');
14.       except
15.         on e:Exception do
16.           writeToMemoParent('checkPNSNalreadyInserted Error...'+e.Message);
17.       end;
18.     finally
19.       ts.Free;
20.     end;
21.   end;
22. end;  
23.  

but this starts looking like an out-of-topic, bragging exercise

[Thaddy]

Anything other than setting the function result upfront is a bad idea.

Setting a function result in a finally section is *a lot wierder*.

[wp]

 

[440bx]

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

1.  try
2.    try
3.       ts.LoadFromFile(dirPathOutputFile);
4.  
5.       Result := (AnsiPos(PNSN, ts.Text) > 0);
6.  
7.       if Result then
8.         writeToMemoParent(PNSN+' already inserted... ');
9.     except
10.       on e:Exception do
11.         writeToMemoParent('checkPNSNalreadyInserted error...'+e.Message);
12.     end;
13.   finally
14.     ts.Free;
15.   end;
16.  

That code contradicts itself.

When Result gets set to true in line 5, writeToMemoParent gets executed and if that causes an exception, the message "checkPNSNalreadyInserted error..." is output.  IOW, an error may be reported yet the function still returns true.

At the very least, in that function, it is unclear what a result of true actually means.

[lucamar]

That code contradicts itself.

When Result gets set to true in line 5, writeToMemoParent gets executed and if that causes an exception, the message "checkPNSNalreadyInserted error..." is output.

One could always locate that if at the end of the function. Nevertheless, if writeToMemoParent() raises an exception it will (most probably) do so both here *and* in the except block, and this second exception will not  be catched, will it?

At the very least, in that function, it is unclear what a result of true actually means.

Huh?! It means Result := (AnsiPos(PNSN, ts.Text) > 0); Exactly the same as in the original code, i.e. that the file contains PNSN.

Maybe this is clearer?

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

1. function TEvarilBuffClass.checkPNSNalreadyInserted(dirPathOutputFile: string; PNSN: string): Boolean;
2. var
3.   ts : TStringList;
4. begin
5.   Result := false;
6.   if (FileExists(dirPathOutputFile)) then begin
7.     ts := TStringList.Create;
8.     try
9.       try
10.         ts.LoadFromFile(dirPathOutputFile);
11.         Result := (AnsiPos(PNSN, ts.Text) > 0);
12.       except
13.         on e:Exception do
14.           writeToMemoParent('checkPNSNalreadyInserted Error...'+e.Message);
15.       end;
16.     finally
17.       ts.Free;
18.     end;
19.   end;
20.   if Result then
21.     writeToMemoParent(PNSN+' already inserted... ');
22. end;

[440bx]

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

1.   if Result then
2.     writeToMemoParent(PNSN+' already inserted... ');
3.  

As long as writeToMemoParent cannot cause an exception then it should be ok.  If writeToMemoParent can cause an exception then the contradiction is still there.

[FTurtle]

I don't understand why people so like long blocks under "if".
On my opinion in this case the most logically is to write:

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

1. function TEvarilBuffClass.checkPNSNalreadyInserted(dirPathOutputFile: string; PNSN: string): Boolean;
2. var
3.   ts : TStringList;
4. begin
5.   if not FileExists(dirPathOutputFile)) then
6.     Exit(False);
7.  
8.   // All other without begin-end
9.  
10. end;
11.  

[creaothceann]

Nice leaky code there 

?

[lucamar]

If writeToMemoParent can cause an exception then the contradiction is still there.

No, if writeToMemoParent raises an exception in this case, the function doesn't return normally so the result doesn't matter at all.

[440bx]

If writeToMemoParent can cause an exception then the contradiction is still there.

No, if writeToMemoParent raises an exception in this case, the function doesn't return normally so the result doesn't matter at all.

If an exception happened, it seems a bit of a contradiction for the function to return a result of true when it didn't execute successfully. 

[lucamar]

If an exception happened, it seems a bit of a contradiction for the function to return a result of true when it didn't execute successfully.

Let's repeat, slowly:

If in my second code writeToMemoParent raises an exception the function doesn't return normally. Because it has raised an uncatched exception. Whatever the function's result is doesn't matter at all since it isn't returned.

Do note, though, that if it were returned it would be the correct result because the intent of the function is to check whether the file contains PNSN. If it were my program, in fact, I would implement just that and let all the writeToMemoParent or whatever to the caller.