Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm very new to C++, (7 days ago I wrote "Hello World") but I really want to write good code and increase my development skill, so I ask you for some review.

Scheme of socket server

This is the scheme of how my socket server works. It creates a socket, binds it, sets to listen state, creates a new thread (in order not to block the execution of the program), which accepts new connections, creates new threads for each client, reads data from them and dispatches data to observers (server is a child of Observable class, which allows to dispatch events to Observers.

#include <iostream>
#include <string.h>
#include <stdio.h>
#include <vector>
#include <arpa/inet.h>
#include <sys/socket.h>
#include <pthread.h>
#include "Observer.h"

using namespace std;

class SocketServer : public Observable
{
    private:
        string                          serverIp;
        int                             serverPort;
        int                             masterSocket;

        void                            SocketCreate ( );
        void                            SocketBind ( );
        void                            SocketListen ( );
        static void*                    SocketAccept ( void* );
        static void*                    SocketRead ( void* );

        void                            RequestDispatch ( int , string );

    public:
                                        SocketServer ( string , int );

        static void                     Send ( int , string );
        static void                     Log ( int , string );
};

struct ClientRequest
{
        int                             socketFD;
        string                          request;
};

struct ServerAndSocket
{
        SocketServer*                   socketServer;
        int                             socketFD;
};


        SocketServer::SocketServer ( string serverIp , int serverPort )
{
    this->serverIp = serverIp;
    this->serverPort = serverPort;

    this->SocketCreate();
    this->SocketBind();
    this->SocketListen();

    pthread_create ( new pthread_t() , NULL , SocketServer::SocketAccept, this );
};

void    SocketServer::SocketCreate ( )
{
    this->masterSocket = socket ( AF_INET , SOCK_STREAM , 0 );
    if ( this->masterSocket < 0 )
    {
        perror ( "socket" );
        _exit(0);
    }
    else
    {
        int opt = 1;
        setsockopt (this->masterSocket, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof (opt));
        cout << "Socket created successfully with file descriptor " << this->masterSocket << "\n";
    };
};

void    SocketServer::SocketBind ( )
{
    struct                      sockaddr_in serverAddress;


    serverAddress.sin_family = AF_INET;
    serverAddress.sin_port = htons ( this->serverPort );
    serverAddress.sin_addr.s_addr = inet_addr ( this->serverIp.data());

    if ( bind ( this->masterSocket , (sockaddr*)&serverAddress , sizeof ( serverAddress ) ) < 0 )
    {
        perror ( "bind" );
        _exit(0);
    }
    else
    {
        cout << "Socket bound successfully to the " << this->serverIp << ":" << this->serverPort << " address\n";
    };
};

void    SocketServer::SocketListen ( )
{
    listen ( this->masterSocket , 5 );
    cout << "Socket is beeing listened now\n";
};

void*   SocketServer::SocketAccept ( void* serverPointer )
{
    int                                     socketFD;
    pthread_t*                              clientThread;
    ClientRequest                           clientRequest;
    SocketServer*                           this_;
    ServerAndSocket                         serverAndSocketPtr;

    this_ = (SocketServer*)serverPointer;

    while ( 1 )
    {
        socketFD = accept ( this_->masterSocket , NULL , NULL );
        if ( socketFD < 0 )
        {
            perror ( "accept" );
        }
        else
        {
            this_->RequestDispatch ( socketFD , "CLIENT_CONNECTED" );

            serverAndSocketPtr.socketServer = this_;
            serverAndSocketPtr.socketFD = socketFD;

            pthread_create ( new pthread_t() , NULL , SocketServer::SocketRead, &serverAndSocketPtr );
        };
    };
};

void*   SocketServer::SocketRead ( void* serverAndSocketPointer )
{
    char                        input[256];
    int                         inputLength;
    SocketServer*               socketServerPtr;
    int                         socketFD;
    ServerAndSocket*            serverAndSocketPtr;

    serverAndSocketPtr = (ServerAndSocket*)serverAndSocketPointer;
    socketServerPtr = serverAndSocketPtr->socketServer;
    socketFD = serverAndSocketPtr->socketFD;

    while ( 1 )
    {
        memset ( (void*)&input , '\0' , sizeof ( input ) );
        inputLength = read ( socketFD , (void*)&input , 255 );
        if ( inputLength < 0 )
        {
            perror ( "read" );
        }
        else if ( inputLength == 0 || input[0] == '\0' )
        {
            socketServerPtr->RequestDispatch ( socketFD , "CLIENT_DISCONNECTED" );
            pthread_exit( NULL );
        }
        else
        {
            socketServerPtr->RequestDispatch ( socketFD , input );
        };
    };
};



void    SocketServer::RequestDispatch ( int socketFD , string request  )
{
    struct ClientRequest                clientRequest;

    SocketServer::Log ( socketFD ,  "---> " + request );
    clientRequest.socketFD = socketFD;
    clientRequest.request = request;
    this->DispatchEvent (  (void*)&clientRequest );
};



void    SocketServer::Send ( int socketFD , string message )
{
    int                                 bytesWritten;

    bytesWritten = write ( socketFD , message.c_str() , message.size() + 1 );

    if ( bytesWritten < 0 )
    {
        perror ( "write" );
    }
    else
    {
        SocketServer::Log ( socketFD , "<--- " + message );
    };
};

void    SocketServer::Log ( int socketFD , string message )
{
    sockaddr                            address;
    socklen_t                           addressLength;
    sockaddr_in*                        addressInternet;
    string                              ip;
    int                                 port;

    getpeername ( socketFD , &address , &addressLength );
    addressInternet = (struct sockaddr_in*)&address;

    ip = inet_ntoa ( addressInternet->sin_addr );
    port = addressInternet->sin_port;

    cout << ip << ":" << port << " " <<  message << "\n";
};
share|improve this question
Using a thread per connection is not very scalable. Its OK if your expect your connection count to be in the low teens but after that not so much. Have a look at select() as a starting point epoll() is a natural extension to select() but takes a bit more work. It allows you to have one thread service all connections simultaneously. – Loki Astari Aug 14 '12 at 18:43
you forgot that ports in sockaddr structs are network byte-order, so use ntohs() in the Log() method – hroptatyr Aug 15 '12 at 18:22

Know someone who can answer? Share a link to this question via email, Google+, Twitter, or Facebook.

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Browse other questions tagged or ask your own question.