mach ich zu viele Kommentare?

DrGreenthumb

peterchen schrieb:

@Doc: falsch, die Dinger sollten "UpdateControls()" und "SaveControlData()" heißen. .rolleyes:
(i.e. klare Namen sind wichtiger als Doku - inline oder extern)

Da sind wir uns einig, aber dann lieber gleich richtige Funktionen schreiben als Blöde zu benutzen und jedesmal ein Kommentar hinter zu packen

Es ist ja nicht so, dass mich zu viele Kommentare stören, es ist nur blöd wenn man dort Kommentare setzt, wo man es mit Sprachmitteln besser hätte ausdrücken können.

Und diese beiden Beispiele hier, sind echt das krasse Gegenteil was ich mir von gut-lesbarem Code vorstelle.

Die ganze Ausgangsfrage ist schon ungünstig. Wenn man soviele halbglobale Variablen hat, brauch man natürlich Kommentare, dass man überhaupt weiß worums geht.

SirLant

SirLant schrieb:

// Tasse zeichnen
				if (tasse_oder_smilie.equals ("tasse") ||
				    tasse_oder_smilie.equals("Tasse")  ||
				    tasse_oder_smilie.equals ("t")     ||
				    tasse_oder_smilie.equals ("T"))
						java_tasse (g, color, x, y, breite, hoehe);
				else if (tasse_oder_smilie.equals ("smilie") ||
				         tasse_oder_smilie.equals ("Smilie") ||
				         tasse_oder_smilie.equals ("s")      ||
				         tasse_oder_smilie.equals ("S"))
				         	smilie (g, color, x, y, breite, hoehe);

Sowas zB. muss doch echt nicht sein.

tasse_zeichnen();

wär viel schöner und braucht kein Kommentar.

Für mich sind diese beiden Beispiele die totale Katastrophe (mal abgesehen davon, dass hier blödsinnige Kommentare drin sind weil's ein Lehrer so will).[/quote]

Wie sollte ich es denn deiner Meinung nach schreiben? Ich habe 4 mögl. Kombinationen
die der benutzer eingeben kann für den Smilie und 4 für die Tasse,
und für den Fall, dass etwas anderes passiert, ist das else da, das kann
ich doch gar nicht einfach so vereinfachen, oder doch?

Zu den Kommentaren, wie gesagt, die meisten in der Klasse würden bei dem Color.decode
bereits verzweifeln trotz dem Kommentar, ohne wäre das für die Undenkbar.

Blue-Tiger

*dreifach_post*

Blue-Tiger

Chickenman schrieb:

Blue-Tiger schrieb:

ich find deinen ersten Vorschlag merkwuerdig. IMO sind Kommentare genau dafuer da, zu erklaeren WIE etwas gemacht wird... Das WAS an sich wird doch eigentlich schon durch die Sprache selbst ausgedrueckt (z. B. durch die Funktionsnamen), und ist mit einem kleinen Ueberfliegen des Codes meistens sowieso mehr oder weniger ersichtlich.

Merkwürdig sind dann aber auch alle die am Linux-kernel coden, und das sind viele die den Code von vielen verstehen müssen. Ich finde dass Wie-Erklärungen entwender total überflüssig sind da entweder der Code selbsterklärend ist oder der Code so schlecht geschrieben ist dass man sowieso neuschreiben sollte. Also nur noch was.

Ich schaetz mal, es gibt aber auch genug Websites oder zumindest Newsgroups, die sich mit dem "wie" ausseinandersetzen, oder?
Ausserdem gehts mir bei den vielen Kommentaren hauptsaechlich darum, auf die Schnelle den Quellcode zu verstehen, quasi schon im Ueberfliegen... ohne dass ich mich richtig tief hinein knien muss. Ich bevorzuge "plain text" gegenueber Code, wenn's ums Verstaendnis geht.

Chickenman schrieb:

Was hast du denn für Funktions/Variablennamen
Wenn du 80 Zeichen damit voll bekommst...
const void * inter_module_get_request(const char *im_name, const char * mod_name)
Das sind 79... Du Programmierst ja nicht Pascal wo man noch "ThisVariableIsATemporaryCounter" anstatt "i" geschrieben hat... Vor allem C/C++ sind spartanische Sprachen!

Humm... ich bin ehrlich gesagt nicht dafuer zu sagen, C/C++ sind spartanische Sprachen. Sie haben nur... hmm... eine spartanische Tradition. Ich persoenlich bevorzuge laengere, aber besser-beschreibende Namen - ich wuerd sagen so im Durchschnitt 8-10 Zeichen je Variablen-Name, und so 20-25 je Funktionsname (was aber IMO nicht wirklich schrecklich lang ist).

*g*... ich muss aber sagen, der Kernel-Code ist wirklich sauber

Na ja, ich werd mir beim naechsten Projekt mal mehr Gedanken ueber Kommentare und Funktionslaenge machen (hab meinen Code vorhin auch mal analysieren lassen... ca. 35% des Codes sind Kommentare... wieviel ist den eurer Meinung nach "normal" ?)

Andere Frage:
Ich hab den anfangs geposteten Quellcode mittlerweile etwas (nur wenig) aufgeteilt, sieht jetzt so aus:

function chooseRandomQuotation() 
/*******************************************************************************
 * return a randomly choosen quotation
 *	RETURN
 *		array of two string containing quotation and author
 ******************************************************************************/
{
	$all_quotes = getAllQuotations();

	srand();
	$random_index = rand(0, count($all_quotes)-1);

	return $all_quotes[$random_index];
}

function getAllQuotations($quotation_file = QUOTATION_FILE,
                          $quotation_separator = QUOTATION_SEPARATOR)
/*******************************************************************************
 * read all quotations in a file and store them in an array
 *	PARAMETER
 *		$quotation_file: the file with the quotation
 *		$quotation_separator: this string separates two quotations in the file
 *	RETURN
 *		an Array of subarrays. Every sub-Array has 2 elements: the quotation
 *		itself and the author...
 *	NOTE
 *		in the file, the quotation itself must not be intended and the author
 *		is written in its own line, intended by 1 tab, in order for this 
 *		function to recognize it :)
 ******************************************************************************/
{
	// IMPLEMENTATION-ALGORITHM
	// ------------------------
	//		until file is completely read out
	//			until you don't find a $QUOTATION_SEPARATOR
	//				read line
	//				if line is empty
	//					read next line
	//				add line to actual quotation
	//			last line could contain the quotation's author
	//			if it does, extract it and store it separatly

	global $DEBUG;

	// leave function if file does not exist
	// test if the file exists in a separate if-statement, because
	// trying to open a non-existing file would result in a PHP-Error
	// (displayed on the created website too)
	if ( !file_exists($quotation_file) )
		return;
	$file = fopen ($quotation_file, 'r');
	if (!$file)
		return;

	// read file
	for ($i = 0; !feof($file); ++$i)
	{
		$quotation = '';
		$line = '';
		$quotation_lines = -1;

		// extract next quotation
		do
		{
			$quotation .= $line;
			++$quotation_lines;
			$line = fgets($file);

			// jump over empty lines
			while (strpos($line, "\n") < 2 && !feof($file))
				$line = fgets($file);
		}while (strcspn($line, $quotation_separator) != 0 && !feof($file));

		// little hack: dunno why, but somethimes empty quotes are read in ?-)
		if (strlen($quotation) < 2)
			continue;

		$quotation = htmlentities($quotation);
		$quotation = str_replace('  ','&nbsp;&nbsp;', $quotation); // conserve indentation

		// get the author
		// the last line in $quotation usually contains the author, but
		// one-liners (as well as some other quotes) don't have any author
		$author = '';
		if ($quotation_lines > 1)
		{
			$tmp = substr($quotation, 0, strlen($quotation)-1); // cut of the last newline from $quotation, 
			$author_line = strrchr($tmp, "\n");                 // otherwhise strrchr would return the wrong line

			// author-line starts with a tab.. if there's no tab,
			// then there's probably no author either 
			if ($author_line[1] == "\t")
			{
				$author = substr($author_line, 2);
				$quotation = str_replace($author_line, '', $quotation);	// strip the author out of the quotation itself
			}
		}

//		if ($DEBUG) echo "<p>quote: $quotation\n<br />author: $author\n<br />lines: $quotation_lines</p>\n";
		$quotation = nl2br($quotation);

		$quotes[$i] = array($quotation, $author); // store the found quote for later use
	}
	return $quotes;
}

Ich koennte getAllQuotations() noch weiter aufteilen, z. B. in readNextQuotation() und stripAuthor() oder so.... wuerd die einzelnen Funktionen kuerzer machen, klar... nur: ich seh irgendwie den Sinn darin nicht, es wuerd den Code unnoetig aufblaehen, und ich haette anschliessend neue Funktionen kreiert, die ich mit Sicherheit nirgendwo anders im Code brauchen wuerde, und die nur an einem einzigen Ort aufgerufen werden. Wuerde das nicht dem Sinn von prozeduraler Programmierung wiedersprechen? Der Code gehoert doch zusammen, wieso sollt ich den aufteilen? getAllQuotations() waere danach vielleicht uebersichtlicher, dafuer haett ich dann irgenwo ein stripAuthor() rumliegen, und wuerd mich das naechste mal, wie ich das Ganze durchsehe fragen, wozu ich das denn verwenden koennt.... IMO

Blue-Tiger

*dreifach_post*

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