4
\$\begingroup\$

I wrote a simple multithread server to use as a base class for other different types of multithread servers. I haven't had a lot of opportunities to get code critique from other people, so I wanted to post here to see how much coding style was. Any constructive feedback on how to improve my code would be appreciated! I'm personally looking more for code style and quality rather than compilability (since it does compile and work)

define.h

#pragma once

#define DEBUG
#ifndef DEBUG
#define _(x) 
#else
#define _(x) x
#endif

Server.h

#pragma once

#include <iostream>
#include <netinet/in.h>
#include <unistd.h>
#include <string.h>
#include <thread>
#include <map>
#include "define.h"

class Server{
    private:
        virtual void handle_connection(int sockfd);
    public:
        Server() {}
        void initialize(int port);
        void start_listening(int socket);
        void authenticate(int sockfd);
        void sendMessage(int sockfd, std::string prompt);
        ~Server() {}
};

Server.cpp

#include "server.h"

void Server::initialize(int port){
    int sockfd;
    struct sockaddr_in serv_addr;

    //Create socket
    sockfd = socket(AF_INET, SOCK_STREAM, 0);
    if(sockfd < 0){
        perror("Error: Failed to initialize socket.\n");
        exit(1);
    }

    //Bind socket
    bzero((char *) &serv_addr, sizeof(serv_addr));
    serv_addr.sin_family        = AF_INET;
    serv_addr.sin_port      = htons(port);
    serv_addr.sin_addr.s_addr   = INADDR_ANY;
    if(bind(sockfd, (struct sockaddr *) &serv_addr, sizeof(serv_addr)) < 0){
        perror("Error: Failed to bind socket to address.\n");
        exit(1);
    }

    start_listening(sockfd);
    return;
}

void Server::start_listening(int socket){
    while(1){
        int newsockfd = -1;
        struct sockaddr_in cli_addr;
        socklen_t clilen = sizeof(cli_addr);

        listen(socket, 5);
        newsockfd = accept(socket, (struct sockaddr *) &cli_addr, &clilen);

        if(newsockfd < 0){
            perror("Error: Failed to connect to incoming connection.\n");
        }

        //Start new thread to handle request
        std::thread t1 (&Server::handle_connection, this, newsockfd);
        t1.detach();
    }
}

void Server::handle_connection(int sockfd){
    char buffer[256];
    std::string response = "Thank you for your response.\n";
    const char* responseChar = response.c_str();
    authenticate(sockfd);
    while(recv(sockfd, buffer, 255, 0) > 0){
        std::cout << buffer;
        send(sockfd, (void*)responseChar, strlen(responseChar), MSG_NOSIGNAL);
        bzero(buffer, 256);
    }
    close(sockfd);
    return;
}

void Server::sendMessage(int sockfd, std::string prompt){
    const char * promptChar = prompt.c_str();
    send(sockfd, (void*)promptChar, strlen(promptChar), MSG_NOSIGNAL);
}
\$\endgroup\$
  • \$\begingroup\$ What's define.h? \$\endgroup\$ – yuri May 17 '18 at 9:13
  • \$\begingroup\$ It's nothing much! I'll add it in the original post \$\endgroup\$ – OrigamiGamer May 17 '18 at 9:33
1
\$\begingroup\$

Include (only) what you use

Users of Server.h don't need most of those headers; they can be reduced to

#include <string>

You might need some of the other headers in the implementation, but try to avoid polluting users' namespaces (and slowing their compilations).

Consider adding daemonize code

Most servers want to be able to daemonize before they start - provide a facility to do the forking and dissociation of controlling terminal and the rest. Make sure it's optional, so users can run in foreground if they want (e.g. for debugging, or when controlled by a supervisor like systemd).

Report errors back to caller

Consider this code:

    if(newsockfd < 0){
        perror("Error: Failed to connect to incoming connection.\n");
    }

This prints a message to stderr, and then keeps going. How does the server (subclass) know that we failed? What if we want to syslog the failure? It's better to communicate this information in return value or an exception, so the application can take the right action - here, in a library, we don't know what's appropriate, nor whether standard error stream is even available to us.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback! I was told during a basic level programming class that the header file should contain all the libraries necessary for the implementation so that the implementation only contains the .h file, but if it affects compile time then moving unnecessary libraries to the implementation file makes more sense. I'm not very familiar with daemonizing code, would this page be a good guide? \$\endgroup\$ – OrigamiGamer May 17 '18 at 10:53
  • \$\begingroup\$ I think that what you were told was wrong (that does happen, unfortunately). That guide looks mostly correct, but we normally fork again - see this answer to What is the reason for performing a double fork when creating a daemon?. \$\endgroup\$ – Toby Speight May 17 '18 at 10:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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