Windows socket class

I need some advice to see if my simple Windows socket class is good enough to be used in a simple chat application.

```
#include
#include
#include

class Socket
{
private:
WSADATA wsaData;
SOCKET hSocket;
sockaddr_in service;
std::string addr;
USHORT port;
int exitCode;

bool initializeWSA();
bool initializeSocket();
bool bind();
bool listen();
bool connect();

/// Modified copy constructor for accepted connection sockets
Socket(Socket& socket, SOCKET hSockect) : hSocket( hSockect ),
wsaData( socket.wsaData ),
service( socket.service ),
addr( socket.addr ),
port( socket.port ){}
public:
bool close();
Socket* acceptConnection();

/// Default Constructor
Socket()
{}

/// Main Constructor
Socket(std::string addr, USHORT port, bool actAsServer = false) : addr( addr ), port( port )
{
initializeWSA();
initializeSocket();

service.sin_family = AF_INET;
service.sin_addr.s_addr = inet_addr(addr.c_str());
service.sin_port = htons(port);

if( actAsServer )
{
bind();
listen();
}
else
connect();
}

template
int recvData(T& i) {

try
{
int ret = ::recv( hSocket, reinterpret_cast(&i), 32, 0 );
if( ret == SOCKET_ERROR )

throw WSAGetLastError();

return ret;
}
catch(std::exception& e)
{
std::cerr (std::string& i)
{
try
{
int cread;
// receive string size
long length = 0;
cread = ::recv( hSocket, reinterpret_cast(&length), sizeof( length ), 0 );
if( cread == SOCKET_ERROR )

• I'm not excited about the structure of the code. I think you're putting more in one class than is really a good idea.

Just for example, I'd start with a a tiny class that does nothing but handle the WSAStartup/WSACleanup sequence:

class WSAUser { 
    WSADATA data;
public:
    WSAUser() { /* call WSAStartup() */ }
    ~WSAUser() { /* call WSACleanup() */ }
};

...then any other class that's going to use winsock just includes an instance of that object to ensure the WSAStartup and WSACleanup are called appropriately.

• Some of your comments are utterly useless, to the point that the code would be better off without them.

For one example:

/// Default Constructor
Socket()
{}

Comments should not just repeat what's already obvious from the code itself. They should explain things that aren't obvious. For example, if you're using an unusual or non-obvious algorithm, it's often best to explain what algorithm you're using, and possibly why (especially if there's some other algorithm that's likely to seem like a better choice, but for non-obvious reasons isn't).

• Some of your code looks broken

One obvious example:

template
int recvData(T& i) {

    try
    {
        int ret = ::recv( hSocket, reinterpret_cast(&i), 32, 0 );

This is attempting to receive and write 32 bytes of data into the destination object, regardless of that object's size. If that object is smaller than 32 bytes, it'll write past its end--a buffer overrun leading to undefined behavior (not to mention probably security problems).

If you change 32 to sizeof(i), this will at least work for primitive types, but will still produce bad results for most types that contain pointers. Given that it's a template, you're currently promising that it'll work for any possible type. Although it's somewhat more advanced, you might want to do some reading about things like std::enable_if and type-traits that will let you specify the types for which this code is intended to work (and prevent the code from compiling if a type that won't work is passed).

• Misuse of exception handling

At least in my opinion, you're making poor use of exception handling. For example:

try
{
    int ret = ::recv( hSocket, reinterpret_cast(&i), 32, 0 );
    if( ret == SOCKET_ERROR )
        throw   WSAGetLastError();
    return ret;
}
catch(std::exception& e)
{
   std::cerr << "Socket//error while receiving; Error#" << WSAGetLastError() << "." << std::endl;
}

First of all, WSAGetLastError() returns an int, so that's the type this will potentially throw. Since int isn't derived from std::exception, there's probably no way this exception handler can ever be invoked. Even if, however, we fix that problem, this would be a poor use of exception handling. If we really want to print out a message to cerr in case of a problem, that can be done a lot more directly:

int ret = ::recv( hSocket, reinterpret_cast(&i), 32, 0 );
    if( ret == SOCKET_ERROR )
        std::cerr << "Socket//error while receiving; Error#" << WSAGetLastError() << "." << std::endl;

The real win from exception handling stems from the fact that this part of the code doesn't really know how to react to the problem appropriately. Printing to the standard error stream makes sense for some applications, but not at all for others (e.g., in a windowed application, there might not even be a standard error stream).

As such, it may well make sense for this code to throw an exception here--but not catch it or attempt to handle it at all:

int ret = ::recv( hSocket, reinterpret_cast(&i), 32, 0 );
if( ret == SOCKET_ERROR ) {
    std::stringstream buf;
    buf << "Socket//error while receiving; Error#" << WSAGetLastError();
    throw std::runtime_error(buf.str().c_str());
}

...then some code at the application level will catch the exception and display the result appropriately for the type of application in which this is being used (e.g., a server might log it, a client might display it in a message box).

StackExchange Code Review Q#43443, answer score: 11