Recent

Author Topic: Please advice  (Read 8055 times)

rvk

  • Hero Member
  • *****
  • Posts: 3842
Re: Please advice
« Reply #15 on: October 21, 2017, 10:38:18 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
Well, the whole discussion was started because howardpc stated that Close wasn't called automatically when calling TDbf.Free.

Perhaps surprisingly, TDbf.Free does not call Close.

But the first line in TDbf.Destroy is inherited Destroy (so TDataSet.Destroy).
And the first line in TDataSet.Destroy is Active:=False.
So for TDbf.Free the DB is actually closed.
 

Munair

  • Sr. Member
  • ****
  • Posts: 473
  • Try to KISS (keep it simple, smart)
    • Ditrianum
Re: Please advice
« Reply #16 on: October 21, 2017, 10:42:27 am »
But the first line in TDbf.Destroy is inherited Destroy (so TDataSet.Destroy).
And the first line in TDataSet.Destroy is Active:=False.
So for TDbf.Free the DB is actually closed.
Thanks.

Does that mean that ASerge's claim is wrong:
In the finally block, a new exception can be generated on the dbf.Close line
« Last Edit: October 21, 2017, 10:49:35 am by Munair »
Lazarus 2.0.4; Manjaro, Debian, Windows

taazz

  • Hero Member
  • *****
  • Posts: 5363
Re: Please advice
« Reply #17 on: October 21, 2017, 10:59:53 am »
Does that mean that ASerge's claim is wrong:
In the finally block, a new exception can be generated on the dbf.Close line
No. Although the code inside the close method has been tested under a huge number of conditions the process is dynamic any one can attach an event handler on the on before/after close events which might raise an exception or you might encounter an unsupported or never seen condition. In any case it is better to protect than to wander.
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

  • Sr. Member
  • ****
  • Posts: 473
  • Try to KISS (keep it simple, smart)
    • Ditrianum
Re: Please advice
« Reply #18 on: October 21, 2017, 11:06:40 am »
Does that mean that ASerge's claim is wrong:
In the finally block, a new exception can be generated on the dbf.Close line
No. Although the code inside the close method has been tested under a huge number of conditions the process is dynamic any one can attach an event handler on the on before/after close events which might raise an exception or you might encounter an unsupported or never seen condition. In any case it is better to protect than to wander.

OK, so to get things right and simply let the function return true/false:

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.     SetLength(FieldsArray, dbf.FieldDefs.Count);
  16.     for i := 0 to High(FieldsArray) do
  17.       FieldsArray[i] := dbf.FieldDefs[i].Name;
  18.     dbf.Close;
  19.     result := true;
  20.   finally
  21.     dbf.Free;
  22.   end;  
  23. end;
Lazarus 2.0.4; Manjaro, Debian, Windows

taazz

  • Hero Member
  • *****
  • Posts: 5363
Re: Please advice
« Reply #19 on: October 21, 2017, 11:18:04 am »

No, that code does not protect against exception only against memory corruption. You need to make sure that the exception is stoped from propagating up the stack.

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.   Try
  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.     dbf.Close;
  20.     result := true;
  21.   finally
  22.     dbf.Free;
  23.   end;
  24.   Except
  25.     On E:Exception do begin
  26.         setlength(FieldsArray,0);
  27.     end;
  28.   end;
  29. end;
  30.  
That would be a quick pach around the working 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

rvk

  • Hero Member
  • *****
  • Posts: 3842
Re: Please advice
« Reply #20 on: October 21, 2017, 11:28:39 am »
No, that code does not protect against exception only against memory corruption. You need to make sure that the exception is stoped from propagating up the stack.
Well, the actual question would be "do we want to protect against exceptions here?"

In a function like GetAllDBFFields() I wouldn't want to protect against exception because if the function eats the exception, I don't know in the caller what might be wrong with my database (or filesystem). The function just returns false. I would prefer to handle the exception in the caller. But maybe that's different for others.

But in the last two post both solutions are provided.

Munair

  • Sr. Member
  • ****
  • Posts: 473
  • Try to KISS (keep it simple, smart)
    • Ditrianum
Re: Please advice
« Reply #21 on: October 21, 2017, 11:31:39 am »
No, that code does not protect against exception only against memory corruption. You need to make sure that the exception is stoped from propagating up the stack.
Well, the actual question would be "do we want to protect against exceptions here?"

In a function like GetAllDBFFields() I wouldn't want to protect against exception because if the function eats the exception, I don't know in the caller what might be wrong with my database (or filesystem). The function just returns false. I would prefer to handle the exception in the caller. But maybe that's different for others.

This was discussed earlier in the thread. See my reply #14.
Lazarus 2.0.4; Manjaro, Debian, Windows

taazz

  • Hero Member
  • *****
  • Posts: 5363
Re: Please advice
« Reply #22 on: October 21, 2017, 11:36:26 am »
No, that code does not protect against exception only against memory corruption. You need to make sure that the exception is stoped from propagating up the stack.
Well, the actual question would be "do we want to protect against exceptions here?"
I usually do not protect against exceptions not with out loggin them somewhere first. Then again handoko that raised the issue specified that the goal of the procedure is to return false if it fails. So that is our working premise.
In a function like GetAllDBFFields() I wouldn't want to protect against exception because if the function eats the exception, I don't know in the caller what might be wrong with my database (or filesystem). The function just returns false. I would prefer to handle the exception in the caller. But maybe that's different for others.

But in the last two post both solutions are provided.
Exactly we can only make sure all relevant info are mentioned then the person that has the responsibility makes the choise.
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

Thaddy

  • Hero Member
  • *****
  • Posts: 9167
Re: Please advice
« Reply #23 on: October 21, 2017, 11:37:04 am »
Code: Pascal  [Select]
  1. ]
  2.   Except
  3.     On E:Exception do begin // <------ that is silly code! It is the top exception so leave it out. Saves pulling in sysutils amongst other things.
  4.         setlength(FieldsArray,0);
  5.     end;
  6.  
You mean that  is serious code? Better leave it out then. The On:E part. Draws in sysutils....and more... Waste of space if you don't know what it does anyway...

Why not specify:
Code: Pascal  [Select]
  1.   Except
  2.     On E:EDatabaseError do begin // <-- less silly code. TDbf can raise this...Correctly....
  3.         setlength(FieldsArray,0) else raise // and handle non-database errors later....
  4.     end;
« Last Edit: October 21, 2017, 11:44:14 am by Thaddy »
also related to equus asinus.

wp

  • Hero Member
  • *****
  • Posts: 6319
Re: Please advice
« Reply #24 on: October 21, 2017, 11:38:34 am »
I go with rvk because a function handling the exception outside will be more general. Suppose a very inner-most function handles an exception by showing a message box. Then all usages of this functions would have to live with this message box - no way to change the message text, no way to log the messge instead of showing the dialog, no way to ignore the message...
Lazarus trunk / fpc 3.0.4 / all 32-bit on Win-10

taazz

  • Hero Member
  • *****
  • Posts: 5363
Re: Please advice
« Reply #25 on: October 21, 2017, 11:41:05 am »
Code: Pascal  [Select]
  1. ]
  2.   Except
  3.     On E:Exception do begin // <------ that is silly code! It is the top exception so leave it out. Saves pulling in sysutils.
  4.         setlength(FieldsArray,0);
  5.     end;
  6.  
You mean that  is serious code? Better leave it out then. The On:E part. Draws in sysutils....
you mean that the unit DB does not draw in the sysutils? Sorry I prefer to be silly and clear than smart and unclear. I'll keep using it and rejecting code that does not use it as too vague.
Why not specify:
Code: Pascal  [Select]
  1.   Except
  2.     On E:EDatabaseError do begin // <-- less silly code. TDbf can raise this
  3.         setlength(FieldsArray,0) else raise // and handle non-database errors later....
  4.     end;
No that is problematic code you have cut of a huge number of exception which will be raised if they occur.
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

  • Sr. Member
  • ****
  • Posts: 473
  • Try to KISS (keep it simple, smart)
    • Ditrianum
Re: Please advice
« Reply #26 on: October 21, 2017, 11:42:37 am »
I go with rvk because a function handling the exception outside will be more general. Suppose a very inner-most function handles an exception by showing a message box. Then all usages of this functions would have to live with this message box - no way to change the message text, no way to log the messge instead of showing the dialog, no way to ignore the message...

An inner function doesn't have to fire the message. I agree that would be less flexible code. As I showed in comment #14:

Code: Pascal  [Select]
  1. if not GetAllDBFFields then
  2.   ShowMessage('could not retrieve records.');

This way, it doesn't matter if the function uses an exception handler or not, unless you want more specific information as to the error or catch something like Taazz showed in his example.
« Last Edit: October 21, 2017, 11:55:19 am by Munair »
Lazarus 2.0.4; Manjaro, Debian, Windows

rvk

  • Hero Member
  • *****
  • Posts: 3842
Re: Please advice
« Reply #27 on: October 21, 2017, 11:43:23 am »
Why not specify:
Code: Pascal  [Select]
  1.   Except
  2.     On E:EDatabaseError do begin // <-- less silly code. TDbf can raise this
  3.         setlength(FieldsArray,0) else raise // and handle non-database errors later....
  4.     end;
That else is in the wrong place, isn't it?

Besides, you don't need to reraise the exception if you don't specifically handle it with a "on E" section.
So this is just fine (you can even leave begin/end out):
Code: Pascal  [Select]
  1. except
  2.   on E:EDatabaseError do // <-- less silly code. TDbf can raise this
  3.   begin
  4.     setlength(FieldsArray,0);
  5.   end;
  6.   // all other exception will fall through to the caller
  7. end;

Thaddy

  • Hero Member
  • *****
  • Posts: 9167
Re: Please advice
« Reply #28 on: October 21, 2017, 11:48:12 am »
No that is problematic code you have cut of a huge number of exception which will be raised if they occur.
Nonsense. On E:Exception does nothing extra. On:E ESpecifiedException does something: It handles a possible exception of a specified type (and foreseeable type), not thousands of them. You should re-raise if it is nort related to the database.
You should know better... That's amateur coding..... <extremely grumpy  >:( >:( >:( >:D >:D >:D >:D>
also related to equus asinus.

wp

  • Hero Member
  • *****
  • Posts: 6319
Re: Please advice
« Reply #29 on: October 21, 2017, 12:33:38 pm »
Thaddy, it seems that you made the same mistake like me: not reading the entire thread...

It is the question how to make a function return a boolean result without raising an exception. Under this premise the function must catch all exceptions, and "try ... except on E: Exception..." is the way to achieve this, call it "amateur coding" or not.

Database access, however, is a quite complex task, therefore, I think eating all exceptions is not a good idea here because the reason of a possible failure is hidden from the caller.
Lazarus trunk / fpc 3.0.4 / all 32-bit on Win-10