Socket wrapper class

Question

I've created this thin wrapper class for a socket project that I'm working on. Can someone review my class and give me some points on if I'm on right track?

#include <iostream>
#include <WinSock2.h>


const int MAX_RECV_LEN = 8096;
const int MAX_MSG_LEN= 1024;
const int PORT_NUM =  1200;

class Sokect
{

private:
    Sokect(){}
    void setSocketID(int socketFb){socketID = socketFb;}



    int portNum;
    int socketID;
    int blockFlag;
    int bindFlag; 

public:
    Sokect(int);

    ~Sokect(){
        closesocket(socketID);
    }



    void setReuseAddr(int);
    void getReuseAddr(int);
    void setKeepAlive(int);
    void getKeepAlive(int);
    void setLingerOnOff(bool);
    void getLingerOnOff(bool);
    void setLingerSecond(int);
    void getLingerSecond(int);
    void setSocketBlockFlag(int);
    void getSocketBlockFlag(int);



    //size of send & Receive Buffer
    void  setSendBufferSize(int);
    void  getSendBufferSize(int);
    void  setReceiveBuffferSize(int);
    void  getReceiveBuffferSize(int);

    int getSocketID() {return socketID;}
    int getPortNum() {return portNum;}


    friend ostream& operator<<(ostream&,mySocket&);

    //get systems error  
    void detectErrorOpenWinSocket(int*,string&);
    void detectErrorSetSocketOption(int*,string&);
    void detectErrorGetSocketOption(int*,string&);

};

Answer 1

There are at least a few things I think I'd do differently, at any rate.

const int MAX_RECV_LEN = 8096;
const int MAX_MSG_LEN= 1024;
const int PORT_NUM =  1200;

The ALL_CAPS convention was originally invented for macros. At least as far as I can tell, it was originally for function-like macros, to give a subtle warning that it might evaluate its argument more than once, so you needed to be careful about exactly how and where you applied it (especially to arguments that might have side effects).

It really doesn't make sense for a situation like this, so I'd prefer to avoid it. Regardless of the exact names you choose, it's probably better to avoid putting these variables in the global namespace any way. I'd consider wrapping the entire class in a namespace instead.

private:
    Sokect(){}

Assuming your compiler is recent enough to support it, I'd prefer to use =delete; to ensure that a ctor can't be accessed, rather than defining it as private.

[ ... ]

void setReuseAddr(int);
void getReuseAddr(int);
void setKeepAlive(int);
void getKeepAlive(int);
void setLingerOnOff(bool);
void getLingerOnOff(bool);
void setLingerSecond(int);
void getLingerSecond(int);
void setSocketBlockFlag(int);
void getSocketBlockFlag(int);

I think rather than embedding these directly into the socket itself, I'd create a separate socket_options (or something similar) struct to hold them. This would allow (for example) storing an entire set of options together, so you can set all the options for a particular socket at once.

//size of send & Receive Buffer
void  setSendBufferSize(int);
void  getSendBufferSize(int);
void  setReceiveBuffferSize(int);
void  getReceiveBuffferSize(int);

Likewise, I'd probably create a small class just to handle the buffering. Then the socket class could just contain two buffer instances, one for receiving and one for sending.

//get systems error  
void detectErrorOpenWinSocket(int*,string&);
void detectErrorSetSocketOption(int*,string&);
void detectErrorGetSocketOption(int*,string&);

These seem to me like they merit at least a little more explanation of what they're actually supposed to be/do.

Answer 2

1. The class name is misspelled - should be Socket rather than Sokect

2. Consider putting your constant definitions as static members into the class like this:

   class Socket
   {
       private:
           static const int MAX_RECV_LEN = 8096;
           static const int MAX_MSG_LEN = 1024;
           static const int PORT_NUM = 1200;
       ....
   }
   

3. You haven't shown the actual implementation but my suspicion is that PORT_NUM should probably be renamed to DEFAULT_PORT_NUM - I assume the user will be able configure the port.

4. I'm not sure what the point is to make the default constructor private in your case. You have declared a constructor which takes an int as parameter - this is enough to prevent the compiler from generating the default constructor in the first place so you can just leave it out (assuming your intention is to prevent a user of your class calling the default constructor).

Answer 3

Your public single-argument constructor Sokect(int); absolutely must be marked explicit unless you wish to allow passing int values where Sokect objects are expected. See some of these answers for more detail on how this works.

Answer 4

It's not clear what the int parameter is/means, which is passed to the constructor: is it a port number, a socket handle, or what?

The class seems to be missing some useful functions: including listen, accept, connect, send, and recv.

I'm not sure why you have 3 detectError methods: I'd get they're all wrappers around the same WSAGetLastError API.

I don't see how void getSendBufferSize(int) can get the size: the int needs to be the return code, or a reference parameter; and perhaps it should be size_t not int.

Maybe your 'set option' functions should return bool if they can fail; if they don't return bool then how is the user to know whether they should call the detectError method?