CopyFile lässt die Anwendung hängen, wie umgehen?

[Sir Rufo]

@mm1256

Also deine FileCopy-Routine ist echt grausam ... wer hat die denn verbrochen?

Ich wollte die gerade etwas überarbeiten, aber das kommt ja einem neu schreiben gleich.

1. Statt einem String als Rückgabe verwende Exceptions
2. Solche Exceptions raise Exception.Create('Da hat der Programmierer was vergessen'); sind nicht gerade aussagekräftig. Mach es doch richtig:
   
   Delphi-Quellcode:

   case FileCopyMode of
     ...
     else raise ENotImplemented.CreateFmt('FileCopyMode %s nicht implementiert', [GetEnumName( TypeInfo(TFileCopyMode), Ord( FileCopyMode )]);
   end;

   und du bekommst die Fehlermeldung
   

   Zitat:

   FileCopyMode fcmFoo nicht implementiert

3. Statt TStream.ReadBuffer solltest du TStream.Read verwenden, dann bekommst du automatisch die Anzahl der echten gelesenen Bytes zurück. Dann weißt du auch wieviel du schreiben musst
4. Statt einer TFileCopyCallback = procedure(ProgressPercent: Integer) of object; nimm eine TFileCopyCallback = reference to procedure(ProgressPercent: Integer); (optional, weil flexibler)

Wenn du das hast, dann kannst du diese Kopieraktion vom http://www.delphipraxis.net/185335-b...ab-xe2%5D.html aus starten und alles wird gut.

Delphi-Quellcode:

procedure TFoo.FileCopyWorker_DoWork(Sender: TObject; e: TDoWorkEventArgs );
var
  FileCopyArg : TFileCopyArg;
begin
  FileCopyArg := e.Argument.AsType<TFileCopyArg>;
  MyCopyFile( 
    FileCopyArg.SourceName, 
    FileCopyArg.DestName, 
    FileCopyArg.FileCopyMode,
    (Sender as TBackgroundWorker).ReportProgress );
end;

procedure TFoo.FileCopyWorker_ProgressChanged( Sender : TObject; e: TProgressChangedEventArgs );
begin
  ProgressBar1.Position := e.Value;
end;

procedure TFoo.FileCopyWorker_RunWorkerCompleted( Sender: TObject; e: TRunWorkerCompletedEventArgs );
begin
  if Assigned( e.Error ) then
    ShowMessage( e.Error.ToString() )
  else if e.Cancelled then
    ShowMessage( 'Du hast das abgebrochen!' )
  else
    ShowMessage( 'Ich habe fertig!' );
end;

Das ist eigentlich alles und du kannst die originale Routine verwenden.

PS Das Wichtigste hatte ich vergessen:
Du greifst immer wieder auf SourceStream.Size zu, und das macht deine Routine mit Progress-Callback extremst langsam. Denn bei jedem Aufruf wandert der Stream an das Ende liest die Position und kehrt zur vorherigen Position zurück!

Lies den Wert am Anfang einmal aus und merk dir diesen, dann läuft auch die Routine mit dem Progress sehr schnell.

[mm1256]

Zitat von Sir Rufo:

Also deine FileCopy-Routine ist echt grausam ... wer hat die denn verbrochen?

Dieser Verbrecher antwortet gerade

Ich freue mich immer sehr, wenn Verbesserungsvorschläge dazu beitragen, etwas "zu verbessern". Darum habe ich mir deine Vorschläge mal zur Brust genommen:

Zitat von Sir Rufo:

[*]Statt einem String als Rückgabe verwende Exceptions

Mittlerweile kenne ich deinen Standpunkt zur Exception-Behandlung, aber in diesem Fall bringen mir Exceptions anstelle des String-Rückgabewertes (und so hast du das wohl gemeint) überhaupt nicht weiter. Welche Exception soll beispielsweise geworfen werden, wenn die Zieldatei neuer ist? Da ist meiner Meinung nach ein schlichter Rückgabewert geeigneter. Der User soll ja bei Bedarf lediglich darüber informiert werden, was los ist.

Zitat von Sir Rufo:

[*]Solche Exceptions raise Exception.Create('Da hat der Programmierer was vergessen'); sind nicht gerade aussagekräftig. Mach es doch richtig:

Delphi-Quellcode:

case FileCopyMode of
  ...
  else raise ENotImplemented.CreateFmt('FileCopyMode %s nicht implementiert', [GetEnumName( TypeInfo(TFileCopyMode), Ord( FileCopyMode )]);
end;

Gerade um das "richtig machen geht es hier. Diese Exception wird im Normalfall nur im Entwicklungsmodus stattfinden. Der User wird diese niemals zu Gesicht bekommen, denn wenn das passiert, hat der Programmierer bereits Bockmist gebaut und sein Werk nicht mal ansatzweise getestet.

Zitat von Sir Rufo:

PS Das Wichtigste hatte ich vergessen:
Du greifst immer wieder auf SourceStream.Size zu, und das macht deine Routine mit Progress-Callback extremst langsam. Denn bei jedem Aufruf wandert der Stream an das Ende liest die Position und kehrt zur vorherigen Position zurück!

Lies den Wert am Anfang einmal aus und merk dir diesen, dann läuft auch die Routine mit dem Progress sehr schnell.

Das mit dem Getter der Filesize habe ich nicht gewusst. Aber, ich habe es jetzt mal geändert (bin ja nicht beratungsresistent) und mit einer 3,5 GB großen Datei ausprobiert und die Zeiten verglichen. Die Formulierung "extremst langsam" ist total überzogen. Der Unterschied liegt bei dieser 3,5 GB-Datei im Bereich von einigen Millisekunden.

Wenn also Performance raus zu holen ist, dann nicht an dieser Stelle.

[Sir Rufo]

• Zu Exception :
  
  Bei einer Exception wird der Benutzer doch informiert ... es poppt eine MessageBox mit der Meldung auf, ganz von alleine, nur weil du eine Exception wirfst.
  
  Und wenn du mit dem Argument FileCopyMode eine Bedingung festlegst, die dann nicht zutrifft, dann ist das eine Ausnahme. Also Exception werfen und gut.
  
  Die Exception könntest du als
  
  Delphi-Quellcode:

  type
    EFileCopyModeMismatchException = class(Exception);

  deklarieren und entsprechend werfen. Dann kannst du im nachhinein sogar noch speziell darauf reagieren -> Dialog und den Benutzer fragen, ob er evtl. eine anderen FileCopyMode haben möchte.
  
  Und da eine Exception auch nur eine Klasse ist, kannst du da sogar noch alle möglichen Informationen hineinpacken (z.B. den FileCopyMode, der diese Exception verursacht hat).
  
• Zu ENotImplemented :
  
  Wenn dich dein Kunde anruft und dir mitteilt, dass da so eine Fehlermeldung aufgetaucht ist, bei welcher Fehlermeldung wüsstest du sofort worum es geht? - Eben, sehe ich auch so
  
  Ach ja, Fehler passieren immer wieder, auch beim Testen ...
  
• Zu TStream.Size :
  
  Wie stark das bremst hängt natürlich stark von deiner Hardware und wie stark fragmentiert die Datei auf der Platte vorliegt. Im Idealfall sollte man davon nicht wirklich viel merken, im Worst Case kannst du in der Zeit halb Asien neu bevölkern

[Dejan Vu]

Zitat von mm1256:

+...aber in diesem Fall bringen mir Exceptions anstelle des String-Rückgabewertes überhaupt nicht weiter.

Und wie willst Du deine Rückgabefehlermeldungszeichenkette auswerten? Also, woher weißt Du, welcher Fehler genau aufgetreten ist? Ach, das ist Dir egal? Man kann ja streiten, ob Exceptions oder Rückgabewerte besser sind, aber wenn schon Rückgabewerte, dann wenigstens numerisch, oder als Enum.

Stell Dir mal vor, dein Programm soll mal im Ausland laufen...

Übrigens ist es keine Tugend, alles in eine Routine zu quetschen. Das ist sowas von gestern, ach was sag ich, vorvorvorgestern... Definier doch einfach eine Klasse, führe einfache kleine Methoden ein und schreibe das Ganze, das es verständlich ist. "Clean Code" nennt sich das. Probiere es mal aus.

[BUG]

Zitat von Dejan Vu:

Übrigens ist es keine Tugend, alles in eine Routine zu quetschen. Das ist sowas von gestern, ach was sag ich, vorvorvorgestern... Definier doch einfach eine Klasse, führe einfache kleine Methoden ein und schreibe das Ganze, das es verständlich ist. "Clean Code" nennt sich das. Probiere es mal aus.

Prinzipiell bin ich deiner Meinung, aber bei dieser "Begründung" ziehst du niemanden aus dem Schatten der seitenlangen Prozeduren in säubernde Licht des blütenreinen Programmierens

Die Reaktionen sind imho leicht übertrieben (bis auf die zur Fehlerbehandlung ). Es gibt deutlich schlimmeren Code.
Der Hinweis auf den Workerpool ist für diese Aufgabe zwar legitim, man sollte das imho aber auch mal zu Fuß gemacht haben, damit man eine Vorstellung davon hat, was bei so einer asynchronen Ausführung so passiert.

Ein Fehler ist mir ins Auge gefallen: Der RuFileCopyExecute wird in Execute das falsche Callback übergeben. Eigentlich willst du ja die synchronisierte Variante.

Das CreateSuspended = true und das Konstrukt mit dem with kannst du dir sparen, wenn du die Eigenschaften, die du im with setzt, direkt im Konstruktor setzt.
OnTerminate wird im Hauptthread ausgeführt, das synchronize solltest du dir an der Stelle sparen; oder du lässt onTerminate leer und rufst ThreadCopyCallback(cRuFileCopyReady); direkt am Ende von Execute auf.

[Dejan Vu]

Zitat von BUG:

Prinzipiell bin ich deiner Meinung, aber bei dieser "Begründung" ziehst du niemanden aus dem Schatten der seitenlangen Prozeduren in säubernde Licht des blütenreinen Programmierens

Ash on my main.

[mm1256]

Hallo,

ich habe den Eindruck, dass ihr mit den Exceptions jetzt total am Ziel vorbei argumentiert. Fangen wir doch mal ganz von vorne an. Wozu bzw. in welchen Anwendungsfällen verwende ich überhaupt eine "eigene" Kopier-Routine? Immer dann, wenn die normale WinApi oder was auch immer nicht ausreicht, oder Probleme verursacht, oder die Funktionalität nicht ausreicht.

In diesem Fall geht es darum, dass dem TE die Mainform einfriert. Also gibt es zwei grundsätzliche Lösungsmöglichkeiten: Separater Thread und/oder eigene Kopier-Routine. Soweit so gut, nun mal zur praktischen Anwendung. Wenn ich eine oder mehrere Dateien kopiere, dann brauche ich eine Rückmeldung, wenn dabei etwas schief geht, z.B. Festplatte ist voll, Ziel-Datenträger ist schreibgeschützt...was auch immer. Aber eine Exception werfen, weil z.B. die Zieldatei neuer ist als die Quelldatei? Sorry, aber das ist meiner Meinung nach ganz schlechter Programmierstil. Wie will ich in dem Fall eine ich nenne sie mal fatale Exception (z.B. Datenträger ist voll) von einer ganz normalen Meldung "Zieldatei ist neuer" unterscheiden? Hinzu kommt, in manchen Situationen will ich eine Meldung vielleicht gar nicht anzeigen, sondern ganz einfach ältere Dateien durch neuere ersetzen. Wie sieht das dann in eurem Quellcode aus?

Delphi-Quellcode:

try
  Dateien kopieren...
except
  // hier den ganzen Rotz auswerten, weil, die ganz normalen User-Messages - wenn 
  // ich sie denn überhaupt dem User anzeigen möchte, landen auch hier.
end;

Das kann nicht wirklich euer Ernst sein. So würde ich das machen:

Delphi-Quellcode:

try
  Dateien kopieren...
  Bei Bedarf dem User entsprechende Messages zeigen, oder bei vielen Dateien alle Messages 
  in eine Liste und am Schluss des Kopiervorganges anzeigen
except
  // hier die "echten" Exceptions auswerten, z.B. Datenträger ist voll
end;

Jeder kann das halten wie er will. Für mich sind Exceptions "Ausnahmen" und wenn ich eine Ausnahme nicht behandeln kann, nach eurer Logik z.B. die Ausnahme "Die Zieldatei ist neuer als die Quelldatei", wozu muss ich sie dann überhaupt erst mal werfen? Das widerspricht dem Grundprinzip der Exceptionbehandlung. Ihr verwechselt da ganz einfach zwei grundverschiedene Situationen: Ein wahlweise nach Bedarf für den User anzuzeigendes Ergebnis eines Kopiervorganges und einer Exception die bei diesem Vorgang aufgetreten ist.

Zitat von Dejan Vu:

Und wie willst Du deine Rückgabefehlermeldungszeichenkette auswerten? Also, woher weißt Du, welcher Fehler genau aufgetreten ist? Ach, das ist Dir egal?

Das ist genau der Punkt wo du total daneben liegst. Wenn ich genau wissen will, was bei der Kopieraktion passiert ist, dann ist beispielsweise ein Enum die richtige Wahl. Doch, das will und muss ich in diesem Fall doch gar nicht.

Wenn ich der Meinung bin, dass dem User eine Meldung angezeigt werden muss, dann zeige ich sie ihm. Wenn ich für den Fall, dass der Kopiervorgang ohne Usermeldung ablaufen soll, dem User etwas zeigen will oder muss, dann ist es eine "echte" Exception, z.B. "deine Platte ist voll".

Zitat von Dejan Vu:

Stell Dir mal vor, dein Programm soll mal im Ausland laufen...

Muss ich mir das vorstellen, und ist diese Aufgabenstellung Bestandteil dieses Threads? Nein. Also wozu dieses "provokannte Argument". Ach ja, die Antwort gibst du ja selber: Es gibt Besserwisser....

Zitat von BUG:

Das CreateSuspended = true und das Konstrukt mit dem with kannst du dir sparen, wenn du die Eigenschaften, die du im with setzt, direkt im Konstruktor setzt.
OnTerminate wird im Hauptthread ausgeführt, das synchronize solltest du dir an der Stelle sparen; oder du lässt onTerminate leer und rufst ThreadCopyCallback(cRuFileCopyReady); direkt am Ende von Execute auf.

Vielen Dank für den - aus meiner Sicht leider einzigen - konstruktiven Hinweis.

[p80286]

Zitat von mm1256:

Wenn ich der Meinung bin, dass dem User eine Meldung angezeigt werden muss, dann zeige ich sie ihm. Wenn ich für den Fall, dass der Kopiervorgang ohne Usermeldung ablaufen soll, dem User etwas zeigen will oder muss, dann ist es eine "echte" Exception, z.B. "deine Platte ist voll".

Kann ich gut nachvollziehen, wobei es drei Arten von Ausnahmen/Fehlern gibt, die an die man vorher denkt und die man im Vorfeld vermeiden kann, die um die sich der Benutzer kümmern muß, und die an die man nicht gedacht hat.(und dafür sind exceptions wirklich was feines)

Zitat von mm1256:

Zitat von Dejan Vu:

Stell Dir mal vor, dein Programm soll mal im Ausland laufen...

Muss ich mir das vorstellen, und ist diese Aufgabenstellung Bestandteil dieses Threads? Nein. Also wozu dieses "provokannte Argument". Ach ja, die Antwort gibst du ja selber: Es gibt Besserwisser....

Ich habe die Erfahrung gemacht, daß ein QuicknDirty Tool eine "internationale" Karriere gemacht hat. Darum Wirf den Einwand nicht zu weit weg. Zumindest rudimentär sollte man die Möglichkeit in Betracht ziehen.

Gruß
K-H

[Sir Rufo]

Zu Exceptions gibt es hier mehrere Themen und Beiträge und es wurde schon erschöpfend behandelt.

Und ich finde es wesentlich einfacher auf einen speziellen Exception-Typ zu reagieren, als einen String-Rückgabewert zu parsen um herauszufinden, was denn jetzt eigentlich los gewesen ist, und wie ich da am sinnvollsten reagieren könnte.

Und wenn dann noch unterschiedliche Sprachen mit ins Spiel kommen, dann brauche ich vor dem Parsen sogar noch G**gle-Translate ...

[mm1256]

Zitat von Sir Rufo:

Zu Exceptions gibt es hier mehrere Themen und Beiträge und es wurde schon erschöpfend behandelt.

Und ich finde es wesentlich einfacher auf einen speziellen Exception-Typ zu reagieren, als einen String-Rückgabewert zu parsen um herauszufinden, was denn jetzt eigentlich los gewesen ist, und wie ich da am sinnvollsten reagieren könnte.

Und wenn dann noch unterschiedliche Sprachen mit ins Spiel kommen, dann brauche ich vor dem Parsen sogar noch G**gle-Translate ...

Sorry Sir Rufo, du hast es immer noch nicht verstanden. Ich parse keinen Rückgabe-String. Wenn ich - oder jemand anders - das für nötig empfinden würde, dann kann er das ja so machen wie er will. Ich würde dann einen ENUM nehmen. In diesem Fall geht es aber ausschließlich darum, den User bei Bedarf über einen Sachverhalt, z.B. "Datei ist neuer", zu informieren. Ob der User darauf reagieren will, und wie, das kann bzw. muss er dann schon selber entscheiden. Das Thema "Exceptionbehandlung" hat damit überhaupt nichts zu tun. Ich weiß, hart für dich als "Exception-Spezialisten", aber manchmal muss man(n) eben auch mal ganz einfach die Aufgabenstellung sehen.

[OT]

Zitat von p80286:

Ich habe die Erfahrung gemacht, daß ein QuicknDirty Tool eine "internationale" Karriere gemacht hat. Darum Wirf den Einwand nicht zu weit weg. Zumindest rudimentär sollte man die Möglichkeit in Betracht ziehen.

Gruß
K-H

"rudimentär" = Ja sicher. Wenn's denn mal wieder so sein sollte greife ich auf mein noch aus den 90-er Jahren stammendes Tool für die Internationalisierung zurück. Damals hatte ich eine KFZ-Software programmiert (die es übrigens immer noch gibt) mit welcher man zur Laufzeit auf Französisch, Englisch, Italienisch, Tschechisch und Polnisch umschalten konnte.
[/OT]