Was ist für euch guter Programmcode?

Darf ich mal fragen, wieso du nicht

return bytesTransfered > 0;

schreibst? Deine 3 ?: sind ja nur noch krass.

SirLant

HumeSikkins schrieb:

bit_vec_int_set(const bit_vec_int_set& other);
	bit_vec_int_set& operator=(const bit_vec_int_set&);
        /*...*/

Hat es einen bestimmten Grund, dass du nur bei op= dem Parameter keinen Namen gibst?

simon kann man den scheiß mit Handle::lastUsedHandle nicht ganz rausnehmen? sieht dirty aus. :p

Shade Of Mine

simon.phoenix schrieb:

Ja, kann man sich drüber streiten. Habe das Template nur, weil ich zwischen char* und SQLCHAR* (= unsigned char*) unterscheide. Aber du hast schon Recht.

Ich habe alles gesagt, was mir aufgefallen ist. In deinem Kontext mag es durchaus sinn machen es anders zu machen als ich es vorschlagen würde.

Ist auch Mist, war mir noch nicht im Klaren darüber, ob ich zwei Fehlerbehandlungen (einmal mit, einmal ohne Exceptions) anbiete. Ich denke ich werde komplett auf Exceptions umsteigen.

Jo, ein
try
{ foo(); }
catch(...)
{}

ist ja auch OK, siehe Optimizers Code.

Ok, könnte auf die Funktionen verweisen, für die lastUsedHandle unerlässlich ist.

Ja, oder sonst irgendwas (beispiel wo es nützlich ist) wo es gesetzt wird, oder so.

Ist es in etwa auch.

Dann wäre ein Kommentar diesbezüglich ganz praktisch

Sorry, verstehe nicht ganz. Wie sollte dann das Design Exception/Fehler/Handle-Klasse aussehen?

Ich nehme als Beispiel mal meine Socketlib.
Hier hat man generell eine SocketException() Klasse, die errno/WSAGetLastError nimmt, also quasi die Fehlernummer, mehr nicht. Die SocketException Klasse ermittelt daraus eine schoene lesbaere Message, etc.

Der Clientcode macht nur
throw SocketException(reason);

Ähnlich könntest du es auch machen. Das würde dir die ganze Logik zum werfen einer Exception sparen, weil du nur das Fehlerhafte Handle der Exceptionklasse geben musst.

Ein GetError() ist IMHO immer unnötig. Denn es kann ja nur ein Fehler aufgetreten sein, wenn irgendwo eine Exception geflogen ist. Und in diesem Fall muss ich die Exception ja sowieso irgendwo gefangen haben...

Diese ganzen GetError, GetLastError, errno Sachen sind C Relikte um Fehlerbehandlung zu erleichtern. Bei Exception braucht man es aber nichtmehr.

Stimmt könnte ich erwähnen. Wer noch nicht mit reiner ODBC-API gearbeitet hat, weiss das nicht.

Wenn es zu der Anwendungsdomain gehört, dass man ein Handle immer durch ein InputHandle erstellt und es klar ist woher man das bekommt, ist ein Kommentar nicht wichtig, weil der Leser des Codes es dann ja sowieso wissen muss.

Alle ODBC-API-Funktionen wollen ein SQLHANDLE. Die Wrapper-Klasse wird da nie direkt verwendet. Somit muss zwangsläufig ein Umwandlungsoperator benutzt werden. Aber sicher sinnvoller durch eine Get-Funktiong gelöst (s.o.). Rein aus Gründen der syntaktischen Bequemlichkeit geschehen...

dh, lastUsedHandle wird immer dann aktualisiert, wenn ein Handle Objekt an eine API Funktion übergeben wurde?
Das könnte man etwas klarer ausdrücken.

Hmm ja lässt sich drüber reden, aber nicht zwingend notwendig, denke ich

Wie gesagt: IMHO würde es den Code besser lesbar machen:
if(SUCCESS(foo))
ist IMHO schöner als
if(foo==THIS || foo==THAT)
zumal wenn foo, THIS und THAT ziemlich lange Namen haben

Meinst du dass die Exception-Klasse die Fehlermeldung komplett selbst anhand des Handles generiert?

Ja. bzw. welche Daten halt nötig sind um die Fehlermeldung & Co zu generieren.

Ja ist nicht ganz klar. Checkt eigentlich nur den Rückgabewert

Wie wäre es mit ApiCallWrapper
Oder SafeApiReturn
oder so etwas.

Oh, habe vergessen da die Alloc-Funktion reinzupacken

Codereview rult halt

Jo stimmt. Free() darf allerdings keine Exception werfen, da es im Destruktor aufgerufen wird. Außerdem ist es die Frage, ob es Sinn macht, auf einen Fehler beim Deallozieren eines Handles (der zu 99,9999% nie auftreten wird), hinzuweisen.

Klar, da kann man nix machen. deshalb im Dtor einfach catchen und vielleicht irgendwo Loggen, aber sonst nix machen... siehe Optimizers Code.
In Java hat man sowas dauernd

Ja ich hasse C-APIS :(. Meinst du, dass buf nach einem fehlerhaften Aufruf von SQLGetDiagRec undefiniert sein könnte? Stimmt, habe ich nicht bedacht...

Ja.
Sowas kann teuflisch sein.

Also riesigen Dank für die vielen Vorschläge und dass du dir den Code so genau durchgesehen hast

Ach, ich mach CodeReviews gerne. Meistens aber bei meinen eigenen Codes (wenn ich zu faul bin etwas zu programmieren, aber dennoch programmieren will/muss.

Nur leider bin ich oft zum refaktoren zu faul

Shade Of Mine

Optimizer schrieb:

Darf ich mal fragen, wieso du nicht
return bytesTransfered > 0;
schreibst? Deine 3 ?: sind ja nur noch krass.

Da war ein wenig Ironie in meinem Code

simon.phoenix

Also noch mal danke Shade

Wir könnten doch mal ein allgemeines Code-Review-Board einrichten

edit: Lohnt es sich in Client-Code (wo man eh kaum ne umfangreiche externe Doku braucht) für doxygen optimiert zu kommentieren?

simon.phoenix

atom schrieb:

simon kann man den scheiß mit Handle::lastUsedHandle nicht ganz rausnehmen? sieht dirty aus. :p

Ich schau mal, wies nun mit nem neuen Design aussieht und ob das da noch nötig ist

simon.phoenix

Sorry für 3fach Post aber mir ist noch was ganz entscheidendes eingefallen, stark beeinflusst von Alexandrescu, den ich grade mal wieder gelesen habe.

Lohnt sich eine ReportingPolicy für Exception-Klassen oder wäre das, wie es auf Shades Page einmal so schön heißt, "Overengineering"?

Mecnels

Shade Of Mine schrieb:

der trick ist: auch eine KI funktion muss nicht mehr als 10 zeilen haben.
die einzigen langen funktionen bei mir (mit lange meine ich 20 zeilen) sind initialisierungs funktionen, die somit keine logik enthalten.

Vielleicht postet einfach jeder einen typischen, durchschnittlichen, kurzen Auszug aus seinen Programmen, und irgendwann küren wir dann den 'übersichtlichsten Code'. Na?

Kannst dir ja mal das TicTacToe aus meinem Tutorial ansehen, ist zwar nicht eine entgültige version - dank den leuten hier im forum gabs viele verbesserungen (die sind aber nicht online)

so sieht etwa ein programm bei mir aus.
du wirst erkennen, dass ich nahezu keine kommentare brauche und relativ kurze klare funktionen habe.
dazu wird auch noch alles plattformunabhängig gehalten

da kannst du noch eine menge lernen (obwohl es, wie gesagt, noch kein wirklicher vorzeigecode ist)

Findest Du nicht auch, dass sich TicTacToe nicht so ohne weiteres mit einer Umsetzung des Brettspieles Stratego vergleichen laesst? In TicTacToe gibt es schlimmstenfalls 9! (also rund 300.000) verschiedene Situationen auf dem Brett, mit denen eine KI zurecht kommen müsste, beim Stratego gibt es 92 Felder und 80 sehr unterschiedliche Steine, die in unberechenbar vielen Variationen auf dem Spielfeld verteilt sein können (zehn hoch fünfzig oder mehr kommt vielleicht hin). Eine KI, die sich unter solchen Voraussetzungen auch in möglichst jeder Spielsituation zurecht finden soll, lässt sich eben nicht mit Tricks oder in 10 Zeilen codieren.

Mir fällt auf, dass viele von Euch 'schleifenscheu' sind, das heisst: in den geposteten Beispielcodes ist zwar alles ganz nett in kleine Teilfunktionen zergliedert, aber kaum einmal entdecke ich (z.B. beim Programm mit den gerichteten Graphen) geschachtelte Schleifen.
Ein heisser Tip: Schleifenvariablen werden üblicherweise in einem Prozessorregister untergebracht und inkrementiert, die Adressen irgendwelcher Funktionen hingegen müssen immer erst aus dem Speicher abgefragt werden. Ausserdem muss für jede lokale Variable in diesen Funktionen sowie für die übergebenen Parameter (welchen Typs auch immer) jedes Mal aufs Neue Speicher allokiert werden, die Adresse dieses Speichers in ein Register geholt werden und so fort. Was ich sagen will: Schleifenreicher Code, egal ob in C++ oder Java, ist immer um mindestens das Doppelte schneller als Code, der mit hunderten Minifunktionen vollgestopft ist (vielleicht ist das für viele Programmierer angenehmer zu lesen und zu verstehen, aber man sollte sich schon im Klaren darüber sein, dass zuviele Funktionen die Performance des gesamten Programmes negativ beeinflussen).

Unverständlich ist mir deshalb die Meinung, schleifenreicher Code sei schlecht, denn in jedem Fall ist er locker um das Doppelte leistungsstärker als Code, der mit Funktionsbezeichnern (Adressen) nur so überhäuft ist.

Jedenfalls ist auch der Code mit dem gerichteten Graphen sehr ordentlich und für einen Aussenstehenden, der nur Grundkenntnisse in Java hat, leicht nachvollziehbar. Hut ab!

Einen netten Abend noch.

Shade Of Mine

Optimizer schrieb:

* @param file Der ursprüngliche Dateiname.
[...]
	protected static File findValidFileName(File file)

Wirklich der _Dateiname_?

int i = 1;
    while( true )
		{
			File newFile = new File(	file.getAbsolutePath() +
										" (" + i + ")" );
			if( !newFile.exists() )
				return newFile;

			++i;
		}

Wäre es mit for nicht schöner?

for(int i=1;;++i)
{
  File newFile = new File(	file.getAbsolutePath() +
                 " (" + i + ")" );
  if( !newFile.exists() )
	  return newFile;
}

/**
	 * Schließt ein Socket und fängt jede IOException, die dabei auftritt.<br>
	 * Leider implementiert <code>ServerSocket</code> nicht das
	 * <code>Closeable</code>-Interface, sonst wäre diese Methode überflüssig.
	 * @param socket Ein Socket, das geschlossen werden soll. Ist dieser
	 * Parameter <code>null</code>, so macht die Methode gar nichts.
	 */

Manchmal rulen templates schon, gell?

/**
	 * Stellt sicher, dass der Remotecomputer die selbe Protokollversion
	 * benutzt wie wir.
	 * @param protocol Die Protokollversion, die der Remotecomputer verwendet.
	 */
	private void validateProtocol(String protocol)
	{
		if( !protocol.equals(FileCarrierFrame.protocol) )
			throw new SecurityException();
	}

Warum ist das eine Ausnahme?
validate*
klingt für mich nach: prüfe ob es passt und sags mir dann
hier wäre der name
ensureValidProtocol()
wohl besser, denn ein validate* klingt für mich nicht als ob
es eine Ausnahme wäre, wenn die Protokolle nicht passen...

/**
	 * Stellt sicher, dass es sich bei dem Zielpfad nicht um eine Datei handelt
	 * und dass das Verzeichnis exisitert.
	 * @throws IsNotAFolderException falls dies nicht der Fall ist.
	 */
	// TODO: Sicherstellen, dass der Dateiname keinen Pfad,
	// der oberhalb des incoming-folders liegt und keinen
	// absoluten Pfad enthält (Sicherheitsrisiko).
	// (Andere Exception werfen)
	private void validateFolder()
		throws IsNotAFolderException
	{
		if( !destinationFolder.isDirectory() )
			throw new IsNotAFolderException();
	}

2 sachen.

wie bei validateProtocol, wäre ensure* hier nicht besser?
kleiner seitenhieb auf checked exceptions.
dein Kommentar
// (Andere Exception werfen)
ist nicht durchführbar, weil du bereits IsNotAFolderException als einzig
mögliche Exception definiert hast. Eine Änderung hier, würde client code brechen.

Mir persönlich gefallen diese Optional Komponenten Progressbar & Co nicht.
Du bindest dich damit an Swing - dh, eine Terminalausgabe erschwerst du damit
ungeheuerlich.

Shade Of Mine

simon.phoenix schrieb:

edit: Lohnt es sich in Client-Code (wo man eh kaum ne umfangreiche externe Doku braucht) für doxygen optimiert zu kommentieren?

Eigentlich nicht wirklich. Weil das Dokumentieren ein großer Aufwand ist und hier kaum nutzen bringt.

Hier denke ich, ist es wesentlicher diverse Designentscheidungen zu dokumentieren und warum etwas wie gelöst wurde, sowie natürlich alle hacks/tricks und sonstigen schmutzigen sache (zB weil es performancemäßig nötig war)

Walli

Mecnels schrieb:

Findest Du nicht auch, dass sich TicTacToe nicht so ohne weiteres mit einer Umsetzung des Brettspieles Stratego vergleichen laesst?

Warum nicht? Beides Umsetzungen eines Brettspiels. Egal wie komplex Stratego ist, das Prinzip ist immer gleich. Teile ein großes Problem in viele kleine auf und die Welt sieht schon ganz anders aus.

Mecnels schrieb:

[...]Eine KI, die sich unter solchen Voraussetzungen auch in möglichst jeder Spielsituation zurecht finden soll, lässt sich eben nicht mit Tricks oder in 10 Zeilen codieren.

Du hast es nicht verstanden. Es geht nicht darum möglichst kurzen Code zu schreiben, sondern sinnvolle Abschnitte zu finden. Damit reduzierst du (falls es keine Redundanzen gibt) nicht die Länge des eigentlichen Codes, aber erhöhst die Übersichtlichkeit um ein Vielfaches. Was du machst ist alles in eine Funktion zu packen. Dadurch riskierst du, dass der Code zu einem Monster wird, das keiner mehr lesen will und übersiehst u.U. redundate Codeabschnitte.

Shade Of Mine

Mecnels schrieb:

Eine KI, die sich unter solchen Voraussetzungen auch in möglichst jeder Spielsituation zurecht finden soll, lässt sich eben nicht mit Tricks oder in 10 Zeilen codieren.

Die KI ansich hat natürlich mehr als 10 Zeilen, vielleicht sogar millionen.
Aber eben in einzelnen Funktionen.
Es gibt keine monster funktion die alles macht, sondern eine million kleine funktionen die alle nur ganz wenig machen.

Mir fällt auf, dass viele von Euch 'schleifenscheu' sind, das heisst: in den geposteten Beispielcodes ist zwar alles ganz nett in kleine Teilfunktionen zergliedert, aber kaum einmal entdecke ich (z.B. beim Programm mit den gerichteten Graphen) geschachtelte Schleifen.

Ist das nicht schön? So schön klar, keine tiefen verschachtelungen und nur wenig code pro funktion.

Ein heisser Tip: Schleifenvariablen werden üblicherweise in einem Prozessorregister untergebracht und inkrementiert, die Adressen irgendwelcher Funktionen hingegen müssen immer erst aus dem Speicher abgefragt werden.

Nö, ist nicht so. Stichwort inlining.
Ausserdem sind Funktionscalls mittlerweile so billig, dass inlining schon fast nutzlos ist.

Ausserdem muss für jede lokale Variable in diesen Funktionen sowie für die übergebenen Parameter (welchen Typs auch immer) jedes Mal aufs Neue Speicher allokiert werden, die Adresse dieses Speichers in ein Register geholt werden und so fort.

Ne.

Was ich sagen will: Schleifenreicher Code, egal ob in C++ oder Java, ist immer um mindestens das Doppelte schneller als Code, der mit hunderten Minifunktionen vollgestopft ist (vielleicht ist das für viele Programmierer angenehmer zu lesen und zu verstehen, aber man sollte sich schon im Klaren darüber sein, dass zuviele Funktionen die Performance des gesamten Programmes negativ beeinflussen).

*lol*
ne, ist sicher nicht so.

Unverständlich ist mir deshalb die Meinung, schleifenreicher Code sei schlecht, denn in jedem Fall ist er locker um das Doppelte leistungsstärker als Code, der mit Funktionsbezeichnern (Adressen) nur so überhäuft ist.

Woher hast du das?

Erstens ist vorzeigige optimierung mal sowieso ganz schlecht, auch wenn man performance beim design im auge behalten muss.
aber du scheinst nur halbwissen diesbezüglich zu haben.
ein compiler kann inlinen, teilweise inlinen die schleife unrollen, die funktion verdammt effektiv aufrufen, etc.

ein funktionsaufruf kostet heutztutage nichts mehr und da es inlining gibt kostet es sogar oft garnix.

Mecnels

Shade Of Mine schrieb:

Woher hast du das?

Nun ja, ich war mal in der Compilerentwicklung tätig. Du auch?

ness

Mecnels schrieb:

Unverständlich ist mir deshalb die Meinung, schleifenreicher Code sei schlecht, denn in jedem Fall ist er locker um das Doppelte leistungsstärker als Code, der mit Funktionsbezeichnern (Adressen) nur so überhäuft ist.

Ich kann mir nicht vorstellen, dass das die allgemeine Meinung ist. Code der weniger Schleifen hat und damit leichter zu lesen ist ist halt besser, da man heutzutage eigentlich die performace etwas weiter hintn anstellen kann als früher. Wenns ans optimieren geht, sieht das ganze dann schon wieder anders aus. Aber ich denke es heißt nicht ohne Grund, eine verfrühte optimierung ist der Ursprung allesn Übels...
Und dann gibt es ja noch inline...

Walli

Mecnels schrieb:

Shade Of Mine schrieb:

Woher hast du das?

Nun ja, ich war mal in der Compilerentwicklung tätig. Du auch?

Man muss kein Compilerentwickler sein um zu wissen das du Dünnsinn erzählst. Werd konkret... an welchem Compiler hast du mitentwickelt? Oder versuchst du am Ende nur zu bluffen?

Mecnels schrieb:

Shade Of Mine schrieb:

Woher hast du das?

Nun ja, ich war mal in der Compilerentwicklung tätig. Du auch?

ROFL

Shade Of Mine schrieb:

Optimizer schrieb:

* @param file Der ursprüngliche Dateiname.
[...]
	protected static File findValidFileName(File file)

Wirklich der _Dateiname_?

ok...

/**
	 * Schließt ein Socket und fängt jede IOException, die dabei auftritt.<br>
	 * Leider implementiert <code>ServerSocket</code> nicht das
	 * <code>Closeable</code>-Interface, sonst wäre diese Methode überflüssig.
	 * @param socket Ein Socket, das geschlossen werden soll. Ist dieser
	 * Parameter <code>null</code>, so macht die Methode gar nichts.
	 */

Manchmal rulen templates schon, gell?

Damit hast du nicht wirklich nen Vorteil. Du würdest dir hier 2 Methoden sparen. Aber du kannst den Namen genauso wenig ändern und wenn du es tust und vielleicht was anderes so nennst, oder etwas an der Funktion erweiterst, hast du Verhalten, was du vielleicht nicht wolltest, aber lassen wir das... wir hatten das ja schon mal vor kurzem. IMHO ist es einfach ein Designfehler, dass die Sockets das Interface nicht implementieren.

Warum ist das eine Ausnahme?
validate*
klingt für mich nach: prüfe ob es passt und sags mir dann
hier wäre der name
ensureValidProtocol()
wohl besser, denn ein validate* klingt für mich nicht als ob
es eine Ausnahme wäre, wenn die Protokolle nicht passen...

Da hast vielleicht Recht.

kleiner seitenhieb auf checked exceptions.
dein Kommentar
// (Andere Exception werfen)
ist nicht durchführbar, weil du bereits IsNotAFolderException als einzig
mögliche Exception definiert hast. Eine Änderung hier, würde client code brechen.

Das ist genau das Misverständniss, was mich so ärgert.
Angenommen, die Exceptions wären nicht checked. Cool, dann kann ich jetzt ja einfach noch nen anderen Ausnahmetypen werfen!!
Aber wer fängt sie? Hast du darüber schon mal nachgedacht? Hast du dann nicht mit Client Code gebrochen, wenn der plötzlich abstirbt, weil du einfach mal schnell noch ne neue Exception werfen musstest? Ein sehr ungelungener Seitenhieb, wie ich finde.
Denn mit Client Code brichst du bei sowas immer. Es ist nur ne Frage, ob es bemerkt wird, oder nicht.
btw. ist es hier natürlich völlig egal, weil die Methode eh private ist und ich das selber schon auf die Reihe krieg. Außerdem ist die Methode schlicht noch nicht fertig. Aber ich weiß schon, was du sagen wolltest, nur leuchten mir deine Argumente gegen checked Exceptions nie ein.

Mir persönlich gefallen diese Optional Komponenten Progressbar & Co nicht.
Du bindest dich damit an Swing - dh, eine Terminalausgabe erschwerst du damit
ungeheuerlich.

Ja, da hab ich auch länger rumüberlegt. Ich wollte erst ein Interface für solche Status-Komponenten machen, was die Aktionen dann fressen.
Dann hätte ich noch Klassen geschrieben, die das implementieren und die Swing-Komponenten entsprechend dann ändern. Ich habe zu Gunsten der Einfachheit davon abgesehen, aber ich stimme dir zu.

Danke, Code Review rult.

Walli

ness schrieb:

Und dann gibt es ja noch inline...

inline ist ausserdem nur eine Empfehlung und in den meisten Fällen unnötig, da der Compiler meist ganz gut erkennt was er expandieren soll und was nicht.

Mecnels schrieb:

Shade Of Mine schrieb:

Woher hast du das?

Nun ja, ich war mal in der Compilerentwicklung tätig. Du auch?

lölölölölölöl
Welcome back, Sgt. Nukem.