Coding style of critical server's code

Question

I was writing a server that should serve more or less a thousand clients.

After writing the net part of the server code I had to stop writing because I saw that the code was chaotic and not very readable. The code work, but it seems all wrong.

Net.h:

 #ifndef H_GUARD_NET
    #define H_GUARD_NET

    #define WIN32_LEAN_AND_MEAN

    #include <WinSock2.h>
    #include <Windows.h>
    #include <WS2tcpip.h>
    #include <iostream>
    #include <WinBase.h>

    #pragma comment(lib,"kernel32.lib");
    #pragma comment(lib,"Ws2_32.lib");


    #define INIT_PORT_NUMBER "64390"
    /*Wrapper class around Server function */

    class SERVER
    {
    public:
        SERVER();
    private:
        int SetUpServer();
        void gatherInformation(addrinfo** hints);
        void * get_in_addr(sockaddr* sa);

        ~SERVER(){}

    };

/*It's supposed to handle error */
    class ERR
    {
    public:

        ERR(DWORD& val) : ERR_NO(val) {}

        int GetError() {return ERR_NO;}



        ~ERR() {}

    private:
        const int ERR_NO;
    };

    #endif

Net.cpp:

 #include "Net.h"

/*Return the sin_addr of the sockaddr struct,
*probably not very windows-like because i took it
*from an old c++ UNIX code */

    void * SERVER::get_in_addr(sockaddr* sa)
    {
        if(sa->sa_family == AF_INET)
        {
            return &(((sockaddr_in*)sa)->sin_addr);
        }else if(sa->sa_family == AF_INET6)
        {
            return &(((sockaddr_in6*)sa)->sin6_addr);
        }
    }


    SERVER::SERVER()
    {
/*return the listener socket from the SetUpServer()
*function */

        int listener = SERVER::SetUpServer();

        sockaddr_storage remoteAddr;
        socklen_t socketlength = sizeof(remoteAddr);

        char buffer[4096];
        memset(&buffer,0,sizeof(buffer));

        int fdmax,nByte;
        fd_set master,read_fd;

        FD_ZERO(&master);
        FD_ZERO(&read_fd);

        char remoteIP[INET6_ADDRSTRLEN];


        if(listen(listener,10) == SOCKET_ERROR)
        {
            std::cout << "listen() failed  with error number " << WSAGetLastError() <<std::endl;
            exit(-1);
        }

        fdmax = listener;

        FD_SET(listener,&master);

/*endless loop to catch the connection and handle them with select() */
        for(;;)
        {
            read_fd = master;

            if(select(fdmax +1,&read_fd,NULL,NULL,NULL)  == SOCKET_ERROR)
            {
                std::cout << "select() failed with error number " << WSAGetLastError() <<std::endl;
                exit(-1);
            }

            for(int i = 0; i <= fdmax; i++)
            {
                if(FD_ISSET(i,&read_fd))
                {
                    if(i == listener)
                    {
                        int newSock = accept(listener,(sockaddr *)&remoteAddr,&socketlength);

                        if(newSock == SOCKET_ERROR)
                        {
                            std::cout << "accept() failed with error number " << WSAGetLastError() << std::endl;
                        }

                        std::cout << "New connection from: " << inet_ntop(remoteAddr.ss_family,get_in_addr((sockaddr*)&remoteAddr),remoteIP,INET6_ADDRSTRLEN) <<std::endl;

                        FD_SET(newSock,&master);

                        if(newSock > fdmax)
                            fdmax = newSock;
                    }else
                    {

/*The server doesn't just recv(),but temporany i use it for
* debugging purpose */
                        if ((nByte = recv(i,buffer,sizeof(buffer),0)) == ERROR)
                        {
                            std::cout << "recv() failed with error number "<<WSAGetLastError() <<std::endl;
                            exit(-1);
                        }else if(nByte == 0)
                        {
                            std::cout << "Connection closed by host: " << i << std::endl;
                            closesocket(i);
                            FD_CLR(i,&master);
                        }else
                        {
                            std::cout << buffer << std::endl;
                            memset(&buffer,0,sizeof(buffer));
                        }
                    }
                }
            }
        }

    }
    /*Do all the Networking stuff */
    int SERVER::SetUpServer()
    {
        addrinfo *result = NULL;
        SOCKET listener = 0;
        const char yes = 1;
        WSAData data;

        WSAStartup(MAKEWORD(2,2),&data);

/*Gather info about the host machine
*and write them inside the result linked list */

        SERVER::gatherInformation(&result);

        for(;result != NULL; result = result->ai_next)
        {
            if((listener = socket(result->ai_family,result->ai_socktype,result->ai_protocol)) == INVALID_SOCKET)
            {
                std::cout << "Socket () failed with error number: " << WSAGetLastError() << std::endl;
            }

            if(setsockopt(listener, SOL_SOCKET, SO_REUSEADDR,&yes,sizeof(int)) == ERROR)
            {
                std::cout << "Setsockopt() failed with error number:" << WSAGetLastError() <<std::endl;
                freeaddrinfo(result);
                exit(-1);
            }
            if(bind(listener,result->ai_addr,result->ai_addrlen) < 0)
            {
                continue;
            }
            return listener;
        }
    }

/*Wrapper around getaddrinfo, i think it should improve readability
*but i think that I just messed up more the code */

    void SERVER::gatherInformation(addrinfo ** result)
    {
        addrinfo hints;
        ZeroMemory(&hints,sizeof(hints));
        hints.ai_flags = AI_PASSIVE;
        hints.ai_family = AF_UNSPEC;

    retry:
        try
        {
            if(getaddrinfo(NULL,INIT_PORT_NUMBER,&hints,result) < 0)
            {
                DWORD error = GetLastError();
                throw new ERR(error);
            }
            freeaddrinfo(&hints);

        }catch(ERR& e)
        {
            if(e.GetError() == WSATRY_AGAIN)
            {
                /* Handling this here because
                * it's just a temporany error so i'll wait 10 second and then retry */
                Sleep(10000);
                goto retry;
            }else
            {
           /*Using another function 
           * to parse non-handled error*/
           std::cout << "getaddrinfo() failed with error number: " <<std::endl;
           freeaddrinfo(*result);
            }
        }

    }

I wonder how I can improve the code quality, also because this will be a long project (probably on the 10-20k LOC) so it's essential that every part is the most synthetic and neat as possible.

Answer 1

There's lots of place for improvement.

retry: // (1)
try
{
    if(getaddrinfo(NULL,INIT_PORT_NUMBER,&hints,result) < 0)
    {
        DWORD error = GetLastError();
        throw new ERR(error); // (2)
    }
    freeaddrinfo(&hints);

}catch(ERR& e) // (3)

First (1), don't use goto in C++. There are (arguably) places where it improves things, but in this case, you can simply replace it with a while (or do) loop.

Second (2, 3), you are throwing a dynamically allocated ERR instance, which is bad (because it won't get caught) and unnecessary. The code should be throw ERR(error);.

Third (3), you should catch a const exception reference, and it would probably be beneficial to make ERR a specialization of std::runtime_error.

void * SERVER::get_in_addr(sockaddr* sa)
{
    if(sa->sa_family == AF_INET)
    {
        return &(((sockaddr_in*)sa)->sin_addr);
    }else if(sa->sa_family == AF_INET6)
    {
        return &(((sockaddr_in6*)sa)->sin6_addr);
    }
}

This API returns a void*, which is among the worst possible ways of returning data in C++ (you are removing type information from the result). In order for client code to know what is actually returned here, you have to look at the implementation (a big no-no), and the runtime configuration on the system (this is even worse).

Consider returning a custom structure that holds the information (preferably as a string, or something that does it's own memory management).

Stop using C-style casts. They introduce points that are inflexible to change and difficult to find (a nightmare for maintenance and extension of the functionality).

#define INIT_PORT_NUMBER "64390"

Consider declaring constants with static const auto instead of #define. If you choose to add a namespace around the code later, the #define will not play well with that.

That said, the listening port should be passed as a parameter to the server constructor. This will allow you to start multiple servers, or at least decide in client code, on what port your server instance should listen to.

if(setsockopt(listener, SOL_SOCKET, SO_REUSEADDR,&yes,sizeof(int)) == ERROR)
{
    std::cout << "Setsockopt() failed with error number:" << WSAGetLastError()
        <<std::endl;
    freeaddrinfo(result);
    exit(-1);
}

Do not report errors to the console: it may be that you want (in some cases) to ignore (or silence) errors. This can only be decided from client code. In other words, in the place where the error occurs you do not have enough information to fix it. You should throw an exception here, and in client code (when you catch it) decide how to handle it.

NEVER EVER EVER use exit in library code (unless you have a scenario that really requires it). In this case, if you get an error, it doesn't matter the state of client code, the application will exit (no resources released, no stack unwinding, no ... anything).

void SERVER::gatherInformation(addrinfo ** result)

This should be:

addrinfo SERVER::gatherInformation();

It should populate a result and return by value.

Other issues:

SERVER class interface is ambiguous (actually, you have no public interface). What can you do with a server? Does it always start listening?

SERVER constructor does too many things. If it fails to run all operations, the object is not fully constructed and you have resource leaks. A constructor, ideally, should only receive fully constructed values and set them as field members. Aditionally, a constructor should (ideally) perform no computations and be as close as possible to code that doesn't fail.

If you have computations to perform for initializing the values to be passed in an object instance, these computations should be performed in a factory function.

Start using RAII (for example, the call to freeaddrinfo should be placed in an object's destructor).

Note: this list is by no means complete.

Answer 2

There are quite a few points of improvement in this code. Networking is not my speciality, but I can give you several suggestions on code improvement.

First, a quick word on naming:

Type names in ALL_CAPS (i.e.: SERVER) are a bad choice. Most programmers associate this kind of naming with constants and macros. For C++ types, you should probably use CamelCase names. E.g.: Server

You must also be consistent with the naming convention used for class methods and global functions. We have three distinct conventions here:

SetUpServer()
gatherInformation()
get_in_addr()

Very confusing. Chose one style and use it thoroughly. Consistency is very important to ease the understanding of the code.

Macros, includes and pragma:

Don't use macros to declare constants. C++ has much better ways. INIT_PORT_NUMBER should be either a const char[] or a const std::string. I would also make it a member of the Server class.

I advise against including windows.h and all the other Windows family header directly in your interface header (Net.h). They will expose a ton of junk into the global namespace. (including the infamous min/max macros that conflict with the standard C++ library). Avoid including those in a header file if you can. In this case, you can. So move all those Win* headers to the .cpp.

The #pragmas are also not proper to be placed in a header file. The user of the Server doesn't care about the linked libraries. This is visual pollution as it is. Move those to the .cpp as well.

Custom throwable type:

Class ERR is a throwable type (an exception type). It should then inherit from std::exception. Also, a more descriptive name would be in order. NetError perhaps, but you can be even more specific and have different exception types for different error categories.

Exiting on error:

The way you are handling some errors is very crude (calling exit()). C++ has exceptions, which is a much better hay of handling errors.

Also, you are logging errors to cout. This is inadequate. cout is for normal program output. For errors, you use STDERR (std::cerr).

goto:

You seriously have to rework SERVER::gatherInformation(). That goto inside the catch statement is awful. I don't even know if it is safe to jump like that inside the catch block...

Miscellaneous:

This { positioning style in the else is not very usual, I find it a bit unpleasant to read. It feels like if someone forgot to break the line:

if(sa->sa_family == AF_INET)
{
    return &(((sockaddr_in*)sa)->sin_addr);
}else if(sa->sa_family == AF_INET6)
{
    return &(((sockaddr_in6*)sa)->sin6_addr);
}

Versus:

if(sa->sa_family == AF_INET)
{
    return &(((sockaddr_in*)sa)->sin_addr);
}
else if(sa->sa_family == AF_INET6)
{
    return &(((sockaddr_in6*)sa)->sin6_addr);
}

Answer 3

I see one thing I would consider a minor quirk: the usage of else if when the if includes a return, exit, break or continue statement.

I expect else if statements to be used when you don't want both branches of the code, not when both branches of the code cannot run. Changing those else if statements to just bare if statements will allow you to seperate them from the prior condition and, IMO, make the code a bit more readable.