API wrapper best practices

Defohin

Hi guys, I'm new with Qt but I've been reading a lot and I think I'm on the right track, but I have a few questions regarding best practices and if you guys have tips of how I can improve my code a little bit.
I'm trying to create a wrapper around a simple API that I made locally but will be online pretty soon.
It's like pastebin, where you can paste text with a title.
The API is simple, it receives the text and the content and the api key that each user has. (I won't bother explaining that 'cause it has nothing to do with Qt).

Here is how the class looks like:

paste.h

#ifndef PASTE_H
#define PASTE_H

#include <QObject>
#include <QNetworkAccessManager>
#include <QNetworkReply>

class Paste : public QObject
{
    Q_OBJECT

public:
    enum PasteError {
        UnknownError,
        NetworkError,
        InvalidApiKeyError,
        InvalidJsonResponse
    };
    Q_ENUM(PasteError)

    explicit Paste(QObject *parent = 0);

    QString getTitle() const;
    void setTitle(const QString &value);

    QString getContent() const;
    void setContent(const QString &value);

    QString getApiKey() const;
    void setApiKey(const QString &value);

public slots:
    void paste();

    void requestFinished();
#ifndef QT_NO_SSL
    void requestSslErrors(QNetworkReply*, const QList<QSslError> &errors);
#endif
    void requestError(QNetworkReply::NetworkError code);
    void requestReadyRead();

signals:
    void error(PasteError error);
    void success(const QString& link);

private:
    QString title;
    QString content;
    QString apiKey;

    QNetworkAccessManager nam;
    QNetworkReply *reply;
};

#endif // PASTE_H

paste.cpp

#include "paste.h"
#include <QNetworkRequest>
#include <QUrlQuery>
#include <QJsonDocument>
#include <QJsonObject>

static const QString API_URL = "http://127.0.0.1/paste/api";

Paste::Paste(QObject *parent): QObject(parent), title(""), content(""), apiKey("")
{
    #ifndef QT_NO_SSL
        connect(&nam, &QNetworkAccessManager::sslErrors, this, &Paste::requestSslErrors);
    #endif
}

QString Paste::getTitle() const
{
    return title;
}

void Paste::setTitle(const QString &value)
{
    title = value;
}

QString Paste::getContent() const
{
    return content;
}

void Paste::setContent(const QString &value)
{
    content = value;
}

QString Paste::getApiKey() const
{
    return apiKey;
}

void Paste::setApiKey(const QString &value)
{
    apiKey = value;
}

void Paste::paste()
{
    QNetworkRequest request(API_URL);
    request.setHeader(QNetworkRequest::ContentTypeHeader, "application/x-www-form-urlencoded; charset=utf-8");

    QUrlQuery query;
    query.addQueryItem("title", getTitle());
    query.addQueryItem("content", getContent());
    query.addQueryItem("api_key", getApiKey());

    QByteArray body(query.query(QUrl::FullyEncoded).toUtf8());

    reply = nam.post(request, body);
    connect(reply, &QNetworkReply::finished, this, &Paste::requestFinished);
    connect(reply, &QIODevice::readyRead, this, &Paste::requestReadyRead);
}

void Paste::requestFinished()
{
    reply->deleteLater();
    reply = Q_NULLPTR;
}

#ifndef QT_NO_SSL
void Paste::requestSslErrors(QNetworkReply *, const QList<QSslError> &errors)
{
    Q_UNUSED(errors);
    reply->ignoreSslErrors();
    emit error(NetworkError);
}
#endif

void Paste::requestError(QNetworkReply::NetworkError code)
{
    Q_UNUSED(code);
    emit error(NetworkError);
}

void Paste::requestReadyRead()
{
    QByteArray response = reply->readAll();
    QJsonDocument jsonDocument(QJsonDocument::fromJson(response));

    if (jsonDocument.isNull() ||
      ! jsonDocument.isObject()
    ) {
        emit error(InvalidJsonResponse);
        return;
    }

    QJsonObject jsonObject = jsonDocument.object();

    if (! jsonObject.contains("status") &&
        ! jsonObject.contains("message")
    ) {
        emit error(InvalidJsonResponse);
        return;
    }

    QJsonValue status = jsonObject.value("status");
    QJsonValue message = jsonObject.value("message");

    if (status == "error") {
        if (message.toString().contains("key")) {
            emit error(InvalidApiKeyError);
            return;
        }

        emit error(UnknownError);
        return;
    }

    emit success(message.toString());
}

The questions I have is:

Is it right the way I declared a static const QString as the api url?
Is it needed in that case to instantiate the members with an empty string like on the constructor where I put title("")?
The only thing that Paste::requestFinished is doing is calling the deleteLater of the reply and setting it as a null pointer. Is there another usage for that method or is it wrong/right?
The same thing on Paste::requestSslErrors... read the 3.
On the Paste::requestReadyRead I check for a few things, like if the response is in Json format, if it contains status and message on it. status can be success or error, there are two errors that might occur, one is the wrong api key and the other one is unknown, maybe due to a malformed request and for that reason I'm using a few "ifs". Is it right the way I'm doing or is there a better way of doing that?
You might notice that I created a kind of struct with errors that it might return and I emitted them several times along the code, for instance on the Paste::requestReadyRead I call it and then return, but I don't know if I'm doing it right... I was wondering if the way I'm doing is also right.
The names success and errors are too abstract for signals? Do you have another alternative? Just to remember, the success will return an address link of the paste inside the message and the error will return the error.

A problem I'm having is to handle network problems on errors cause all the problems that a network might have is coming from the QNetworkReply but I'm having a class already with errors so I don't know how to give to the user the errors if it's a network error. I created an enum named NetworkError but it seems to be really abstract and not informative, what is the best way to integrate both my errors with the errors from the QNetworkReply?

Thank you all.

? Offline

Hi!

Yes, it's ok.
No, not needed, QStrings get initialized to an empty string automatically.

I think there is a logic bug in:

if (! jsonObject.contains("status") &&
        ! jsonObject.contains("message")
    ) {

If I understand your intentions right then it should be || instead of &&.

SGaist

Hi,

To add to @Wieland, it seems that you only have one public slot with paste all the others are "internal" to you class so you should either make them private or use lambdas.

Defohin

@Wieland I used && cause the Json has to be in that format, with status and message.

@SGaist I don't get what you mean... the usage for that class is like that:

Paster paster = new Paster(this);
paster->setTitle("a title");
paster->setContent("This is a content");
paster->setApiKey("abcdef");
paster->paste();

connect(paster, &Paster::sucess, [] (const QString& link) {
    qDebug() << link;
});

connect(paster, &Paster::error, [] (PasteError error) {
    qDebug() << error;
});

jsulm

@Defohin If you need to have both "status" and "message" in your json then you need to change to ||
Else you will only print error and return if none of them is inside.

Defohin

I realized what @SGaist said and now all the slots are private.
I also changed the logic that @jsulm and @Wieland mentioned.
Removed the QStrings from the constructor
Now it looks great. Thank you guys.