Здравсвуйте.
Совсем недавно начал серьезно программировать на C++ под Линукс. Ну в общем сначала обычное баловство и все такое. Потом решил подковаться в теории и около 3 месяцев штудировал Страуструпа и, дополнительно, книги по объктно-ориентированному программированию. Сейчас написал свой класс для создания сетевых соединений. То есть сокетов. Есть просьба к профессионалам — проверьте код. Нет, вы не поняли, он работает, но мне нужно знать все ли я правильно сделал с идеологической точки зрения и правильно ли понял эти книги. Поэтому и прошу. Там всего около 150 строк. Если кто-то согласиться — пишите в почту anton.ermolenko@gmail.com или в аську 455-922-455. В принципе могу код здесь выложить.

Здравствуйте, DiKeert, Вы писали:

DK>В принципе могу код здесь выложить.

Так выкладывай.

Здравствуйте, Roman Odaisky, Вы писали:

RO>Так выкладывай.

Выкладываю. С указанием структуры. Есть каталог server. В нем лежит main.cpp. В каталоге server есть каталог include и в нем лежит socketClass.cpp — объявление класса Socket. В каталоге server есть также каталог src — в нем лежит socketClass.cpp — определение класса Socket. Для сборки этого в каталоге server лежит также файл Makefile. То есть структура такая:

server =>
include =>
socketClass.h
<=
src =>
socketClass.cpp
<=
main.cpp
Makefile
<=

Итак socketClass.h:

/* 
   ******Socket******    
   Class, which contains IPv4 sockets
  
 */

#ifndef SOCKET_H_
#define SOCKET_H_

#include <sys/socket.h>
#include <netinet/in.h>
#include <string>

class Socket
{
    public:
        Socket();
        Socket(int);    
        Socket(int, int);
        ~Socket();
        int createSocket();
        int bind();
        int listen();
        void accept(Socket*);
        void setPort(int port) {this->port = port;};
        void setBacklogSize(int backlog) {this->backlog = backlog;};
        void showIp();
        int getSocketDesk();
    
        std::string getIp();
    
    private:
        int socketDesk;
        int port;
        int backlog;
    
        sockaddr_in *factAddr; // This is the fact addr
        sockaddr *usingAddr; // This is the automatic link to soclkladdr_in addr
    
        socklen_t factAddrLen;
        
        std::string ip;
    
        bool toggleMemory();
        void setDefaults();    
};

#endif /*SOCKET_H_*/

Файл socketClass.cpp:

#include <socketClass.h>
#include <new>
#include <string>
#include <iostream>
#include <arpa/inet.h>


Socket::Socket()
{
    toggleMemory();
    setDefaults();
}

Socket::Socket(int port, int backlog)
{
    toggleMemory();
    this->port = port;
    this->backlog = backlog;
        
    try
    {
        createSocket();
        bind();
        listen();
    }
    catch(std::string xa)
    {
        std::cout << xa;
        throw(xa);
    }        
}

bool Socket::toggleMemory()
{
    try
    {
        factAddr = new sockaddr_in;        
    }
    catch (std::bad_alloc xa)
    {
        throw(std::string("Cannot allocate memory!"));
        return false;
    }
    
    usingAddr = (sockaddr*) factAddr;
    factAddrLen = sizeof(*factAddr);
    
    return true; 
}

int Socket::createSocket()
{
    socketDesk = ::socket(PF_INET, SOCK_STREAM, 0);
    if(socketDesk < 0)
    {
        throw(std::string("Cannot create Socket"));
    }
    
    return true;
}

int Socket::bind()
{
    factAddr->sin_family = AF_INET;
    factAddr->sin_port = htons(port);
    factAddr->sin_addr.s_addr = htonl(INADDR_ANY);
    int result = ::bind(socketDesk, usingAddr, factAddrLen);

    if(result)
    {
        throw(std::string("Cannot bind socket"));
    }
    else return true;
}

int Socket::listen()
{
    if(::listen(socketDesk, backlog))
    {
        throw(std::string("Cannot listen port."));
        return false;
    }
    else 
        return true;        
}

int Socket::getSocketDesk()
{
    return socketDesk;
    
}

void Socket::accept(Socket *parent)
{
    if((this->socketDesk = ::accept(parent->getSocketDesk(), usingAddr, &factAddrLen)) < 0)
    {
        throw(std::string("Cannot accept incoming connection!"));
    }    
}

void Socket::setDefaults()
{
    backlog = 5;
    port = 8008;
}

std::string Socket::getIp()
{
    char ipaddr[INET_ADDRSTRLEN];
    const char *ipAddr = inet_ntop(AF_INET, &(factAddr->sin_addr), ipaddr, sizeof(ipaddr));
    std::string *result = new std::string(ipAddr);
    ip = *result;
    return ip;
}

void Socket::showIp()
{
    std::cout << ip << std::endl;
}

Socket::~Socket()
{
    delete factAddr;
}

Файл main.cpp (который этот класс использует — создает сокет, ждест соединения, когда есть соединение, выводит ip адрес подсоеденившегося и отрубается)

#include <iostream>
#include <socketClass.h>

using namespace std;

int main()
{
    Socket *sockPointer;
    try{
        Socket *socket1 = new Socket(8008, 16);
        sockPointer = socket1;        
    }
    catch(string error)
    {

        exit(1);        
    }
    sockPointer->getIp();
    sockPointer->showIp();
        
    while(true)
    {
        Socket *newConn = new Socket();
        newConn->accept(sockPointer);
        newConn->getIp();
        newConn->showIp();
        close(newConn->getSocketDesk());
        delete newConn;
        break;
    }
    
    cout << "Hello, world!" << endl;
    
    return 0;
}

А вот Makefile что бы это собрать. Для GNU Make ессною

includeMain = -Iinclude/
includeSrc = -I../include

server : serverClass.o
    g++ -o server $(includeMain) main.cpp socketClass.o
serverClass.o : 
    g++ -c $(includeMain) src/socketClass.cpp

Здравствуйте, DiKeert, Вы писали:

DK>Выкладываю.

вопрос первый.
почему везде бросается std::string ?
мало того, что этого будет мало кто ожидать при использовании Вашего кода, так еще и собственно информации для приложения никакой нет.
тоесть, я бы предложил — создать наследника std::exception, возможно с некоторым контекстом исключения, хранящем информацию о произошедшем.

далее, зачем постоянно ловить по значению ?
в случае std::string это запросто может привести к аллокации памяти, которая в свою очередь, может вызвать тот же std::bad_alloc, который Вы сами же ловите и сразу же создаете еще один объект std::string в одном из методов.

std::string Socket::getIp()
{
    char ipaddr[INET_ADDRSTRLEN];
    const char *ipAddr = inet_ntop(AF_INET, &(factAddr->sin_addr), ipaddr, sizeof(ipaddr));

    std::string *result = new std::string(ipAddr); /* можно спросить, зачем так ? да еще и нет проверки на (NULL == ipAddr) */
    ip = *result;
    return ip; /* в этом месте, как Вы понимаете, никто за Вас result освобождать не будет, поэтому тут утечка памяти */    
}

к тому же, функция не совсем логична. getIp() есть функция от factAddr, более она ни отчего не зависит. поэтому логичнее было бы формировать строку ip в момент изменения factAddr, это у Вас один только метод bind(). далее просто возвращать сформированную строку.

const std::string& Socket::getIp() const { return this->ip; }

почему некоторые методы объявлены как 'int methodName()', а в теле идет 'return true /* false */ ;' ?
если кодов возврата всего 2, так и объявить соответственно, если же больше, то выделить их скажем в набор констант.

многие параметры, вроде INADDR_ANY, AF_INET, а так же волшебные цифры 8008 и 16 забиты прямо в коде.
это как минимум неудобно.

зачем память для 'sockaddr_in *factAddr;' выделять динамически ?
храните структуру как 'sockaddr_in factAddr;', не нужно будет следить за выделением\освобождением памяти.

не очень красиво, что класс завязан на std::cout.
перегрузить operator << (std::ostream&, const Socket&), или как то еще.
но метод void Socket::showIp() не нужен.

что бегло заметил, то спросил.

Здравствуйте, DiKeert, Вы писали:

DK> throw(std::string("Cannot allocate memory!"));
DK> return false;
Вторая строка лишняя

DK>int Socket::bind()
DK>{
DK> ...
DK> factAddr->sin_addr.s_addr = htonl(INADDR_ANY);
Поддерживаю других ораторов: вместо INADDR_ANY нужно передавать параметр в функцию или добавить атрибут в класс

DK>void Socket::accept(Socket *parent)
DK>{
DK> if((this->socketDesk = ::accept(parent->getSocketDesk(), usingAddr, &factAddrLen)) < 0)
DK> {
DK> throw(std::string("Cannot accept incoming connection!"));
DK> }
DK>}
Я бы сделал по-другому: метод accept должен возвращать новый объект который соотвествует принятому соединению.
В функции main это отразится так:

Socket *newConn = sockPointer->accept();

Здравствуйте, De Bug, Вы писали:

DK>>int Socket::bind()
DK>>{
DK>> ...
DK>> factAddr->sin_addr.s_addr = htonl(INADDR_ANY);
DB>Поддерживаю других ораторов: вместо INADDR_ANY нужно передавать параметр в функцию или добавить атрибут в класс

Спасибо большое за критику. Все совету учту и применю.

Но вот с этим INADDR_ANY небольшая неясность.
Может кто не знает, но htonl — эта функция преобразует порядок байтов из обычного в сетевой. Тоже предопрелена в системном файле.
INADDR_ANY — оно предопределено в заголовочном файле с системным вызовом.

Если нужда переоределить его в классе?

Здравствуйте, DiKeert, Вы писали:

DK>Здравствуйте, Roman Odaisky, Вы писали:

RO>>Так выкладывай.

DK>Выкладываю. С указанием структуры. Есть каталог server. В нем лежит main.cpp. В каталоге server есть каталог include и в нем лежит socketClass.cpp — объявление класса Socket. В каталоге server есть также каталог src — в нем лежит socketClass.cpp — определение класса Socket. Для сборки этого в каталоге server лежит также файл Makefile. То есть структура такая:

— не используется const
— не проверяются указатели на NULL (хотя бы с помощью assert)
— в методах, которые должны возвращать int, по факту возвращается bool
— левые исключения, ловятся по значению
— не все параметры методов в хедере имеют имена ( Socket(int) Socket(int, int) )
— зачем-то прикручен вывод в cout

Здравствуйте, DiKeert, Вы писали:

DK>А вот Makefile что бы это собрать. Для GNU Make ессною

Нет проверки зависимостей, хотя бы примитивной. При изменении заголовочного файла не произойдет пересборка зависящих от него объектников.
Затруднено добавление новых файлов к проекту — придется каждый раз править Makefile.
Нет возможности передать опции компилятору из командной строки, и вообще изменить компилятор.

Здравствуйте, DiKeert, Вы писали:

DK> Socket(int);

У одноаргументных конструкторов в 90% случаев должен быть explicit. В данном случае эта ошибка компенсируется тем, что конструктор всё равно нигде не определен :-)

Здравствуйте, Roman Odaisky, Вы писали:

RO>У одноаргументных конструкторов в 90% случаев должен быть explicit. В данном случае эта ошибка компенсируется тем, что конструктор всё равно нигде не определен

=)))) Это конечно все очень здорово, но я не знаю что такое explicit.

Здравствуйте, DiKeert, Вы писали:

DK>Здравствуйте, Roman Odaisky, Вы писали:

RO>>У одноаргументных конструкторов в 90% случаев должен быть explicit. В данном случае эта ошибка компенсируется тем, что конструктор всё равно нигде не определен

DK>=)))) Это конечно все очень здорово, но я не знаю что такое explicit.

это служебное слово, используется при объявлении конструктора, запрещает использование конструктора для неявных преобразований таких как, например:

struct Test
{
   Test() {};
   Test(int) {};
};

Test t;
int i=0;
t=i;

Здравствуйте, DiKeert, Вы писали:

Много чего было сказано , согласен со всеми .Попытаюсь вставить свои 5 копеек.
Надеюсь тоже поможет:

1. Следующии методы

        int createSocket();
        int bind();
        int listen();

считаю необходимо перенести в закрытую часть класса. В твоей программе ты вызываешь их
из конструктора. То есть они вспомогательные. Если же ты оставляешь их в видимой части класса и даешь возможность
пользователю напрямую их вызывать — то нарушается принцип инкапсуляции. Ведь именно для этого ты создавал класс , чтобы скрыть низкоуровневые ньюансы имплементации сокета. А кроме того , что делать если вызвали listen до bind ит.д ит.п.

2 . Методы

        void setPort(int port) {this->port = port;};
        void setBacklogSize(int backlog) {this->backlog = backlog;};

не вижу смысла. Если ты передаешь их в конструкторе и сокет уже создается. После того как сокет создан конструктором — эти методы не имеют смысла. Может идея такова , что создаешь сокет контруктором без аргументов , а потом все инициализируешь ? Но тогда снова сталкиваешься с проблемой порядка вызова низкоуровневых методов , о которых я говорил в части 1.

3. Методы :

        void showIp();
        int getSocketDesk(); 
        std::string getIp();

Добавил бы const так как методы не меняют объект.

Ну вроде все по коду.

Если интересно, вот мой велосипед под UNIX для TCP соединений. Не совсем по Страуструпу конечно, но работает .

<...>
typedef int RESULT;
<...>
//soket type
typedef int SOCKET;
const int INVALID_SOCKET = -1;
<...>
//socket - generic socket wrapper
class socket : private non_copyable
{
public:
    //ctor/dtor
    explicit socket(SOCKET handle = INVALID_SOCKET) throw() : Handle(handle) {}
    ~socket() throw() {if(Handle != INVALID_SOCKET) ::close(Handle);}

    //initialization
    RESULT create(int type = SOCK_STREAM)
    {
        if(Handle != INVALID_SOCKET)
            ::close(Handle);
        Handle = ::socket(AF_INET, type, NULL); //create new socket
        return (INVALID_SOCKET == Handle) ? errno : RESULT_SUCCESS;
    }
    void close()
    {
        if(Handle != INVALID_SOCKET)
        {
            ::close(Handle);
            Handle = INVALID_SOCKET;
        }
    }
    SOCKET attach(SOCKET new_handle = INVALID_SOCKET) throw()
    {
        const SOCKET previous_handle = Handle;
        Handle = new_handle;
        return previous_handle;
    }
    SOCKET detach() throw()
    {
        const SOCKET previous_handle = Handle;
        Handle = INVALID_SOCKET;
        return previous_handle;
    }

    //access
    bool valid() const throw() {return Handle != INVALID_SOCKET;}
    SOCKET handle() const throw() {return Handle;}

    //master bind method
    RESULT bind(unsigned long address, unsigned short port) const
    {
        sockaddr_in socket_address = {};
        socket_address.sin_family = AF_INET;
        socket_address.sin_addr.s_addr = ::htonl(address);
        socket_address.sin_port = ::htons(port);
        if(RESULT_ERROR == ::bind(Handle,
            reinterpret_cast<sockaddr*>(&socket_address),
            sizeof(socket_address)))
            return errno;
        return RESULT_SUCCESS;
    }
    RESULT bind(const char* address_string, unsigned short port) const
    {
        //convert address
        const unsigned long address = ::inet_addr(address_string);
        if(NULL == address)
            return errno;

        //bind
        sockaddr_in socket_address = {};
        socket_address.sin_family = AF_INET;
        socket_address.sin_addr.s_addr = address;
        socket_address.sin_port = ::htons(port);
        if(RESULT_ERROR == ::bind(Handle,
           reinterpret_cast<sockaddr*>(&socket_address),
           sizeof(socket_address)))
            return errno;
        return RESULT_SUCCESS;
    }

    //server socket operations
    RESULT listen(int backlog = SOMAXCONN) const throw()
    {
        return result_call(::listen(Handle, backlog));
    }
    SOCKET accept(  sockaddr* address = NULL,
                    socklen_t* address_length = NULL) const
    {
        return ::accept(Handle, address, address_length);
    }
    RESULT accept(  SOCKET& accepted_socket,
                    sockaddr* address = NULL,
                    socklen_t* address_length = NULL) const
    {
        accepted_socket = ::accept(Handle, address, address_length);
        return (INVALID_SOCKET == accepted_socket) ? errno : RESULT_SUCCESS;
    }

    //client socket operations
    RESULT connect(unsigned long address, unsigned short port) const
    {
        sockaddr_in socket_address = {};
        socket_address.sin_family = AF_INET;
        socket_address.sin_addr.s_addr = ::htonl(address);
        socket_address.sin_port = ::htons(port);
        if(RESULT_ERROR != ::connect(Handle,
            reinterpret_cast<sockaddr*>(&socket_address),
            sizeof(socket_address)));
            return errno;
        return RESULT_SUCCESS;
    }
    RESULT connect(const char* address_string, unsigned short port) const
    {
        //convert address
        const unsigned long address = ::inet_addr(address_string);
        if(NULL == address)
            return errno;

        //connect
        sockaddr_in socket_address = {};
        socket_address.sin_family = AF_INET;
        socket_address.sin_addr.s_addr = address;
        socket_address.sin_port = ::htons(port);
        if(RESULT_ERROR != ::connect(Handle,
            reinterpret_cast<sockaddr*>(&socket_address),
            sizeof(socket_address)));
            return errno;
        return RESULT_SUCCESS;
    }
    RESULT shutdown(int operations) const throw()
    {
        return result_call(::shutdown(Handle, operations));
    }

    //IO operations
    ssize_t send(const void* data, size_t data_size, int flags = 0) const
    {
        return ::send(Handle, data, data_size, flags);
    }
    ssize_t receive(void* buffer, size_t buffer_size, int flags = 0) const
    {
        return ::recv(Handle, buffer, buffer_size, flags);
    }

    //socket options
    RESULT set_option(  int option,
                        const void* value,
                        int option_length,
                        int level = SOL_SOCKET)
    {
        assert(value != NULL);
        if(RESULT_ERROR == ::setsockopt(Handle, level, option,
            reinterpret_cast<const char*>(value), option_length))
            return errno;
        return RESULT_SUCCESS;
    }
    RESULT get_option(  int option,
                        void* value,
                        int* option_length,
                        int level = SOL_SOCKET)
    {
        assert(value != NULL && option_length != NULL);
        if(RESULT_ERROR == ::getsockopt(Handle, level, option,
            reinterpret_cast<char*>(value),
            reinterpret_cast<socklen_t*>(option_length)))
            return errno;
        return RESULT_SUCCESS;
    }

    //IO control
    RESULT ioctl(unsigned long command, u_long* argument) const
    {
        assert(argument != NULL);
        return result_call(::ioctl(Handle, command, argument));
    }
    RESULT set_nonblocking() //helper
    {
        return result_call(::fcntl(Handle, F_SETFL, O_NONBLOCK));
    }

protected:
    SOCKET Handle;
};