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

TCP Socket Wrapper

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

Problem

I'm trying to build a simple server software for training purpose, most likely a IRC server, but I'm not there yet.

I'm currently implementing a TCP socket class, to ease the use of the C socket API. It uses the addrinfo struct and getaddrinfo() (which add some constraints).

TcpSocket.hpp

#ifndef TCPSOCKET_H
#define TCPSOCKET_H

#include 
#include 
#include 

namespace Socket {

    /**
     * TcpSocket class implementation to facilitate the use of sockets
     */

    class TcpSocket
    {
    public:
        TcpSocket();
        TcpSocket(int family, int flags);
        TcpSocket(int socket, addrinfo info, bool connected, bool bound);
        virtual ~TcpSocket();

        //Avoiding copy
        TcpSocket(const TcpSocket &socket) = delete;
        TcpSocket &operator=(const TcpSocket &socket) = delete;

        void bind(int port);
        void connect(std::string adress, int port);
        void listen(int maxQueue);
        std::shared_ptr accept();
        void send(const char *data, unsigned int length, int flags);
        /**
         * Receive data (blocking)
         *@return true if socket is still open, false otherwise
         */
        bool receive(char* msg, int len, int flags);
        void close();

    private:
        void setInfo(int port);
        void setInfo(std::string adress, int port);
        void openSocket(addrinfo *info);
        addrinfo * mInfo;
        int mSock = -1;
        bool mSockCreated = false;
        bool mBound = false;
        bool mConnected = false;
        bool mClosed = false;
    };

}

#endif


TcpSocket.cpp
(Please don't mind the DEBUG function, this is going away)

```
#include "TcpSocket.hpp"

#include
#include
#include
#include
#include
#include
#include
#include "exceptions.hpp"

//DEBUG
#include

#define DEBUG_ACT true

namespace Socket {

void DEBUG(std::string message)
{
if(DEBUG_ACT)
std::cout ai_family = AF_UNSPEC;
mInfo->ai_socktype = SOCK_STREAM

Solution

The code is generally clear, readable and easy to understand, which is great. However, you can still improve a few things.

-
Generally speaking, you don't want to manage memory manually like you're currently doing for mInfo. There is an easy way to make it managed effortless. Create the following type:

struct addrinfo_delete
{
    void operator()(addrinfo* ptr) const
    {
        freeaddrinfo(ptr);
    }
};


Now, instead of storing an addrinfo field in your TcpSocket class, you can make it store and std::unique_ptr field instead and let it memory by itself. Call it addrinfo_ptr and you will probably want to use it in some other places where you need an addrinfo.

Moreover, having an std::unique_ptr pointer member won't make you lose anything fature-wise since you already explicity made your class uncopyable in the first place.

-
When possible, use std::make_shared(bar); instead of std::shared_ptr foo(new Foo{bar});. It will prevent the new/delete visual mismatch and it can also save some reference counting. Therefore, instead of defining a Socket_p class, I would be as explicit as possible and use std::shared_ptr directly.

-
@Zeks advice is really good: you can pack your boolean flags into an std::bitset to make your class more compact.

-
You shouldn't have to write client->close(); at the end of your main since your class already does that in its destructor. Let the destructor do its job and it will be easier for everyone.

-
It is good practice to always fully qualify every component from the standard library. For example use std::memset instead of memset. Not only can it avoid some name clashes, but it will also make it easier to look for std:: if you need to know which components from the standard library you're using.

-
Whenever you use control flow statements like if, try to always use curly braces, even when you have a single statement following them. It will make it easier to add more statements (debug statements for example) and avoid problems of the Apple's goto fail kind.

-
Another good practice is to always use nullptr instead of NULL. While it doesn't matter most of the time, when it does you're glad you used nullptr everywhere.

Code Snippets

struct addrinfo_delete
{
    void operator()(addrinfo* ptr) const
    {
        freeaddrinfo(ptr);
    }
};

Context

StackExchange Code Review Q#98775, answer score: 3

Revisions (0)

No revisions yet.