HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Socket class for use in web server

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
classforwebserverusesocket

Problem

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
#include
#include
#include
#include
#include
#include

#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(buff), bufferSize, 0);
}
int RecvData(SOCKET S,void* buff,int bufferSize){
return recv(S, reinterpret_cast(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(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(WSASt

Solution

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.

Context

StackExchange Code Review Q#44928, answer score: 5

Revisions (0)

No revisions yet.