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.

Welcome to Code Review! Could you add a bit more description about what your code is doing ? — Marc-Andre
– Marc-Andre, Commented Oct 2, 2014 at 14:33
Not all system call errors are unrecoverable. std::cout << "select() failed with error number "...;exit(-1). Look up EINTR if you get this error code then you can just re-try on most system calls. — Loki Astari
– Loki Astari, Commented Oct 2, 2014 at 18:20
Also select() is an old outdated API (that can be very slow with thousands of connections). You may want to look at pselect() and epoll(). All of these selection mechanisms can be extract by the library libevent which usually picks the most efficient mechanism for your system. — Loki Astari
– Loki Astari, Commented Oct 2, 2014 at 20:49
Just curious to know, why wouldn't you use a library for such a thing ? wouldn't that be more safe/performant ? — Arunmu
– Arunmu, Commented Oct 3, 2014 at 13:21

utnapistim · Accepted Answer · 2014-10-02 16:41:04Z

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.

Thanks for the list of error,just one thing: instead of exit() what should I use? Should I return to the caller function with return ERROR_NUMBER? — Azazel
– Azazel, Commented Oct 2, 2014 at 16:01
May also want to mention that throwing short distance is not that useful an error code would be just as valid. Exceptions are very useful when crossing interface boundaries and the thrower does not have any knowledge of who would catch. Where the thrower knows who will catch the code is usually more succinctly expressed with error codes. — Loki Astari
– Loki Astari, Commented Oct 2, 2014 at 16:31
@user3275699, you should throw an instance of a class derived from std::exception. I usually use throw std::runtime_error{ "error message here" };. — utnapistim
– utnapistim, Commented Oct 2, 2014 at 16:47

Community · Accepted Answer · 2020-06-10 13:24:26Z

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);
}

jmoreno · Accepted Answer · 2014-10-02 14:59:48Z

5

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.

answered Oct 2, 2014 at 14:59

jmoreno

1,4928 silver badges15 bronze badges

Add a comment |

Stack Exchange Network

Coding style of critical server's code

3 Answers 3

First, a quick word on naming:

Macros, includes and pragma:

Custom throwable type:

Exiting on error:

`goto`:

Miscellaneous:

You must log in to answer this question.

Hot Network Questions

Coding style of critical server's code

3 Answers 3

First, a quick word on naming:

Macros, includes and pragma:

Custom throwable type:

Exiting on error:

goto:

Miscellaneous:

You must log in to answer this question.

Related

Hot Network Questions

`goto`: