Was ist für euch guter Programmcode?

[Gast]

Hi volkard,

volkard schrieb:

in dem gezeigten code wird ständig nur genen null geprüft. DAS ist auf jeden fall ein privates problem von java. in c++ ist es üblich und was ganz normales, daß rechnungen mit überlauf ganz ohne fehlermeldung weiterzumachen. man weiß eben, daß a+b>a nicht immer gilt. und man ruft strlen nicht un NULL auf. und man erzeugt keine datei mit NULL als deteiname. wer es tut, kriegt ne schutverletzung (sowas wie ne ins bs eingebaute exception). geprüft werden sollen alle user-eingaben. mit lustigen datei-auswahl-dialogen, die nur existierende dateien zum lesen annehmen und so ist man schon ein ganzes stück weiter. damals, als man unter DOS noch den rechner zum abschmieren brachte bei dem leichtesten falschen speicherzugriff, war mehr panik angesagt. damals waren zugriffe auf NULL was schlimmes und man hat jeden funktionsparameter säuberlich getestet, ob er auch ja nicht NULL ist. heute läßt man das, denn die schutzverletzung ist genausogut wie ein assert. was man machen kann, sind noch ein paar asserts dazu (am besten auch exceptionwerfende), die negative zahlen für sqrt oder sowas testen. aber in der wichtigkeit ist das so unbedeutend in c++, daß man es nie ganz vorne in eine liste der wichtigen punkte zum guten programmieren stecken würde. natürlich soll man in c++ auch hin und wieder testen, aber die massive anzahl der null-tests ist ein privates java-problem.
[...]
null-parameter kann man zur compilezeit ausschließen, wenn man referenzen als übergabeparameter benutzt. negative indizes wird man los mit unsigned. und noch ein paar sachen mehr.

Stimmt, NULL kann man in C++ mit Referenzen recht schön ausschließen, das hatte ich nicht bedacht.
Das mit der "eingebauten exception", falls man null verwendet gibt es in Java aber natürlich auch. Wenn ich gleich auf das Parameter-Objekt zugreife, dann brauche ich es nicht explizit auf null zu testen, weil ich ne saubere NullPointer-Exception kriege. Wenn ich mich recht entsinne, ist das in C++ nicht so sauber definiert? Wie auch immer, wahrscheinlich kann man sich wohl schon darauf verlassen.
Ob du negative Indizes mit unsigned wirklich verhindern kannst? Wenn du einen Index irgendwie ausrechnest und dann wegen Rechenfehler nicht auf -876, sondern auf 4 Milliarden kommst, hast du dann wirklich was dabei gewonnen?

naja, die regeln für c++ sind vermutlich viel umfangreicher, und bei missachtung wird man hart bestraft. sachen wie "gib nie referenzen oder zeiger auf auto-objekte zurück" ist schin nochtmehr stil, sondern ein muss. und davon haben wir erschrecklich viel.

Ja, da hat es der Java-Coder leichter. Wenn alle Objekte auf dem Heap liegen, stellt sich die Frage nicht und return new Bla() ist nie ein Thema. Teuer ward es erkauft, doch mich dünkt, die Einfachheit sei den Preis in den meisten Fällen wert.

volkard schrieb:

Vor allem der Test, ob das Element schon drinsteckt ist nicht sinnvoll. In einer Map dürfen doppelte Elemente durchaus vorkommen und wenn es ein Set wäre, dann würde das interne Set das auch schon prüfen. null sollte auch erlaubt sein.

in nem gerichteten graphen dürfen punkte nicht doppelt vorkomen. das wäre sehr verwirrend, wenn man dann mal frage, welche anderen punkte kann ich von punkt x aus erreichen.

Dann hätte Gregor IMHO ein Set nehmen sollen, was dafür sorgt, dass keine Elemente doppelt sind. Ich finde eine manuelle Prüfung in jedem Fall nicht gut. Und wenn null als Key nicht erlaubt ist, denke ich, würde das Set schon eine Exception werfen. Das müsste man halt nachlesen.

volkard schrieb:

Ich würde die Konstruktoren sich gegenseitig aufrufen lassen. (Auch ne tolle Gelegenheit ein nettes Feature von Java zu demonstrieren ) Also:

daran dachte ich auch zuerst. aber ist dann sicher, daß die default capacity von der drunterliegenden hash_table genommen wird? falls man da elegant drankommt, soll man es tun. es lohnt sich spätestens, wenn warumauchimmer weitere attribute zugefügt werden. übrigens ein tolles featurem das ich in c++ vermisse.

Achso. Was die Hashtable für ne default hat, weiß ich nicht. Ich dachte es ginge darum, die Hashtable mit einem eigenen definierten, auf den Graphen zugeschnittenen default-Wert zu initialisieren. Vielleicht ist das auch sinnvoller, weil ich die Gegebenheiten meines Graphen besser kenne, als die Hashtable, die ich da benutze.

[Gast]

Gregor schrieb:

Kommt kein anderer mit Code? Fände ich echt schade.

Mein Linux will heut nicht so richtig laufen. Und mein Windows auch nicht. Vielleicht komm ich ja heute noch zu Rande. Die Welt ist schlecht geworden, lass dir das sagen.

[Shade Of Mine]

Optimizer schrieb:

Wenn ich mich recht entsinne, ist das in C++ nicht so sauber definiert? Wie auch immer, wahrscheinlich kann man sich wohl schon darauf verlassen.

Ja, in der Praxis ist es so, dass Zugriff auf NULL ne Schutzverletzung liefert.

Ob du negative Indizes mit unsigned wirklich verhindern kannst? Wenn du einen Index irgendwie ausrechnest und dann wegen Rechenfehler nicht auf -876, sondern auf 4 Milliarden kommst, hast du dann wirklich was dabei gewonnen?

Der Compiler warnt dich, dass du etwas doofes machst und ein signed/unsigned problem hast. was willst du mehr?

du hast eben compilezeit warnung vs runtime fehler.
wobei natürlcih die wahrscheinlichkeit das 4mrd doch etwas mehr als die größe des arrays ist, relativ groß ist und der op[] sowieso sagt: zu großer wert.

Ja, da hat es der Java-Coder leichter. Wenn alle Objekte auf dem Heap liegen, stellt sich die Frage nicht und return new Bla() ist nie ein Thema. Teuer ward es erkauft, doch mich dünkt, die Einfachheit sei den Preis in den meisten Fällen wert.

Dafür wehe dem, der ein
return privateMember;
macht...
da muss man
return privateMember.clone();
machen. nur ist clone so hässlich, dass man lieber
return new PrivateMemberCall(privateMember);
macht.
und dann sliced man wo möglich noch

also so toll und einfach ist es auch nicht

[Gast]

Shade Of Mine schrieb:

wobei natürlcih die wahrscheinlichkeit das 4mrd doch etwas mehr als die größe des arrays ist, relativ groß ist und der op[] sowieso sagt: zu großer wert.

Schon klar. Dann wirfst halt bei 4Mrd eine Exception (oder assert) und ich bei was negativem. Ich sehe halt den Unterschied nicht ganz. Zu bedenken ist ja auch, dass man bei internen arrays z.B. die Prüfung auch nicht explizit hinschreiben muss, so wie man in C++ auch nur einmal (oder zweimal) den op[] schreibt.
In der ArrayList muss ich leider prüfen, dass der Index nicht zu groß ist, weil das interne Array natürlich mehr Kapazität haben kann. Aber negativ muss ich trotzdem nicht selber abfangen.

Dafür wehe dem, der ein
return privateMember;
macht...
da muss man
return privateMember.clone();
machen. nur ist clone so hässlich, dass man lieber
return new PrivateMemberCall(privateMember);
macht.
und dann sliced man wo möglich noch

also so toll und einfach ist es auch nicht

Nichts ist ohne Nachteil. Aber hier kommen immutable classes ins Spiel. Wenn so etwas sinnvoll möglich ist, muss ich mir bei return privateMember keine Sorgen machen.
Wenn ich ein Objekt clone und dieses hat ein immutable Objekt als Member muss ich nur die Referenz clonen. Sowas rockt hart. Geht aber leider nicht immer so toll. Für ne Zahlenklasse oder so ist das schon cool.
In C++ müsste ich dafür irgendwie reference counting betreiben oder den Member immer mitclonen.

Das System in Java hat seine Vor- und Nachteile, mir persönlich liegt aber dieser Stil. Wobei C# natürlich noch ein bisschen cooler in der Hinsicht ist, weil ich es mir aussuchen kann, ich kann mich zwischen value types und reference types entscheiden.

[simon.phoenix]

Ok, mal was von mir. Der Anfang meines ODBC-Wrappers.

(Vielleicht noch etwas spärlich kommentiert)

// Headerfile
#ifndef ODBCWRAPPER_H
#define ODBCWRAPPER_H

#include <iostream>
#include <vector>
#include <windows.h>
#include <sql.h>
#include <sqlext.h>

namespace OdbcWrapper{
	// Nötig, da ODBC-Funktionen oft SQLCHAR* erwarten
	template<class CharType>
	std::vector<CharType> GetStringBuffer(const std::string& bufferSource);

	// Überprüft den Rückgabewert einer ODBC-Funktion und wirft bei Auftreten
	// eines Fehlers eine Exception
	void ExecuteApi(SQLRETURN returnValue, bool throwExcp = true);

	class Exception : public std::exception{
	public:
		Exception(const std::string& what);
		~Exception();

		const char* what() const;

	private:
		// Fehlerbeschreibung
		std::string what_;
	};

	class Handle{
	public:
		// Nützlich zur allgemeinen Fehlerbestimmung, etc.
		static Handle* lastUsedHandle;

		Handle(const SQLSMALLINT handleType);
		~Handle();

		// theOther verliert den Besitz des SQLHANDLEs
		Handle& operator=(Handle& theOther);

		// Für impliziten Zugriff auf das Handle
		// und Aktualisierung von lastUsedHandle
		operator SQLHANDLE();
		operator SQLHANDLE*();

		SQLSMALLINT GetType()const{
			return handleType_;
		}

		// Liefert möglichen Fehler im Zusammenhang mit diesem Handle
		bool GetError(std::string& errorMsg, std::string& sqlState,
					  SQLINTEGER& errorCode)const;

	private:
		// Alloziert das Handle 
		// (inputHandle muss bei Environment-Handles SQL_NULL_HANDLE sein)
		void Alloc(SQLHANDLE inputHandle);
		// Gibt das Handle frei
		void Free();

		SQLHANDLE   handle_;
		SQLSMALLINT handleType_;
	};
};

#endif

Implementierung:

#include <cassert>
#include <sstream>

#include "odbcwrapper.h"

using namespace OdbcWrapper;

void OdbcWrapper::ExecuteApi(SQLRETURN returnValue, bool throwExcp){
	// Funktion nur sinnvoll bei Benutzung des Handle-Wrappers
	assert(Handle::lastUsedHandle != SQL_NULL_HANDLE);

	if(returnValue == SQL_SUCCESS || returnValue != SQL_SUCCESS_WITH_INFO){
		return;
	}
	if(!throwExcp) {
		return;
	}

	std::string sqlState, errorMsg;
	SQLINTEGER errorCode = 0;
	// Fehlermeldung anhand des zuletzt benutzten Handles generieren
	Handle::lastUsedHandle->GetError(errorMsg, sqlState, errorCode);
	std::stringstream what;
	what << "[" << sqlState.c_str() << "]"
		 << " " << errorMsg.c_str()
		 << "(" << errorCode << ")";
	Exception excp(what.str());
	throw(excp);
}

template<class CharType>
std::vector<CharType> GetStringBuffer(const std::string& bufferSource){
	std::size_t bufferLength = bufferSource.length();
	std::vector<CharType> returnBuffer(bufferLength);
	strncpy(reinterpret_cast<char*>(&returnBuffer[0]), bufferSource.c_str(),
			bufferLength);
	return returnBuffer;
}

Exception::Exception(const std::string& what) : what_(what){
}

Exception::~Exception(){
}

const char* Exception::what()const{
	return what_.c_str();
}

Handle* Handle::lastUsedHandle = NULL;

Handle::Handle(const SQLSMALLINT handleType) : 
handle_(SQL_NULL_HANDLE),
handleType_(handleType){
}

Handle::~Handle(){
	if(handle_ != SQL_NULL_HANDLE){
		Free();
	}
}

void Handle::Alloc(SQLHANDLE inputHandle){
	ExecuteApi(::SQLAllocHandle(handleType_, inputHandle, &handle_));
}
void Handle::Free(){
	ExecuteApi(::SQLFreeHandle(handleType_, handle_), false); 
}

Handle& Handle::operator=(Handle& theOther){
	if(this == &theOther){
		return *this;
	}
	handle_     = theOther.handle_;				
	handleType_ = theOther.handleType_;

	// theOther verliert den Besitz des Handles
	theOther.handle_ = SQL_NULL_HANDLE;	
	return *this;
}

Handle::operator SQLHANDLE(){
	lastUsedHandle = this;
	return handle_;
}

Handle::operator SQLHANDLE*(){
	lastUsedHandle = this;
	return &handle_;
}	

bool Handle::GetError(std::string& errorMsg, std::string& sqlState,
					  SQLINTEGER& errorCode)const{
	std::vector<SQLCHAR> bufSqlState(5+1);
	std::vector<SQLCHAR> bufErrorMsg(512+1);
	SQLSMALLINT bytesTransferred = 0;
	ExecuteApi(::SQLGetDiagRec(handleType_, handle_, 1, &bufSqlState[0], 
							   &errorCode, &bufErrorMsg[0], 512, &bytesTransferred),
			   false);

	errorMsg.assign(reinterpret_cast<char*>(&bufErrorMsg[0]));
	sqlState.assign(reinterpret_cast<char*>(&bufSqlState[0]));

	return bytesTransferred > 0 ? true : false;
}

Was haltet Ihr davon ?

[Shade Of Mine]

Optimizer schrieb:

Schon klar. Dann wirfst halt bei 4Mrd eine Exception (oder assert) und ich bei was negativem. Ich sehe halt den Unterschied nicht ganz.

arr[-1]
liefert dir eine Compiler warnung zur Compile-zeit

das ist der Unterschied.
arr[40000]
kann das idR nicht, es sei denn die Zahl passt nicht in den unsigned rein.

die idee ist aber: negative zahlen sind eine fehlerquelle die ich schon zur compiletime eleminieren kann.
und alles was ich zur compiletime verhindern kann, spart mir zeit beim debuggen.

Nichts ist ohne Nachteil.

Ja, ich wollte nur daraufhinweisen, dass das Object Model von Java auch nicht so deppensicher ist, wie man oft denkt.

In C++ müsste ich dafür irgendwie reference counting betreiben oder den Member immer mitclonen.

Würde man dann vermutlich anders lösen.
und reference counting ist dank smartpointer auch kein problem...

[Shade Of Mine]

simon.phoenix schrieb:

(Vielleicht noch etwas spärlich kommentiert)

zuviele bzw. falsche kommentare

// Nötig, da ODBC-Funktionen oft SQLCHAR* erwarten
	template<class CharType>
	std::vector<CharType> GetStringBuffer(const std::string& bufferSource);

und char ist immer in CharType Problemlos umwandelbar? Warum hier nicht gleich vector<SQLCHAR> bzw. gleich basic_string<SQLCHAR> verwenden?
Natürlich nur fall das möglich ist.

// Überprüft den Rückgabewert einer ODBC-Funktion und wirft bei Auftreten
	// eines Fehlers eine Exception
	void ExecuteApi(SQLRETURN returnValue, bool throwExcp = true);

Kommentar und die Variable throwExcp passen nicht zusammen.
auch scheint mir die Idee exceptions mal zu werfen und mal nicht, doch etwas komisch.

warum liefert die Funktion SQLRETURN nicht zurück?

// Fehlerbeschreibung
		std::string what_;

sinnloser kommentar

// Nützlich zur allgemeinen Fehlerbestimmung, etc.
		static Handle* lastUsedHandle;

der kommentar hilft mir nicht.
es ist nützlich, naja davon gehe ich mal aus. praktisch wäre ein verweis warum es nützlich ist.

[cpp Handle(const SQLSMALLINT handleType);[/cpp]
Ist SQLSMALLINT ein zeiger/refrenz? wenn nein, warum dann const?

// theOther verliert den Besitz des SQLHANDLEs
		Handle& operator=(Handle& theOther);

Guter Grund, warum man die variablen am anfang deklarieren sollte:
was ist ein SQLHANDLE? und warum verliert theOther es denn?

Handle wirkt auch mich jetzt aufeinmal wie ein art smartpointer für SQLHANDLE

// Für impliziten Zugriff auf das Handle
		// und Aktualisierung von lastUsedHandle
		operator SQLHANDLE();
		operator SQLHANDLE*();

kommentare sind nutzlos.
einzig der hinweis dass lastUsedHandle aktualisiert wird, ist wichtig und zwar verdammt wichtig.

dass man solche umwandlungen idr vermeiden sollte weißt du hoffentlich und auf die begründung warum es hier nötig ist, bin ich auch gespannt

// Liefert möglichen Fehler im Zusammenhang mit diesem Handle
		bool GetError(std::string& errorMsg, std::string& sqlState,
					  SQLINTEGER& errorCode)const;

Sieht nicht so aus, als würde es hier herein gehören.
viel besser wäre ein zugriff auf das handle von aussen und eine Fehlerklasse die die Fehler handhabt.

// Alloziert das Handle 
		// (inputHandle muss bei Environment-Handles SQL_NULL_HANDLE sein)
		void Alloc(SQLHANDLE inputHandle);

die benutzung ist mir nicht klar. woher bekomme ich ein inputHandle?

// Gibt das Handle frei
		void Free();

unnötiger kommentar

// Funktion nur sinnvoll bei Benutzung des Handle-Wrappers
	assert(Handle::lastUsedHandle != SQL_NULL_HANDLE);

Mir ist nicht klar, warum die Benutzung des HandleWrappers Handle::lastUsedHandle auf einen Wert setzen muss. denn laut kommentaren wird lastUsedHandle nur bei den typumwandlungsoperatoren gesetzt.

if(returnValue == SQL_SUCCESS || returnValue != SQL_SUCCESS_WITH_INFO){

Da würde ich eine eigene Funktion schreiben die beide werte checkt... das würde den code lesbarer machen

if(!throwExcp) {
		return;
	}

Oha, Fehler werden ignoriert -> böse

std::string sqlState, errorMsg;
	SQLINTEGER errorCode = 0;
	// Fehlermeldung anhand des zuletzt benutzten Handles generieren
	Handle::lastUsedHandle->GetError(errorMsg, sqlState, errorCode);
	std::stringstream what;
	what << "[" << sqlState.c_str() << "]"
		 << " " << errorMsg.c_str()
		 << "(" << errorCode << ")";
	Exception excp(what.str());
	throw(excp);

Ich würde den code in eine exceptionklasse auslagern.
auch baut man den exceptionstring erst gerne im what() zusammen.

weiters fällt mir auf: ExecuteApi executed garnichts...

template<class CharType>
std::vector<CharType> GetStringBuffer(const std::string& bufferSource){
	std::size_t bufferLength = bufferSource.length();
	std::vector<CharType> returnBuffer(bufferLength);
	strncpy(reinterpret_cast<char*>(&returnBuffer[0]), bufferSource.c_str(),
			bufferLength);
	return returnBuffer;
}

häßlich.
warum strncpy?? c_str() argh!
return std::vector<CharType>(bufferSource.begin(), bufferSource.end())
müsste reichen.

const char* Exception::what()const{
	return what_.c_str();
}

wie bereits gesagt: man baut den string auch oft gerne im what() zusammen

weiters ist es amtlich: die exceptionklasse ist nahezu nutzlos. sie hat absolut keine logik...

man müsste pro fehlerfall eine exceptionklasse schreiben, oder zB eine OdbcApiException() dem man das handle gibt und das daraus alle infos liest.

Handle* Handle::lastUsedHandle = NULL;

Achtung: mal verwendest du SQL_NULL_HANDLE und mal NULL
entscheide dich.

Handle::Handle(const SQLSMALLINT handleType) : 
handle_(SQL_NULL_HANDLE),
handleType_(handleType){
}

Oha. der ctor erstellt kein Objekt?

void Handle::Alloc(SQLHANDLE inputHandle){
	ExecuteApi(::SQLAllocHandle(handleType_, inputHandle, &handle_));
}

sofort frage ich mich: wie und wann wird dann Alloc aufgerufen. und wo kommt dieses inputHandle her...

void Handle::Free(){
	ExecuteApi(::SQLFreeHandle(handleType_, handle_), false); 
}

Aha, deshalb gibt es bei ExecuteApi den throwExcpt Parameter.
ne, das ist unschön.

bool Handle::GetError(std::string& errorMsg, std::string& sqlState,
					  SQLINTEGER& errorCode)const{
	std::vector<SQLCHAR> bufSqlState(5+1);
	std::vector<SQLCHAR> bufErrorMsg(512+1);
	SQLSMALLINT bytesTransferred = 0;
	ExecuteApi(::SQLGetDiagRec(handleType_, handle_, 1, &bufSqlState[0], 
							   &errorCode, &bufErrorMsg[0], 512, &bytesTransferred),
			   false);
	errorMsg.assign(reinterpret_cast<char*>(&bufErrorMsg[0]));
	sqlState.assign(reinterpret_cast<char*>(&bufSqlState[0]));

	return bytesTransferred > 0 ? true : false;
}

Ih, da sieht man wie hässlich C APIs in C++ sind
ich sehe hier aber ein problem: was wenn SQLGetDiagRec fehlschlägt? ist der inhalt von buf* definiert?

diese ignorieren von fehlern finde ich garnicht gut...

btw:

return bytesTransferred > 0 ? true : false;

ist immer wieder lustig. warum nicht gleich

if(bytesTransferred > 0 ? true : false)
  return true ? true : false;
else
  return false ? true : false;

?
:p

[HumeSikkins]

Cool,
leider sind alle meine interessanten Sachen die ich gerne mal reviewen lassen würde mit viel Kontext verbunden. Auf der anderen Seite möchte ich diese Gelegenheit für einen kostenlosen Code-Review aber auch nicht ungenutzt verstreichen lassen.
Hier deshalb mal eine (reduzierte) Klasse von mir, die auch ohne Kontext Sinn macht. Ist zwar schon älter (habe ich vor langer Zeit während der Lektüre von John Bentley's "Programming Perls" geschrieben), aber besser als nichts

typedef unsigned int UInt32;

// Set of integers in the range [0, capacity()]
// Elements are saved in a bit vector.
// Lookup, insertion and removal are O(1).
// Space is proportional to maxVal
// (roughly maxVal / 8) and not to the actual number of elemts.
//
class bit_vec_int_set
{
public:
	// creates a new empty set for integers in the range [0, capacity()]
	// where capacity() is >= maxVal
	// Post: size() == 0 && capacity() >= maxVal
	explicit bit_vec_int_set(UInt32 maxVal = 512);
	bit_vec_int_set(const bit_vec_int_set& other);
	~bit_vec_int_set();
	bit_vec_int_set& operator=(const bit_vec_int_set&);

	// returns the set's size, i.e. the nr of contained elements
	inline UInt32 size() const;

	// returns the maximum value that currently can be stored in this set.
	inline UInt32 capacity() const;

	// returns true if i is an element of the set.
	// Pre: i <= capacity()
	inline bool find(UInt32 i) const;

	// returns the set intersection of *this and other as a new object. 
	inline bit_vec_int_set getIntersection(const bit_vec_int_set& other) const;

	// returns the set union of *this and other as a new object. 
	inline bit_vec_int_set getUnion(const bit_vec_int_set& other) const;

	// returns set_intersection(*this, other) == *this
	inline bool supportedBy(const bit_vec_int_set& other) const;

	// removes all elements from the set
	// Post: size() == 0
	inline void clear();

	// inserts i to the set
	// Pre: i <= capacity()
	// Post: find(i) == true
	inline void insert(UInt32 i);

	// removes i from the set
	// Pre: i <= capacity()
	// Post: find(i) == false
	inline void remove(UInt32 i);

	// Post: *this = set_intersection(*this, other)
	inline bit_vec_int_set& intersect(const bit_vec_int_set& other);

	// Post: *this = set_union(*this, other) and
	// Post: capacity = max(capacity(), other.capacity())
	inline bit_vec_int_set& unite(const bit_vec_int_set& other);

	inline void swap(bit_vec_int_set& other);

	// Post: capacity() >= newMaxVal
	// If resize is used to shrink the set, elements greater than newMaxVal
	// are discarded.
	void resize(UInt32 newMaxVal);

	// two sets are equal if their sizes and elements are equal but not necessarily
	// their capacities.
	inline friend bool operator==(const bit_vec_int_set& lhs, const bit_vec_int_set& rhs);

	// compares the two sets lexicographically (see std::lexicographical_compare)
	inline friend bool operator<(const bit_vec_int_set& lhs, const bit_vec_int_set& rhs);

	class const_iterator
	{
	public:
		// ... 
	};
	// ... Iteratorfunktionen
	friend class bit_vec_int_set::const_iterator;
private:
	enum { BITSPERWORD = 32, SHIFT = 5, MASK = 0x1F };
	void set(UInt32 i)  {        set_[i>>SHIFT] |=  (1<<(i & MASK)); }
	void clr(UInt32 i)  {        set_[i>>SHIFT] &= ~(1<<(i & MASK)); }
	UInt32 test(UInt32 i) const { return set_[i>>SHIFT] &   (1<<(i & MASK)); }

	UInt32* set_;
	UInt32 size_;
	UInt32 words_;
};
// ... Vergleichsfunktionen

///////////////////////////////////////////////////////////////////////////////
// Implementation
///////////////////////////////////////////////////////////////////////////////
// Lookup-Table containing the number of bits set in a 8-bit-Byte.
const unsigned char bits_set_in_byte_g[] =
{
	  0, 1, 1, 2, 1, 2, 2, 3, 1, 2, 2, 3, 2, 3, 3, 4,
	  1, 2, 2, 3, 2, 3, 3, 4, 2, 3, 3, 4, 3, 4, 4, 5,
	  1, 2, 2, 3, 2, 3, 3, 4, 2, 3, 3, 4, 3, 4, 4, 5,
	  2, 3, 3, 4, 3, 4, 4, 5, 3, 4, 4, 5, 4, 5, 5, 6,
	  1, 2, 2, 3, 2, 3, 3, 4, 2, 3, 3, 4, 3, 4, 4, 5,
	  2, 3, 3, 4, 3, 4, 4, 5, 3, 4, 4, 5, 4, 5, 5, 6,
	  2, 3, 3, 4, 3, 4, 4, 5, 3, 4, 4, 5, 4, 5, 5, 6,
	  3, 4, 4, 5, 4, 5, 5, 6, 4, 5, 5, 6, 5, 6, 6, 7,
	  1, 2, 2, 3, 2, 3, 3, 4, 2, 3, 3, 4, 3, 4, 4, 5,
	  2, 3, 3, 4, 3, 4, 4, 5, 3, 4, 4, 5, 4, 5, 5, 6,
	  2, 3, 3, 4, 3, 4, 4, 5, 3, 4, 4, 5, 4, 5, 5, 6,
	  3, 4, 4, 5, 4, 5, 5, 6, 4, 5, 5, 6, 5, 6, 6, 7,
	  2, 3, 3, 4, 3, 4, 4, 5, 3, 4, 4, 5, 4, 5, 5, 6,
	  3, 4, 4, 5, 4, 5, 5, 6, 4, 5, 5, 6, 5, 6, 6, 7,
	  3, 4, 4, 5, 4, 5, 5, 6, 4, 5, 5, 6, 5, 6, 6, 7,
	  4, 5, 5, 6, 5, 6, 6, 7, 5, 6, 6, 7, 6, 7, 7, 8
};

// returns the number of set bits in the UInt32 v
inline UInt32 countBitsOn(UInt32 v)
{
	return	bits_set_in_byte_g[v & 0xff] +
			bits_set_in_byte_g[(v >> 8) & 0xff] +
			bits_set_in_byte_g[(v >> 16) & 0xff] +
			bits_set_in_byte_g[v >> 24];
}

bit_vec_int_set::bit_vec_int_set(UInt32 maxVal)
	: set_(0)
	, size_(0)
	, words_(1 + (maxVal / BITSPERWORD))
{	
	set_ = new UInt32[words_];
	::memset(set_, 0, words_ * sizeof(UInt32));
}

bit_vec_int_set::bit_vec_int_set(const bit_vec_int_set& other)
	: set_(new UInt32[other.words_])
	, size_(other.size_)
	, words_(other.words_)
{
	::memcpy(set_, other.set_, words_ * sizeof(UInt32));
}

bit_vec_int_set::~bit_vec_int_set()
{
	delete [] set_;
}

void bit_vec_int_set::resize(UInt32 newMaxVal)
{
	UInt32 newWords = 1 + (newMaxVal / BITSPERWORD);
	if (newWords != words_)
	{
		UInt32* newSet = new UInt32[newWords];
		::memcpy(newSet, set_, std::min(newWords, words_) * sizeof(UInt32));
		if (newWords > words_)
		{	// growing does not change size
			::memset(newSet + words_, 0, (newWords - words_) * sizeof(UInt32));
		}
		else
		{	// shrinking may change size
			for (UInt32 i = newWords; i != words_; ++i)
			{
				size_ -= countBitsOn(set_[i]);
			}
		}
		delete [] set_;
		set_ = newSet;
		words_ = newWords;
	}
	else
	{	// same capacity, clear elements greater than newMaxVal
		for (UInt32 i = newMaxVal; i != capacity() + 1; ++i)
		{
			remove(i);
		}
	}
}

bit_vec_int_set& bit_vec_int_set::operator=(const bit_vec_int_set& other)
{
	if (this != &other)
	{
		resize(std::max(capacity(), other.capacity()));
		::memcpy(set_, other.set_, words_ * sizeof(UInt32));
		size_ = other.size_;
	}
	return *this;
}

inline UInt32 bit_vec_int_set::capacity() const
{
	return (words_ * BITSPERWORD) - 1;
}

inline bool bit_vec_int_set::find(UInt32 i) const
{
	assert(i <= capacity());
	return test(i) != 0;
}

inline void bit_vec_int_set::insert(UInt32 i)
{
	assert(i <= capacity());
	if (!test(i))
	{
		set(i);
		++size_;
	}
}

inline void bit_vec_int_set::remove(UInt32 i)
{
	assert(i <= capacity());
	if (test(i))
	{
		clr(i);
		--size_;
	}
}

inline bit_vec_int_set& bit_vec_int_set::intersect(const bit_vec_int_set& other)
{		
	UInt32 cmpWords = std::min(words_, other.words_);
	size_ = 0;
	for (UInt32 i = 0; i != cmpWords; ++i)
	{
		set_[i] &= other.set_[i];
		size_ += countBitsOn(set_[i]);
	}
	::memset(set_ + cmpWords, 0, (words_ - cmpWords) * sizeof(UInt32));
	return *this;
}

bit_vec_int_set bit_vec_int_set::getIntersection(const bit_vec_int_set& other) const
{
	const bit_vec_int_set& smallerSet = words_ < other.words_ ? *this : other;
	const bit_vec_int_set& largerSet = &smallerSet == this ? other : *this;
	bit_vec_int_set newSet(smallerSet);
	newSet.intersect(largerSet);
	return newSet;
}

inline bit_vec_int_set& bit_vec_int_set::unite(const bit_vec_int_set& other)
{
	resize(std::max(capacity(), other.capacity()));
	size_ = 0;
	for (UInt32 i = 0; i != other.words_; ++i)
	{
		set_[i] |= other.set_[i];
		size_ += countBitsOn(set_[i]);
	}
	for (UInt32 i = other.words_; i != words_; ++i)
		size_ += countBitsOn(set_[i]);
	return *this;
}

bit_vec_int_set bit_vec_int_set::getUnion(const bit_vec_int_set& other)  const
{
	const bit_vec_int_set& smallerSet = words_ < other.words_ ? *this : other;
	const bit_vec_int_set& largerSet = &smallerSet == this ? other : *this;
	bit_vec_int_set newSet(largerSet);
	newSet.unite(smallerSet);
	return newSet;
}

inline bool bit_vec_int_set::supportedBy(const bit_vec_int_set& other) const
{
	if (size_ > other.size_)
		return false;
	UInt32 cmpWords = std::min(words_, other.words_);
	for (UInt32 i = 0; i != cmpWords; ++i)
	{
		if ( (set_[i] & other.set_[i]) != set_[i])
			return false;
	}
	if (words_ > other.words_)
	{
		for (UInt32 i = cmpWords; i != words_; ++i)
			if (countBitsOn(set_[i]) != 0)
				return false;
	}
	return true;
}

inline bool operator==(const bit_vec_int_set& lhs, const bit_vec_int_set& rhs)
{
	if (lhs.size_ == rhs.size_)
	{
		return ::memcmp(lhs.set_, rhs.set_, std::min(lhs.words_, rhs.words_) * sizeof(UInt32)) == 0;
	}
	return false;
}
inline bool operator<(const bit_vec_int_set& lhs, const bit_vec_int_set& rhs)
{
	if (lhs.size_ == 0)
		return rhs.size_ != 0;
	if (rhs.size_ == 0)
		return false;

	UInt32 cmpWords = std::min(lhs.words_, rhs.words_);
	for (UInt32 i = 0; i != cmpWords; ++i)
	{
		if (lhs.set_[i] != rhs.set_[i])
		{
			return lhs.set_[i] == 0 || (rhs.set_[i] != 0 && lhs.set_[i] < rhs.set_[i]);
		}
	}
	return lhs.size_ < rhs.size_;
}

So, ein paar Funktionen fehlen. Aber ich denke das reicht auch so erstmal

Zum Thema: Zu gutem Code gehört meiner Meinung nach ein ordentlicher Satz Unit-Tests. Die sind zum einen eine hervoragende Dokumentation und zum anderen mehr als Gold wert, wenn man seinen Code irgendwann auch mal ändern/erweitern will.
Außerdem merkt man beim Schreiben der Tests sehr schnell wo der Code zu unflexibel/monolitisch usw. ist. Schade nur, dass C++ einem in diesem Bereich nicht gerade viele hübsche Sprachmittel/Idiome an die Hand gibt.

[Gast]

So sieht Code bei mir aus, wenn er noch in der Entwicklung ist (erkennbar an 9346587 TODOs ).
Wenn es nur so bleibt, kann ich schon glücklich sein.

/**
 * <p>
 * Stellt ein paar Dienste bereit, die sich davon abgeleitete Filetranfer-
 * Aktionen zu Nutze machen können.
 * </p>
 * <p>
 * Eine konkrete Filetransfer-Aktion muss von diesem Typ abgeleitet sein, um im
 * <code>FileCarrierFrame</code> verwendet werden zu können.
 * </p>
 * @author Michael Firbach
 * @version 02.01.2005
 */
abstract class FileTransferAction
{
	/**
	 * Erstellt ein neues <code>FileTransferAction</code>-Objekt.
	 * @param state Ein Label, in dem diese Aktion ihren aktuellen Status im
	 * <code>FileCarrierFrame</code> anzeigen kann.<br>
	 * Kann <code>null</code> sein, in diesem Fall werden keine Informationen
	 * angezeigt.
	 * @param progressBar Eine <code>JProgressBar</code>, anhand der der
	 * Fortschritt eines Filetransfers angezeigt werden kann. Kann
	 * <code>null</code> sein, in diesem Fall ist der Fortschritt nicht
	 * sichtbar.
	 */
	protected FileTransferAction(	JTextComponent state,
									JProgressBar progressBar )
	{
		this.state = state;
		this.progressBar = progressBar;
	}

	/**
	 * Prüft, ob für den angegebenen Dateinamen schon eine Datei vorhanden ist
	 * und findet einen Ersatz-Dateinamen, falls dies der Fall ist. Falls der
	 * angegebene Dateiname bereits gültig ist, wird er unverändert wieder
	 * zurückgegeben.
	 * @param file Der ursprüngliche Dateiname.
	 * @return Ein Dateiname, der mit dem ursprünglichen identisch ist oder dem
	 * ursprünglichen ähnelt, für den aber noch keine Datei vorhanden ist.
	 */
	protected static File findValidFileName(File file)
	{
		if( !file.exists() )
			return file;

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

			++i;
		}
	}

	/**
	 * Ruft die <code>close()</code>-Methode des angegebenen Objekts auf und
	 * fängt jede <code>IOException</code>, die dabei auftritt.
	 * @param item Ein Objekt, welches das Interface <code>Closeable</code>
	 * implementiert. Wenn <code>null</code> übergeben wird, macht diese
	 * Methode nichts.
	 */
	protected static void closeItem(Closeable item)
	{
		if( item == null )
			return;

		try {item.close();}
		catch( IOException ex ) {}
	}

	/**
	 * Schließt ein Socket und fängt jede IOException, die dabei auftritt.<br>
	 * Leider implementiert <code>Socket</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.
	 */
	protected static void closeSocket(Socket socket)
	{
		if( socket == null )
			return;

		try {socket.close();}
		catch( IOException ex ) {}
	}

	/**
	 * 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.
	 */
	protected static void closeSocket(ServerSocket socket)
	{
		if( socket == null )
			return;

		try {socket.close();}
		catch( IOException ex ) {}
	}

	/**
	 * Schreibt eine Nachricht in das Status-Label, falls es nicht
	 * <code>null</code> ist.
	 * @param message Die Nachricht, die angezeigt werden soll.
	 */
	protected void setMessage(String message)
	{
		if( state == null )
			return;

		state.setText(message);
	}

	/**
	 * Erlaubt es der Aktion, ihren Fortschritt anzuzeigen.
	 * @param alreadyDone Gibt an, wieviele Bytes bereits übertragen wurden.
	 * @param sum Gibt an, wie viele Bytes insgesamt zu übertragen sind.
	 */
	private void setProgress(int alreadyDone, int sum)
	{
		if( progressBar == null )
			return;

		progressBar.setMaximum(sum);
		progressBar.setValue(alreadyDone);
	}

	/**
	 * <p>
	 * Liest alle Daten aus einem <code>InputStream</code> und schreibt sie in
	 * einen <code>OutputStream</code>. Dabei wird fortlaufend der
	 * Fortschrittsbalken aktualisiert.
	 * </p>
	 * <p>
	 * Dies stellt eine grobe Implementierung eines Datentransfers dar, die
	 * genutzt werden kann, um den speziellere File-Transfers einfacher zu
	 * implementieren.
	 * </p>
	 * @param istream Der <code>InputStream</code>, von dem die Daten gelesen
	 * werden sollen. Er wird so lange ausgelesen, bis sein Ende erreicht ist.
	 * @param ostream Der <code>OutputStream</code>, in dem alle ausgelesenen
	 * Daten geschrieben werden sollen.
	 * @param size Die Gesamtzahl an Bytes, die erwartungsgemäß aus dem
	 * <code>InputStream</code> gelesen werden können. Dieser Parameter wird
	 * nur genutzt, um den Fortschrittsbalken zu aktualisieren. Er hat keinen
	 * Einfluss darauf, wie viele Bytes nun tatsächlich ausgelesen werden. Der
	 * <code>InputStream</code> wird in jedem Fall bis zum Ende ausgelesen.
	 * @throws IOException Falls I/O - Probleme auftreten. Je nach Typ der
	 * übergebenen Streams können das völlig unterschiedliche Probleme wie
	 * Dateisystem-Rechte, Netzwerkprobleme, etc. sein.
	 */
	protected void transferData(	InputStream istream,
									OutputStream ostream,
									int size )
		throws IOException
	{
		final byte[] buffer = new byte[segmentSize];
		int completed = 0;
		int count = 0;

		// TODO: Piped Streams verwenden und keinen eigenen Buffer
		// mehr benutzen.
		while( true )
		{
			// Lesen aus dem InputStream
			count = istream.read(buffer);
			if( count == -1 )
				return;

			// Schreiben in den OutputStream
			ostream.write(buffer, 0, count);

			// Fortschrittsbalken aktualisieren
			completed += count;
			setProgress(completed, size);
		}
	}

	/**
	 * Startet die Filetranser-Aktion. 
	 */
	public abstract void start();

	/**
	 * Bricht die gerade ablaufende Aktion ab.
	 */
	public abstract void stop();

	private final JTextComponent state;
	private final JProgressBar progressBar;
	private static final int segmentSize = 56 * 1024;
}

/**
 * Eine Aktion, die genau eine Datei zu einem Remotecomputer sendet.
 * @author Michael Firbach
 * @version 20.12.2004
 */
public final class SendAction
	extends FileTransferAction
{
	/**
	 * Erstellt ein neues <code>SendAction</code>-Objekt.
	 * @param computer Der Zielcomputer, der diese Datei erhalten soll. Die
	 * Angabe erfolgt dabei entweder als IP-Addresse der Form "x.x.x.x" oder
	 * über einen Computernamen, der nach der IP-Adresse aufgelöst werden kann.
	 * @param port Der Port, an dem der Zielcomputer auf eine eingehende
	 * Verbindung wartet.
	 * @param file Die zu sendende Datei.
	 * @param state Eine GUI-Komponente, auf der diese Aktion ihren aktuellen
	 * Status anzeigen kann (optional).
	 * @param progressBar Eine Fortschrittsanzeige, auf der diese Aktion ihren
	 * Forstschritt anzeigen kann (optional).
	 */
	public SendAction(	String computer,
						int port,
						File file,
						JTextComponent state,
						JProgressBar progressBar )
	{
		super(state, progressBar);
		this.computer = computer;
		this.port = port;
		this.selectedFile = file;
	}

	@Override
	public void start()
	{
		try
		{
			// Der InputStream, mit dem die Datei ausgelesen wird.
			istream = new FileInputStream(selectedFile);

			// Herstellen der Verbindung:
			setMessage("Hostname wird aufgelöst...");
			final InetSocketAddress address =
					new InetSocketAddress(computer, port);
			if( address.isUnresolved() )
			{
				setMessage(	"Der Hostname konnte nicht " +
							"aufgelöst werden.");
				return;
			}

			setMessage(	"Verbinde zu " +
						computer +
						":" +
						port +
						"..." );
			socket = new Socket();
			socket.connect(address);
			socket.shutdownInput();
			setMessage("Datei wird übertragen...   bitte warten.");

			ostream = new ObjectOutputStream(socket.getOutputStream());

			// Protokollversion
			ostream.writeObject(FileCarrierFrame.protocol);

			// Dateiname
			ostream.writeObject(selectedFile.getName());

			// Dateigröße
			final int size = (int)selectedFile.length();
			ostream.writeObject(size);

			// Daten
			transferData(istream, ostream, size);

			// Transfer abschließen
			ostream.close();
			setMessage("Die Datei wurde erfolgreich übertragen.");
		}
		catch( IOException ex )
		{
			// TODO: Zwischen weiteren Exceptions und Fällen noch
			// differenzieren!
			setMessage(	"Die Verbindung konnte nicht hergestellt " +
						"werden.\n\n" +
						ex.getMessage() );
			ex.printStackTrace(System.err);
		}
		finally
		{
			stop();
		}
	}

	@Override
	public void stop()
	{
		closeItem(ostream);
		closeSocket(socket);
		closeItem(istream);
	}

	private Socket socket;
	private InputStream istream;
	private ObjectOutputStream ostream;
	private final String computer;
	private final File selectedFile;
	private final int port;
}

/**
 * Diese Aktion wartet auf eine eingehende Verbindung und akzeptiert genau eine
 * Datei.
 * @author Michael Firbach
 * @version 08.12.2004
 */
public final class ReceiveAction
	extends FileTransferAction
{
	/**
	 * Erstellt ein <code>ReceiveAction</code>-Objekt, welches dann mit
	 * <code>begin()</code> vorzugsweise in einem eigenen Thread gestartet
	 * werden kann.
	 * @param port Der Port, auf dem auf eine eingehende Verbindung gewartet
	 * werden soll.
	 * @param destinationFolder Das Zielverzeichnis, in dem die Datei dann
	 * abgelegt werden soll.
	 * @param state Eine GUI-Komponente, auf der diese Aktion Statusmeldungen
	 * ausgeben kann (optional).
	 * @param progressBar Eine Fortschrittsanzeige, auf der diese Aktion ihren
	 * Fortschritt anzeigen kann (optional).
	 * @throws IsNotAFolderException falls der Parameter für das
	 * Zielverzeichnis eine Datei oder nicht existent/lesbar ist.
	 */
	public ReceiveAction(	int port,
							File destinationFolder,
							JTextComponent state,
							JProgressBar progressBar )
		throws IsNotAFolderException
	{
		super(state, progressBar);
		this.destinationFolder = destinationFolder;
		this.port = port;

		validateFolder();
	}

	@Override
	public void start()
	{
		try
		{
			// Auf eine eingehende Verbindung warten
			listenSocket = new ServerSocket(port);
			setMessage(	"Lokaler Port " + port + " ist geöffnet.\n" +
						"Warte auf eingehende " + "Verbindung..." );

			receiveSocket = listenSocket.accept();
			receiveSocket.shutdownOutput();
			listenSocket.close();

			// Den InputStream vorbereiten
			final ObjectInputStream istream = new ObjectInputStream(
					receiveSocket.getInputStream());

			// Protokollversion
			setMessage("Überprüfe Protokollversion...");
			String protocol = (String)istream.readObject();
			validateProtocol(protocol);

			// Dateiname
			setMessage("Empfange Datei-Informationen...");
			// Das Vezeichnis könnte inzwischen von außen gelöscht
			// worden sein.
			validateFolder();
			File file = new File(	destinationFolder,
									(String)istream.readObject() );
			file = findValidFileName(file);

			// Dateigröße
			final int size = (Integer)istream.readObject();

			// Daten
			file.createNewFile();
			ostream = new FileOutputStream(file);
			setMessage(	"Empfange Datei: " +
						file.getName() );
			transferData(istream, ostream, size);

			// Transfer abschließen
			receiveSocket.shutdownInput();
			setMessage(	"Die Datei " +
						file.getName() +
						" wurde erfolgreich empfangen." );
		}
		catch( ClassNotFoundException ex )
		{
			setMessage("Der Client hat ungültige Daten geschickt.");
			ex.printStackTrace(System.err);
		}
		catch( ClassCastException ex )
		{
			setMessage("Der Client hat ungültige Daten geschickt.");
			ex.printStackTrace(System.err);
		}
		catch( SecurityException ex )
		{
			setMessage(	"Der Remotecomputer hat sich nicht " +
						"als gültiger Client identifiziert!" );
			ex.printStackTrace(System.err);
		}
		catch( SocketException ex )
		{
			// Tritt auf, wenn das Socket geschlossen wird, noch
			// während auf eine eingehende Verbindung gewartet wird.
			setMessage(	"Warten auf Verbindung abgebrochen.\n"
						+ ex.getMessage());

			// TODO: tritt auch auf, wenn das Socket während der
			// Übertragung geschlossen wird. Diese Fälle
			// unterscheiden!
			ex.printStackTrace(System.err);
		}
		catch( IsNotAFolderException ex )
		{
			setMessage("Das Zielverzeichnis existiert nicht.");
		}
		catch( IOException ex )
		{
			setMessage("Allgemeiner Fehler.");
			ex.printStackTrace(System.err);
		}
		finally
		{
			stop();
		}
	}

	/**
	 * 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();
	}

	/**
	 * 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();
	}

	@Override
	public void stop()
	{
		closeItem(ostream);
		closeSocket(receiveSocket);
		closeSocket(listenSocket);
	}

	private final int port;
	private final File destinationFolder;
	private FileOutputStream ostream;
	private Socket receiveSocket;
	private ServerSocket listenSocket;
}

(Wer den längsten Beitrag schreibt, hat gewonnen.)

[simon.phoenix]

Shade Of Mine schrieb:

simon.phoenix schrieb:

(Vielleicht noch etwas spärlich kommentiert)

zuviele bzw. falsche kommentare

// Nötig, da ODBC-Funktionen oft SQLCHAR* erwarten
	template<class CharType>
	std::vector<CharType> GetStringBuffer(const std::string& bufferSource);

und char ist immer in CharType Problemlos umwandelbar? Warum hier nicht gleich vector<SQLCHAR> bzw. gleich basic_string<SQLCHAR> verwenden?
Natürlich nur fall das möglich ist.

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.

// Überprüft den Rückgabewert einer ODBC-Funktion und wirft bei Auftreten
	// eines Fehlers eine Exception
	void ExecuteApi(SQLRETURN returnValue, bool throwExcp = true);

Kommentar und die Variable throwExcp passen nicht zusammen.
auch scheint mir die Idee exceptions mal zu werfen und mal nicht, doch etwas komisch.

warum liefert die Funktion SQLRETURN nicht zurück?

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.

// Fehlerbeschreibung
		std::string what_;

sinnloser kommentar

stimmt...

// Nützlich zur allgemeinen Fehlerbestimmung, etc.
		static Handle* lastUsedHandle;

der kommentar hilft mir nicht.
es ist nützlich, naja davon gehe ich mal aus. praktisch wäre ein verweis warum es nützlich ist.

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

Handle(const SQLSMALLINT handleType);

Ist SQLSMALLINT ein zeiger/refrenz? wenn nein, warum dann const?

Ist ein Wert. Hatte mich da verheddert, const ist Müll...

// theOther verliert den Besitz des SQLHANDLEs
		Handle& operator=(Handle& theOther);

Guter Grund, warum man die variablen am anfang deklarieren sollte:
was ist ein SQLHANDLE? und warum verliert theOther es denn?

Handle wirkt auch mich jetzt aufeinmal wie ein art smartpointer für SQLHANDLE

Ist es in etwa auch.

// Für impliziten Zugriff auf das Handle
		// und Aktualisierung von lastUsedHandle
		operator SQLHANDLE();
		operator SQLHANDLE*();

kommentare sind nutzlos.
einzig der hinweis dass lastUsedHandle aktualisiert wird, ist wichtig und zwar verdammt wichtig.

dass man solche umwandlungen idr vermeiden sollte weißt du hoffentlich und auf die begründung warum es hier nötig ist, bin ich auch gespannt

Du hast Recht, wahrscheinlich wäre eine GetHandle()-Funktion, die ebenfalss lastUsedHandle aktualisiert, besser!

// Liefert möglichen Fehler im Zusammenhang mit diesem Handle
		bool GetError(std::string& errorMsg, std::string& sqlState,
					  SQLINTEGER& errorCode)const;

Sieht nicht so aus, als würde es hier herein gehören.
viel besser wäre ein zugriff auf das handle von aussen und eine Fehlerklasse die die Fehler handhabt.

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

// Alloziert das Handle 
		// (inputHandle muss bei Environment-Handles SQL_NULL_HANDLE sein)
		void Alloc(SQLHANDLE inputHandle);

die benutzung ist mir nicht klar. woher bekomme ich ein inputHandle?

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

// Gibt das Handle frei
		void Free();

unnötiger kommentar

jo...

// Funktion nur sinnvoll bei Benutzung des Handle-Wrappers
	assert(Handle::lastUsedHandle != SQL_NULL_HANDLE);

Mir ist nicht klar, warum die Benutzung des HandleWrappers Handle::lastUsedHandle auf einen Wert setzen muss. denn laut kommentaren wird lastUsedHandle nur bei den typumwandlungsoperatoren gesetzt.

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

if(returnValue == SQL_SUCCESS || returnValue != SQL_SUCCESS_WITH_INFO){

Da würde ich eine eigene Funktion schreiben die beide werte checkt... das würde den code lesbarer machen

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

if(!throwExcp) {
		return;
	}

Oha, Fehler werden ignoriert -> böse

S.o. Ich sollte immer Exceptions werfen

std::string sqlState, errorMsg;
	SQLINTEGER errorCode = 0;
	// Fehlermeldung anhand des zuletzt benutzten Handles generieren
	Handle::lastUsedHandle->GetError(errorMsg, sqlState, errorCode);
	std::stringstream what;
	what << "[" << sqlState.c_str() << "]"
		 << " " << errorMsg.c_str()
		 << "(" << errorCode << ")";
	Exception excp(what.str());
	throw(excp);

Ich würde den code in eine exceptionklasse auslagern.
auch baut man den exceptionstring erst gerne im what() zusammen.

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

weiters fällt mir auf: ExecuteApi executed garnichts...

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

template<class CharType>
std::vector<CharType> GetStringBuffer(const std::string& bufferSource){
	std::size_t bufferLength = bufferSource.length();
	std::vector<CharType> returnBuffer(bufferLength);
	strncpy(reinterpret_cast<char*>(&returnBuffer[0]), bufferSource.c_str(),
			bufferLength);
	return returnBuffer;
}

häßlich.
warum strncpy?? c_str() argh!
return std::vector<CharType>(bufferSource.begin(), bufferSource.end())
müsste reichen.

Oh da treffen mich meine fehlenden STL-Kenntnisse ganz gewaltig...

const char* Exception::what()const{
	return what_.c_str();
}

wie bereits gesagt: man baut den string auch oft gerne im what() zusammen

Oh, macht denke ich Sinn.

weiters ist es amtlich: die exceptionklasse ist nahezu nutzlos. sie hat absolut keine logik...

Stimmt schon. Hättest du nen Vorschlag zum Gesamtdesign (s.o.)?

man müsste pro fehlerfall eine exceptionklasse schreiben, oder zB eine OdbcApiException() dem man das handle gibt und das daraus alle infos liest.

Handle* Handle::lastUsedHandle = NULL;

Achtung: mal verwendest du SQL_NULL_HANDLE und mal NULL
entscheide dich.

Du bringts mich auf die sinnvolle Idee die Art der Exception an SQLRETURN zu koppeln

Das mit NULL ist eine Unachtsamkeit...

Handle::Handle(const SQLSMALLINT handleType) : 
handle_(SQL_NULL_HANDLE),
handleType_(handleType){
}

Oha. der ctor erstellt kein Objekt?

Oh, habe vergessen da die Alloc-Funktion reinzupacken

void Handle::Alloc(SQLHANDLE inputHandle){
	ExecuteApi(::SQLAllocHandle(handleType_, inputHandle, &handle_));
}

sofort frage ich mich: wie und wann wird dann Alloc aufgerufen. und wo kommt dieses inputHandle her...

Sollte eigentlich automatisch im Konstruktor aufgerufen werden... Das inputHandle ergibt sich aus der ODBC-Api. Sollte ich wohl aber besser dokumentieren.

void Handle::Free(){
	ExecuteApi(::SQLFreeHandle(handleType_, handle_), false); 
}

Aha, deshalb gibt es bei ExecuteApi den throwExcpt Parameter.
ne, das ist unschön.

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.

bool Handle::GetError(std::string& errorMsg, std::string& sqlState,
					  SQLINTEGER& errorCode)const{
	std::vector<SQLCHAR> bufSqlState(5+1);
	std::vector<SQLCHAR> bufErrorMsg(512+1);
	SQLSMALLINT bytesTransferred = 0;
	ExecuteApi(::SQLGetDiagRec(handleType_, handle_, 1, &bufSqlState[0], 
							   &errorCode, &bufErrorMsg[0], 512, &bytesTransferred),
			   false);
	errorMsg.assign(reinterpret_cast<char*>(&bufErrorMsg[0]));
	sqlState.assign(reinterpret_cast<char*>(&bufSqlState[0]));

	
	return bytesTransferred > 0 ? true : false;
}

Ih, da sieht man wie hässlich C APIs in C++ sind
ich sehe hier aber ein problem: was wenn SQLGetDiagRec fehlschlägt? ist der inhalt von buf* definiert?

diese ignorieren von fehlern finde ich garnicht gut...

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

btw:

return bytesTransferred > 0 ? true : false;

ist immer wieder lustig. warum nicht gleich

if(bytesTransferred > 0 ? true : false)
  return true ? true : false;
else
  return false ? true : false;

?
:p

Implizite bool-Umwandlung nutzen?

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

Ciao...

PS: Wieso maximal zehn Smilies erlaubt o_O?

[Gast]

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?

[atom]

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.

1. wie bei validateProtocol, wäre ensure* hier nicht besser?
2. 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.