trjhtr

Task:

Class for connecting to a mqtt broker using QMqttClient and getting notified on connect and disconnect.

Context:

I'm trying to write modern C++ (standard 2011 and up) code - using smart pointers. I have only created small projects for learning C++. I'm not very confident if I'm using those pointers correctly.

The implementation:

The implementation is in the client.h and client.cpp header file. The first code snippet is the client.h one.

#ifndef CLIENT_H
#define CLIENT_H

#include <QString>
#include <QObject>
#include <QtMqtt/QtMqtt>
#include <memory>
#include <iostream>

struct MqttConfig 
 MqttConfig() 
 hostname = "localhost";
 port = 1883;
 

 QString hostname;
 quint16 port;
;

struct SubscriptionConfig

 SubscriptionConfig() = delete;
 SubscriptionConfig(QString t, quint16 q) :
 topic(t), qos(q)
 

 QString topic;
 quint16 qos; // quality of service
;

class Client : public QObject

 Q_OBJECT

private:
 std::unique_ptr<QMqttClient> mqtt;
 std::unique_ptr<MqttConfig> c;

public:
 Client();
 void connectToSignals();
 void configureClient(std::unique_ptr<MqttConfig>&);
 void connect();

public slots:
 void onConnected()
 std::cout << "Connected to mqtt broker " << std::endl; 
 void onDisconnected()
 std::cout << "Disconnected from mqtt broker" << std::endl; 
;

#endif // CLIENT_H

Am I violating the DRY principle (don't repeat yourself) in the method connectToSignals ? And how could I avoid this?

#include "client.h"

Client::Client()

 mqtt = std::make_unique<QMqttClient>();
 c = std::make_unique<MqttConfig>();
 configureClient(c);
 connectToSignals();
 connect();


void Client::connectToSignals()

 QObject::connect(Client::mqtt.get(), &QMqttClient::connected, this,
 &Client::onConnected);
 QObject::connect(Client::mqtt.get(), &QMqttClient::disconnected, this,
 &Client::onDisconnected);


void Client::configureClient(std::unique_ptr<MqttConfig>& c)

 mqtt->setHostname(c->hostname);
 mqtt->setPort(c->port);


void Client::connect()

 mqtt->connectToHost();

Qt-ness

My QT is a little bit rusty, but you're probably not doing the right thing here. First QObject have their own memory management, usually each QObject constructor takes a parent, this parent takes ownership of the created object. As the setParent() function is always available, interfering with the memory management via std::unique_ptr or std::shared_ptr is usually not a good idea.

Additionally you are containing QMQTTClient for what looks like just configuration reasons, there really doesn't seem to be any need to wrap it in another layer, you might as well just replace your client with QMQTTClient and connect to it. If you want to wrap configuration, a free function will do with much less overhead.

Stack-vs-Heap

Both the QMQTTClient and the MQTTConfig could as well be stack allocated, making everything easier. Passing MQTTConfig by reference will work well enough.

MQTTConfig

The constructor should use initializer lists and not the function body for member variable initialization.

MqttConfig() : hostname("localhost"), port(1883) 

I'd probably prefer default values for this,

MqttConfig(QString h = "localhost", qint16 p = 1883 ) : hostname(h), port(p) 

SubscriptionConfig

The default constructor doesn't need to be deleted if you are defining a custom constructor