Verbesserungsvorschläge für Konsolen-Chat-Anwendung

[Francis123]

ich hab hier eine kleine chat- konsolenanwendung erstellt und hätte gerne dass ihr mal rüberschaut und mir verbesserungsvorschläge oder sonstwas gebt. ist wohl meine erste fertige c++ anwendung, hab vorher nur irgendwelche beispielcodes aus büchern und tutorials nachgemacht.
so wie er jetzt ist müsste der code kompilierbar und ausführbar sein. ich hab es noch nicht mit nem client auf nem anderen pc getestet, aber canyouseeme.org erkennt den server schonmal und über die lokale ip kann ich auch dahin connecten.
der code müsste ausreichend kommentiert sein.

****************
der Server:

//Main.cpp
#include <winsock2.h>
#include <WinSock.h>
#include <Windows.h>
#include <iostream>
#include <string>
#include "SockThread.h"
using namespace std;

#define PORT htons(6394)

#pragma comment(lib,"wsock32.lib")

int startWinSock();
SOCKET s;		bool sOpen=false;
SOCKADDR_IN addr;	
SockThread* peers[MAX_PEERS];
SOCKET connectedSocket;	
int createServerSocket();
int main()
{
	bool running=true;
	int rc;
	rc = startWinSock();

	if((rc=startWinSock())!=0)
	{
		cerr<<"WinSock konnte nicht gestartet werden."<<endl;
		system("PAUSE");
		return rc;
	}
	else
	{
		cout<<"WinSock gestartet."<<endl;
	}

	memset(&addr, 
			0,
			sizeof(SOCKADDR_IN));
	addr.sin_addr.s_addr=ADDR_ANY;
	addr.sin_family=AF_INET;
	addr.sin_port=PORT;

	createServerSocket();

	while(running)
	{
	if(peercnt<MAX_PEERS)
		{
				if(sOpen==false)
				{
					createServerSocket();
				}

				if((connectedSocket=accept(s, NULL,NULL))!=INVALID_SOCKET)
				{

					int index=0;
					string name="default";
					char charname[16]={};
					for(int i=0; i<16; i++) {charname[i]=0;}			//hab erst vor kurzen gecheckt dass das scheiß char array nicht mit 0 initialisiert wird, sondern irgendwelche
																		//zufälligen werte enthält bei der ausgabe wird dann nur irgendwelcher mist ausgegeben weil da kein \0 drin ist
				    int h=0;
				    h=recv(connectedSocket,charname, 16, 0); 
					if(h<1){	continue;	}
					else
					{
					name=charname;
					}
						peercnt++;
					for(index=0; index<MAX_PEERS; index++)
					{
						if(peers[index]==NULL)
						{	peers[index]=new SockThread(true,connectedSocket, peers, name); break;	}		//nach einer lücke im array suchen und den neuen peer da einfügen.
					}

					cout<<"Client Nr. "<<peercnt<<", "<<name<<" verbunden."<<endl;

					string welcome=	"Willkommen bei Nikolais Chat\n";

					h=send(peers[index]->socket,
					welcome.c_str(),
					strlen(welcome.c_str()),
					0);
				}
				else
				{	
					cerr<<"Fehler bei accept. Fehlercode: "<<WSAGetLastError()<<endl;	
				}
		}
	else
	{

		closesocket(s);
		sOpen=false;		//hab keine andere möglichkeit gefunden um keine verbindungen mehr anzunehmen.
							//selbst wenn kein accept durchgeführt wird connectet einfach jeder socket und dann crasht der scheiß server
	}						//wenn irgendwas gesendet wird. ich erstelle also den serversocket jedes mal neu wenn wieder platz für neue peers da ist.

		Sleep(50);
	}

	system("PAUSE");
}

int startWinSock()
{

	WSAData wsa;
	return WSAStartup(MAKEWORD(2,0), &wsa);

}

	int createServerSocket()
	{
		//müsste hier alles standartmäßig sein. socket erstellen, binden, listen diesdas
		int rc;
	if((s=socket(AF_INET, SOCK_STREAM,0))==INVALID_SOCKET)
	{
		cerr<<"Serversocket konnte nicht erstellt werden."<<endl;
		system("PAUSE");
		return 1;
	}
	else
	{
			cerr<<"Serversocket erstellt."<<endl;
	}

	if((rc=bind(s, (SOCKADDR*)&addr, sizeof(SOCKADDR_IN)))==SOCKET_ERROR)
	{
		cerr<<"Fehler beim Binden. Fehlercode: "<<WSAGetLastError()<<endl;
		system("PAUSE");
		return rc;

	}
	else
	{
		cout<<"ServerSocket an port "<<PORT<<" gebunden."<<endl;
	}

	if((rc=listen(s,0))==SOCKET_ERROR)
	{
		cerr<<"Serversocket konnte nicht in Lauschmodus versetzt werden. Fehlercode: "<<WSAGetLastError()<<endl;
		system("PAUSE");
		return rc;

	}
	else
	{
		cout<<"Serversocket ist im Lauschmodus. "<<endl;
		sOpen=true;

	}

	return 0;
	}

//SockThread.h
#ifndef _SOCKTHREAD_H
#define _SOCKTHREAD_H

#include <winsock2.h>
#include <WinSock.h>
#include <Windows.h>
#include <iostream>
#include <string>

using namespace std;

#define MAX_PEERS 10

static int peercnt=0;		//das wurde auch nicht erkannt wenns in der main.cpp war

class SockThread
{

public:

	string name;
	bool run;
	HANDLE hThread;
	DWORD dwThread;
	SOCKET socket;
	 SockThread** peers;		//ich wusste nicht wie ich das array mit den SockThreads anders benutzen sollte, es war in der main.cpp aber wurde nichtmal erkannt.

	SockThread(bool run=false, SOCKET s=NULL,SockThread** st=NULL,  string name="default" )
	{

	this->run=run;
	this->socket=s;
	this->peers=st;
	this->name=name;

		if(run)
		{
			hThread=CreateThread(
				NULL,
				0,
				ThreadFunc,
				(LPVOID)this,
				0,//run*4,			wenn als boolean false übergeben wäre dann würde der thread nicht gestartet werden weil false*1 = 4 wäre, ich hab drauf geschissen und einfach 0 gemacht.
				&dwThread
				);
		}
	}

	~SockThread()
	{

		for(int i=0; i<MAX_PEERS; i++) {	if(peers[i]==this){	peers[i]=NULL;	}	}		//hat ewig gedauert bis ich gecheckt hab dass nach dem löschen die adresse nicht NULL ist...

			peercnt--;

			string msg="\nClient "+name+" hat die Verbindung getrennt.\n";
			cout<<msg<<endl
				<<"Peercount: "<<peercnt<<endl;
			sendToClients(peers, msg.c_str(), strlen(msg.c_str()));
			closesocket(socket);
			ExitThread(0);	
			CloseHandle(hThread);		//wird bestimmt nichtmal ausgeführt nachdem ich den thread beendet hab.

	}

	static  DWORD WINAPI ThreadFunc(LPVOID data)
	{
		SockThread *st=(SockThread*)data;
		int bytesRead;
		int bytesSent;
		char buffer[256];
		string str=st->name+" hat verbunden.\n";

		bytesSent=st->sendToClients(st->peers, str.c_str(), strlen(str.c_str()));

		while(true)
		{

		bytesRead=recv(st->socket, buffer, 256, 0);
		if(bytesRead>0)		//es crasht hier auf mysteriöse weise wenn es bytesRead>1 ist...
		{
			bytesSent=st->sendToClients(st->peers, (st->name+string(" :")+buffer).c_str(),bytesRead+2+strlen(st->name.c_str()));
			bytesRead=0;
		}
		else
		{		
			delete st;		//hab keine möglichkeit gefunden um zu checken ob der socket noch offen ist, bin einfach davon ausgegangen wenn -1 oder 0 gesendet wurde.
		}		
		Sleep(30);

		}
		return 0;
	}

int sendToClients( SockThread** peers, const char* c, int len)		//nachricht wird an jeden client außer den absendet geschickt.
{
	int bytessent=0;

	for(int i=0; i<MAX_PEERS; i++)
	{
		SockThread* st=peers[i];
		if(st!=NULL && st!=this)
		{
			bytessent=send(st->socket, c, len,0);
		}

	}
	return bytessent;

}
};

#endif

****************

der client:

//Main.cpp
#include <WinSock.h>
#include <Windows.h>
#include <iostream>
#include <string>
using namespace std;

#pragma comment(lib,"wsock32.lib")
#define PORT htons(6394)
#define IP "127.0.0.1"

int startWinSock();

HANDLE hThread;
DWORD dwThread;
DWORD WINAPI ThreadFunc(LPVOID data);

SOCKET s;
SOCKADDR_IN addr;
bool running=true;
int rc;
char* ip=IP;
string name="";
int connectSocket();
int menu();
void startMessaging();

int main()
{
	memset(&addr, 0, sizeof(SOCKADDR_IN));
	addr.sin_addr.s_addr=inet_addr(ip);
	addr.sin_family=AF_INET;
	addr.sin_port=PORT;

	if((rc=startWinSock())!= 0)
	{
		cerr<<"WinSock konnte nicht gestartet werden."<<endl;
   	  system("PAUSE");
		return 1;
	}
	else
	{
		cout<<"Winsock gestartet."<<endl;
	}

	while(true)
	{
		startMessaging();		//das menü wird aufgerufen um den namen einzugeben, die ziel ip zu ändern und zu verbinden.
	}

system("PAUSE");

}

void startMessaging()
{

while(menu()!=0);

hThread=CreateThread(NULL, 0, ThreadFunc, 0, 0, &dwThread);

	while(running)
	{
		string str;
		getline(cin, str);	

		str+='\n';					//nötig oder unnötig?
		send(s, str.c_str(), strlen(str.c_str()), 0);		//alles senden was über cin reinkommt.
		cin.sync();	cin.clear();//keine ahnung wozu, im buch wurde das immer so gemacht.
	}

}

int startWinSock()
{
	WSADATA wsa;
	return WSAStartup(MAKEWORD(2,0), &wsa);
}

DWORD WINAPI ThreadFunc(LPVOID data)
{
	int bytesRead=0;
	char buffer[256];

	while(running)
	{

		if((bytesRead=recv(s, buffer, 256, 0))>0)		//einfach alles ausgbene was reinkommt.
		{
			cout<<buffer;
			for(int i=0; i<256; i++)
			{buffer[i]='\0';}
		}
		else
		{
			//wenn der server beendet wird gibt bytesRead in endlosschleife -1 zurück
			cout<<bytesRead<<" bytes gelesen. Socket wird geschlossen. \n Enter druecken."<<endl;
			closesocket(s);
			running=false;

		}

	}
	return 0;
}

int connectSocket()
{
	if((s=socket(AF_INET, SOCK_STREAM, 0))==INVALID_SOCKET)
	{
		cerr<<"Socket konnte nicht erstellt werden"<<endl;
		  system("PAUSE");
  return 1;
	}
	else
	{
		cout<<"Socket erstellt."<<endl;
	}

		if(rc=connect(s, (SOCKADDR*)&addr, sizeof(SOCKADDR_IN))==SOCKET_ERROR)
		{
		  printf("Fehler: connect gescheitert, Fehlercode: %d\n",WSAGetLastError());
		  system("PAUSE");
		  return 1;
		}
		else
		{
		 cout<<"Verbunden mit "<<addr.sin_addr.s_addr<<"\n";
		 running=true;
		 send(s, name.c_str(), strlen(name.c_str()), 0);

		}

return 0;
}

int menu()
{
	char c;

	cout<<"|*********Nikolais Chatclient*********|"<<endl
		<<"|                                     |"<<endl
		<<"|        I   =   Ziel-IP aendern      |"<<endl
		<<"|                                     |"<<endl
		<<"|        N   =   Name eingeben        |"<<endl;
if(name!=string("")){
	cout<<"|                                     |"<<endl
		<<"|        C   =   Verbinden            |"<<endl;
			}
	cout<<"|                                     |"<<endl
		<<"|*************************************|"<<endl;

	cout<<endl<<"Eingabe: ";cin>>c;		//wenn man hier nen string aus mehreren buchstaben eingibt dann kommt der kasten direkt so oft wie buchstaben im string sind...

	switch(c)
	{
	case 'N':{
		case 'n':
				{
					cout<<"Name eingeben: ";cin.sync(); cin.clear();	getline(cin, name);
					if(strlen(name.c_str())<=16)	{	cout<<"\nName "<<name<<" gespeichert\n";	}
						else
						{	cout<<"\nName zu lang. Max. 16 Zeichen erlaubt.\n"; name="";}
						break;
				}
				break;
			 }

		case 'c':
			{
			case'C':
				{
					if(name!=string(""))
					{
					if(connectSocket()==0)
					return 0;
					}

				}
				break;
			}
		case'I':
			{
			case'i':
				{
					cout<<"IP des Chat-Servers eingeben: "; cin.sync(); cin.clear();string ipaddr;	getline(cin, ipaddr);
					addr.sin_addr.s_addr=inet_addr(ipaddr.c_str());						//ip=ipaddr.c_str() geht nicht...
					cout<<endl<<"IP "<<addr.sin_addr.s_addr<<" gespeichert."<<endl;		//wie zum teufel gibt man das ding mit punkten getrennt aus? mit der zahl kann man jetzt nicht viel anfangen

				}

			}

	}

	return 1;
}

[Kellerautomat]

Wenn du deinen Code einrückst, wäre ich bereit, deinen Server oder Client (suchs dir aus) zu überarbeiten, um dir zu zeigen, wie man es besser machen könnte.

[hustbaer]

@Nikolai

Ein paar Anmerkungen (Kommentare im Code).
Zeilenumbrüche hab ich 1:1 von deinem Code übernommen, Einrückung hab ich angepasst (in Visual Studio Text markieren, Ctrl+K, Ctrl+F - wow, schwierig).

#include <winsock2.h> 
#include <WinSock.h> 
#include <Windows.h> 
#include <iostream> 
#include <string> 
#include "SockThread.h" 
using namespace std; 

// static const int wäre besser
// und wenn schon #define, dann mit dem Projektnamen prefixen, also MYCHAT_PORT oder sowas
#define PORT htons(6394)

#pragma comment(lib,"wsock32.lib") 

// 1) Total unaufgeräumt, erst eine Funktion, dann ein paar globale Variablen, dann wieder eine Funktion.
//    Stehst du auf Chaos, oder bist du bloss unendlich faul?
// 2) Globale Variablen sind für so ein Mini-Projekt vielleicht noch OK, aber dann bitte wenigstens
//    irgendwie kennzeichnen. z.B. mit dem Prefix "g_", also "g_s", "g_addr" etc.
// 3) Verwende ordentliche Namen für deine Variablen. s -> g_listenSocket, addr -> g_listenAddress etc.
// 4) Einige der globalen Variablen sind überhaupt komplett unnötig, weil sie als Parameter übergeben werden könnten/sollten.
//    Wie z.B. "addr".

int startWinSock(); 
SOCKET s;        bool sOpen=false;  // Zwei Deklarationen in einer Zeile? Wirklich? Wieso? Ausserdem ist sOpen sinnlos, ...
SOCKADDR_IN addr;                   // ... verwende einfach INVALID_SOCKET um zu kennzeichnen dass der Socket nicht offen ist.
SockThread* peers[MAX_PEERS];
SOCKET connectedSocket;
int createServerSocket(); 
int main() 
{ 
	bool running=true; // verwendest du erst viel weiter unten, wieso dann hier schon definieren? Definier' Variablen dort wo du sie zum 1. mal brauchst.
	int rc; 
	rc = startWinSock(); // Erst definieren dann eine Zeile drunter initialisieren. Ganz grosses Kino. Mach das in einer Zeile.

	if((rc=startWinSock())!=0)
	{ 
		cerr<<"WinSock konnte nicht gestartet werden."<<endl; 
		system("PAUSE"); // Kein Kommentar
		return rc; 
	} 
	else 
	{ 
		cout<<"WinSock gestartet."<<endl; // Sinnlose Information
	} 
	// wozu
	// die
	// vier
	// Leerzeilen - wäre eine zu wenig?
	memset(&addr, 
		0, 
		sizeof(SOCKADDR_IN)); // Wozu der Split über 3 Zeilen? ...?
	addr.sin_addr.s_addr=ADDR_ANY; 
	addr.sin_family=AF_INET; 
	addr.sin_port=PORT; 

	createServerSocket(); 

	while(running) // Du setzt running sowieso nirgends auf false, wieso dann überhaupt eine Variable? Einfach for(;;) als Schleife und gut.
	{ 
		if(peercnt<MAX_PEERS) 
		{ 
			if(sOpen==false) // Erledigt man normalerweise vor der Schleife.
			{ 
				createServerSocket(); 
			} 

			if((connectedSocket=accept(s, NULL,NULL))!=INVALID_SOCKET) // 
			{ 

				int index=0; 
				string name="default"; // Wozu "default"? Du machst entweder "continue" oder weisst name einen neuen Wert zu, "default" wird nie drinnen stehen bleiben. Unfug.
				char charname[16]={}; // "name" und "charname" - weil da jetzt jeder weiss was gemeint ist. OMG. Bessere Namen!
				for(int i=0; i<16; i++) {charname[i]=0;} // Blödsinn, so wie es jetzt dasteht (mit dem "={}") wird sehr wohl alles mit 0 initialisiert.
				                                         // Davon abgesehen gibt es dafür memset bzw. std::fill.
				int h=0; // Siehe "int rc;". Ausserdem ... "h"? H? HHHHHH? Für was zum Geier steht "h"?
				h=recv(connectedSocket,charname, 16, 0); 
				if(h<1){    continue;    } // if und Statement auf der selben Zeile. Super-mega-grosses Kino. Pack das "continue" in eine eigene Zeile.
				else 
				{ 
					name=charname; 
				} 
				peercnt++; 
				for(index=0; index<MAX_PEERS; index++) 
				{ 
					if(peers[index]==NULL) 
					{    peers[index]=new SockThread(true,connectedSocket, peers, name); break;    }        // Wieder einrückung: geht's noch?
				} 

				cout<<"Client Nr. "<<peercnt<<", "<<name<<" verbunden."<<endl; 

				// Das sollte besser der Connection-Thread übernehmen.
				string welcome=    "Willkommen bei Nikolais Chat\n"; 

				h=send(peers[index]->socket, 
					welcome.c_str(), 
					strlen(welcome.c_str()), 
					0); 
			} 
			else 
			{    
				cerr<<"Fehler bei accept. Fehlercode: "<<WSAGetLastError()<<endl;    
			} 
		} 
		else 
		{ 
			// Fehler: closesocket() wiederholt auf "s" aufrufen, immer wieder, so lange "peercnt => MAX_PEERS".
			// Richtig: sobald du closesocket() ist der Socket weg, und einen Socket den's nicht mehr gibt darfst du auch kein 2. mal schliessen.
			closesocket(s); 
			sOpen=false;

			// Wenn du nicht willst dass neue Connections angenommen werden, dann ... tjo, Pech. Das macht man so einfach nicht.
			// Windows, Linux etc. unterstützen das nicht.
			// Was man statt dessen macht ist die Connection anzunehmen und dann sofort wieder zu schliessen.
			// Wenn man nett ist schickt man noch vorher den Grund raus warum man das gemacht hat, dann muss der Client nich raten.
			// Dann musst du auch nicht dauernd den Socket auf und zu machen.

		Sleep(50); 
	} 

	system("PAUSE"); // Kein Kommentar
} 

int startWinSock() 
{ 

	WSAData wsa; 
	return WSAStartup(MAKEWORD(2,0), &wsa); 

} 

int createServerSocket() 
{ 
	int rc; 
	if((s=socket(AF_INET, SOCK_STREAM,0))==INVALID_SOCKET) 
	{ 
		cerr<<"Serversocket konnte nicht erstellt werden."<<endl; 
		system("PAUSE"); // Kein Kommentar^2
		return 1; 
	} 
	else 
	{ 
		cerr<<"Serversocket erstellt."<<endl; // Sinnlose Information
	} 

	if((rc=bind(s, (SOCKADDR*)&addr, sizeof(SOCKADDR_IN)))==SOCKET_ERROR) 
	{ 
		cerr<<"Fehler beim Binden. Fehlercode: "<<WSAGetLastError()<<endl; 
		system("PAUSE"); // Kein Kommentar^2
		return rc; 

	} 
	else 
	{ 
		cout<<"ServerSocket an port "<<PORT<<" gebunden."<<endl; // Sinnlose Information
	} 

	if((rc=listen(s,0))==SOCKET_ERROR) 
	{ 
		cerr<<"Serversocket konnte nicht in Lauschmodus versetzt werden. Fehlercode: "<<WSAGetLastError()<<endl; 
		system("PAUSE"); // Kein Kommentar^2
		return rc; 

	} 
	else 
	{ 
		cout<<"Serversocket ist im Lauschmodus. "<<endl; // Sinnlose Information
		sOpen=true; 

	} 

	return 0; 
}

//SockThread.h 
#ifndef _SOCKTHREAD_H // Bezeichner die mit Underscore + Grossbuchstabe beginnen sind für die Implementierung reserviert. Heisst: du nicht machen, Finger weg.
#define _SOCKTHREAD_H 

#include <winsock2.h> 
#include <WinSock.h> 
#include <Windows.h> 
#include <iostream> 
#include <string> 

using namespace std; // "using namespace" in Headerfiles macht man nicht. Warum googelst du dir bitte selbst.

#define MAX_PEERS 10 

static int peercnt=0;        //das wurde auch nicht erkannt wenns in der main.cpp war -- Natürlich nicht, lern bitte C++. Und noch dazu static - wenn dann bitte extern.

class SockThread 
{ 

public: 
	// 1) Keine Kennzeichnung von Membervariablen. Das ist Top, damit kann man alle schön verwirren.
	// 2) Alles public? Wirklich? Weil?
	string name; 
	bool run; // Verwendest du nirgends - WTF?
	HANDLE hThread; 
	DWORD dwThread; // Verwendest du nirgends - WTF?
	SOCKET socket; 
	SockThread** peers;        //

	// "run" als Parameter? Wozu? ...? Übergibtst du denn jemals "false"?
	// Und wozu die ganzen sinnlosen Default-Arguments? Damit du einen Default-Konstruktor bekommst? Den du bitte wozu brauchst?
	SockThread(bool run=false, SOCKET s=NULL,SockThread** st=NULL,  string name="default" ) 
	{ 

		this->run=run; 
		this->socket=s; 
		this->peers=st; 
		this->name=name; 

		if(run) 
		{ 
			hThread=CreateThread( 
				NULL, 
				0, 
				ThreadFunc, 
				(LPVOID)this, 
				0,//run*4,  -- WTF?
				&dwThread 
				); 
		} 
	} 

	~SockThread() 
	{ 
		// Sorry, aber ich muss das hier nochmal fragen: bist du vollkommen (such-dir-was-aus)? Einrückung? Hallo?
		// Davon abgesehen... schonmal was von Synchronisation gehört?
		// Du kannst nicht einfach aus mehreren Threads auf das selbe Array zugreifen ohne zu synchronisieren.
		// Stichwort Mutex und so
		for(int i=0; i<MAX_PEERS; i++) {    if(peers[i]==this){    peers[i]=NULL;    }    }

		// Siehe oben: synchronisieren
		peercnt--; 

		string msg="\nClient "+name+" hat die Verbindung getrennt.\n"; 
		cout<<msg<<endl 
			<<"Peercount: "<<peercnt<<endl; 
		sendToClients(peers, msg.c_str(), strlen(msg.c_str())); 
		closesocket(socket); 
		ExitThread(0);    
		// Bingo, das wird hier wirklich nicht ausgeführt. Deswegen schreiben wir es auch hin,
		// weil es überhaupt keinen Sinn macht, einfach nur damit schön was dasteht was so tut als ob wir das Handle freigeben würden,
		// was wir aber nicht tun. Meine Fresse...
		// Schreib das CloseHandle(hThread) VOR das ExitThread() hin verdammt nochmal.
		CloseHandle(hThread);        

		// Oder, noch besser: lass das Aufräumen den Main-Thread erledigen. Dann musst du nämlich auch nicht synchronisieren.
		// Und ein anderes Problem erledigt sich dadurch auch von selbst: du musst nicht mehr ExitThread() aufrufen. Weil ExitThread()
		// in Kombination mit C++ nämlich problematisch ist. Ganz speziell so wie du es hier machst.
	} 

	static  DWORD WINAPI ThreadFunc(LPVOID data) 
	{ 
		SockThread *st=(SockThread*)data; 
		int bytesRead; 
		int bytesSent; 
		char buffer[256]; 
		string str=st->name+" hat verbunden.\n"; // Lieber 10x "st->" schreiben als einfach "st->ThreadFunc2()" aufzurufen, und den restlichen
		                                         // Code nach ThreadFunc2() zu verschieben. Ja, gut, manche mögen's unnötig kompliziert.

		bytesSent=st->sendToClients(st->peers, str.c_str(), strlen(str.c_str())); 

		while(true) 
		{ 

			bytesRead=recv(st->socket, buffer, 256, 0); // 256 Zeichen grosser Puffer, dann bis zu 256 empfangen,
			                                            // und dann als Nullterminierten String interpretieren. Fällt dir was auf?
			                                            // Ne, vermutlich nicht. Stichwort Buffer-Overflow.
			// recv() macht auch nicht das was du meinst dass es macht. recv() empfängt bis zu N zeichen, aber Sockets haben keine
			// Message-Grenzen, d.h. wenn der Client 100 Zeichen wegschickt kann es sein dass recv() beim 1. Aufruf trotzdem nur 1 oder 10
			// oder 99 oder auch 42 Zeichen empfängt.
			// Doku Lesen macht schlau.
			if(bytesRead>0)        //es crasht hier auf mysteriöse weise wenn es bytesRead>1 ist... 
			{
				// "st->name+string(" :")+buffer" und dann mit "bytesRead+2+strlen(st->name.c_str())" ausrechnen wie lange das ist? WTF?
				// 1) Statt "strlen(st->name.c_str())" könnte man auch einfach "st->name.size()" nehmen. Aber warum einfach ... naja.
				// 2) Wieso baust du den String nicht vorher mit
				//        std::string message = st->name + " :" + buffer;
				//    zusammen und übergibst dann message.c_str() und message.size()? ...?
				//    Oder noch besser gleich nur message - wäre ja kein Beinbruch wenn sendToClients() direkt nen std::string als
				//    Parameter nimmt.
				bytesSent=st->sendToClients(st->peers, (st->name+string(" :")+buffer).c_str(),bytesRead+2+strlen(st->name.c_str())); 
				bytesRead=0; 
			} 
			else 
			{
				// Auja, das ist fein. Das Objekt löschen, damit es dann im Destruktor den aktuellen Thread mit ExitThread zumacht.
				// Besser: Objekt löschen und dann HIER den Thread beenden, und zwar indem du einfach "return 0;" machst.
				// Noch besser: Objekt nicht löschen sondern nur als ungültig markieren, damit der Main-Thread es dann löschen kann.
				delete st;
			}        
			// Wozu Sleep?
			Sleep(30); 

		} 
		return 0; 
	} 

	int sendToClients( SockThread** peers, const char* c, int len)
	{ 
		int bytessent=0; 

		for(int i=0; i<MAX_PEERS; i++) 
		{ 
			// Wieder: synchronisieren
			SockThread* st=peers[i]; 
			if(st!=NULL && st!=this) 
			{ 
				// Ei das ist fein, du sendest von allen Threads auf alle Sockets, ohne irgendwie zu synchronisieren.
				// Na das muss ja immer fein funktionieren.
				// Und nochwas: was passiert wohl wenn ein Client zwar noch connected ist, aber gerade mal für ein paar Sekunden
				// oder auch ne Minute keine Nachrichten mehr empfangen kann?
				// Dann blockiert das send() hier bald mal, und dann steht dein lustiger fideler Chat-Server.
				// Zumindest alle Connection-Threads die ab dem Zeitpunkt wo der Client nix mehr empfangen kann versuchen was zu senden.
				bytessent=send(st->socket, c, len,0); 
				// Besser: stell' alle empfangenen Nachrichten in eine Queue. In der Queue hebst du z.B. max. 100 oder 1000 Nachrichten auf.
				// In jedem Queue-Eintrag ist die Nachricht, von wem sie stammt, die Uhrzeit zu der sie empfangen wurde und eine
				// fortlaufende Nummer enthalten.
				// Jeder Connection-Thread prüft dann periodisch diese Queue.
				// Jeder Connection-Thread weiss die fortlaufende Nummer des letzten Eintrags den er versendet hat, und weiss dadurch was für ihn neu
				// dazugekommen ist.
				// Jeder Connection-Thread kopiert sich dann die neuen Nachrichten raus, und schickt diese an seinen Client raus.
				// Dadurch kann eine Client höchstens sich selbst blockieren, nicht aber Connections zu anderen Clients.
				// Selbstverständlich muss man die Zugriffe auf diese Queue auch synchronisieren.
			} 

		} 
		return bytessent; 

	} 
}; 

#endif

Das sind jetzt nur die gröbsten Sachen die mir so auf die Schnelle aufgefallen sind. Falls dir die Erklärungen zu wenig sind, dann google es bitte selbst. Ich nehme mir sicher nicht stundenlang Zeit um dir das alles haarklein zu erklären.

Und: das kommt dabei 'raus wenn man sich übernimmt. Such dir ein einfacheres Projekt, dann stehen die Chancen auch besser dass du es - mit ein wenig Hilfe - halbwegs akzeptabel hinbekommst.
Und lass entweder die Finger von Threads, oder mach dich zu dem Thema erstmal ordentlich schlau bevor du sie einfach irgendwie ala "wird schon passen" verwendest.

[piratmull]

Wow, du hast ihn ja wirklich zu tode gebashed.
Ich hoffe er hat jetzt überhaupt noch Motivation irgendwas zu programmieren.

[floorball]

Wieso includeierst du winsock.h und winsock2.h das ist komplett unsinnig!!!!
Und warum dann nur die lib von winsock.h obwohl du zusätzlich winsock2.h hast???
Schmeiß winsock .h raus incl. deren lib und nehme nur winsock2.h

PS: lib von winsock2.h heißt "ws2_32.lib"
winsock.h ist auch in windows.h enthalten also entweder auskommentieren oder die 2 einfügen...

Edit: wieso MAKEWORD(2,0) und nicht MAKEWORD(2,2)????
der Rest wurde von hustbaer erwähnt
halt moment nicht ganz...
was machst du in zeile 26??? wieso rufst du startWinSock 2 mal auf?
so wärs besser:
if(startWinSock() != 0)
außerdem könntest du startWinSock als inline deklarieren