Aplikacja sieciowa :: 4programmers.net

0

Dam wam kod do code review, który możecie zmasakrować. Ciekaw jestem opinii.

Już o szczegółach się nie rozpiszę, bo po co ? W programie brakuje tylko aby wczytane ustawienia z pliku podstawiał do

void MainWindow::sendMessage()
{
    QTcpSocket *mySocket;
    mySocket = new QTcpSocket(this);
    mySocket->connectToHost(QHostAddress::LocalHost,12345); //tutaj na razie nie wstawia ustawień z pliku

    //dalszy kod
}

link do kodu

8

Bardzo ładny.

3

zkubinski napisał(a):

zamiast na GIT - umieszczę to w archiwum - tak, żeby się chociaż trochę nad wami poznęcać :>

Mamy wpisane w regulaminie forumowym, żeby nie umieszczać kodu w postaci archiwum: Dlaczego nikt nie odpowiada w moim wątku?

0

No super, i co ten program umie robić? Poza zapisywaniem ustawień?
Żadnej też walidacji danych nie robi btw:

~/P/QTcpAdminClient $ cat build-QTcpAdminClient-Desktop-Debug/settings.json
{
    "settings": {
        "serverSettings": {
            "ipv4": "aaXDD",
            "port": "99999"
        }
    }
}

wysyła wiadomość za pomocą protokołu TCP do hosta zdalnego — zkubinski 0 sekund temu

no to jak postawiłem serwer TCP to program się zawiesza przy próbie wysłania komunikatu, wszystko staje się nieresponsywne, nawet ustawień nie mogę zmienić. Nawet nie mogę zabić aplikacji XD

0

Nazwa zmiennej mySocket nic nie mówi do czego ta zmienna służy

0

Jedyna uwaga jaką tutaj mogę dać (bo tyle przeczytałem) - jak robisz mySocket->close() to nie ma potrzeby wcześniej spłukiwać gniazda.

2

Jak się ma nazwa funkcji do jej implementacji?

void AdvancedInformationWindow::createActionMenuFile()
{
    actionMenuFile = new QAction(this);
    actionMenuFile->setText(QString(tr("Zamknij")));

    QObject::connect(actionMenuFile, &QAction::triggered, this, &AdvancedInformationWindow::close);
}

Jak się ma nazwa funkcji do jej implementacji? Po co zacommitowałeś zakomentowany kod?


void Settings::showSettings()
{
    QJsonObject tmp;

    for(QString &key : jsonIPv4.keys()){
        tmp.insert(key,jsonIPv4.value(key));
    }
    for(QString &key : jsonPort.keys()){
        tmp.insert(key,jsonPort.value(key));
    }
    for(QString &key : jsonServerSettings.keys()){
        jsonServerSettings.insert(key,tmp);
    }
    for(QString &key : jsonMainSettings.keys()){
        jsonMainSettings.insert(key,jsonServerSettings);
    }

    emit settings(jsonMainSettings);

//    QJsonObject::iterator it1;

//    for(it1=jsonMainSettings.begin(); it1!=jsonMainSettings.end(); ++it1){
//        qInfo()<< it1.key() << it1.value().toString();

//        QJsonObject::iterator it2;
//        QJsonObject next1 = it1.value().toObject();

//        for(it2=next1.begin(); it2!=next1.end(); ++it2){
//            qInfo()<< "\t" << it2.key() << it2.value().toString();

//            QJsonObject::iterator it3;
//            QJsonObject next2 = it2.value().toObject();

//            for(it3=next2.begin(); it3!=next2.end(); ++it3){
//                qInfo()<< "\t\t" << it3.key() << it3.value().toString();

//                QJsonObject::iterator it4;
//                QJsonObject next3 = it3.value().toObject();

//                for(it4=next3.begin(); it4!=next3.end(); ++it4){
//                    qInfo()<< "\t\t\t" << it4.value().toString();
//                }
//            }
//        }
//    }
}

Nie obsługujesz błędów

    else{
        QByteArray data;

        data = readFileJson.readAll();

        QJsonParseError errorData;

        settingsFile = QJsonDocument::fromJson(data, &errorData);
    }

Qt przeszło na CMake, więc używanie QMake staje się powoli bezcelowe.

Ogólnie po szybkim przejrzeniu mogłoby być gorzej.

0

kq napisał(a):

Jak się ma nazwa funkcji do jej implementacji?

void AdvancedInformationWindow::createActionMenuFile()
{
    actionMenuFile = new QAction(this);
    actionMenuFile->setText(QString(tr("Zamknij")));

    QObject::connect(actionMenuFile, &QAction::triggered, this, &AdvancedInformationWindow::close);
}

nazwa funkcji ma mi mówić, że to jest menu Akcji - nie wiem jak to ująć, ogólnie chodzi o to, że jest to funkcja działająca na obiektach typu QAction, zeby było łatwiej mi się połapać przy pisaniu menu

Jak się ma nazwa funkcji do jej implementacji? Po co zacommitowałeś zakomentowany kod?


void Settings::showSettings()
{
    QJsonObject tmp;

    for(QString &key : jsonIPv4.keys()){
        tmp.insert(key,jsonIPv4.value(key));
    }
    for(QString &key : jsonPort.keys()){
        tmp.insert(key,jsonPort.value(key));
    }
    for(QString &key : jsonServerSettings.keys()){
        jsonServerSettings.insert(key,tmp);
    }
    for(QString &key : jsonMainSettings.keys()){
        jsonMainSettings.insert(key,jsonServerSettings);
    }

    emit settings(jsonMainSettings);

//    QJsonObject::iterator it1;

//    for(it1=jsonMainSettings.begin(); it1!=jsonMainSettings.end(); ++it1){
//        qInfo()<< it1.key() << it1.value().toString();

//        QJsonObject::iterator it2;
//        QJsonObject next1 = it1.value().toObject();

//        for(it2=next1.begin(); it2!=next1.end(); ++it2){
//            qInfo()<< "\t" << it2.key() << it2.value().toString();

//            QJsonObject::iterator it3;
//            QJsonObject next2 = it2.value().toObject();

//            for(it3=next2.begin(); it3!=next2.end(); ++it3){
//                qInfo()<< "\t\t" << it3.key() << it3.value().toString();

//                QJsonObject::iterator it4;
//                QJsonObject next3 = it3.value().toObject();

//                for(it4=next3.begin(); it4!=next3.end(); ++it4){
//                    qInfo()<< "\t\t\t" << it4.value().toString();
//                }
//            }
//        }
//    }
}

ta funkcja zwraca utworzonego jsona do klasy SettingsFile aby tego jsona zapisać do pliku - nie miałem pomysłu jak to nazwać, jeżeli pytasz o zakomentowany kod, to on wyświetla ustawienia jakich dokonałem, docelowo będę chciał to wykorzystać do podglądu struktury pliku przed zapisaniem

Nie obsługujesz błędów

gdzie nie obsługuję błędów ? Przy zapisie do pliku ? W sumie robiłem to wczoraj ale stwierdziłem, że nie ma to sensu, bo nie sądzę aby klasa która ma zapisać plik zgubiła po drodze np średnik lub inną daną z pliku

    else{
        QByteArray data;

        data = readFileJson.readAll();

        QJsonParseError errorData;

        settingsFile = QJsonDocument::fromJson(data, &errorData);
    }

Qt przeszło na CMake, więc używanie QMake staje się powoli bezcelowe.

wiem, ale CMake mnie przeraża ilością plików które dodaje... nie wiem na co mi to potrzebne - na razie nie zagłębiałem się w to po co to jest i do czego mi to może się przydać

Ogólnie po szybkim przejrzeniu mogłoby być gorzej.

tzn ? Mam rozumieć, że jest progres ?

3

Popatrzyłem tylko na funkcję na którą zwróciłeś nam uwagę:

void MainWindow::sendMessage()
{
    QTcpSocket *mySocket;
    mySocket = new QTcpSocket(this);
    mySocket->connectToHost(QHostAddress::LocalHost,12345);
//    mySocket->connectToHost(QHostAddress("172.29.184.202"),12345);

    if(mySocket->waitForConnected()){
        if(mySocket->state()==QAbstractSocket::UnconnectedState){
            QMessageBox errorConnect(QMessageBox::Critical, QString("Błąd połączenia"), QString("Nie można się połączyć z hostem zdalnym"), QMessageBox::Ok, this);
            errorConnect.exec();
            mySocket->close();
        }
        if(mySocket->state()==QAbstractSocket::ConnectedState){
            mySocket->write(textMessage);
            inputMessage->addItem(textMessage);
            mySocket->flush();
            mySocket->close();
            outputMessage->clear();
        }
    }
    else if(!mySocket->waitForConnected()){
        QMessageBox errorConnect(QMessageBox::Critical, QString("Błąd połączenia"), QString("Przekroczono limit czasu na połączenie\n"+mySocket->errorString()), QMessageBox::Ok, this);
        errorConnect.exec();
        mySocket->close();
    }
}

Tu masz wyciek pamięci, których żadne narzędzie nie wykryje, bo od formalnej strony wyciekająca pamięć jest zawsze osiągalna, ale logika twojej aplikacji traci z nią kontakt.
Każde użycie tej funkcji tworzy QTcpSocket na stercie. Własność jest przekazywana do MainWindow (dlatego dal narzędzi nie ma wycieku), które żyje tak długo jak aplikacja.
Po wysłaniu wiadomości QTcpSocket nie jest niszczone, więc za każdym razem dodajesz nowe i natychmiast o nim zapominasz.

Na dodatek uczyniłeś ta funkcję synchroniczną, co jest złą praktyką.

Czemu jest else if(!mySocket->waitForConnected()){? Samo else pełni chyba taką samą rolę. Co jeśli za drugim wywołaniem mySocket->waitForConnected() zwraca true?

Za dużo jest w MainWindow. MainWindow powinno pełnić rolę kontrolera. Powinno być maksymalnie głupie. Komunikacja siecowa pownna znaleźć się w jakimś QObject, które ma sygnały i sloty.
MainWindow tylko łączy te sygnały i sloty z odpowiednim UI (lub sobą).

0

MarekR22 napisał(a):

Potrzymam tylko na funkcję na którą zwróciłeś nam uwagę:
void MainWindow::sendMessage()
{
    QTcpSocket *mySocket;
    mySocket = new QTcpSocket(this);
    mySocket->connectToHost(QHostAddress::LocalHost,12345);
//    mySocket->connectToHost(QHostAddress("172.29.184.202"),12345);

    if(mySocket->waitForConnected()){
        if(mySocket->state()==QAbstractSocket::UnconnectedState){
            QMessageBox errorConnect(QMessageBox::Critical, QString("Błąd połączenia"), QString("Nie można się połączyć z hostem zdalnym"), QMessageBox::Ok, this);
            errorConnect.exec();
            mySocket->close();
        }
        if(mySocket->state()==QAbstractSocket::ConnectedState){
            mySocket->write(textMessage);
            inputMessage->addItem(textMessage);
            mySocket->flush();
            mySocket->close();
            outputMessage->clear();
        }
    }
    else if(!mySocket->waitForConnected()){
        QMessageBox errorConnect(QMessageBox::Critical, QString("Błąd połączenia"), QString("Przekroczono limit czasu na połączenie\n"+mySocket->errorString()), QMessageBox::Ok, this);
        errorConnect.exec();
        mySocket->close();
    }
}
Tu masz wyciek pamięci których narzędzie narzędzie nie wykryje, bo od formalnej strony wyciekająca pamięć jest zawsze osiągalna, ale logika twojej aplikacji traci z nią kontakt.
Każde użycie tej funkcji tworzy QTcpSocket na stercie. Własność jest przekazywana do MainWindow (dlatego dal narzędzi nie ma wycieku), które żyje tak długo jak aplikacja.
Po wysłaniu wiadomości QTcpSocket nie jest niszczone, więc za każdym razem dodajesz nowe i natychmiast o nim zapominasz.

faktycznie, nawet nie zauważyłem tego wycieku - mogłem zrobić to jako składowa klasy i zniszczyć w destruktorze albo dodać na końcu funkcji delete mySocket

void MainWindow::sendMessage()
> {
>     QTcpSocket *mySocket;
>     //dalszy kod
>
>     delete mySocket; //powinno być delete
> }

Na dodatek uczyniłeś ta funkcję synchroniczną, co jest złą praktyką.

synchroniczna ? Nie znam tego pojęcia, więc co muszę zrobić ?

Czemu jest else if(!mySocket->waitForConnected()){? Samo else pełni chyba taką samą rolę. Co jeśli za drugim wywołaniem mySocket->waitForConnected() zwraca true?

sprawdzałem dla przypadku połączenie i nie połączenie na tej samej uruchomionej instancji programu i debugując miałem dla połączenia true, a dla nieudanej próby false. Więc chyba powinno być dobrze

Za dużo jest w MainWindow. MainWindow

nie rozumiem co masz na myśli ?

powinno pełnić rolę kontrolera.

tutaj też nie rozumiem ?

Powinno być maksymalnie głupie. Komunikacja siecowa pownna znaleźć się w jakimś QObject, które ma sygnały i sloty.
MainWindow tylko łączy te sygnały i sloty z odpowiednim UI (lub sobą).

czyli utworzyć jakąś klasę np TCPConnect dziedziczącą po QObject i za pomocą metod umożliwić aplikacji połączenie i rozłączenie z gniazdem ?

2

zkubinski napisał(a):

void MainWindow::sendMessage()
> {
>     QTcpSocket *mySocket;
>     //dalszy kod
>
>     delete mySocket; //powinno być delete
> }

W takim przypadku bezpieczniej i prościej jest:

void MainWindow::sendMessage()
{
    QTcpSocket mySocket;
    //dalszy kod
}

Ale i tak to rozwiażanie jest słabe. Jak pisałem: logikę biznesową powinno się przenieść do innej klasy, nie mającej pojęcia o czymś takim jak UI.

1

Offtopic:
Komentowanie kodu na forum nie jest zbyt poręczne.
Najlepiej stowrzyć branch na pierwszym commit'cie następnie wystawić merge request master do tego nowego branch'a wskazującego początek.
Pozwolić wszystkim komentować w merge request.
Nie mam konta na gitlab, ale na pewno to jest najlepsza metoda proszenia o feedback do kodu.

3

w logikę się nie będę nawet zagłębiał.

    QString message;
    message = outputMessage->toPlainText();

    if(message.isEmpty()){
        bttSend->setEnabled(false);
        bttClearMessage->setEnabled(false);
    }
    else if(!message.isEmpty()){
        bttSend->setEnabled(true);
        bttClearMessage->setEnabled(true);
    }

jak zawsze brzydzisz się refaktoryzacją kodu.... żeby nie być gołosłownym przykład

bool flag = (outputMessage->toPlainText()).isEmpty();
bttSend->setEnabled(flag);
bttClearMessage->setEnabled(flag);

ten this po co?

void Page1::setPort(const QString &port)
{
    this->_port = port;
}

i tak musiałbym jeszcze ileż wklejać, jak mówiłem odmowa lektury i pomyślunku się mści. az strach pomyśleć co by było jak by w logikę zaczął wchodzić.

0

@MarekR22:

mówiłeś, żeby przenieść kod odpowiedzialny za socketa do innej klasy. Poniżej koncepcja. Co o tym myślisz ? Dobrze ? Źle ? Do bani ? Proponujesz coś lepszego ?

class TcpConnect
{
public:
    QTcpSocket &function(){
        mySocket = new QTcpSocket();
        return *mySocket;
    }
    
private:
    QTcpSocket *mySocket;
};

int main(void)
{
    TcpConnect connect;
    
    connect.function().connectToHost(QHostAddress::LocalHost,12345);
}

0

@enedil:

Masz wyciek pamięci, nazwy do bani, nie wiadomo po co ten socket jako składowa, wcale nie enkapsulujesz dostępu do składowych

a co ty teraz na to powiesz ? Jest ok ?

I druga rzecz - gdybym chciał mieć konstruktor, to co w nim umieścić ?

class TcpConnect : public QObject
{
public:
    void createConnectToHost(QHostAddress addr, quint16 port){
        mySocket.connectToHost(addr, port);
    }

    bool connectStatus(){
        if(mySocket.waitForConnected()){
            if(mySocket.state()==QAbstractSocket::ConnectedState){
                return true;
            }
        }

        return false;
    }

    void sendMessage(const char *msg){
        mySocket.write(msg);
        mySocket.flush();
    }

private:
    QTcpSocket mySocket;
};

int main(void)
{
    TcpConnect connect;

    connect.createConnectToHost(QHostAddress::LocalHost, 12345);
    connect.connectStatus();
    connect.sendMessage("test");

    return 0;
}

1

Kod:

bool connectStatus(){
    if(mySocket.waitForConnected()){
        if(mySocket.state()==QAbstractSocket::ConnectedState){
            return true;
        }
    }

    return false;
}

można zredukować do:

bool connectStatus() {
  return mySocket.waitForConnected() && mySocket.state()==QAbstractSocket::ConnectedState;
}

O ile nie płacą Ci od linijki…

Nie rozumiem, czemu TcpConnect nie ma konstruktora, tylko sobie może wisieć w nonsensownym stanie, w którym nikt nie wywołał createConnectToHost, a zabrał się za sendMessage.

0

@Althorion:

no i co ty teraz na to ?

class TcpConnect : public QObject
{
public:
    TcpConnect(QHostAddress addr, quint16 port){
        mySocket.connectToHost(addr, port);
    }

    enum error{CONNECTED, ERROR_CONNECT, CONNECTION_TIMEOUT, ERROR_CONNECT_TO_REMOTE_HOST};

    error connectStatus(){
        if(mySocket.waitForConnected()){
            if(mySocket.state()==QAbstractSocket::ConnectedState){
                return CONNECTED;
            }
            if(mySocket.state()==QAbstractSocket::UnconnectedState){
                return ERROR_CONNECT_TO_REMOTE_HOST;
            }
        }
        return CONNECTION_TIMEOUT;
    }

    void sendMessage(const char *msg){
        mySocket.write(msg);
        mySocket.flush();
    }

private:
    QTcpSocket mySocket;
};

int main(void)
{
    TcpConnect connect(QHostAddress::LocalHost, 12345);
    connect.connectStatus();
    connect.sendMessage("test");

    return 0;
}

1

Coraz ładniej!

zkubinski napisał(a):

enum error{CONNECTED, ERROR_CONNECT, CONNECTION_TIMEOUT, ERROR_CONNECT_TO_REMOTE_HOST}

Skoro masz tutaj też wymieniony niewadliwy stan, a funkcja się nazywa connectStatus, to bardziej odpowiednią nazwą enuma byłby result, albo właśnie status.
Warto pewnie zrobić enum class status { a nie po prostu enum (jak nie wiesz po co to jest to zgooglaj).
Warto rozważyć czy const char* to dobry typ na wiadomość. W normalnym kodzie C++ rzadko kiedy będziesz mieć dostępny tylko stringa w stylu C. Lepiej brać std::string_view, const std::string&, albo być może const QString& (nie wiem na co Qt pozwala). Trochę nieoczywiste jest też to, że connectStatus jest blokująca (wcześniej wspominane synchroniczne vs asynchroniczne IO, również wyszukaj), przez użycie waitForConnect.

0

enedil napisał(a):

Coraz ładniej!

zkubinski napisał(a):

enum error{CONNECTED, ERROR_CONNECT, CONNECTION_TIMEOUT, ERROR_CONNECT_TO_REMOTE_HOST}

Skoro masz tutaj też wymieniony niewadliwy stan, a funkcja się nazywa connectStatus, to bardziej odpowiednią nazwą enuma byłby result, albo właśnie status.

ok, dzięki - zmienię

Warto pewnie zrobić enum class status { a nie po prostu enum (jak nie wiesz po co to jest to zgooglaj).

też się nad tym zastanawiałem czy dać inaczej

Warto rozważyć czy const char* to dobry typ na wiadomość. W normalnym kodzie C++ rzadko kiedy będziesz mieć dostępny tylko stringa w stylu C. Lepiej brać std::string_view, const std::string&, albo być może const QString& (nie wiem na co Qt pozwala). Trochę nieoczywiste jest też to, że connectStatus jest blokująca (wcześniej wspominane synchroniczne vs asynchroniczne IO, również wyszukaj), przez użycie waitForConnect.

akurat ta funkcja nie przyjmuje stringa, są trzy przeładowane

 qint64 write(const char *data, qint64 maxSize)
 qint64 write(const char *data)
 qint64 write(const QByteArray &data)

mogę dać QByteArray

1

zkubinski napisał(a):

akurat ta funkcja nie przyjmuje stringa, są trzy przeładowane
 qint64 write(const char *data, qint64 maxSize)
 qint64 write(const char *data)
 qint64 write(const QByteArray &data)
mogę dać QByteArray

To że QTcpSocket ma tylko takie trzy przeładowania, nie znaczy że chcesz taki właśnie interfejs udostępniać użytkownikowi. Np. std::string_view zdaje się mieć sens, i można by go konwertować do const char* na potrzeby wykonania QTcpSocket::write.

0

no dobra, kod przerobiony i działa - czy teraz jest dobrze ? Są jakieś uwagi ?

plik TcpConnect.h

#include <QObject>
#include <QTcpSocket>
#include <QHostAddress>

enum class result{Connected=0, Error_Connect=1, Connection_Timeout=2, Error_Connect_To_Remote_Host=3};

class TcpConnect : public QObject
{
    Q_OBJECT
public:
    explicit TcpConnect(QHostAddress address, quint16 port, QObject *parent=nullptr);

    result connectStatus();
    void sendMessage(const QString &msg);
    void closeConnect();
    QString error() const;

private:
    QTcpSocket mySocket;

signals:

};

plik TcpConnect.cpp

#include "tcpconnect.h"

TcpConnect::TcpConnect(QHostAddress address, quint16 port, QObject *parent) : QObject{parent}
{
    mySocket.connectToHost(address, port);
}

result TcpConnect::connectStatus()
{
    if(mySocket.waitForConnected()){
        if(mySocket.state()==QAbstractSocket::ConnectedState){
            return result::Connected;
        }
        if(mySocket.state()==QAbstractSocket::UnconnectedState){
            return result::Error_Connect_To_Remote_Host;
        }
    }

    return result::Connection_Timeout;
}

void TcpConnect::sendMessage(const QString &msg)
{
    QByteArray data;

    data = msg.toUtf8();

    mySocket.write(data);
    mySocket.flush();
}

void TcpConnect::closeConnect()
{
    mySocket.close();
}

QString TcpConnect::error() const
{
    QString error;
    error = mySocket.errorString();

    return error;
}

zmodyfikowana funkcja w MainWindow.cpp

void MainWindow::sendMessage()
{
    TcpConnect connect(QHostAddress::LocalHost, 12345);

    if(connect.connectStatus()==result::Connected){
        connect.sendMessage(textMessage);
        inputMessage->addItem(textMessage);
        outputMessage->clear();
    }
    if(connect.connectStatus()==result::Error_Connect_To_Remote_Host){
        QMessageBox error(QMessageBox::Critical, QString(tr("Błąd połączenia")), QString(tr("Nie można połączyć się z hostem zdalnym")), QMessageBox::Ok, this);
        error.exec();
        connect.closeConnect();
    }
    if(connect.connectStatus()==result::Connection_Timeout){
        QMessageBox error(QMessageBox::Critical, QString(tr("Błąd połączenia")), QString(tr("Przekroczono limit czasu na połączenie\n")+connect.error()), QMessageBox::Ok, this);
        error.exec();
        connect.closeConnect();
    }

    connect.closeConnect();
}

2

closeConnect() się prosi o bycie destruktorem, żeby się nie dało tego przypadkiem pominąć, ale tak na szybko to chyba moja jedyna uwaga. Zaczyna to wyglądać nieźle.

1

moża zrobić tak:

void MainWindow::sendMessage()
{
    TcpConnect connect(QHostAddress::LocalHost, 12345);

    switch (connect.connectStatus()){
    case result::Connected:
        connect.sendMessage(textMessage);
        inputMessage->addItem(textMessage);
        outputMessage->clear();
        break;
    case result::Error_Connect_To_Remote_Host: {
            QMessageBox error(QMessageBox::Critical, QString(tr("Błąd połączenia")), QString(tr("Nie można połączyć się z hostem zdalnym")), QMessageBox::Ok, this);
            error.exec();
        }
        break;
    case result::Connection_Timeout: {
            QMessageBox error(QMessageBox::Critical, QString(tr("Błąd połączenia")), QString(tr("Przekroczono limit czasu na połączenie\n")+connect.error()), QMessageBox::Ok, this);
            error.exec();
        }
        break;
    default:
        break;
    }
}

Oobecna wersja jest trochę bardziej skomplikowana. Funkcja closeSocket nie powinna być używana. Dokumentacja Qt mówi:

[virtual]
QTcpSocket::~QTcpSocket()

Destroys the socket, closing the connection if necessary.

WIęc jak twój obiekt connect wyjdzie z zakresu, to gniazdo samo się zamknie.

enum class result{Connected=0, Error_Connect=1, Connection_Timeout=2, Error_Connect_To_Remote_Host=3};

wcześniej ten enum był ładnie w środku klasy. Teraz każdy jak includuje TcpConnect.h, ma zaśmieconą globalną przestrzeń nazw jakimś enumem result który mało znaczy w kontekście.

0

no dobra, po przeróbkach wygląda to tak

class TcpConnect : public QObject
{
    Q_OBJECT
public:
    enum class result{Connected=0, Error_Connect=1, Connection_Timeout=2, Error_Connect_To_Remote_Host=3};

    explicit TcpConnect(QHostAddress address, quint16 port, QObject *parent=nullptr);
    ~TcpConnect();

    result connectStatus();
    void sendMessage(const QString &msg);
    QString error() const;

private:
    QTcpSocket mySocket;

signals:

};

#include "tcpconnect.h"

TcpConnect::TcpConnect(QHostAddress address, quint16 port, QObject *parent) : QObject{parent}
{
    mySocket.connectToHost(address, port);
}

TcpConnect::result TcpConnect::connectStatus()
{
    if(mySocket.waitForConnected()){
        if(mySocket.state()==QAbstractSocket::ConnectedState){
            return result::Connected;
        }
        if(mySocket.state()==QAbstractSocket::UnconnectedState){
            return result::Error_Connect_To_Remote_Host;
        }
    }

    return result::Connection_Timeout;
}

void TcpConnect::sendMessage(const QString &msg)
{
    QByteArray data;

    data = msg.toUtf8();

    mySocket.write(data);
    mySocket.flush();
}

QString TcpConnect::error() const
{
    QString error;
    error = mySocket.errorString();

    return error;
}

TcpConnect::~TcpConnect()
{
    mySocket.disconnectFromHost();
}

void MainWindow::sendMessage()
{
    TcpConnect connect(QHostAddress::LocalHost, 12345, this);

    if(connect.connectStatus()==TcpConnect::result::Connected){
        connect.sendMessage(textMessage);
        inputMessage->addItem(textMessage);
        outputMessage->clear();
    }
    if(connect.connectStatus()==TcpConnect::result::Error_Connect_To_Remote_Host){
        QMessageBox error(QMessageBox::Critical, QString(tr("Błąd połączenia")), QString(tr("Nie można połączyć się z hostem zdalnym")), QMessageBox::Ok, this);
        error.exec();
    }
    if(connect.connectStatus()==TcpConnect::result::Connection_Timeout){
        QMessageBox error(QMessageBox::Critical, QString(tr("Błąd połączenia")), QString(tr("Przekroczono limit czasu na połączenie\n")+connect.error()), QMessageBox::Ok, this);
        error.exec();
    }
}

czy są jeszcze jakieś uwagi ?

0

mam jeszcze jedno pytanie odnośnie tego programu

w poprzedniej wersji miałem funkcję sendMessage która wyglądała jak niżej

void MainWindow::sendMessage()
{
    TcpConnect connect(QHostAddress::LocalHost, 12345, this);

    if(connect.connectStatus()==result::Connected){
        connect.sendMessage(textMessage);
        inputMessage->addItem(textMessage);
        outputMessage->clear();
    }
    if(connect.connectStatus()==result::Error_Connect_To_Remote_Host){
        QMessageBox error(QMessageBox::Critical, QString(tr("Błąd połączenia")), QString(tr("Nie można połączyć się z hostem zdalnym")), QMessageBox::Ok, this);
        error.exec();
        connect.closeConnect();
    }
    if(connect.connectStatus()==result::Connection_Timeout){
        QMessageBox error(QMessageBox::Critical, QString(tr("Błąd połączenia")), QString(tr("Przekroczono limit czasu na połączenie\n")+connect.error()), QMessageBox::Ok, this);
        error.exec();
        connect.closeConnect();
    }

    connect.closeConnect();
}

idąc za radą @MarekR22 - że klasa MainWindow ma być głupia jak but - co też uczyniłem

void MainWindow::sendMessage()
{
    emit _sendMessage();
}

i teraz w konstruktorze MainWindow muszę utworzyć obiekt TcpConnect

this->connect = new TcpConnect(QHostAddress::LocalHost, 12345, this);

i moje pytanie brzmi - czy warto tworzyć socket w konstruktorze MainWindow ?

Bo szczerze mówiąc bardziej podobało mi się rozwiązanie, gdy ten socket tworzył się podczas wywoływania tej funkcji jak pokazałem na samym początku tego postu.

Czysą są lepsze rozwiązania ? Czy to co robię jest dobre ?

0

A kiedy sobie uwidziałeś, że powstaje socket?

W momencie uruchomienia programu? To MainWindow nie jest takim złym miejscem na tworzenie socketa. W momencie kliknięcia na przycisk? To wtedy niech ten przycisk się tym zajmuje. I tak dalej — niech należy to do elementu, który logicznie za to odpowiada u Ciebie, żeby potem nie trzeba było szukać.

0

Althorion napisał(a):

A kiedy sobie uwidziałeś, że powstaje socket?

wtedy gdy kliknę przycisk Wyślij

W momencie uruchomienia programu?

teraz to się dzieje w momencie uruchomienia programu, bo robiąc teraz sygnał nie mam innego wyjścia

To MainWindow nie jest takim złym miejscem na tworzenie socketa.

czyli sugerujesz, że w konstruktorze tworzenie socketa będzie ok ?

W momencie kliknięcia na przycisk? To wtedy niech ten przycisk się tym zajmuje. I tak dalej — niech należy to do elementu, który logicznie za to odpowiada u Ciebie, żeby potem nie trzeba było szukać.

no tak ale to wtedy klasa MainWindow nie będzie już głupia jak but... i czego szukać bo nie rozumiem ?

0

@zkubinski:

wtedy gdy kliknę przycisk Wyślij

No to powinien to robić przycisk „Wyślij”.

teraz to się dzieje w momencie uruchomienia programu, bo robiąc teraz sygnał nie mam innego wyjścia

Dlaczego nie masz innego wyjścia?

czyli sugerujesz, że w konstruktorze tworzenie socketa będzie ok ?

Jeśli coś nie ma sensu istnieć bez czegoś, to powinno to mieć w konstruktorze. Jeśli uważasz, że okno główne nie ma sensu istnieć bez socketa — to jest to dziwne, ale niewykluczone. Ja bym oczekiwał, jako użytkownik, że coś tam mi wczytuje lub mnie pyta o konfigurację i potem coś z tej konfiguracji korzysta. Wówczas sens ma walidacja konfiguracji (w tym możliwości stworzenia binda) u konfiguratora, który potem udostępni bind dla (cokolwiek tego chce), które to sobie będzie brało jako parametr konstruktora.

no tak ale to wtedy klasa MainWindow nie będzie już głupia jak but... i czego szukać bo nie rozumiem ?

Ma być głupia jak but, ale nie głupsza. Ma robić tylko to, co potrzebuje i starać się potrzebować jak najmniej, żeby móc powstać i działać i komunikować użytkownikowi różne problemy; ale jeśli jest program, w którym absolutnie nie może żyć bez czegoś, to musi się tym zająć sama. Nie za kogoś po drodze — dla siebie samej.

0

Althorion napisał(a):

@zkubinski:

teraz to się dzieje w momencie uruchomienia programu, bo robiąc teraz sygnał nie mam innego wyjścia

Dlaczego nie masz innego wyjścia?

bo muszę połączyć sygnał ze slotem - czyli w MainWindow utworzyłem sygnał, który odpala slota w TcpConnect i żeby w ogóle to zrobić, to muszę utworzyć obiekt klasy TcpConnect w konstruktorze - jak zrobię inaczej, to funkcja QObject::connect nie zadziała.

czyli sugerujesz, że w konstruktorze tworzenie socketa będzie ok ?

Jeśli coś nie ma sensu istnieć bez czegoś, to powinno to mieć w konstruktorze.

teoretycznie się zgodzę - ale pytanie - jak po wysłaniu wiadomości zamknąć gniazdo ? Bo w sumie po co ma wisieć ?

Aplikacja sieciowa

zkubinski napisał(a):

kq napisał(a):

MarekR22 napisał(a):

zkubinski napisał(a):

zkubinski napisał(a):

enedil napisał(a):

zkubinski napisał(a):

zkubinski napisał(a):

Althorion napisał(a):

Althorion napisał(a):

1 użytkowników online, w tym zalogowanych: 0, gości: 1

Praca dla programistów

Forum dyskusyjne

Sprawy administracyjne

O nas

Skontaktuj się z nami