   Suggestions & criticisms needed for class design
  EDIT: Source updated to include some of the suggestions. More to come. Please keep your helpful criticisms coming in! 1 question: Though I'm using vectors now for the buffer, I still need to use a char array with the recv call. Is there anyway i can do it with a std::string or anything else which does not require me to define a MAXDATASIZE? I wish to hide the implementation of recv and at the same time ensure that all the data gets read into the buffer, but doing char temp[100000]; doesn't seem like a wise choice either.Thank you.----------------------Greetings to all. I am a to-be computer science student at a local university in Singapore this coming term and my interest lies in network applications. I have had some experience with c++, and as an effort to reach my goal of being able to write good server code(specifically MMOs), this is my first try at writing a simple IRC client which does nothing but connect to a server, join a channel and reply to PINGs from the server to stay connected.As this is one of my first experiences with network programming, it definitely isn't anything I'm proud of. Therefore, any constructive criticisms are welcomed. feel free to tell me if it's the worst design you've ever seen, or if there're concepts I'm totally misunderstanding. I'll rewrite this as many times as it takes to advance a step closer to professional network code.The client uses Event Objects as I don't think i'm ready for IOCP yet.TCPBuffer.h#ifndef TCPBUFFER_H#define TCPBUFFER_H#include <string>#include <vector>#include <iostream>class TCPBuffer{private:std::vector<char> data;public:TCPBuffer();int size();void clear();bool getline(std::string&, char);void append(const std::string&, int);};TCPBuffer::TCPBuffer(){}int TCPBuffer::size(){return data.size();}void TCPBuffer::clear(){data.clear();}bool TCPBuffer::getline(std::string& str, char delim){std::vector<char>::iterator it;str.clear();for(it = data.begin(); it < data.end(); it++){str.push_back(*it);if(*it == delim){data.erase(data.begin(), it+1);break;}}if(str.find_last_of(delim) == std::string::npos){str.clear();return false;}elsereturn true;}void TCPBuffer::append(const std::string& d, int length){data.insert(data.end(), d.begin(), d.begin() + length);}#endifTCPSocket.h#ifndef TCPSOCKET_H#define TCPSOCKET_H#include <winsock2.h>#pragma comment(lib, "ws2_32.lib")#include "TCPBuffer.h"#define MAXDATASIZE 2048class TCPSocket{private:SOCKET s;TCPBuffer buffer;public:TCPSocket();SOCKET GetSock();TCPBuffer* GetBuffer();int Recv();int Disconnect();int Send(std::string);int Connect(std::string, int);};TCPSocket::TCPSocket(){s = WSASocket(AF_INET, SOCK_STREAM, IPPROTO_TCP, NULL, NULL, NULL);}SOCKET TCPSocket::GetSock(){return s;}TCPBuffer* TCPSocket::GetBuffer(){return &buffer;}int TCPSocket::Recv(){char temp[MAXDATASIZE];int nRet = recv(s, temp, MAXDATASIZE - 1, NULL);buffer.append(std::string(temp), nRet);return nRet;}int TCPSocket::Disconnect(){return closesocket(s);}int TCPSocket::Send(std::string data){return send(s, data.c_str(), data.length(), NULL);}int TCPSocket::Connect(std::string host, int port){LPHOSTENT lpHostEntry;lpHostEntry = gethostbyname(host.c_str());SOCKADDR_IN sa;sa.sin_family = AF_INET;sa.sin_addr = *((LPIN_ADDR)*lpHostEntry->h_addr_list);sa.sin_port = htons(port);return connect(s, (LPSOCKADDR)&sa, sizeof(SOCKADDR_IN));}#endifTCPClient.h#ifndef TCPCLIENT_H#define TCPCLIENT_H#include <map>#include <vector>#include <winsock2.h>#pragma comment(lib, "ws2_32.lib")#include "TCPSocket.h"class TCPClient{private:WSADATA wsaData;WSANETWORKEVENTS events;std::vector<WSAEVENT> hEvents;std::map<WSAEVENT, TCPSocket*> TCPSockets;virtual void ON_READ(TCPSocket*) = 0;   virtual void ON_CONNECT(TCPSocket*) = 0;   virtual void ON_CLOSE(TCPSocket*) = 0;public:TCPClient();~TCPClient();void Run();void AddSocket(TCPSocket*, long);};TCPClient::TCPClient(){WSAStartup(WINSOCK_VERSION, &wsaData);}TCPClient::~TCPClient(){WSACleanup();}void TCPClient::Run(){while(1){DWORD iEvent;iEvent = WSAWaitForMultipleEvents(hEvents.size(), (WSAEVENT*)&hEvents[0], FALSE, WSA_INFINITE, FALSE);//Signaled EventDWORD sEvent = iEvent - WSA_WAIT_EVENT_0;WSAResetEvent(hEvents.at(sEvent));WSAEVENT hEvent = hEvents.at(sEvent);TCPSocket* s = TCPSockets[hEvents.at(sEvent)];if(WSAEnumNetworkEvents(s->GetSock(), hEvent, &events) == 0){if (events.lNetworkEvents & FD_CONNECT){ON_CONNECT(s);}if (events.lNetworkEvents & FD_READ){ON_READ(s);}if (events.lNetworkEvents & FD_CLOSE){ON_CLOSE(s);}}}}void TCPClient::AddSocket(TCPSocket* s, long lNetworkEvents){WSAEVENT hEventObject = WSACreateEvent();WSAEventSelect(s->GetSock(), hEventObject, lNetworkEvents);hEvents.push_back(hEventObject);TCPSockets[hEventObject] = s;}#endifmain.cpp#include <iostream>#include "TCPClient.h" class IRCClient : public TCPClient{virtual void ON_CONNECT(TCPSocket* s){s->Send("NICK testbot\r\nUSER abc 8 * :toy\r\n");//s->Send("JOIN #test\r\n");}virtual void ON_READ(TCPSocket* s){s->Recv();std::string msg;while(s->GetBuffer()->getline(msg, '\n')){std::cout << msg.c_str();if(msg.find("PING ") == 0){std::string reply = msg;reply.replace(1, 1, "O");s->Send(reply);std::cout << reply.c_str();}}}virtual void ON_CLOSE(TCPSocket* s){s->Disconnect();}}; void main() {IRCClient c; TCPSocket s; c.AddSocket(&s, FD_READ | FD_CONNECT | FD_CLOSE); s.Connect("irc.rizon.net", 6667); c.Run();}Please help me improve and write better code! Thanks in advance =][Edited by - djarren on May 18, 2009 12:00:55 AM]
  Sorry for the somewhat useless reply, but a suggestion: either post the source in the post using source tags, or at least use actual links (< a > tag) to the source code pages.   ;  thanks. i've edited. thought i'd post pastes first while i figure out how to use code tags.   ;  Read Q14 of forum FAQ. You cannot receive TCP data the way you currently are.   ;  Thanks for the reminder. If you were referring to the possibilities of messages arriving together, I am aware but have not handled such a case since the responsibility would lie in the client code(which isn't my main concern) to handle the messages according to its protocol.The areas I'm hoping to improve on are:1. more reliable buffers2. more efficient sockets and event objects3. more intuitive class design which I have yet to think of.Nevertheless, thank you for your reply and I'll be sure to fix that when I'm doing a fully featured client based on a more matured socket class.   ;Quote:Original post by djarrenThanks for the reminder. If you were referring to the possibilities of messages arriving together, I am aware but have not handled such a case since the responsibility would lie in the client code(which isn't my main concern) to handle the messages according to its protocol.No, your code is broken. It will not work on WAN, except by pure luck.if(strncmp(Buffer->buf, "PING ", 5) == 0) At this point, you are not guaranteed to have received 5 bytes.TCP is stream protocol. Several messages may arrive as joined, but they may also be split apart, so that you only receive "PIN", there will also be no zero terminating character. So you will compare to PIN$#. On next receive, you will get 'G '. Since you don't check received length, you will be comparing against 'G $#@". This isn't some fancy feature, it's the minimum required to work with TCP.Also make sure that you check each and every socket return code, since a lot of information is returned there.The following will also crash the application. reply has not been allocated.Quote:char* reply;strncpy(reply, Buffer->buf, Buffer->bytes);And there's more. new matches delete, new[] matches delete[].Quote:TCPBuffer::~TCPBuffer(){delete buf;}In your code you are using standard containers. Why not use them for TCPBuffer and related manipulation as well. It would have prevented both of the above bugs.   ;  Remove public members, use const, use initialiser lists, initialise all members (not just some), put implementation in cpp files, use forward declarations, avoid 'protected' keyword, include some way for that while(1) loop to exit, pair new and delete correctly, and do the other things people have suggested :)Hopefully that'll give you a few things to google!   ;  Thank you very much for your kind replies. I will look into this.   ;Quote:the responsibility would lie in the client code(which isn't my main concern) to handle the messages according to its protocolCommunication is symmetric. The server would need to decode the protocol just as much as the client, and data from the client may be chopped/merged just as much as data to the client.;Quote:Original post by hplus0603Quote:the responsibility would lie in the client code(which isn't my main concern) to handle the messages according to its protocolCommunication is symmetric. The server would need to decode the protocol just as much as the client, and data from the client may be chopped/merged just as much as data to the client.Please let me rephrase that.I was referring to the abstract TCPClient class as the Socket code portion, and IRCClient which derives from it but implements IRC functions as the "client code" I mentioned earlier.Because I had thought that the responsibility of deciphering the packet lay in the client specific code, for example every IRC message is \n delimited.However, I do realise what I've done wrong when Antheus pointed out. Thus I'll be doing something like this instead.ON_READappend to bufferwhile getline(line, "\n")->processio(line)clear buffer of processed line   