Socket class for use in web server

Question

I've created this socket class to use in my web server application. I have two classes: one for the socket and the other for the webserver.

//#define WIN32_LEAN_AND_MEAN
#include <iostream>
#include <string>
#include <Winsock2.h>
#include <Windows.h>
#include <Ws2tcpip.h>
#include <stdexcept>
#include <stdio.h>

#pragma comment(lib,"ws2_32")
using namespace std;
#define WM_SOCKET 0x10000

class Socket 
{
private:
    HWND WindowHandle;
    SOCKET  hSocket;
    short   port;//port number 
    string  addr; //address
    WSADATA wsaData;
    bool vlisten;
    bool init;
    bool async;



public:

    Socket() {}
    Socket(short port, std::string addr, bool vlisten , HWND WindowHandle, bool async);
    ~Socket() { Close(); }
    int RecvData(void* buff, int bufferSize){
        return recv(hSocket, reinterpret_cast<char*>(buff), bufferSize, 0);
    }
    int RecvData(SOCKET S,void* buff,int bufferSize){
        return recv(S, reinterpret_cast<char*>(buff), bufferSize, 0);
    }
    int SendData(void* buff, int bufferSize){
        return send(hSocket,0);
    }


    int SendData(SOCKET S,void* buff,int bufferSize)
    {
        return send(S, reinterpret_cast<char*>(buff), bufferSize, 0);
    }

    bool Connect(short port,std::string addr,bool vlisten,HWND WindowHandle,bool async);
    //SOCKET Accept(sockaddr* clientInfo,int* clientInfoSize)
    SOCKET Accept()
    {
        static int size = sizeof(sockaddr);
        return accept(this->hSocket, 0,0);
    }
    SOCKET GetSocket() const{return this->hSocket;}
    void Close()
    {
        if (hSocket)
        {
            shutdown(hSocket,SD_BOTH);
            closesocket(hSocket);
            hSocket = 0;
        }
        if(init)
        {
            WSACleanup();
        }
    }

    bool Connect(short port,std::string addr,bool vlisten,HWND WindowHandle,WSADATA& wsaData,bool async)
    {
        if(!hSocket)
        {
            this->port = port;
            t
            this->wsaData =wsaData;
            this->init = true;


            struct sockaddr_in* sockaddr_ipv4;

            if(WSAStartup(MAKEWORD(2,2),&wsaData) !=0)
            {
                throw runtime_error("Error WSAStartup:" + WSAGetLastError());
            }

            if((this->hSocket = ::socket(AF_INET,SOCK_STREAM,IPPROTO_TCP))== INVALID_SOCKET)
            {
                Close();
                throw runtime_error("Error init sockect:" + WSAGetLastError());
            }

            if(addr != "INADDR_ANY")
            {
                struct addrinfo *result = nullptr;

                struct addrinfo *it;
                for (it = result; it != nullptr; it = it->ai_next)
                {
                    struct addrinfo *result = nullptr;
                    getaddrinfo(addr.c_str(), nullptr, nullptr, &result);
                    struct addrinfo* it;
                    for (it = result; it != nullptr; it = it->ai_next)
                    {
                        sockaddr_ipv4 = reinterpret_cast<sockaddr_in*>(it->ai_addr);
                        addr = inet_ntoa(sockaddr_ipv4->sin_addr);
                        if (addr != "0.0.0.0") break;
                    }
                    freeaddrinfo(result);
                }
            }
            SOCKADDR_IN sockAddrIn;
            memset(&sockAddrIn,0,sizeof(sockAddrIn));
            sockAddrIn.sin_port = htons(port);
            sockAddrIn.sin_family =  AF_INET;
            sockAddrIn.sin_addr.s_addr = (addr == "INADDR_ANY" ? htonl(INADDR_ANY) : inet_addr(addr.c_str()));

            if(vlisten && (bind(hSocket,reinterpret_cast<SOCKADDR*>(&sockAddrIn),sizeof(sockAddrIn))== SOCKET_ERROR))
            {
                Close();
                throw runtime_error("Error vlisten & bind: " + WSAGetLastError());
            }

            if(async && WindowHandle)
            {
                if(WSAAsyncSelect(hSocket,WindowHandle,WM_SOCKET,FD_READ|FD_WRITE|FD_CONNECT|FD_CLOSE|FD_ACCEPT) !=0)
                {
                    Close();
                    throw runtime_error("Error async & WindowHandle: " + WSAGetLastError());
                }

            }

            if(vlisten && (listen(hSocket,SOMAXCONN)== SOCKET_ERROR))
            {
                Close();
                throw runtime_error("Error async & WindowHandle: " + WSAGetLastError());
            }

            if(!vlisten && (connect(hSocket, reinterpret_cast<SOCKADDR*>(&sockAddrIn), sizeof(sockAddrIn)) == SOCKET_ERROR))
            {
                if(async && WindowHandle && (WSAGetLastError() != WSAEWOULDBLOCK))
                {
                    Close();
                    throw runtime_error("Error async & WindowHandle: " + WSAGetLastError());
                }
            }
        }
    }
    void SendLine(string str)
    {
        SendData(str,str.size());
    }

}; 

Answer 1

Code feedback:

This should not be there:

#include <stdio.h>

If you really want stdio (normally you shouldn't), you should probably include <cstdio> instead.

This should never be in a header file:

using namespace std;

This should be a static const:

#define WM_SOCKET 0x10000

Why do you have these two functions?

int RecvData(SOCKET S,void* buff,int bufferSize) { ... }
int SendData(SOCKET S,void* buff,int bufferSize) { ... }

The methods of the class should work on their own data, not on S.

These should be RAII:

SOCKET  hSocket;
WSADATA wsaData;

The calls to Close and throw std::runtime_error should not be repeated. Consider creating an aditional function (Socket::ThrowRuntimeError) that also calls Close, or placing the code in a try-catch and calling Close when catching an exception.

Answer 2

You use "short port" in several places, but a port number should be an "unsigned short".

You're calling Close() all over the place before throwing an exception. It smells like duplicate code. Consider catching the exception then freeing the resources.

Instead of manually freeing resources, consider following the RAII pattern. Encapsulate any resource that might leak (In your case WSAStartup() needs to be offset with a call to WSACleanup(), and an open socket needs to be closed.) That way when the resource goes out of scope it cleans itself up, even when the code throws an exception.

Connect() is doing an awful lot of work. It's a long method full of code, and its cyclomatic complexity is very high; my eyeballs put it over 20 (according to cccc it's 29.) Consider breaking it up.

One way to break it up would be to have two versions of connect: ConnectAndListen() or Connect(). That way you don't have a flag controlling behavior, your client simply calls the version that it needs.

And async and WindowHandle appear to go together like hands and gloves: do you really need the 'async' flag when the existence of a non-null WindowHandle tells it how to behave? That also adds complexity. If you have two versions of Connect() and ConnectAndListen(), one accepting a WindowHandle and one not, you could completely control that behavior without an if statement.