Recent

Author Topic: Please advice  (Read 12507 times)

Handoko

  • Hero Member
  • *****
  • Posts: 5129
  • My goal: build my own game engine using Lazarus
Please advice
« on: October 19, 2017, 03:43:33 pm »
Hello. I'm not good in using exception, I rarely use exception. Please advice me how to write this code better:

Code: Pascal  [Select][+][-]
  1. function GetAllFields(dbFile: string): TStringArray;
  2. var
  3.   Database: TDbf;
  4.   i: Integer;
  5. begin
  6.   Database := TDbf.Create(nil);
  7.   try
  8.     with Database do
  9.       begin
  10.         FilePathFull := ExtractFileDir(dbFile);
  11.         TableName := ExtractFileName(dbFile);
  12.         Open;
  13.         i := FieldDefs.Count;
  14.         SetLength(Result, i);
  15.         for i := 0 to High(Result) do
  16.           Result[i] := FieldDefs[i].Name;
  17.         Close;
  18.       end;
  19.   except
  20.     SetLength(Result, 0);
  21.   end;
  22.   Database.Free;
  23. end;

What the function does:
- Retrieve all the fields names and give the result as TStringArray
- If it fails return empty result

howardpc

  • Hero Member
  • *****
  • Posts: 4144
Re: Please advice
« Reply #1 on: October 19, 2017, 04:34:03 pm »
I don't see a need to use exception syntax. My suggestion would be a boolean function, which if False indicates some error (possibly an exception, or some other error condition).
Code: Pascal  [Select][+][-]
  1. function GetAllDBFFields(const aDbfFilename: String; out FieldsArray: TStringArray): Boolean;
  2. var
  3.   dbf: TDbf;
  4.   i: Integer;
  5. begin
  6.   SetLength(FieldsArray, 0);
  7.   if not FileExists(aDbfFilename) then
  8.     Exit(False);
  9.   dbf:=TDbf.Create(Nil);
  10.   try
  11.     dbf.FilePathFull:=ExtractFileDir(aDbfFilename);
  12.     dbf.TableName:=ExtractFileName(aDbfFilename);
  13.     dbf.Open;
  14.     SetLength(FieldsArray, dbf.FieldDefs.Count);
  15.     for i:=0 to High(FieldsArray) do
  16.       FieldsArray[i]:=dbf.FieldDefs[i].Name;
  17.     Result:=True;
  18.   finally
  19.     dbf.Close;
  20.     dbf.Free;
  21.   end;
  22. end;
« Last Edit: October 19, 2017, 04:38:33 pm by howardpc »

rvk

  • Hero Member
  • *****
  • Posts: 6110
Re: Please advice
« Reply #2 on: October 19, 2017, 04:44:03 pm »
Can you rely on Result being false at the beginning of the function?

Otherwise there should be a Result := false at the top.

Although... in case of an exception you wouldn't use the function-result anyway because the code would skip the result-check. It just didn't seem quite right to not set Result to false at the top but it probably doesn't matter.


Kays

  • Hero Member
  • *****
  • Posts: 569
  • Whasup!?
    • KaiBurghardt.de
Re: Please advice
« Reply #3 on: October 19, 2017, 04:55:22 pm »
Well, for starters I limit my try … except on E: exception do … finally … end blocks to where the exception may raise. I guess that's database.open here. So it should be the first statement in the try block. Also I gather all procedures,  which are supposed to be called in the end, in the finally block, even those, which don't make sense in the exception case (but don't harm, e.g. closing a file, which wasn't opened, because there was an exception). I don't know, whether that's particularly good, but it feels tidier.
Code: Pascal  [Select][+][-]
  1. function getAllFields(dbFile: string): TStringArray;
  2. var
  3.         database: TDBF;
  4.         i: integer;
  5. begin
  6.         database := TDBF.create(nil);
  7.         setLength(result, 0);
  8.         if not assigned(database) then
  9.         begin
  10.                 exit;
  11.         end;
  12.        
  13.         database.filePathFull := extractFileDir(dbFile);
  14.         database.tableName := extractFileName(dbFile);
  15.        
  16.         try
  17.         begin
  18.                 // may raise exception EMyHilariousException
  19.                 database.open;
  20.                
  21.                 with database do
  22.                 begin
  23.                         setLength(result, fieldDefs.count);
  24.                         for i := 0 to length(result) - 1 do
  25.                         begin
  26.                                 // I haven't read the documentation, whether fieldDefs is zero-based
  27.                                 result[low(result) + i] := fieldDefs[low(fieldDefs) + i].name;
  28.                         end;
  29.                 end;
  30.         end;
  31.         except on E: EMyHilariousException do
  32.         begin
  33.         end;
  34.         finally
  35.         begin
  36.                 database.close;
  37.                 database.free;
  38.         end;
  39.         end;
  40. end;

Maybe a bad habit of mine I adapted from the assembler world is to assign a default return value at first.

I also tend to put everything into begin..end; blocks, because especially if you nest try..except-blocks it can become pretty hairy.
« Last Edit: October 19, 2017, 05:00:50 pm by Kays »
Yours Sincerely
Kai Burghardt

taazz

  • Hero Member
  • *****
  • Posts: 5368
Re: Please advice
« Reply #4 on: October 19, 2017, 05:02:16 pm »
In general it is bad to stop all exceptions like that because you are left with no information on why it failed. That is why the win32 api usually returns an error code instead of true/false. I'm assuming you have thought it over and decide that at this point plays no role in the flow of your application in which case the only thing I have to add is a way to allow you to distinguish between known and unknown errors. something along the lines of
Code: Pascal  [Select][+][-]
  1. function GetAllFields(dbFile: string): TStringArray;
  2. var
  3.   Database: TDbf;
  4.   i: Integer;
  5. begin
  6.   Database := TDbf.Create(nil);
  7.   try
  8.     with Database do
  9.       begin
  10.         FilePathFull := ExtractFileDir(dbFile);
  11.         TableName := ExtractFileName(dbFile);
  12.         Open;
  13.         i := FieldDefs.Count;
  14.         SetLength(Result, i);
  15.         for i := 0 to High(Result) do
  16.           Result[i] := FieldDefs[i].Name;
  17.         Close;
  18.       end;
  19.   except
  20.     On E:EDatabaseError do begin
  21.         //this is a database error search for the db specific problems
  22.     end;
  23.     On E:EFCreateError do begin
  24.     end;
  25.     On E:EFOpenError do begin
  26.     end;
  27.     On E:EStreamError do begin
  28.     end;
  29.     On E:Exception do begin
  30.       //log the exception somewhere for future reference.
  31.       SetLength(Result,
  32.     end;
  33.   end;
  34.   Database.Free;
  35. end;
  36.  
Pay close attention to the order of the errors in the section and the error's order in the inheritance tree. The check runs from top to bottom and only executes the first check it finds, the rest will be ignored so if you put Exception on top the rest then the rest of the checks are dead code.
Good judgement is the result of experience … Experience is the result of bad judgement.

OS : Windows 7 64 bit
Laz: Lazarus 1.4.4 FPC 2.6.4 i386-win32-win32/win64

munair

  • Hero Member
  • *****
  • Posts: 798
  • compiler developer @SharpBASIC
    • SharpBASIC
Re: Please advice
« Reply #5 on: October 19, 2017, 09:03:40 pm »
Can you rely on Result being false at the beginning of the function?

Otherwise there should be a Result := false at the top.

Although... in case of an exception you wouldn't use the function-result anyway because the code would skip the result-check. It just didn't seem quite right to not set Result to false at the top but it probably doesn't matter.

Yes. I usually set the default Result at the beginning and change it where applicable. Howardpc's example would then look like:

Code: Pascal  [Select][+][-]
  1. function GetAllDBFFields(const aDbfFilename: String; out FieldsArray: TStringArray): Boolean;
  2. var
  3.   dbf: TDbf;
  4.   i: Integer;
  5. begin
  6.   // assume
  7.   Result := False;
  8.   SetLength(FieldsArray, 0);
  9.   if not FileExists(aDbfFilename) then
  10.     Exit;
  11.   dbf:=TDbf.Create(Nil);
  12.   try
  13.     dbf.FilePathFull:=ExtractFileDir(aDbfFilename);
  14.     dbf.TableName:=ExtractFileName(aDbfFilename);
  15.     dbf.Open;
  16.     SetLength(FieldsArray, dbf.FieldDefs.Count);
  17.     for i:=0 to High(FieldsArray) do
  18.        FieldsArray[i]:=dbf.FieldDefs[i].Name;
  19.     // success
  20.     Result:=True;
  21.   finally
  22.     dbf.Close;
  23.     dbf.Free;
  24.   end;
  25. end;

I find it more logical and easier to read. Any False (in this case) can simply be returned by 'Exit;'

Just my preference here.

Great example by Taaz BTW if you want more error detail.
« Last Edit: October 19, 2017, 09:05:38 pm by Munair »
keep it simple

ASerge

  • Hero Member
  • *****
  • Posts: 2222
Re: Please advice
« Reply #6 on: October 20, 2017, 09:27:44 pm »
I vote for the @howardpc solution, but with two exceptions:
1. (insignificant) I prefer space around :=  :)
2. Significant. dbf.Close in the finally block is an error. What happens if the exception occurs in the line "dbf.FilePathFull: = ..."? In the finally block, a new exception can be generated on the dbf.Close line, which does not help to understand the cause of the error and will be skipped .Free with a memory leak. Solution: remove .Close (because almost certainly .Free will call .Close) or create a separate try finally block for .Open; .Close.

howardpc

  • Hero Member
  • *****
  • Posts: 4144
Re: Please advice
« Reply #7 on: October 20, 2017, 10:52:32 pm »
Perhaps surprisingly, TDbf.Free does not call Close. It deals only with freeing allocated resources.
So, following ASerge's suggestion, an improved routine would be:

Code: Pascal  [Select][+][-]
  1. function GetAllDBFFields(const aDbfFilename: String; out FieldsArray: TStringArray): Boolean;
  2. var
  3.   dbf: TDbf;
  4.   i: Integer;
  5. begin
  6.   Result := False;
  7.   SetLength(FieldsArray, 0);
  8.   if not FileExists(aDbfFilename) then
  9.     Exit;
  10.   dbf := TDbf.Create(Nil);
  11.   try
  12.     dbf.FilePathFull := ExtractFileDir(aDbfFilename);
  13.     dbf.TableName := ExtractFileName(aDbfFilename);
  14.     dbf.Open;
  15.     try
  16.       SetLength(FieldsArray, dbf.FieldDefs.Count);
  17.       for i := 0 to High(FieldsArray) do
  18.         FieldsArray[i] := dbf.FieldDefs[i].Name;
  19.     finally
  20.       dbf.Close;
  21.     end;
  22.   finally
  23.     dbf.Free;
  24.   end;
  25.   Result := True;
  26. end;

munair

  • Hero Member
  • *****
  • Posts: 798
  • compiler developer @SharpBASIC
    • SharpBASIC
Re: Please advice
« Reply #8 on: October 21, 2017, 07:33:52 am »
Perhaps surprisingly, TDbf.Free does not call Close. It deals only with freeing allocated resources.
So, following ASerge's suggestion, an improved routine would be:

Code: Pascal  [Select][+][-]
  1. function GetAllDBFFields(const aDbfFilename: String; out FieldsArray: TStringArray): Boolean;
  2. var
  3.   dbf: TDbf;
  4.   i: Integer;
  5. begin
  6.   Result := False;
  7.   SetLength(FieldsArray, 0);
  8.   if not FileExists(aDbfFilename) then
  9.     Exit;
  10.   dbf := TDbf.Create(Nil);
  11.   try
  12.     dbf.FilePathFull := ExtractFileDir(aDbfFilename);
  13.     dbf.TableName := ExtractFileName(aDbfFilename);
  14.     dbf.Open;
  15.     try
  16.       SetLength(FieldsArray, dbf.FieldDefs.Count);
  17.       for i := 0 to High(FieldsArray) do
  18.         FieldsArray[i] := dbf.FieldDefs[i].Name;
  19.     finally
  20.       dbf.Close;
  21.     end;
  22.   finally
  23.     dbf.Free;
  24.   end;
  25.   Result := True;
  26. end;

That would leave us with the problem that the result is true even if an exception occurs.

Turning the result approach around might be the solution:

Code: Pascal  [Select][+][-]
  1. function GetAllDBFFields(const aDbfFilename: String; out FieldsArray: TStringArray): Boolean;
  2. var
  3.   dbf: TDbf;
  4.   i: Integer;
  5. begin
  6.   SetLength(FieldsArray, 0);
  7.   if not FileExists(aDbfFilename) then
  8.     exit(false);
  9.   dbf := TDbf.Create(Nil);
  10.   dbf.FilePathFull := ExtractFileDir(aDbfFilename);
  11.   dbf.TableName := ExtractFileName(aDbfFilename);
  12.   result := true;
  13.   try
  14.     dbf.Open;
  15.     try
  16.       SetLength(FieldsArray, dbf.FieldDefs.Count);
  17.       for i := 0 to High(FieldsArray) do
  18.         FieldsArray[i] := dbf.FieldDefs[i].Name;
  19.     except
  20.       dbf.Close;
  21.       result := false;
  22.     end;
  23.   except
  24.     result := false;
  25.   end;
  26.   dbf.Free;
  27. end;
« Last Edit: October 21, 2017, 09:35:19 am by Munair »
keep it simple

wp

  • Hero Member
  • *****
  • Posts: 11853
Re: Please advice
« Reply #9 on: October 21, 2017, 09:49:39 am »
That would leave us with the problem that the result is true even if an exception occurs.

Turning the result approach around might be the solution:

Code: Pascal  [Select][+][-]
  1. function GetAllDBFFields(const aDbfFilename: String; out FieldsArray: TStringArray): Boolean;
  2. var
  3.   dbf: TDbf;
  4.   i: Integer;
  5. begin
  6.   SetLength(FieldsArray, 0);
  7.   if not FileExists(aDbfFilename) then
  8.     exit(false);
  9.   dbf := TDbf.Create(Nil);
  10.   dbf.FilePathFull := ExtractFileDir(aDbfFilename);
  11.   dbf.TableName := ExtractFileName(aDbfFilename);
  12.   result := true;
  13.   try
  14.     dbf.Open;
  15.     try
  16.       SetLength(FieldsArray, dbf.FieldDefs.Count);
  17.       for i := 0 to High(FieldsArray) do
  18.         FieldsArray[i] := dbf.FieldDefs[i].Name;
  19.     except
  20.       dbf.Close;
  21.       result := false;
  22.     end;
  23.   except
  24.     result := false;
  25.   end;
  26.   dbf.Free;
  27. end;
Absolutely no. It is the purpose of a try-finally block to protect resources, i.e. make sure that everything which has been created is destroyed againg. Therefore "dbf.Free" must be in a "finally-end" section. In addition, the "dbf.Close" is executed only in case of an exception (BTW: Since the dbf is closed automatically when the dbf is destroyed I don't think that a resource protection block is required for dbf.Open - dbf.Close).

People are always focussing on the try-except kind of exception handling. But it is not important at all since it cannot prevent an exception, you only can modify the error message displayed. try-finally is much more important because it protects the stability of the program since it makes sure that all resources allocated will be released again.

Just move the "Result := true" up before the "finally" to guarantees that the function result is true only if no exception occurs
Code: Pascal  [Select][+][-]
  1.     function GetAllDBFFields(const aDbfFilename: String; out FieldsArray: TStringArray): Boolean;
  2.     var
  3.       dbf: TDbf;
  4.       i: Integer;
  5.     begin
  6.       Result := False;
  7.       SetLength(FieldsArray, 0);
  8.       if not FileExists(aDbfFilename) then
  9.         Exit;
  10.       dbf := TDbf.Create(Nil);
  11.       try
  12.         dbf.FilePathFull := ExtractFileDir(aDbfFilename);
  13.         dbf.TableName := ExtractFileName(aDbfFilename);
  14.         dbf.Open;
  15.         try
  16.           SetLength(FieldsArray, dbf.FieldDefs.Count);
  17.           for i := 0 to High(FieldsArray) do
  18.             FieldsArray[i] := dbf.FieldDefs[i].Name;
  19.           Result := True;   // <-- This is not reached if an exception occurs somewhere above.
  20.         finally
  21.           dbf.Close;
  22.         end;
  23.       finally
  24.         dbf.Free;
  25.       end;
  26.     end;
« Last Edit: October 21, 2017, 09:59:31 am by wp »

munair

  • Hero Member
  • *****
  • Posts: 798
  • compiler developer @SharpBASIC
    • SharpBASIC
Re: Please advice
« Reply #10 on: October 21, 2017, 10:10:51 am »
@wp, that was my initial solution as well, but the problem was if an exception occurs with closing the database like ASerge said, the function still returns true.

Also, am I wrong in assuming that creating and freeing the dbf object can be done outside the try...finaly block? This block doesn't exit the function, so dbf.Free; will always be executed.

(BTW: Since the dbf is closed automatically when the dbf is destroyed I don't think that a resource protection block is required for dbf.Open - dbf.Close).

According to howardpc this is not the case. I haven't checked it yet.
« Last Edit: October 21, 2017, 10:27:56 am by Munair »
keep it simple

munair

  • Hero Member
  • *****
  • Posts: 798
  • compiler developer @SharpBASIC
    • SharpBASIC
Re: Please advice
« Reply #11 on: October 21, 2017, 10:24:47 am »
Maybe not elegant, but this might work to make sure Result is properly set:
Code: Pascal  [Select][+][-]
  1. function GetAllDBFFields(const aDbfFilename: String; out FieldsArray: TStringArray): Boolean;
  2. var
  3.   dbf: TDbf;
  4.   i: Integer;
  5. begin
  6.   SetLength(FieldsArray, 0);
  7.   if not FileExists(aDbfFilename) then
  8.     exit(false);
  9.   dbf := TDbf.Create(Nil);
  10.   dbf.FilePathFull := ExtractFileDir(aDbfFilename);
  11.   dbf.TableName := ExtractFileName(aDbfFilename);
  12.   result := true;
  13.   try
  14.     dbf.Open;
  15.     try
  16.       SetLength(FieldsArray, dbf.FieldDefs.Count);
  17.       for i := 0 to High(FieldsArray) do
  18.         FieldsArray[i] := dbf.FieldDefs[i].Name;
  19.     except
  20.       result := false;
  21.     end;
  22.     dbf.Close;
  23.   except
  24.     result := false;
  25.   end;
  26.   dbf.Free;
  27. end;
keep it simple

wp

  • Hero Member
  • *****
  • Posts: 11853
Re: Please advice
« Reply #12 on: October 21, 2017, 10:25:56 am »
Then move the "Result := true" after the "dbf.Close" but still before the "finally dbf.Free". BTW, I would not use a try-finally block for dbf.Open...dbf.Close at all because the dbf is closed automatically when it is freed (you could even skip dbf.Close, but this is bad programming style in my eyes).

Code: Pascal  [Select][+][-]
  1.       dbf := TDbf.Create(Nil);
  2.       try
  3.         dbf.FilePathFull := ExtractFileDir(aDbfFilename);
  4.         dbf.TableName := ExtractFileName(aDbfFilename);
  5.         dbf.Open;
  6.         SetLength(FieldsArray, dbf.FieldDefs.Count);
  7.         for i := 0 to High(FieldsArray) do
  8.           FieldsArray[i] := dbf.FieldDefs[i].Name;
  9.         dbf.Close;
  10.         Result := True;
  11.       finally
  12.         dbf.Free;
  13.       end;

"Eating" exceptions like you do is bad practice too because the calling program does not get a chance to react on the exception any more. You are just trying to achieve what try-finally is doing.

munair

  • Hero Member
  • *****
  • Posts: 798
  • compiler developer @SharpBASIC
    • SharpBASIC
Re: Please advice
« Reply #13 on: October 21, 2017, 10:28:53 am »
Then move the "Result := true" after the "dbf.Close" but still before the "finally dbf.Free". BTW, I would not use a try-finally block for dbf.Open...dbf.Close at all because the dbf is closed automatically when it is freed (you could even skip dbf.Close, but this is bad programming style in my eyes).
See my edited comment above.
« Last Edit: October 21, 2017, 10:33:05 am by Munair »
keep it simple

munair

  • Hero Member
  • *****
  • Posts: 798
  • compiler developer @SharpBASIC
    • SharpBASIC
Re: Please advice
« Reply #14 on: October 21, 2017, 10:32:13 am »
"Eating" exceptions like you do is bad practice too because the calling program does not get a chance to react on the exception any more. You are just trying to achieve what try-finally is doing.

Wasn't that the idea of returning true/false in the first place? I wouldn't call it bad practice, just a choice of where critical control is handled. If you want the calling program to have more control by putting the function in an exception block then yes, try..except shouldn't be in the function. But if you simply what to handle:
Code: Pascal  [Select][+][-]
  1. if not GetAllDBFFields then
  2.   ShowMessage('could not retrieve records.');
  3.  
then try..except inside the function is just fine.
« Last Edit: October 21, 2017, 10:41:35 am by Munair »
keep it simple

 

TinyPortal © 2005-2018