mach ich zu viele Kommentare?

[DrGreenthumb]

peterchen schrieb:

bei UpdateData(true/false) freu ich mich immer, wenn#s kommentiert ist, weil ich mir nie die Richtung merken kann...

das darf aber kein Grund sein, dass unnötige Kommentare den Code befüllen. Sowas kann man immer in der Doku nachsehen.

[peterchen]

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

Ich würd# von niemandem verlangen, seine UpdateData's zu kommentieren, aber auch niemanden zurückhalten. Es gibt viel sinnlosere Kommentare als z.B. "// saveAndValidate" oder "// update controls"

Wenn man komplexen Code bearbeitet, der evtl. von jmd anders geschrieben wurde, ist jeder (richtige) Hinweis wertvoll, weil selbst ein Blick in die Doku einen mentalen Kontextswitch darstellt, der evtl. wichtigere Informationen rauswirft. Ist alles eine Frage der Kompexität. Als ich noch "Kleinkram" programmiert hab, ging mit der ganze Krempel auch A. vorbei. Aber wenn man 500.000 Zeilen Quelltext zusammenhalten muß, an denen 5 verschiedene Leute gearbeitet haben, sieht man die Dinge halt ein bißchen anders.

Du kannst dir ja einen Editor (oder Plugin) schreiben, der alle Kommentare ausblendet

[Helium]

Versuche NIEMALS zu erklären WIE dein Code etwas macht. erkläre knapp WAS er macht. Versuche deinen Code so zu schreiben dass er selbsterklärend ist. Und es ist reinste zeitverschwendung schlechten Code zu erklären

Was soll das denn? Wie er es macht ist offensichtlich, nur was er macht nicht.

if (x > 1)
   x = 2-1/x;

Wie erledigt der Code seine Aufgabe? Er prüft, ob x größer als eins ist. Wenn ja weist er x den Wert 2-1/x zu.
Das ist ja hochinteressant und hätte ich da ich überhaupt nicht programmieren kann selbst nie rausgefunden. Schwachsinn!
Was macht er das ist viel interessanter. Gut der Code ist jetzt aus dem Kontext gerissen, aber ich garantiere dir, selbst wenn ich dir den Kontext zeige wirst du nicht erschließen können, was der Code macht.

Und hier ist eine Stelle, an der ich persönlich momentan keine eigene Funktion aus dem Code machen, sondern ein kleines Komentar dran pappe.

Nebenbei: Ich hab mal ein paar meiner Projekte analysieren lassen. Bei den meisten kam raus, das 3% des Projekts aus Komentar besteht. Der höchste Anteil war 15%.

[Chickenman]

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.

Blue-Tiger schrieb:

Die vielen "im folgenden Block tu ich dieses und jenes" Kommentare, dich ich manchmal machen, kommen mir oft wie die ueberfluessigsten vor...

Deswegen sollte am Anfang der Funktion einmal Erklärt sein was gemacht wird und fertig.

Blue-Tiger schrieb:

Und wenn man deshalb so lange Namen verwendest, aber trotzdem in den 80 Spalten bleiben willst, wie sieht dann ein Funktionsaufruf auf? Auf x Zeilen aufgeteilt? Schoen und gut, aber wie willst du dann in den 24 Zeilen bleiben? Noch dazu, wo doch gut strukturierter Code auch nach Whitelines schreit?

Und ausserdem verwenden - von den EMACS-Juengern mal abgesehen - heutzutage doch die meisten Programmierer Entwicklungsumgebungen, wo mehr als 1 Terminalseite auf einmal auf dem Bildschirm platz hat

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 Paskal wo man noch "ThisVariableIsATemporaryCounter" anstatt "i" geschrieben hat... Vor allem C/C++ sind spartanische Sprachen!

Und EMACS nutzen noch viele
Das mit der Terminalseite hat jedoch nicht nur Platz-Gründe sondern soll vor allem auch eine Richtlinie zur Selbstkontrolle sein. Superfunktionen in Megabreite verleiten nunmal zu Fehlern und sind unübersichtlich!

peterchen schrieb:

Versuche NIEMALS zu erklären WIE dein Code etwas macht.
Einspruch gegen das geschriene "niemals". Es gibt sehr wohl Fälle, in denen ein kurzer Kommentar zum "Wie" arge Kopfschmerzen beim späteren Leser wermeiden kann. Nicht jedes "Wie" ist für den besten programmierer immer aus dem Code ersichtlich, und für den durchschnittlichen programmierer schon gar nicht.

Die Ausnahme bestätigt die Regel. Aber es sollte auf jeden Fall eine Ausnahme bleiben! Und der Kommentar solte so kurz wie nur möglich schreiben. Und am besten sollte man eine Woche später nochmal nachgucken und nochmal überlegen obs nicht nen besseren Weg gäbe.

peterchen schrieb:

Problem: wenn eine Funktion viel internen State zu verwalten hat, kann ich das nicht einfach auf mehrere Funktionen aufbrechen

Stimmt. Ausnahme. Lange Case oder if-Szenarios können nicht aufteilt werden, das würde es unübersichtlich machen.

Blue-Tiger schrieb:

Zeig doch mal ein bischen Beispielcode von dir

Nö aber ausm Linuxkernel gibts zum Beispiel module.c (hab ich halt grad offen)

/**
 * inter_module_register - register a new set of inter module data.
 * @im_name: an arbitrary string to identify the data, must be unique
 * @owner: module that is registering the data, always use THIS_MODULE
 * @userdata: pointer to arbitrary userdata to be registered
 *
 * Description: Check that the im_name has not already been registered,
 * complain if it has.  For new data, add it to the inter_module_entry
 * list.
 */
void inter_module_register(const char *im_name, struct module *owner, const void *userdata)
{
	struct list_head *tmp;
	struct inter_module_entry *ime, *ime_new;

	if (!(ime_new = kmalloc(sizeof(*ime), GFP_KERNEL))) {
		/* Overloaded kernel, not fatal */
		printk(KERN_ERR
			"Aiee, inter_module_register: cannot kmalloc entry for '%s'\n",
			im_name);
		kmalloc_failed = 1;
		return;
	}
	memset(ime_new, 0, sizeof(*ime_new));
	ime_new->im_name = im_name;
	ime_new->owner = owner;
	ime_new->userdata = userdata;

	spin_lock(&ime_lock);
	list_for_each(tmp, &ime_list) {
		ime = list_entry(tmp, struct inter_module_entry, list);
		if (strcmp(ime->im_name, im_name) == 0) {
			spin_unlock(&ime_lock);
			kfree(ime_new);
			/* Program logic error, fatal */
			printk(KERN_ERR "inter_module_register: duplicate im_name '%s'", im_name);
			BUG();
		}
	}
	list_add(&(ime_new->list), &ime_list);
	spin_unlock(&ime_lock);
}

/**
 * inter_module_unregister - unregister a set of inter module data.
 * @im_name: an arbitrary string to identify the data, must be unique
 *
 * Description: Check that the im_name has been registered, complain if
 * it has not.  For existing data, remove it from the
 * inter_module_entry list.
 */
void inter_module_unregister(const char *im_name)
{
	struct list_head *tmp;
	struct inter_module_entry *ime;

	spin_lock(&ime_lock);
	list_for_each(tmp, &ime_list) {
		ime = list_entry(tmp, struct inter_module_entry, list);
		if (strcmp(ime->im_name, im_name) == 0) {
			list_del(&(ime->list));
			spin_unlock(&ime_lock);
			kfree(ime);
			return;
		}
	}
	spin_unlock(&ime_lock);
	if (kmalloc_failed) {
		printk(KERN_ERR
			"inter_module_unregister: no entry for '%s', "
			"probably caused by previous kmalloc failure\n",
			im_name);
		return;
	}
	else {
		/* Program logic error, fatal */
		printk(KERN_ERR "inter_module_unregister: no entry for '%s'", im_name);
		BUG();
	}
}

/**
 * inter_module_get - return arbitrary userdata from another module.
 * @im_name: an arbitrary string to identify the data, must be unique
 *
 * Description: If the im_name has not been registered, return NULL.
 * Try to increment the use count on the owning module, if that fails
 * then return NULL.  Otherwise return the userdata.
 */
const void *inter_module_get(const char *im_name)
{
	struct list_head *tmp;
	struct inter_module_entry *ime;
	const void *result = NULL;

	spin_lock(&ime_lock);
	list_for_each(tmp, &ime_list) {
		ime = list_entry(tmp, struct inter_module_entry, list);
		if (strcmp(ime->im_name, im_name) == 0) {
			if (try_inc_mod_count(ime->owner))
				result = ime->userdata;
			break;
		}
	}
	spin_unlock(&ime_lock);
	return(result);
}

/**
 * inter_module_get_request - im get with automatic request_module.
 * @im_name: an arbitrary string to identify the data, must be unique
 * @modname: module that is expected to register im_name
 *
 * Description: If inter_module_get fails, do request_module then retry.
 */
const void *inter_module_get_request(const char *im_name, const char *modname)
{
	const void *result = inter_module_get(im_name);
	if (!result) {
		request_module(modname);
		result = inter_module_get(im_name);
	}
	return(result);
}

/**
 * inter_module_put - release use of data from another module.
 * @im_name: an arbitrary string to identify the data, must be unique
 *
 * Description: If the im_name has not been registered, complain,
 * otherwise decrement the use count on the owning module.
 */
void inter_module_put(const char *im_name)
{
	struct list_head *tmp;
	struct inter_module_entry *ime;

	spin_lock(&ime_lock);
	list_for_each(tmp, &ime_list) {
		ime = list_entry(tmp, struct inter_module_entry, list);
		if (strcmp(ime->im_name, im_name) == 0) {
			if (ime->owner)
				__MOD_DEC_USE_COUNT(ime->owner);
			spin_unlock(&ime_lock);
			return;
		}
	}
	spin_unlock(&ime_lock);
	printk(KERN_ERR "inter_module_put: no entry for '%s'", im_name);
	BUG();
}

Die Kommentarregeln sind übrigens von dem für Kernelprogrammierer vorgegebenen CodingStyle. Was ich bei C++-Programmierung sonst noch gut find ist unter http://geosoft.no/style.html

Ausserdem sind das alles nur wohlgemeinte Richtlinien die en-Masse erprobt worden für gut befunden worden sind...

[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*

[Ein ehemaliger Benutzer]

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!)

[Ein ehemaliger Benutzer]

"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.

[Ein ehemaliger Benutzer]

@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