mach ich zu viele Kommentare?

So, ich will mal testen, ob meine Kommentare so in Ordnung wären. Es handelt sich dabei für euch um eine unbekannte Funktion. Stellt euch vor ihr hätten diesen Quellcode vorgesetzt bekommen und solltet euch da jetzt reinarbeiten. Für euch besteht leider noch zusätlich die Schwierigkeit, dass es sich, um für euch eine etwas nicht sogeläufige Sprach handelt: DelphiLanguage.

////////////////////////////////////////////////////////////////////////////////
// Extract files
// Comment   : Core of the programm
// Arguments : SFXArchive, DestFolder, DestFile: string; DestFileSize, OffSet: LongInt
// Result    : Integer - Syserrorcode

function Extract(SFXArchive, DestFolder, DestFile: string; DestFileSize,
  OffSet: LongInt): Integer;
var
  FSFXArchive, FDestFile: file of Byte;
  MemBuffer: array[0..BLOCKSIZE - 1] of Byte;
  FileOffSet, BytesRead, BytesToRead, TotalBytesRead: Integer;
begin
  SetLastError(0);
  TotalBytesRead := 0;
  // Format path strings
  if HasBackslash(DestFolder) then
    DestFolder := DelBackSlash(DestFolder);
{$IOCHECKS OFF}
  // Assign EXe file with files
  AssignFile(FSFXArchive, SFXArchive);
  // Open readonly
  FileMode := $0000;
  Reset(FSFXArchive);
  // IO Checks off -> handle IO errors ourself
  // IOResult <> 0: something went wrong, don't proceed!
  if IOResult = 0 then
  begin
    // Create directories if needed
    if not FileExists(DestFolder, True) then
      ForceDirectories(DestFolder);
    AssignFile(FDEstFile, DestFolder + '\' + DestFile);
    Rewrite(FDestFile);
    // IO Checks off -> handle IO errors ourself
    // IOResult <> 0: something went wrong, don't proceed!
    if IOResult = 0 then
    begin
      SendMessage(hApp, EFM_FILENAME, 0, lParam(PChar(DestFile)));
      // Jump file to extract
      FileOffSet := OffSet;
      Seek(FSFXArchive, FileOffSet);
      // Loop as long as we haven't copied size of destination file
      while FilePos(FSFXArchive) < FileOffSet + DestFileSize do
      begin
        // how many bytes are left to read?
        BytesToRead := CalcBytesToRead(TotalBytesRead, DestFileSize);
        // Copy
        BlockRead(FSFXArchive, MemBuffer, BytesToRead, BytesRead);
        BlockWrite(FDestFile, MemBuffer, BytesRead);
        // keep in mind how many bytes we have copied so far
        TotalBytesRead := TotalbytesRead + BytesRead;
        SendMessage(hApp, EFM_PROGRESS, 0, Trunc(TotalBytesRead * 100 / DestFileSize));
        ProcessMessages(hApp);
      end;
    end;
    CloseFile(FDestFile);
  end;
  CloseFile(FSFXArchive);
{$IOCHECKS ON}
  // Return what went wrong
  SendMessage(hApp, EFM_FINISH, GetLastError(), lParam(PChar(DestFile)));
  result := GetLastError();
end;

Bei den EFM Nachrichten handlet es sich um selbst definierte Nachrichten, die wie folgt definiert sind (mit Kommentar):

const
  EFM_FILENAME = WM_USER + 2004; // lParam = DestFile
  EFM_PROGRESS = WM_USER + 2005; // lParam = Percent done
  EFM_FINISH = WM_USER + 2006; // wParam = Syserrorcode, lParam = Destfile

Könntet ihr an Hand der Kommentare jetzt nachvollziehn, was da passiert? Die Funktionen HasBachSlash und DelBackSlash sollten selbsterklärend sein.

Zur Info eventuell noch die Erklärung was das Programm macht:
An die Anwendung, in der sich diese Funktion befindet, wurden mittels einer anderen Anwendung Dateien drangehangen und die Exe, aus welcher diese Funktion stammt, extrahiert sie nun zur Laufzeit.

Das ganze soll OpenSource werden und ich wüßte gerne, ob es so möglich ist, dass andere den Code evetuell weiterentwickeln bzw. Fehler könnten.

peterchen

Mal grob drübergebraust sieht's gut aus.

Du kannst dir etwas sparen, indem du vielleicht etwas prägnanter wirst (z.b: "remember no. of bytes already stored" kürzer als "keep in mind..."), aber das ist imemr ein trade-off, und da schreib lieber so wie dir der Schnabel gewachsen ist.

Wo ich mir arge Gedanken machen würde ist der Funktions-Header. Hier fehlt wirklich "fast alles".

Minimum wäre m.E.

"Extract a single file [DestFile] from Archive File [SFXArchive] to Folder [DestFolder]. [Offset]: offset in Archive file, [DestFileSize]: size of the extracted file"

Mir lieber (wenn auch wortreicher):
"Extract a single file from an Archive.
\param SFXArchive [string]: Name of the archive file
\param DestFolder [string]: Folder to extract file to
\param DestFile[string]: name of the file to extract
...
\return [integer] 0 if successful, or Syserrcode otherwise
"
evtl. kann man sich das Parametertyp-Wiederholen für "logische" Parametertypen sparen - aber da kann man sich ja schon wieder streiten was logisch ist.

Wichtig sind vor allem Randbedingungen, Sonderfälle usw. (schein bei deiner Funktion keine zu geben - noch viel besser!)

"Extract a single file [DestFile] from Archive File [SFXArchive] to Folder [DestFolder]. [Offset]: offset in Archive file, [DestFileSize]: size of the extracted file"

Das ist gut, das werde ich als Comment nehmen.

Hier übrigens, was mir in meinem Stammforum gesagt wurde:
http://www.delphipraxis.net/topic12317_kommentare+gut+schlecht+zu+viel+zu+wenig.html

Bashar

Ich finde, die meisten Kommentare in dieser Funktion helfen nicht wirklich beim Verstehen. Sie beschreiben entweder, was der Code gerade tut nochmal in Englisch, oder sie helfen einem, wenn man in Delphi nicht ganz sattelfest ist. Beides ist IMHO unnötig, wichtiger wäre oben drüber ein Kommentar, was die Funktion überhaupt genau macht, und was die Parameter bedeuten.

peterchen

Ich kann mich vielen der Sachen in deinem "Stammforum" anschließen (keine Umkehrung der if - Bedingung, stattdessen "if ioresult = 0 // kein I/O-Fehler", kurze kommentare dahinter statt darüber usw.), finde diese Sachen aber nicht so wichtig das ich mir darüber den Kopf zerbrechen würde.

@Bashar:
Hier gehts es, glaub ich, nicht um "sattelfest" oder nicht.
Wenn du fremden Code pflegst, mußt du viel Code überfliegen, und brauchst deinen Context-Stack (bzw. "First Level Cache") meist für ganz andere Dinge.

Pflegearbeiten beschränken sich halt nicht auf eine Funktion die man sich genau anguckt und dann ändert, sondern auf die Interaktion von Funktionen. z.B.: ich kann die Dateien jetzt auch komprimiert anhängen. Was muß ich dabei beachten, welche Funktionen sind davon betroffen, ist die übergebene Größe jetzt die gepackte oder die entpackte, usw.

Insofern ist ein "// Format path strings" viel schneller verstanden als ein "HasBackslash, RemoveBackslash - aha, hier formatiert er den pfad".

Das ist wirklich alles total sinnlos wenn man mit seinem eigenen, nicht zu alten Code arbeitet. Aber die heutzutage verwendeten Bibliotheken sind komplex genug das man von niemandem mehr erwarten kann, daß er in der ganzen WinAPI, oder in der ganzen MFC "sattelfest" ist.

@Bashar: Dann zeig mir doch mal, wie du es machen würdest?

Headhunter

Luckie schrieb:

So, ich will mal testen, ob meine Kommentare so in Ordnung wären. [..]

Hi!

Ich fand die Funktion recht leicht verständlich. Die Kommentare sind imo passend, und gut verteilt. Der Code ist auch relativ leicht verständlich.

Kritikpunkte fallen mir spontan nur zwei ein:
Die Information am Funktionskopf ist ziemlich überflüssig. Man erfährt nur den Returncode, Parameter kann ich zwei Zeilen weiter ablesen. Hier wäre eine weitere Erklärung nötig.
Und ich denke der Code wäre einfach zu lesen, wenn du ihn die Abschnitte abteilen würdest, d.h. wenn der eine Sinnblock zu Ende ist, einfach 2x Enter drücken.

^^
Naja, ist nur meine Meinung dazu

Bashar

// Extract files from SFX archive
{ Extract portion of archive starting at Offset with length DestFileSize,
  then store into location given by DestFolder/DestFile.
  Return error code. }
function Extract(SFXArchive, DestFolder, DestFile: string; DestFileSize, 
                 OffSet: LongInt): Integer; 
var 
   FSFXArchive, FDestFile: file of Byte; 
   MemBuffer: array[0..BLOCKSIZE - 1] of Byte; 
   FileOffSet, BytesRead, BytesToRead, TotalBytesRead: Integer; 

   procedure CanonicalizePath(var Path: string);
   begin
      if HasBackSlash(Path) then
         DelBackSlash(Path)
   end;

   procedure EnsurePathExists(var Path: string);
   begin
      if not FileExists(Path, True) then
         ForceDirectories(Path)
   end;

begin 
   SetLastError(0); 
   TotalBytesRead := 0; 
   CanonicalizePath(DestFolder);

   {$IOCHECKS OFF} 

   // Open archive
   AssignFile(FSFXArchive, SFXArchive); 
   FileMode := $0000;
   Reset(FSFXArchive); 
   if IOResult = 0 then 
   begin 
      EnsurePathExists(DestFolder);

      // Open destination file
      AssignFile(FDestFile, DestFolder + '\' + DestFile); 
      Rewrite(FDestFile);

      if IOResult = 0 then 
      begin 
         // Update progress display
         SendMessage(hApp, EFM_FILENAME, 0, lParam(PChar(DestFile)));

         // do extraction
         FileOffset := Offset; 
         Seek(FSFXArchive, FileOffset);

         while FilePos(FSFXArchive) < FileOffSet + DestFileSize do 
         begin 
            BytesToRead := CalcBytesToRead(TotalBytesRead, DestFileSize); 
            BlockRead(FSFXArchive, MemBuffer, BytesToRead, BytesRead); 
            BlockWrite(FDestFile, MemBuffer, BytesRead); 
            TotalBytesRead := TotalbytesRead + BytesRead; 

            // Update progress bar
            SendMessage(hApp, EFM_PROGRESS, 0, Trunc(TotalBytesRead * 100 / DestFileSize)); 
            ProcessMessages(hApp)
         end; 
      end; 
      CloseFile(FDestFile) 
   end; 
   CloseFile(FSFXArchive); 

   {$IOCHECKS ON} 

   // Return what went wrong 
   SendMessage(hApp, EFM_FINISH, GetLastError(), lParam(PChar(DestFile))); 
   result := GetLastError()
end;

peterchen schrieb:

Hier gehts es, glaub ich, nicht um "sattelfest" oder nicht.

Sag mir doch bitte einen triftigen Grund, warum in der Funktion 2mal erklärt wird, was IOError bedeutet.

Wenn du fremden Code pflegst, mußt du viel Code überfliegen, und brauchst deinen Context-Stack (bzw. "First Level Cache") meist für ganz andere Dinge.

Genau. Entweder ich will die Funktion verstehen und bearbeiten, dann brauch ich solche Kommentare nicht. Oder ich will nur wissen, was sie tut, und dann brauche ich einen Kommentar im Kopf. Sowas wie "{ Assign x to y } AssignFile(x, y);" ist doch völlig überflüssig.

Insofern ist ein "// Format path strings" viel schneller verstanden als ein "HasBackslash, RemoveBackslash - aha, hier formatiert er den pfad".

"Pfad" und "formatieren" passt bei mir irgendwie nicht zusammen. Soll er nett eingerückt werden, vielleicht kursiv und mit Silbentrennung? Nö, man will sicherstellen, dass der Pfad am Ende keinen Backslash hat. Mit einem Kommentar ala "Ensure Path doesn't end with backslash" könnte ich leben, aber "Format path" zwingt mich so oder so, den Code zu lesen, und ist daher überflüssig.

Ich hab das bei mir als eigene Funktion ausgelagert, aber bin nicht so ganz glücklich damit. Das könnte auch einfach drin stehen bleiben.

Was EnsurePathExists angeht: Eigentlich müßte ForceDirectories ohne Check reichen, kann mir nicht vorstellen, dass die Funktion fehlschlägt, wenn sie nichts tun soll. Dann würde man Ensure... einfach durch Force... ersetzen. Ich kenn aber Delphi nicht gut genug (eigentlich gar nicht :p ) um das beurteilen zu können.

Aber die heutzutage verwendeten Bibliotheken sind komplex genug das man von niemandem mehr erwarten kann, daß er in der ganzen WinAPI, oder in der ganzen MFC "sattelfest" ist.

Wieso sind dann die ganzen Standardfunktionen kommentiert, und die Winapi-Funktionen nicht?

Frenzy

CrazyOwl schrieb:

Na gut, von mir aus, ich geb auf, ihr habt gewonnen.
Nur ich kann immer noch nicht zustimmen, dass es sinnvoll ist jede Zeile zu kommentieren.

Das kommt auf den Zweck an denke ich.

Wenn man nun noch Schüler ist und der Lehrer das Verständnis überprüfen will, ohne den Schüler auszuquetschen sind Kommentare an "fast jeder" Codestelle sinnvoll. Ob man nun bei

// Deklarieren und Initalisieren der Integer-Variable nPos
int nPos = 0

hinschreiben sollte ist in meinen Augen fraglich, außer es ist das erste Modul eines Schülers.

Aber bei komplexeren Aufrufen sollte man dennoch nen Kommentar verwenden. Ab wann das der Fall ist hängt denke ich von jedem Programmieren und seinem eigenen Level ab.

Fragen die ich mir stelle wegen Kommentaren:

- Wie leicht habe ich diese Zeile geschrieben?
- Ist es "geklauter" Code von einer Codeseite?
- Mußte ich viel nachschlagen in einem Buch/Site etc.?

Naja...zuviel oder zuwenig kann man sich drüber streiten halt....

Allerdings macht es an vielen Stellen mehr Sinn anstatt unzähliger Kommentare lieber mehrere Methoden zu deklarieren.

http://www.codeproject.com/tips/doxysetup.asp

peterchen