patterncppModerate

First socket class

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

codereview cpp stackoverflow c++windows socket

socketfirstclass

Problem

I want my socket class to be reviewed, including recommendations and optimizations.

Sokect.hpp

```
class Socket {
protected:

Socket(SOCKET s);
Socket();
SOCKET s_;
sockaddr_in sa_;
int* count_;

private:
static void StartSocket();
static void EndSocket();
static int totalNumberSockets_;

public:

virtual ~Socket();
Socket(const Socket&);

std::string RecvData();
void SendData(std::string);

void Close();

};

using namespace std;

//start sockect
void Socket::StartSocket() {
if (!totalNumberSockets_) {
WSADATA info;
if (WSAStartup(MAKEWORD(2, 0), &info)) {
throw "Could not start WSA";
}
}
++totalNumberSockets_;
}

//end end the socket
void Socket::EndSocket() {
WSACleanup();
}

Socket::Socket() : s_(0) {
StartSocket();
s_ = socket(AF_INET, SOCK_STREAM, 0);

if (s_ == INVALID_SOCKET) {
cerr << "Error at Socket(): " << WSAGetLastError() << endl;
throw "INVALID_SOCKET";
}

count_ = new int(1);
}

Socket::Socket(SOCKET s) : s_(s) {
StartSocket();
count_ = new int(1);
};

Socket::~Socket() {
if (!--(*count_)) {
Close();
delete count_;
}
--totalNumberSockets_;
if (!totalNumberSockets_)
EndSocket();
}

Socket::Socket(const Socket& o) {
count_ = o.count_;
(*count_)++;
s_ = o.s_;

totalNumberSockets_++;
}

void Socket::Close() {
closesocket(s_);
}

std::string Socket::RecvData() {
std::string strBuffer;
do{
char buffer;
int recvInt = recv(s_, &buffer, 1, 0);
if (recvInt == INVALID_SOCKET)
{
return "";
}
if (recvInt == SOCKET_ERROR)
{
if (errno == EAGAIN) {
return strBuffer;
}
else {
// not connected anymore
return "";
}
}

Solution

I think you have too many different responsibilities bound up into one class. That ends up not only making that class more complex, but adding extra complexity overall as well. As a starting point, I'd have a really trivial class that does nothing but stack-based management of the WSAStartup/WSACleanup:

struct socket_user {
    socket_user() {
        WSADATA info;
        if (WSAStartup(MAKEWORD(2, 0), &info)) 
            throw "Could not start WSA";       
    }
    ~socket_user() { 
        WSACleanup();
    }
};

Then you can just embed an instance of that into your socket class:

class Socket {
    socket_user s;
    // ...
};

Windows keeps a counter internally, so you're allowed to call WSAStartup repeatedly with matching calls to WSACleanup. No need for extra work on your own part to do that counting over again. So, that trivial class replaces about half the code in the question.

That leaves (mostly) your RecvData and SendData. I think RecvData is open to some improvement as well. Right now, it makes a separate call to recv for each byte of data. That's likely to add a fair amount of overhead. I'd consider passing a somewhat larger buffer to recv so you can receive more data at a time, and spend proportionally less time just jump to/returning from recv. Even increasing the buffer size to 16 bytes is likely to reduce overhead substantially.

I don't really like how you're handling errors either. In particular, if you get a SOCKET_ERROR, you throw away data you've already received and read. If you've already received some data, I'd prefer to see that returned, and let the application decide whether it's really worth keeping or not.

if (recvInt == INVALID_SOCKET)
    {
        return strBuffer;
    }

That brings us to another point: I'd rather see more descriptive names than recvInt and strBuffer. I'd consider something like socket_error and just buffer respectively (odd how removing part of the latter name actually makes the result more meaningful, but there it is).

I'd also like to see the code put into relevant name spaces. For example, those parts that relate to IP in general (e.g., IP addresses) could be an an IP namespace. Those parts specific to TCP or UDP would go in that namespace (which would itself be inside the IP namespace, since UDP and TCP are both IP protocols).

Code might look something like this:

```
#ifndef SOCK2_INCLUDED_
#define SOCK2_INCLUDED_

#include
#include
#include
#include

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

namespace IP {
struct socket_user {
WSADATA data;
socket_user() {
WSAStartup(MAKEWORD(2, 2), &data);
}
~socket_user() {
WSACleanup();
}
};

class address {
socket_user u;
struct sockaddr_in dest;
struct in_addr addr;

hostent *lookup(std::string const &hostname) {
hostent *host;

if (isdigit(hostname[0])) {
addr.s_addr = inet_addr(hostname.c_str());
host = gethostbyaddr((char const *)&addr, sizeof(addr), AF_INET);
}
else
host = gethostbyname(hostname.c_str());
return host;
}

public:

address(std::string const &hostname, short port=80) {
hostent *host = lookup(hostname);

dest.sin_family = AF_INET;
dest.sin_port = htons(port);

addr.S_un.S_un_b.s_b1 = host->h_addr_list[0][0];
addr.S_un.S_un_b.s_b2 = host->h_addr_list[0][1];
addr.S_un.S_un_b.s_b3 = host->h_addr_list[0][2];
addr.S_un.S_un_b.s_b4 = host->h_addr_list[0][3];
dest.sin_addr = addr;
}

address(unsigned char b0, unsigned char b1, unsigned char b2, unsigned char b3, short port)
{
addr.S_un.S_un_b.s_b1 = b0;
addr.S_un.S_un_b.s_b2 = b1;
addr.S_un.S_un_b.s_b3 = b2;
addr.S_un.S_un_b.s_b4 = b3;
dest.sin_family = AF_INET;
dest.sin_port = htons(port);
dest.sin_addr = addr;
}

operator SOCKADDR () const { return (SOCKADDR )&dest; }

size_t size() const { return sizeof(dest); }

friend std::ostream &operator
int setopt(int option, T val) {
return setsockopt(s, SOL_SOCKET, option, (char *) &val, sizeof(val));
}

template
int send(T const &t, address const &a) {
return sendto(s, (char *)&t, sizeof(t), 0, a, a.size());
}

template
void read(address const &a, T &buffer) {
connect(s, a, a.size());
recv(s, (char *)&buffer, sizeof(buffer), 0);
}

~socket() { closesocket(s); }
};
}

namespace TCP {
class socket {
socket_user u;
SOCKET s;
address a;
bool connected;

void connect() {
if (connected)
return;
::connect(s, a, a.size());
connected = true;
}

void disconnect() {
closesocket(s);
connected = false;
}

public:
socket(address const &a_)
: s(::socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)), a(a_), connected(false)

Code Snippets

struct socket_user {
    socket_user() {
        WSADATA info;
        if (WSAStartup(MAKEWORD(2, 0), &info)) 
            throw "Could not start WSA";       
    }
    ~socket_user() { 
        WSACleanup();
    }
};

class Socket {
    socket_user s;
    // ...
};

if (recvInt == INVALID_SOCKET)
    {
        return strBuffer;
    }

#ifndef SOCK2_INCLUDED_
#define SOCK2_INCLUDED_

#include <string>
#include <iostream>
#include <winsock2.h>
#include <algorithm>

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

namespace IP {
struct socket_user { 
    WSADATA data;
    socket_user() {
        WSAStartup(MAKEWORD(2, 2), &data);
    }
    ~socket_user() { 
        WSACleanup();
    }
};

class address {
    socket_user u;
    struct sockaddr_in dest;
    struct in_addr addr;

    hostent *lookup(std::string const &hostname) { 
        hostent *host;

        if (isdigit(hostname[0])) {
            addr.s_addr = inet_addr(hostname.c_str());
            host = gethostbyaddr((char const *)&addr, sizeof(addr), AF_INET);
        }
        else 
            host = gethostbyname(hostname.c_str());
        return host;
    }

public:

    address(std::string const &hostname, short port=80) {
        hostent *host = lookup(hostname);

        dest.sin_family = AF_INET;
        dest.sin_port = htons(port);

        addr.S_un.S_un_b.s_b1 = host->h_addr_list[0][0];
        addr.S_un.S_un_b.s_b2 = host->h_addr_list[0][1];
        addr.S_un.S_un_b.s_b3 = host->h_addr_list[0][2];
        addr.S_un.S_un_b.s_b4 = host->h_addr_list[0][3];
        dest.sin_addr = addr;
    }

    address(unsigned char b0, unsigned char b1, unsigned char b2, unsigned char b3, short port)
    {
        addr.S_un.S_un_b.s_b1 = b0;
        addr.S_un.S_un_b.s_b2 = b1;
        addr.S_un.S_un_b.s_b3 = b2;
        addr.S_un.S_un_b.s_b4 = b3;
        dest.sin_family = AF_INET;
        dest.sin_port = htons(port);
        dest.sin_addr = addr;
    }

    operator SOCKADDR *() const { return (SOCKADDR *)&dest; }

    size_t size() const { return sizeof(dest); }

    friend std::ostream &operator<<(std::ostream &os, address const &a) {
        return os << (short)a.addr.S_un.S_un_b.s_b1
            << "." << (short)a.addr.S_un.S_un_b.s_b2
            << "." << (short)a.addr.S_un.S_un_b.s_b3
            << "." << (short)a.addr.S_un.S_un_b.s_b4;
    }
};

namespace UDP { 
class socket { 
    SOCKET s;
    socket_user u;
public:
    socket() : s(::socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP)) {}

    int setopt(int option, char const *val=NULL, int len = 0) {
        return setsockopt(s, SOL_SOCKET, option, val, len);
    }

    template <class T>
    int setopt(int option, T val) {
        return setsockopt(s, SOL_SOCKET, option, (char *) &val, sizeof(val));
    }

    template <class T>
    int send(T const &t, address const &a) { 
        return sendto(s, (char *)&t, sizeof(t), 0, a, a.size());
    }

    template <class T>
    void read(address const &a, T &buffer) {
        connect(s, a, a.size());
        recv(s, (char *)&buffer, sizeof(buffer), 0);
    }

    ~socket() { closesocket(s); }
};
}

namespace TCP {
class socket { 
    socket_user u;
    SOCKET s;
    address a;
    bool connected;

    void connect() { 
        if (connected) 
            return;
        ::connect(s, a, a.size()); 
        connected = true; 
    }

    void di

#include "sock2.h"
#include <time.h>

#if RFC_868
// TIME (RFC 868) version
int main(){
    IP::address a("time-c.nist.gov", 37);

    IP::TCP::socket s(a);
    DWORD t;
    s.read(t);
    time_t now = htonl(t) - 2208988800L;
    struct tm *n = localtime(&now);
    std::cout << n->tm_year + 1900 << "/" << n->tm_mon << "/" << n->tm_mday
        << "\t" << n->tm_hour << ":" << n->tm_min << ":" << n->tm_sec << "\n";
}

#else
// SNTP (RFC 4330) version
struct ntp_packet {
    // conversion from NTP epoch to Unix/Windows epoch (midnight jan 1, 1900 to midnight jan 1, 1970).
    static const int epoch = (86400U * (365U * 70U + 17U));
    static const int port = 123;
    static const int timeout = 6000;

    unsigned char mode_vn_li;
    unsigned char stratum;
    char poll;
    char precision;

    unsigned long root_delay;
    unsigned long root_dispersion;
    unsigned long reference_identifier;
    unsigned long reference_timestamp_secs;
    unsigned long reference_timestamp_fraq;
    unsigned long originate_timestamp_secs;
    unsigned long originate_timestamp_fraq;
    unsigned long receive_timestamp_seqs;
    unsigned long receive_timestamp_fraq;
    unsigned long transmit_timestamp_secs;
    unsigned long transmit_timestamp_fraq;

    ntp_packet() {
        memset(this, 0, sizeof(*this));
        mode_vn_li = (4 << 3) | 3;
        originate_timestamp_secs = htonl(time(0) + epoch);
    }

    operator time_t() { return ntohl(transmit_timestamp_secs) - epoch; }
};

int main() {
    IP::address a("time-c.nist.gov", ntp_packet::port);
    IP::UDP::socket s;

    int timeout = ntp_packet::timeout;
    s.setopt(SO_RCVTIMEO, timeout);

    ntp_packet packet;
    s.send(packet, a);
    s.read(a, packet);
    time_t now = (time_t) packet;
    struct tm *n = localtime(&now);
    std::cout << n->tm_year + 1900 << "/" << n->tm_mon+1 << "/" << n->tm_mday
        << "\t" << n->tm_hour << ":" << n->tm_min << ":" << n->tm_sec << "\n";
}

#endif

Context

StackExchange Code Review Q#46350, answer score: 11

Revisions (0)

No revisions yet.