debugcppModerate
Coding style of critical server's code
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:
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)
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;
};
#endifNet.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
this kind of naming with constants and macros. For C++ types, you should probably use
You must also be consistent with the naming convention used for class methods and global functions. We have three distinct conventions here:
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.
I would also make it a member of the Server class.
I advise against including
header directly in your interface header (
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
The
the Server doesn't care about the linked libraries. This is visual pollution
as it is. Move those to the
Custom throwable type:
Class
from
Exiting on error:
The way you are handling some errors is very crude (calling
C++ has exceptions, which is a much better hay of handling errors.
Also, you are logging errors to
normal program output. For errors, you use STDERR (
You seriously have to rework
inside the
to jump like that inside the
Miscellaneous:
This
a bit unpleasant to read. It feels like if someone forgot to break the line:
Versus:
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 associatethis kind of naming with constants and macros. For C++ types, you should probably use
CamelCase names. E.g.: ServerYou 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 familyheader directly in your interface header (
Net.h). They will exposea 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 ofthe 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 inheritfrom
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 fornormal program output. For errors, you use STDERR (
std::cerr).goto:You seriously have to rework
SERVER::gatherInformation(). That gotoinside the
catch statement is awful. I don't even know if it is safeto jump like that inside the
catch block...Miscellaneous:
This
{ positioning style in the else is not very usual, I find ita 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.