Qt - ćwiczenai z klas QJson*

Uczę się obsługi klas QJson* chciałbym wiedzieć czy mój kod technicznie jest poprawnie napisany - w sensie programistycznym. Nie wiem jak moje pytanie uściślić, może jak ktoś z was zerknie i wyłapie jakieś błędy czy to myślowe czy to składniowe czy cokolwiek to dajcie znać

#include <QJsonObject>
#include <QJsonArray>
#include <QJsonDocument>
#include <QJsonValue>
#include <QFile>
#include <QVariant>
#include <QDebug>

using namespace std;

class Settings
{
public:
    Settings();

    inline void setConnect(QString _connect){
        connect=_connect;
    }

    inline QString getConnect() const
    {
        return connect;
    }

    inline void setIPv4(QString _ipv4){
        ipv4=_ipv4;
    }

    inline QString getIPv4()
    {
        return ipv4;
    }

    inline void writeSetg(QJsonObject &obj){
        obj["connection_type"]=getConnect();
        obj["ipv4"]=getIPv4();
    }

private:
    QString connect;
    QString ipv4;
};

Settings::Settings() : connect("localhost"), ipv4("127.0.0.1")
{
}

class saveSettings
{
public:

    inline void saveSetg(QJsonObject &obj){
        QJsonObject _obj;
        setg.writeSetg(_obj);
        obj["settings"]=_obj;
    }

private:
    Settings setg;
};

int main()
{
    QJsonObject object;

    saveSettings saveSetg;
    saveSetg.saveSetg(object);

    QJsonDocument document(object);

    QFile file;
    file.setFileName("settings.json");

    if(!file.open(QIODevice::WriteOnly | QIODevice::Text | QIODevice::Truncate)){
        qDebug()<< "Blad otwarcia bufora pliku";
    }
    else{
        file.write(document.toJson());
    }

    return 0;
}

Nie jestem zwolennikiem nazywania getterów od "get", ale to kwestia konwencji. Natomiast powiedzieć, że nazwa connect przypisywana do connectionType zawierająca coś co wygląda jak hostname wprowadza w zakłopotanie to eufemizm.
Dobrą praktyką jest nienazywanie niczego z podłogą (_) na początku, bo możesz odbić się od kolizji nazw.
Nie trzymasz się konwencji klamerek, czasem piszesz od nowej linii, czasem po nawiasie.
Nazewnictwo klas, czasem od wielkiej, czasem od małej litery.
Po co w ogóle klasa saveSettings mają pole settings? Po co ona w ogóle? Jak już musisz funktor to nazwij "SettingsSaver", ale równie dobrze mógłbyś zrobić "freestanding function" przyjmującą obiekt settings jako parametr. Tzn. nie jest to wymóg, ale dobrą praktyką jest nazywanie klas rzeczownikami, funkcji/metod czasownikami, wtedy kod się czyta dość naturalnie.

Symbole zaczynające się od undersocre są zarezerwowane dla kompilatora i standardowej biblioteki.

inline jest historycznym spadkiem i w zasadzie nie ma obecnie zastosowania.
Jedyny wyjątek: to funkcje (nie metody), które są definiowane w nagłówku (wtedy to jest sygnał dla kompilatora by ignorować duplikaty definicji). W przypadku metod to się dzieje domyślnie.

connect to jest czasownik, wiec użyte w kontekście pola klasy, jest niezrozumiałe. A jak jeszcze widzę connect("localhost") oraz obj["connection_type"]=getConnect(); to już zupełnie nie rozumiem o co chodzi, bo localhost to domyślna nazwa adresu "loop back" 127.0.0.1, więc nie ma przełożenia na "rodzaj" połączenia.

tutaj miałem na myśli rodzaj połączenia w sensie - czy ktoś się łączy do zdalnej bazy czy lokalnej - jeżeli do lokalnej, to nie musi pisać adresu, jeżeli zdalny to musi podać adres - tak czy inaczej też nie wiem jak to sensownie by nazwać

@MarekR22

inline jest historycznym spadkiem i w zasadzie nie ma obecnie zastosowania.

rozumiem, że w żadnym przypadku nie ma już sensu tego pisać ?

@MarekR22:

Jedyny wyjątek: to funkcje (nie metody) (...)

tak się czepnę - często czytam w różnej literaturze, że funkcja == metoda i te pojęcia stosują wymiennie dając do zrozumienia, że czytelnik rozumie o co chodzi, czyli funkcja coś robi i zwraca wynik bądź nie zwraca nic, z metodą jest tak samo - tylko problem jest taki, że nie spotkałem wyjaśnienia jakie są różnice w pojęciach funkcja i metoda - więc ktoś wyjaśni jakie są różnice między jednym, a drugim ?

tutaj miałem na myśli rodzaj połączenia w sensie - czy ktoś się łączy do zdalnej bazy czy lokalnej - jeżeli do lokalnej, to nie musi pisać adresu,

A dlaczego? 127.0.0.1 . Powinno to być ogólne.

Czuję dużą niepewność z twojej strony. Wiec klasyki(wiadomo nie we wszystkim trzeba się zgadzać z klasykami)
https://helion.pl/ksiazki/czysty-kod-podrecznik-dobrego-programisty-robert-c-martin,czykov.htm#format/d
https://helion.pl/ksiazki/refaktoryzacja-ulepszanie-struktury-istniejacego-kodu-wydanie-ii-martin-fowler,refak2.htm#format/d
https://helion.pl/ksiazki/software-craftsman-profesjonalizm-czysty-kod-i-techniczna-perfekcja-sandro-mancuso,prorze.htm#format/d

chcę jeszcze wrócić do wątku

@alagner

Nie jestem zwolennikiem nazywania getterów od "get", ale to kwestia konwencji.

czytam artykuł i w moim odczuciu nie za wiele z niego wynika oprócz kosmetycznej zmiany nazw metod.

np według autora złe jest użycie

Dog dog = new Dog();

dog.setBall(new Ball()); //to jest złe

natomiast poprawne jest

Dog dog = new Dog();

dog.take(new Ball()); //to jest dobre

oprócz zmiany nazwy nic tu nie widzę - dobrze by było gdyby pokazana była też "mechanika" tego kodu, bo może coś więcej z tego wyniknie ? Innymi słowy nie widzę żadnych istotnych zmian, oprócz nazewnictwa metod. W takim razie całe Qt jest niedobre, bo ma od groma metod zaczynających się na get lub set

Właśnie o nazewnictwo się rozchodzi. Nie bez powodu mówi się

There are only two hard things in Computer Science: cache invalidation, naming things and off-by-one errors

zkubinski napisał(a):

czytam artykuł i w moim odczuciu nie za wiele z niego wynika oprócz kosmetycznej zmiany nazw metod.

np według autora złe jest użycie

Dog dog = new Dog();

dog.setBall(new Ball()); //to jest złe

natomiast poprawne jest

Dog dog = new Dog();

dog.take(new Ball()); //to jest dobre

Nie zgadzam się. Gdyby klasa miała kilka pól które można ustawiać, przy takim podejściu nie wiadomo bez szukania jak się nazywa metoda przyjmująca akurat obiet typu Ball. Zamiast czytelnego set i get mamy radosną twórczość.
Przyczepianie się do dog.setWeight("23kg"); że to nierealne też uważam za niepoważne.

trochę zmieniłem kod według waszych rad i wygląda on tak

#include <QJsonObject>
#include <QJsonArray>
#include <QJsonDocument>
#include <QJsonValue>
#include <QVector>
#include <QDebug>
#include <QFile>

class Network
{
public:
    Network();

    void setConnect(QString conn){
        connect=conn;
    }

    QString Connect(){
        return connect;
    }

    void setIPv4(QString addr){
        ipv4=addr;
    }

    QString IPv4(){
        return ipv4;
    }

    void WriteNetwork(QJsonObject &obj){
        obj["connect"]=connect;
        obj["ipaddr"]=ipv4;
    }

private:
    QString ipv4;
    QString connect;
};

Network::Network() : ipv4("127.0.0.1"), connect("localhost")
{}

class Data
{
public:
    Data();

    void setDept(QString str){
        data=str;
    }

    QString Dept(){
        return data;
    }

    void WriteData(QJsonObject &obj){
        obj["dept"]=data;
    }

private:
    QString data;
};

Data::Data() : data("Wybierz wydzial")
{}

class Settings
{
public:
    Settings();

    QJsonObject ChangedSettings(QJsonObject &obj){
        QJsonObject net;
        nt.WriteNetwork(net);
        obj["network"]=net;

        QJsonObject dat;
        dt.WriteData(dat);
        obj["data-from"]=dat;

        return obj;
    }

    void SaveSettings(QJsonObject &obj){
        QJsonObject set;
        obj["settings"]=ChangedSettings(set);
    }

//    void ReadSettings();

private:
    Network nt;
    Data dt;
    QVector <QJsonObject> set;
};

Settings::Settings()
{
    qDebug()<< nt.IPv4();
    qDebug()<< dt.Dept();
}

int main()
{
    Settings st;
    QJsonObject object;

    st.ChangedSettings(object);
    st.SaveSettings(object);

    QJsonDocument document(object);

    QFile file;
    file.setFileName(QString("settings.json"));

    if(!file.open(QIODevice::WriteOnly | QIODevice::Text | QIODevice::Truncate)){
        qDebug()<< "Blad otwarcia bufora pliku";
    }
    else{
        file.write(document.toJson());
        file.flush();
    }
}

moim zamiarem jest uzyskać taką strukturę pliku:

"settings": {
        "data-from": {
            "dept": "Wybierz wydzial"
        },
        "network": {
            "connect": "localhost",
            "ipaddr": "127.0.0.1"
        }
    }

a otrzymuję coś takiego

{
    "data-from": {
        "dept": "Wybierz wydzial"
    },
    "network": {
        "connect": "localhost",
        "ipaddr": "127.0.0.1"
    },
    "settings": {
        "data-from": {
            "dept": "Wybierz wydzial"
        },
        "network": {
            "connect": "localhost",
            "ipaddr": "127.0.0.1"
        }
    }
}

wiem, że robię coś źle w funkcji ChangedSettings ale nie wiem jak to poprawić. Czy ktoś coś podpowie ?

Już doszedłem co robię źle - poniżej poprawiona funkcja w kilku wersjach i zastanawiam się nad kilkoma rzeczami

1. Która z tych funkcja jest lepsza ?

void SaveSettings(QJsonObject &obj){
        QJsonObject set, net, _net, dat, _dat;

        nt.WriteNetwork(_net);
        net["network"]=nt.ShowJsonObjectNetwork();

        dt.WriteData(_dat);
        dat["data-from"]=dt.ShowJsonObjectData();

        for(QString &str : net.keys()){
            set.insert(str,_net); //set.insert(net.keys().at(0),_net);
        }

        for(QString &str : dat.keys()){
            set.insert(str,_dat); //set.insert(dat.keys().at(0),_dat);
        }

        obj["settings"]=set;
    }

czy ta (wiem, że można poniższą funkcję można zrobić na pętlach ale to było robione "na szybko"

void SaveSettings(QJsonObject &obj){
        QJsonObject set, net, dat;

        net.insert("connect",nt.Connect());
        net.insert("ipaddr",nt.IPv4());

        dat.insert("dept",dt.Dept());

        set.insert("network",net);
        set.insert("data-form",dat);

        obj["settings"]=set;
    }

2. jak tam oceniacie poprawiony kod ? Niebawem wrzucę całość ale muszę dorobić wczytywanie JSON-a

Nie rozumiem czemu pracujesz na out parametrach zamiast normalnie zwracać wartość. Ponadto net, _net i nt, dat, _dat i dt to jakieś ćwiczenie w tworzeniu nieczytelnego nazewnictwa?

        for(QString &str : net.keys()){
            set.insert(str,_net); //set.insert(net.keys().at(0),_net);
        }

        for(QString &str : dat.keys()){
            set.insert(str,_dat); //set.insert(dat.keys().at(0),_dat);
        }

Dodajesz klucze z tą samą zawartością.

Nie rozumiem co tu się dzieje, nie przepuściłbym tego przez code review.

@kq:

Nie rozumiem czemu pracujesz na out parametrach zamiast normalnie zwracać wartość.

nazwy nadałem skrótowe od pełnych nazw np network - net, nt bo nie chciałem ich powielać - a jak mam zwracać normalnie wartość ? Nie rozumiem ? Możesz podać przykład ? Tzn wiem jak normalnie zwrócić wartość ale w tym przypadku to się zastanawiam jak to sensownie zrobić

Nie chciałeś powielać nazw, więc masz net, _net i nt? Boję się ile tego by było gdybyś jednak chciał powielać.

Nie rozumiem czemu sygnatura funkcji nie wygląda tak:

QJsonObject settings();

@kq:

Nie rozumiem czemu sygnatura funkcji nie wygląda tak:

QJsonObject settings();

bo w sumie wszystko robię według dokumentacji i tam mniej więcej w ten sposób robią i do funkcji musiałem przyjąć argument QJsonObject bo bez tego argumentu nie uzyskałbym nawet takiej struktury pliku

{
    "settings": {
        "data-from": {
            "dept": "Wybierz wydzial"
        },
        "network": {
            "connect": "localhost",
            "ipaddr": "127.0.0.1"
        }
    }
}

zakładając, że coś by mi się zapisało w tym pliku

Jak nie? To nie wolno zrobić jak poniżej?

QJsonDocument doc;
QJsonObject root;
root["settings"] = settings();
doc.setObject(root);
// write

Wersja, na której się wzorujesz zakłada zapewne C++03-ową przedwczesną optymalizację, nie widzę powodu aby bez podparcia benchmarkami zmniejszać czytelność kodu mając do dyspozycji move semantics.

robiłem tak ale niestety to nie działało, dostawałem inną strukturę - bez zagnieżdżeń jak wyżej pokazałem

kod skończony - gdyby ktoś z was mógł na to rzucić okiem i powiedzieć czy można coś zrobić lepiej... (na razie pomijam to co wspominał @kq o nazewnictwie zmiennych)

Kod zapisuje do pliku JSON, wczytuje z pliku JSON i wyniki wyświetla na ekranie

#include <QJsonObject>
#include <QJsonArray>
#include <QJsonDocument>
#include <QJsonValue>
#include <QVector>
#include <QDebug>
#include <QFile>
#include <QByteArray>
#include <QTextStream>

class Network
{
public:
    Network();

    void setConnect(QString conn){
        connect=conn;
    }

    QString Connect(){
        return connect;
    }

    void setIPv4(QString addr){
        ipv4=addr;
    }

    QString IPv4(){
        return ipv4;
    }

    void WriteNetwork(QJsonObject &obj){
        obj["connect"]=connect;
        obj["ipaddr"]=ipv4;

        myObj=obj;
    }

    QJsonObject ShowJsonObjectNetwork(){
        return myObj;
    }

private:
    QString ipv4;
    QString connect;

    QJsonObject myObj;
};

Network::Network() : ipv4("127.0.0.1"), connect("localhost")
{}

class Data
{
public:
    Data();

    void setDept(QString str){
        data=str;
    }

    QString Dept(){
        return data;
    }

    void WriteData(QJsonObject &obj){
        obj["dept"]=data;

        myObj=obj;
    }

    QJsonObject ShowJsonObjectData(){
        return myObj;
    }

private:
    QString data;
    QJsonObject myObj;
};

Data::Data() : data("Wybierz wydzial")
{}

class Settings
{
public:
    Settings();

    void SaveSettings(QJsonObject &obj){
        QJsonObject set, net, _net, dat, _dat;

        nt.WriteNetwork(_net);
        net["network"]=nt.ShowJsonObjectNetwork();

        dt.WriteData(_dat);
        dat["data-from"]=dt.ShowJsonObjectData();

        for(QString &str : net.keys()){
            set.insert(str,_net); //set.insert(net.keys().at(0),_net);
        }

        for(QString &str : dat.keys()){
            set.insert(str,_dat); //set.insert(dat.keys().at(0),_dat);
        }

        obj["settings"]=set;
    }

    void ReadSettings(QJsonObject &obj){
        QFile loadFile;

        loadFile.setFileName(QString("settings.json"));

        if(!loadFile.open(QIODevice::ReadOnly)){
            qDebug()<< "Nie mozna zaladowac pliku";
        }
        else{
            QByteArray loadData;
            loadData = loadFile.readAll();

            QJsonDocument loadDoc;
            loadDoc = QJsonDocument::fromJson(loadData);
            obj = loadDoc.object();

            qDebug()<< "Plik wczytano" << Qt::endl;

            QJsonObject::iterator i;

            for(i=obj.begin(); i!=obj.end(); ++i){
                qDebug()<< "object" << i.key();

                QJsonObject nextObj = i.value().toObject();
                QJsonObject::iterator j;

                for(j=nextObj.begin();j!=nextObj.end(); ++j){
                    if(j.value().isObject()==true){
                        qDebug()<< "\t object" << j.key();
                    }

                    QJsonObject nextObj = j.value().toObject();
                    QJsonObject::iterator k;

                    for(k=nextObj.begin(); k!=nextObj.end(); ++k){
                        qDebug()<< "\t\t"<< nextObj.value(k.key()).toString();
                    }
                }
            }

            qDebug()<< Qt::endl;          
        }
    }

private:
    Network nt;
    Data dt;
};

Settings::Settings()
{
}

int main()
{
    Settings st;

    QJsonObject object;

    st.ReadSettings(object);
    st.SaveSettings(object);

    QJsonDocument document(object);

    QFile file;
    file.setFileName(QString("settings.json"));

    if(!file.open(QIODevice::WriteOnly | QIODevice::Text | QIODevice::Truncate)){
        qDebug()<< "Blad otwarcia bufora pliku";
    }
    else{
        file.write(document.toJson());
        file.flush();

        qDebug()<< "Plik zapisany";
    }
}

Wybacz za zwłokę, ale trochę mi się weekendowe plany pozmieniały. Ad rem:
odnośnie setterów/getterów: liczy się kontekst. Niemniej u Ciebie to są "przelotki" do pól klasy, nic więcej. Interfejs:

class Network 
{
public:
	void setIp(const QString& addr) {
		m_addr = addr;
	}
	QString addr() const {
		returm m_addr;
	}
private:
	QString m_addr;
};

możesz z powodzeniem zamienić na:

class Network 
{
public:
	QString m_addr;
};

albo nawet pójść po całości:

struct Network 
{
	QString m_addr;
};

W QT settery mają jakiś tam sens z uwagi na ew. możliwość emitowania sygnałów, kiedy coś się zmieni, ale nie korzystasz z tego.

Ktoś może powiedzieć "ale walidacja". Tu jej nie robisz, ale weźmy przykład

class ProgressBar {
public:
    void setPercentage(uint8_t percents) {
        if (percents > 100) throw out_of_range("Progress's percentage cannot exceed 100");
        m_percentage = percents;
    }
    void redraw();
private:
    uint8_t m_percentage;
};

Tak naprawdę walidację/nasycanie można przerzucić na samą klasę przechowującą dane i będzie to miało sens o tyle większy, że od razu widzisz jakiego typu wartości są

//poniższą klasę może być warto "stemplatkować", zarówno z uwagi na typ, jak i wartości...
struct U8ProgressPercentage {
    explicit U8ProgressPercentage (uint8_t number)
        : m_number(number)
    {
        if (number > 100) throw out_of_range("Progress's percentage cannot exceed 100");
    }
    operator uint8_t() const {return m_number;}
    //ciach, dalej resztę interfejsu sobie dośpiewaj....
};


class ProgressBar {
public:
    U8ProgressPercentage m_percentage;
    void redraw();
};

@Azarien jeżeli nie piszesz w gołym Vimie albo Emacsie, to typ argumentu albo i całą sygnaturę funkcji podpowie Ci edytor, także Dog może tego Balla fetch, pickUp, albo carry i będzie to OK, w zależności od tego co ten pies ma konkretnie reprezentować. Także argument, że nie widać co się dzieje jest imho słaby. Jak psu potrzebujesz ustawiać piłkę, to raczej abstrakcja kuleje. Ale dla jasności: nie widzę problemu żeby np. moduł klimatyzatora miał setTemperature. Oczywiście, to są wszystko zalecenia i dobre praktyki a życie sobie...
Odn. setWeight: jeżeli ten pies reprezentuje NPCa w grze, jego waga może być zmieniana powiedzmy czarami i ma to wpływ na zachowanie z punktu widzenia fizyki (nie wiem, pęd przy uderzeniu w ziemię podczas upadku przelicza się na obrażenia... przykład z kapelusza) to ten setter może mieć sens, można także pomyśleć o upublicznieniu pola i może styknie. Ale z drugiej strony, jak ten pies ma być lekkim obiektem z weterynaryjnej bazy danych, to nie widzę sensu tego settera, można po prostu stary obiekt podmienić na nowy, interfejs będzie czystszy imho (albo i będzie to niemożliwe, bo np. framework wymaga że setter ma być i wtedy koniec).
Ale znowu: kontekst, kontekst, kontekst.

Imho usprawiedliwionym jest istnienie settera (ale nie będę się przy tym upierał, no i pytanie czy to nadal jest typowy setter) jeśli ma jeszcze wywołać skutki uboczne np. emitować sygnał albo przerysować obiekt po zmianie wartości, vide (ale zobacz jak ta funkcja się nazywa):

class ProgressBar {
public:
    void setValueAndRedraw(uint8_t value) {
        m_percentage = U8ProgressPercentage(value);
        redraw();
   }
    void redraw();
private:
    U8ProgressPercentage m_percentage;
    
};

Tyle ogólników.

Co do Twojego kodu:
Settery i gettery są głupimi przelotkami. Ja bym je wyrzucił. Opcja druga: wszystkie pola zrobić prywatne, dać tylko gettery jeśli to potrzebne a ustawiać je na etapie konstrukcji.
Ładujesz wszystko przez wartość, np. QString. To niekoniecznie błąd, ale imho dobrą praktyką jest używanie albo const referencji, albo odpowiednie używanie move semantics (std::move) jeśli jednak lecisz po wartościach.
Metody niemodyfikujące obiektu powinny być const, tego nie ma.
Nazewnictwo tragedia. Coś się na ten connect tak uparł? No i decyduj się czy metody od małych czy dużych liter. bo tu zero konsekwencji.

Te wszystkie for od begin do end to chyba dałoby się for_each/ranged for zastąpić?
BTW, zamiast

JasiuIKasia::iterator i;
for (i = kasiazjasiem.begin(); //ciach....

możesz zrobić

for (auto i {kasiazjasiem.begin()}; //ciach

Na koniec: metoda ReadSettings sprawia wrażenie prób chodzenia po drzewie. Jak wiesz jaki tam masz format konfiga, będzie on dobry i taki jak zakładasz, to do diabła z tym, ale koncepcyjnie to imho możnaby też poprawić.