7
\$\begingroup\$

So, I'm new to socket programming. I wanted to get started learning, and start making stuff, but was really surprised to find out there was no standard C++ network library yet. I'd have to look for a third party library, or make my own.

A third party library search turned up some solutions. Boost::Asio seemed to be the best. However, I wanted something that worked with iostream, similarly to fstream, but for networks. Boost::Asio didn't seem to really support that. Nor did any other library I found. Such a library might exist, but I didn't find it. So I decided to write one.

This, of course, meant I had to learn about making custom iostreams and about networking, 2 things at once. However, I dove in head first and decided to just take it all on. I don't recommend that, but I was up for the challenge.

Overall, I'm quite happy with the result. As far as I can tell, it works perfectly with iostreams, and has an interface similar to fstream, with networking tweaks. Okay, enough introduction.

The design of my Network Streams (as I have called them) is simple. There is a base class NetworkStream which inherits from std::iostream. This class handles all the stream style reading and writing. Most everything else is built off this.

Next is the Client class, which inherits from NetworkStream and handles connecting to a server. Once the connection is set, you can start using it like a regular stream.

Then there is a Server. This basically handles all the listening and has an accept() function that returns an individual connection. Server does not inherit from anything, since it merely serves as the matchmaker.

Finally, there is the ServerSocket class, which basically represents a single connection to a client. This is returned by accept(). The idea being that you have to accept a connection, and then start writing to that connection. Once you have this connection, you can write to it like any old stream.

That's basically all the main classes. There are some custom exceptions thrown in there, but that generally is it.

One thing I should mention about my design: I tried to make it as platform-independent as possible. That sounds crazy, given that socket writing is platform dependent by nature, but fortunately I think I was able to stuff all the platform dependent stuff in its own namespace/file. My thinking was originally that I could write all code platform independent except for some platform-dependent macros. This became messy, as all the macros eventually became spread all over the place, and then I needed to reuse some of them in different files, so there were multiple copies, and such. In the end, I reduced everything down to a simple file that has some inline functions (since macros are generally bad). It made the design much simpler, and, I think, platform independent except for that file.

Things I would like to get reviewed:

  • Style. Make sure my code is up to par style-wise.
  • Platform independency: I know I tried to make it platform independent. However, trying and doing are two different things. I'm only on windows, so I haven't even tried porting to another platform yet. How easy do you think this is to port? Is there something that probably won't port well to a new platform, specifically linux?
  • Thread Safety: I'm on VS2010. As far as I'm aware, VS2010 doesn't have #include <thread> or anything else that works with it like <atomic>. I looked into multi-threading in VS2010, and it doesn't look easy. Thus, I haven't even tried making sure this works well with multi-threading. To do that, I've decided I just need a new compiler that is fully C++11 compliant. In the mean time, I'd like someone with more experience to gauge of how thread-safe this code is, since threaded servers are used in the real world. iostream is supposedly thread-safe, so I'd imagine anything that inherits from it already has some safety built in, but I'm not going to assume that. How safe is this code?
  • Error reporting: I think my current error reporting is adequate, but not great. How can this be improved?
  • Efficiency: Specifically, NetworkStream uses a read size of 30 for its buffer. Ultimately, I'd like to make it adjustable. Even then, I'd need a default, though. So, I'll ask: what is a good reading buffer size for a stream like this? I've heard everything from 255 to like 1028. What is your recommendation?
  • API design: How can this design be improved? Is there anything I should add that most network APIs would support that is missing from here? I want to add UDP, and maybe RAW sockets in the future, but besides that, is there anything that stands out as missing?

Overall, I think it's not bad, for a first pass at least. It does what it's supposed to. It could be more powerful and more flexible, but I think that's a project for the future.

One final thing about the nomenclature before I show my code: "ComputerBytez" is the name of my blog on which I hope to post this stream some day. I'm just putting it in its own namespace since I doubt it will conflict with anything. Plus, once my blog expands (as I'm still really new), having everything encased in a ComputerBytez namespace will just make life easier for readers.

"NetBytez" is just what I decided to call this little project, named after my blog with the stylized "bytez".

Okay, now finally, my code if you're still here:

NetworkStream.h:

#pragma once

#include <iostream>
#include "Platform.h"

// so, this stream runs the basic reading and writing from the network,
// platform independently (except where specified in Platform.h), hopefully

namespace ComputerBytez {
namespace Socket {

    // Special thanks to Cay S. Horstmann, who without this post: http://www.horstmann.com/cpp/iostreams.html
    // I wouldn't be able to make any of this

    class NetworkStream : public std::iostream {
        public: 

            // constructors
            NetworkStream(Platform::Socket socket);
            virtual ~NetworkStream(); 

            // other functions 

            // see if we are connected
            bool Connected();

            // class network streambuff
            // should handle the actual writing
            class NetworkBuff : public std::streambuf {
                public:
                    NetworkBuff(Platform::Socket s);

                    void setSocket (Platform::Socket s);
                    Platform::Socket getSocket ();

                    // to detect connection
                    void setConnected(bool c);
                    bool getConnected();
                protected:
                    // also, shoutout to PanGalactic from http://www.cplusplus.com/forum/general/38408/
                    // that post helped understand what this function is supposed to do
                    virtual std::streamsize xsputn(const char* buffer, std::streamsize size);


                    // Same shoutout as above
                    virtual std::streambuf::int_type underflow();

                private:
                    Platform::Socket socket;
                    std::string inputbuffer; // a buffer for the input data
                    std::string outputbuffer; // a buffer for the output data
                    bool connected;
            }; // end NetworkStream::NetworkBuf

        protected:
            // prevent copying
            NetworkStream(const NetworkStream&);
            void operator=(const NetworkStream&);

            // internal function to set the state of connected if we need to
            void setConnected(bool c);

            // internal functions to manipulate our socket.
            void setSocket(Platform::Socket socket);
            Platform::Socket getSocket();
    }; // end network stream

};
}; // end namespace computerbytez

NetworkStream.cpp:

#include "NetworkStream.h"
#include "Exceptions.h"
#include "Platform.h"

// Platform independent except for everything in Platform.h

namespace ComputerBytez {
namespace Socket {

// class network streambuff
// should handle the actual writing

//constructor
NetworkStream::NetworkBuff::NetworkBuff(Platform::Socket s) {
    socket = s;
    connected = true; // assume we are connected until told otherwise. If we get disconnected, we can log it.
}

// socket setter
void NetworkStream::NetworkBuff::setSocket(Platform::Socket s) {
    socket = s;
}

// socket getter
Platform::Socket NetworkStream::NetworkBuff::getSocket() {
    return socket;
}

// to detect connection
void NetworkStream::NetworkBuff::setConnected(bool c) {
    connected = c;
}
bool NetworkStream::NetworkBuff::getConnected() {
    return connected;
}

std::streamsize NetworkStream::NetworkBuff::xsputn(const char* buffer, std::streamsize size) {
    // well, let's send the data
    int result = Platform::Send(socket,buffer,static_cast<int>(size));

    // set up the buffer:
    outputbuffer = buffer;

    // if that didn't work, throw an error
    if(result == Platform::States::SocketError) {
        if (Platform::LastError() == Platform::States::Disconnect) {
            connected = false;
        }
        throw(Exceptions::SocketIOError("send"));// Platform::LastError()); 
    }
    // NOTE: I realized after I wrote this that this throw may be useless, 
    // since I think iostream catches any errors thrown at this level, but 
    // just in case

    // set up the pointer
    if(outputbuffer.size()==0) {
        // do nothing. Probably throw an error in the future
        //setp(&outputbuffer[0],&outputbuffer[0],&outputbuffer[0]);
    } else {
        setp(&outputbuffer[0],&outputbuffer[0],&outputbuffer[outputbuffer.size()-1]);
    }
    // now return the size
    return size;
}

// Shoutout to http://www.voidcn.com/article/p-vjnlygmc-gy.html where I found out
// how to do this proper
std::streambuf::int_type NetworkStream::NetworkBuff::underflow() {
    const int readsize = 30;

    // first, check to make sure the buffer is not exausted:
    if(gptr() < egptr()) {
        return traits_type::to_int_type(*gptr());
    }

    // Now, let's read...


    // clear the buffer
    inputbuffer.clear();
    inputbuffer.resize(readsize+1); // +1 is to give our selves room to keep from crashing. Not a good solution, but it works, so...

    // let's read
    int bytesread = Platform::Recv(socket,&inputbuffer[0],static_cast<int>(readsize)); 

    // return the end of file if we read no bytes
    if(bytesread == 0) {
        return traits_type::eof();
    }

    if(bytesread <0) {
        // do nothing right now. Throw an error maybe. Maybe return eof. Perhaps this 
        // object should be smart enough to hold its own error state that is more useful
        // so we can detect things like disconnect. For now, we're just gonna return eof
        connected = false;
        return traits_type::eof();
    }
    // set the pointers for the buffer...
    setg(&inputbuffer[0],&inputbuffer[0],&inputbuffer[bytesread]);

    // finally, let's return the data type
    return traits_type::to_int_type(*gptr());

}

// network stream constructors/destructor
NetworkStream::NetworkStream(Platform::Socket socket) : std::iostream(new NetworkBuff(socket)), std::ios(0) {}
NetworkStream::~NetworkStream() {delete rdbuf();}

void NetworkStream::setSocket(Platform::Socket socket) {
    static_cast<NetworkBuff *>(rdbuf())->setSocket(socket);
}

Platform::Socket NetworkStream::getSocket() {
    return static_cast<NetworkBuff *>(rdbuf())->getSocket();
}

void NetworkStream::setConnected(bool c) {
    static_cast<NetworkBuff *>(rdbuf())->setConnected(c);
}

bool NetworkStream::Connected() {
    return static_cast<NetworkBuff *>(rdbuf())->getConnected();
}
}; // end namespace Socket
}; // end namespace ComputerBytez

Platform.h:

/* Basically a way to put all the platform dependent stuff in one place.
 * My hope is that everything that is platform dependent makes its way into this module somehow
 * That way, porting to a new platform is as simple as adding #ifdefs to this file and/or its cpp
 */
#pragma once
// first, the includes
#ifdef _WIN32
    // includes
    #include <WinSock2.h>
    #include <Windows.h>
    #include <WS2tcpip.h>

    // and, of course, we need to link:
    #pragma comment(lib,"Ws2_32.lib")
#else
#error "Unknown target/platform. You must be using this on a platform it's not built for. Please use on a different platform or consider contributing to extend this"
#endif


namespace ComputerBytez {
namespace Socket {
namespace Platform {
    // now the implementation.

    // Since I decided to make these guys inline (thoughts on that?), it didn't make sense to 
    // necessarily create prototypes for them since I have to have the implementation in this header anyway. Also, I'm not
    // sure that the prototypes are platform-independent, so I can't have these outside a platform-dependent block (I think)
    // So, I decided to just make 1 block and put the implementations next to the prototypes. I do recognize, now that I think
    // about it, that it might be nice to still have the prototypes, even if they are platform dependent all in one place
    // So, I'll just ask: does anyone have any input on which is best?

#ifdef _WIN32

    typedef SOCKET Socket;
    typedef struct addrinfo Info;

        namespace States {
        extern long Disconnect;
        extern int SocketError;
        extern unsigned int InvalidSocket;
    }; // end namespace states

    namespace Protocol {
        extern int TCP;
    };

    // init and cleanup
    inline void Init() {
        WSADATA wsaData;
        int result = WSAStartup(MAKEWORD(2,2), &wsaData);
        if(result != 0) throw ("Winsock startup failed");
    }

    inline void Cleanup() {
        WSACleanup();
    }

    // reading and writing
    inline int _stdcall Send(Socket sock, const char * buf, int len) {
        return send(sock,buf,len,0);
    }

    inline int _stdcall Recv(Socket sock, char* buf, int len) {
        return recv(sock,buf,len,0);

    }

    // creating a socket
    inline Socket _stdcall CreateSocket(int af, int type, int protocol) {
        return socket(af,type,protocol);
    }

    // closing the socket
    inline int _stdcall CloseSocket(Socket s) {
        return closesocket(s);
    }

    // client stuff
    inline Info GetClientHints(int protocol) {
        Info hints;
                // first, let's zero out the struct
        ZeroMemory(&hints, sizeof(hints));

        // now, initialize the data we're planning on using
        hints.ai_family = AF_UNSPEC; // for now
        // detect TCP vs UDP
        if(protocol == Protocol::TCP) {
            hints.ai_socktype = SOCK_STREAM;
        }
        // and of course, the protocol will be the same
        hints.ai_protocol = protocol;


        return hints;
    }

    inline int _stdcall Connect(Socket sock, const sockaddr* name, int namelen) {
        return connect(sock,name,namelen);
    }


    // server stuff
    inline Socket _stdcall Accept(Socket s) {
        return accept(s, NULL, NULL);
    }

    inline int _stdcall Listen(Socket s, int back=SOMAXCONN) {
        return listen(s,back);
    }

    inline int _stdcall Bind(Socket s, const sockaddr* name, int namelen) {
        return bind(s,name,namelen);
    }

    inline Info GetServerHints(int protocol) {
        Info hints;
        // first, let's zero out the struct
        ZeroMemory(&hints, sizeof(hints));

        // now, initialize the data we're planning on using
        hints.ai_family = AF_INET; // for now

        // detect TCP vs UDP
        if(protocol == Platform::Protocol::TCP) {
            hints.ai_socktype = SOCK_STREAM;
        }
        // and of course, the protocol will be the same
        hints.ai_protocol = protocol;
        hints.ai_flags = AI_PASSIVE;

        return hints;
    }

    inline INT _stdcall GetInfo(PCSTR NodeName,PCSTR ServiceName,Info* hints,Info** result) {
        return getaddrinfo(NodeName, ServiceName, hints, result);
    }

    inline void _stdcall FreeInfo(Info* i) {
        return freeaddrinfo(i);
    }


    // get error
    inline int _stdcall LastError() {
        return WSAGetLastError();
    }

#else 
#error "Unknown target/platform. You must be using this on a platform it's not built for. Please use on a different platform or consider contributing to extend this"
#endif

}; // end namespace Platform
}; // end namespace Socket
}; // end namespace ComputerBytez

Platform.cpp

/* Basically a way to put all the platform dependent stuff in one place.
 * My hope is that everything that is platform dependent makes its way into this module somehow
 * That way, porting to a new platform is as simple as adding #ifdefs to this file and/or its cpp
 */
#include "Platform.h"

namespace ComputerBytez {
namespace Socket {
namespace Platform {

#ifdef _WIN32

    namespace States {
        long Disconnect = WSAENOTCONN;
        int SocketError = SOCKET_ERROR;
        unsigned int InvalidSocket = INVALID_SOCKET;
    }; // end namespace states

    namespace Protocol {
        int TCP = IPPROTO_TCP;
    };
#endif

}; // end namespace Platform
}; // end namespace Socket
}; // end namespace ComputerBytez

Client.h

#include "NetworkStream.h"
#include "Protocol.h"


namespace ComputerBytez {
namespace Socket {

    class ClientSocket : public NetworkStream {
    public:
        void Connect(std::string addr, std::string p, int protocol = Protocol::TCP);

        void disconnect();

        ClientSocket(std::string addr, std::string por, int pro = Protocol::TCP);

        ~ClientSocket();

    private:
        std::string ServerAddress;
        std::string port;

    }; // end ClientSocket

}; // end namespace socket
}; // end namespace computerbytez

Client.cpp

#include "Client.h"
#include "Exceptions.h"

namespace ComputerBytez {
namespace Socket {
    void ClientSocket::Connect(std::string addr, std::string p, int protocol) {
        ServerAddress = addr;
        port = p;
        Platform::Socket sock = Platform::States::InvalidSocket; // a tmp sock
        int connectresult;

        // so, some setup vars
        Platform::Info *result = NULL; // a result
        Platform::Info hints = Platform::GetClientHints(protocol); // and an initial struct 
        Platform::Info * ptr = NULL; // a loop variable

        // try to get host and port
        connectresult = Platform::GetInfo(ServerAddress.c_str(), port.c_str(), &hints, &result);

        // if we failed, throw an error
        if(connectresult != 0) throw(Exceptions::NoRouteToHost());

        // now, let's attempt to connect
        for(ptr = result; ptr != NULL; ptr = ptr->ai_next) {
            // create a socket
            sock = Platform::CreateSocket(ptr->ai_family,ptr->ai_socktype,ptr->ai_protocol);
            if(sock == Platform::States::InvalidSocket) throw(Exceptions::SocketCreationError());

            // now the actual actual connection attempt
            connectresult = Platform::Connect(sock,ptr->ai_addr,static_cast<int>(ptr->ai_addrlen));

            // if we were unsuccessful
            if(connectresult == Platform::States::SocketError) {
                Platform::CloseSocket(sock);
                sock = Platform::States::InvalidSocket;
                continue;
            }
            break;
        }

        // free some data we created 
        Platform::FreeInfo(result);

        // if the connection was unsuccessful, throw another error
        if(sock == Platform::States::InvalidSocket) throw (Exceptions::ConnectionFailed());

        // if we've made it here, congratulations! We've connected. one final thing before we go, set up the socket so we can read/write
        NetworkStream::setSocket(sock);

        // and know that our connection is sucessful
        setConnected(true);

    }

    void ClientSocket::disconnect() {
        if(Connected() && NetworkStream::getSocket() != Platform::States::InvalidSocket) {
            Platform::CloseSocket(NetworkStream::getSocket());
            // and of course, we're disconnected, so...
            setConnected(false);
        }
    }

    ClientSocket::ClientSocket(std::string addr, std::string por, int pro) : NetworkStream(0) {
        setConnected(false);

        Connect(addr, por, pro);


    }

    ClientSocket::~ClientSocket() {disconnect();}


}; // end namespace socket
}; // end namespace computerbytez

Protocol.h

#pragma once
// Originally designed before Platform::Protocol. It was to initially to do Platform::Protocol's job,
// and hold the platform dependent identification of the protocols.

// After making Platform::Protocol, I liked that everything platform related was in one place, yet I also thought Socket::Protocol
// was more understandable name wise. Plus, I had already used Socket::Protocol in code, and since I didn't want the main level to
// ever see the Platform Namespace unless necessary anyway, I decided to keep both. I moved the implelmentation to Platform::Protocol, and
// basically made this another name for it I suppose I could use namespace Platform::Protocol here instead. That might work. Thoughts?

namespace ComputerBytez {
namespace Socket {
namespace Protocol {
    extern int TCP;
}; // end namespace Protocol
}; // end namespace Socket
}; // end namespace ComputerBytez

Protocol.cpp

#include "Protocol.h"
#include "Platform.h"

// Platform independent except for everything in Platform.h

namespace ComputerBytez {
namespace Socket {
namespace Protocol {
    int TCP = Platform::Protocol::TCP;
}; // end namespace Protocol
}; // end namespace Socket
}; // end namespace ComputerBytez

Server.h

#include "NetworkStream.h"
#include "ServerSocket.h"
#include "Protocol.h"

namespace ComputerBytez {
namespace Socket {
    class Server {
    private:
        // linda, linda, listen to me, listen to me!
        Platform::Socket ListenSocket; // the socket we're listening on
        std::string port;
        bool listening;

        // prevent copying
        // if anyone wants to chime in on how I can allow copying, let me know
        // for now, I'm going to prevent it, so the listen socket doesn't get closed 
        // accidently due to copying and destructors executing when I don't want them
        Server(const Server& other);
        void operator=(Server& other);

    public:
        // contructors and destructors
        Server();
        Server(std::string p, int protocol = Protocol::TCP);

        bool isListening() const;

        void StartListen(std::string p, int protocol = Protocol::TCP);

        void StopListen();

        ~Server();

        ServerSocket AcceptConnection();

    };

}; // end namespace socket
}; // end namespace computerbytez

Server.cpp

#include "Server.h"
#include "Exceptions.h"

namespace ComputerBytez {
namespace Socket {

    bool Server::isListening() const { return listening; }

    void Server::StartListen(std::string p, int protocol) {
        port = p;
        int connectresult;

        Platform::Info *result = 0; // a result
        Platform::Info hints = Platform::GetServerHints(protocol); // and an initial struct 

        // try to get host and port
        connectresult = Platform::GetInfo(NULL, port.c_str(), &hints, &result);

        // if we failed, throw an error
        if(connectresult != 0) throw(Exceptions::NoRouteToHost());

        ListenSocket = Platform::CreateSocket(result->ai_family,result->ai_socktype,result->ai_protocol);

        if(ListenSocket == Platform::States::InvalidSocket) {
            Platform::FreeInfo(result);
            throw(Exceptions::SocketCreationError());
        }

        // now the bind attempt
        connectresult = Platform::Bind(ListenSocket,result->ai_addr,static_cast<int>(result->ai_addrlen));

        // if we were unsuccessful
        if(connectresult == Platform::States::SocketError) {
            Platform::FreeInfo(result);
            Platform::CloseSocket(ListenSocket);
            throw(Exceptions::BindFailed());
        }

        Platform::FreeInfo(result);


        // listen time
        connectresult = Platform::Listen(ListenSocket);

        // if we were unsuccessful
        if(connectresult == Platform::States::SocketError) {
            Platform::CloseSocket(ListenSocket);
            throw(Exceptions::ListenFailed());
        }


        listening = true;
    }

    void Server::StopListen() {
        // only stop listening if we have a valid socket
        if(ListenSocket != Platform::States::InvalidSocket) {
            Platform::CloseSocket(ListenSocket);        
            // now invalidate the socket
            ListenSocket = Platform::States::InvalidSocket;

        }
        // regardless, we are not listening
        listening = false;
    }

    // contructors and destructors
    Server::Server() {
        ListenSocket = Platform::States::InvalidSocket;
        listening = false;
    }

    Server::Server(std::string p, int protocol) {
        ListenSocket = Platform::States::InvalidSocket;
        listening = false;
        StartListen(p,protocol);

    }


    Server::~Server() {
        // stop listening
        StopListen();
    }

    // This is platform independent
    ServerSocket Server::AcceptConnection() {
        Platform::Socket s = Platform::States::InvalidSocket;

        s = Platform::Accept(ListenSocket);

        if(s == Platform::States::InvalidSocket) {
            Platform::CloseSocket(ListenSocket);
            throw (Platform::LastError());
            //throw ("Invalid accept");
        }
        return ServerSocket(s);
    }


}; // end namespace socket
}; // end namespace computerbytez

ServerSocket.h

#include "NetworkStream.h"

// Platform independent except for everything in Platform.h

// this class is so small, I'm not sure if it is worth putting into its own cpp.
// I don't want it to conflict if it's accident;y included multiple times, though, so I may have to
// #pragma once could do the trick. Thoughts?

namespace ComputerBytez {
namespace Socket {
    class ServerSocket : public NetworkStream {
    public:
        ServerSocket(Platform::Socket s) : NetworkStream(s) {

        }

        ~ServerSocket() {
            if(NetworkStream::getSocket() != Platform::States::InvalidSocket) {
                Platform::CloseSocket(NetworkStream::getSocket());
            }
        }
    };

};
}; // end namespace computerbytez

Exceptions.h

#include <exception>
#include <string>

// Platform independent except for everything in Platform.h

#pragma once
// client exceptions
namespace ComputerBytez {
namespace Socket {
namespace Exceptions {
class NoRouteToHost : public std::exception {
    public:
     virtual const char* what() {
         return "Could not resolve host or port";
     }
};

class ConnectionFailed : public std::exception {
    public:
     virtual const char* what() {
         return "Unable to connect to host";
     }
};

class BindFailed : public std::exception {
    public:
     virtual const char* what() {
         return "Bind failed";
     }
};

class ListenFailed : public std::exception {
    public:
     virtual const char* what() {
         return "Listen failed";
     }
};


class SocketCreationError : public std::exception {
    public:
     virtual const char* what() {
         return "Socket creation failed";
     }
};

// io errors
class SocketIOError : public std::exception {
    private:
        std::string type;
    public:
        SocketIOError(std::string t) : type(t) {}

        virtual const char* what() {
            return ("Socket encouneted a " + type + "error from the socket").c_str();
         }
};


}; // end namespace Exceptions
}; // end namespace Socket
}; // end namespace ComputerBytez

NetBytez.h

#include "Server.h"
#include "Client.h"

// Platform independent except for what's in Platform.h


namespace ComputerBytez {
namespace Socket {
    // some intialization functions

    void Init ();


    void Cleanup();
}; // end namespace Socket
}; // end namespace ComputerBytez

namespace NetBytez {
    // NetBytez is a shortcut for the computerbytez socket namespace
    using namespace ComputerBytez::Socket;
};

NetBytez.cpp

#include "NetBytez.h"
#include "Platform.h"

// Platform independent except for what's in Platform.h

namespace ComputerBytez {
namespace Socket {
    // some intialization functions

    void Init () {
        Platform::Init();
    }


    void Cleanup() {
        Platform::Cleanup();
    }

}; // end namespace Socket
}; // end namespace ComputerBytez

Now some testing code:

TestServer.cpp

#include "NetBytez.h"
#include <string>

int main() {
    std::string input = "";

    NetBytez::Init();

try {
    NetBytez::Server server("3500", NetBytez::Protocol::TCP);
    //server.StartListen("3500", NetBytez::Protocol::TCP);

    if(server.isListening()) {
        std::cout << "Listening on port 3500" << "\n";
    }

    auto conn = server.AcceptConnection();
    std::cout << "Accepted connection on port 3500" << "\n";
//  system("pause");

    while(conn.Connected()) {
        input.clear();

        // read input from the connection
        getline(conn, input);

        std::cout << "Recieved data. Echoing..." << "\n";
        std::cout << "Input: \"" << input << "\"\n";

        // echo it back
        conn << input + '\n';

//      std::cout << "Input: \"" << input << "\"\n";
    }

} catch (char * exception) {
    std::cout << "Error! \"" << exception << "\"\n";
} catch (int exception) {
    std::cout << "Error code: \"" << exception << "\"\n";
}

    NetBytez::Cleanup();

    system("pause");

    return 0;
}

TestClient.cpp

#include <iostream>
#include <string>

#include "NetBytez.h"
using namespace std;

int main() {
    NetBytez::Init();

    NetBytez::ClientSocket client("127.0.0.1","3500");
    std::string input;

    while(true) {
        input.clear();
        getline(cin,input);
        client << input + '\n';
//      input = "";
        getline(client, input);

        cout << "Got response \"" << input << "\"\n";

    }

    NetBytez::Cleanup();

    return 0;
}

This is a minimal client and server that simply echo data back and forth to make sure that the streams work as intended.

Anyway, after posting this, it made me realize just how much code I have. If anyone has reduction suggestions, that would be appreciated too!

\$\endgroup\$
2
  • \$\begingroup\$ If you can upgrade to at least VS 2015, VS 2010 doesn't even fully support C++11. \$\endgroup\$
    – pacmaninbw
    Jun 20, 2019 at 12:23
  • \$\begingroup\$ Yeah. It supports enough to do most C++11 things. Like I said, I'm gonna get a new compiler eventually \$\endgroup\$
    – Chipster
    Jun 20, 2019 at 13:18

2 Answers 2

2
\$\begingroup\$

Naming things

There are several ways in which you can improve the way you name things. For example, don't do unnecessary abbreviations. Why NetworkBuff instead of NetworkBuffer? If you do want to abbreviate, make sure you do it the same way as other things do. For example, as you've already seen, the standard library provides std::streambuf, so NetworkBuf might be a better choice.

Also avoid repeating yourself. For example, NetworkStream::NetworkBuff has Network in it twice, and similarly there is Socket::Platform::Socket, Socket::ClientSocket and so on. You use namespaces a lot, maybe some, like Socket, are just not necessary. Or alternatively, rename Socket to Network, and rename NetworkStream to Stream.

Also try to be consistent. Consider these two member functions of ClientSocket:

void Connect(std::string addr, std::string p, int protocol = Protocol::TCP);
ClientSocket(std::string addr, std::string por, int pro = Protocol::TCP);

Why is the port named por in the constructor, but p in Connect()? Why protocol and pro? Again, don't needlessly abbreviate, as it is hard to see from just p that you mean port, and having both por and pro is confusing and a potential source of mistakes.

Also be consistent with the formatting of names. You have some member variables using PascalCase, others use flatcase.

Use enums where appropriate

Use enum or even better, enum class, where appropriate. For example, instead of:

namespace Protocol {
    int TCP = IPPROTO_TCP;
};

Write:

enum class Protocol: int {
    TCP = IPPROTO_TCP,
};

This is will allow the compiler to be much stricter in checking that you don't accidentily pass a value that is not one of the valid enum choices. The only drawback is that sometimes you have to explicitly cast such a variable back to the underlying integer type.

Pass large objects by const reference where appropriate

C++ normally passes parameters by value, which means it has to make copies. For small parameters like ints, that's totally fine, but for something like std::string it would mean it will allocate memory for a new string, and then copy the string. That is inefficient, which is why you should consider passing them via const references instead. For example:

void ClientSocket::Connect(const std::string &addr, const std::string &port, Protocol protocol) {
    ...
}

Unnecessary and dangerous casts

Why is readsize explicitly cast to an int in NetworkStream::NetworkBuff::underflow(), when it is already an int? That cast is not necessary. Then there is ptr->ai_addrlen being cast to an int in ClientSocket::Connect(). A socklen_t might be larger than an int, in which case this cast could silently truncate the size, or perhaps even turn it into something negative. While it is probably fine here (after all, who would expect addrlen to be larger than a few tens of bytes?), casts like these are subtle sources of security bugs. Try to find a way to avoid these casts, and if you really need a cast, consider always casting to a larger type if possible.

\$\endgroup\$
2
\$\begingroup\$

I tried to compile it in Visual Studio 2019. I got several warnings and 1 error.

I would recommend the following change to the SocketIOError class because the what function caused a warning:

class SocketIOError : public std::exception {
    private:
        std::string type;
        std::string msg;
    public:
        SocketIOError(std::string t) : type(t) {}

        virtual const char* what()
        {
            msg = "Socket encouneted a " + type + "error from the socket";
            return msg.c_str();
        }
};

I would also recommend to change the type of InvalidSocket from unsigned int to Socket. That way it doesn't generate a warning when compiled for 64 bits.

In the function Server::AcceptConnection() I get an delete function error (E1776) on the statement

return ServerSocket(s);

At the moment I don't know what to do about that but I think it is caused by the NetworkStream class that prevents copying.

\$\endgroup\$
2
  • \$\begingroup\$ It was clearly mentioned that the code was written using Visual Studio 2010. That limits it to C++11. \$\endgroup\$
    – pacmaninbw
    May 30, 2021 at 14:35
  • \$\begingroup\$ Also, copying an exception should not ever throw. \$\endgroup\$ May 30, 2021 at 15:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.