HiveBrain v1.2.0
Get Started
← Back to all entries
debugcppModerate

Coding style of critical server's code

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
criticalcodestyleservercoding

Problem

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 
    #include 
    #include 
    #include 
    #include 

    #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 fdmax)

Solution

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

Code Snippets

SetUpServer()
gatherInformation()
get_in_addr()
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);
}
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);
}

Context

StackExchange Code Review Q#64539, answer score: 14

Revisions (0)

No revisions yet.