Canbusutil: General cleanup and simplification

* Order includes and use forward-declarations
* Use member initialization and nullptr
* Transfrom last connects to Qt5 style
* Translate output messages
* Name slots better
* Discard frames with invalid frame ID or payload size
* Simplified some error handling and error outputting

Change-Id: Id4d4329afe909ba932e91ad1deeae411806fd77f
Reviewed-by: Alex Blasche <alexander.blasche@qt.io>
Reviewed-by: Rolf Eike Beer <eb@emlix.com>

5 files changed, 84 insertions, 81 deletions

diff --git a/src/tools/canbusutil/canbusutil.cpp b/src/tools/canbusutil/canbusutil.cpp
index c553c52..38433bb 100644
--- a/src/tools/canbusutil/canbusutil.cpp
+++ b/src/tools/canbusutil/canbusutil.cpp
@@ -36,8 +36,11 @@
 
 #include "canbusutil.h"
 
-CanBusUtil::CanBusUtil(QTextStream &output, QCoreApplication &app, QObject *parent)
-  : QObject(parent),
+#include <QCoreApplication>
+#include <QTextStream>
+
+CanBusUtil::CanBusUtil(QTextStream &output, QCoreApplication &app, QObject *parent) :
+    QObject(parent),
     m_canBus(QCanBus::instance()),
     m_output(output),
     m_app(app),
@@ -58,7 +61,7 @@ void CanBusUtil::setShowFdFlags(bool showFdFlags)
 bool CanBusUtil::start(const QString &pluginName, const QString &deviceName, const QString &data)
 {
     if (!m_canBus) {
-        m_output << "Unable to create QCanBus" << endl;
+        m_output << tr("Error: Cannot create QCanBus.") << endl;
         return false;
     }
 
@@ -73,29 +76,41 @@ bool CanBusUtil::start(const QString &pluginName, const QString &deviceName, con
     if (m_listening) {
         if (m_readTask->isShowFdFlags())
              m_canDevice->setConfigurationParameter(QCanBusDevice::CanFdKey, true);
-        connect(m_canDevice.data(), &QCanBusDevice::framesReceived, m_readTask, &ReadTask::checkMessages);
+        connect(m_canDevice.data(), &QCanBusDevice::framesReceived,
+                m_readTask, &ReadTask::handleFrames);
     } else {
         if (!sendData())
             return false;
-        QTimer::singleShot(0, &m_app, SLOT(quit()));
+        QTimer::singleShot(0, &m_app, QCoreApplication::quit);
     }
 
     return true;
 }
 
-void CanBusUtil::printPlugins()
+int CanBusUtil::printPlugins()
 {
+    if (!m_canBus) {
+        m_output << tr("Error: Cannot create QCanBus.") << endl;
+        return 1;
+    }
+
     const QStringList plugins = m_canBus->plugins();
-    for (int i = 0; i < plugins.size(); i++)
-        m_output << plugins.at(i) << endl;
+    for (const QString &plugin : plugins)
+        m_output << plugin << endl;
+    return 0;
 }
 
 int CanBusUtil::printDevices(const QString &pluginName)
 {
+    if (!m_canBus) {
+        m_output << tr("Error: Cannot create QCanBus.") << endl;
+        return 1;
+    }
+
     QString errorMessage;
     const QList<QCanBusDeviceInfo> devices = m_canBus->availableDevices(pluginName, &errorMessage);
     if (!errorMessage.isEmpty()) {
-        m_output << "Error: " << errorMessage << endl;
+        m_output << tr("Error gathering available devices: '%1'").arg(errorMessage) << endl;
         return 1;
     }
 
@@ -108,12 +123,12 @@ bool CanBusUtil::parseDataField(quint32 &id, QString &payload)
 {
     int hashMarkPos = m_data.indexOf('#');
     if (hashMarkPos < 0) {
-        m_output << "Data field invalid: No hash mark found!" << endl;
+        m_output << tr("Data field invalid: No hash mark found!") << endl;
         return false;
     }
 
     id = m_data.leftRef(hashMarkPos).toUInt(nullptr, 16);
-    payload = m_data.right(m_data.length() - hashMarkPos - 1);
+    payload = m_data.right(m_data.size() - hashMarkPos - 1);
 
     return true;
 }
@@ -121,37 +136,30 @@ bool CanBusUtil::parseDataField(quint32 &id, QString &payload)
 bool CanBusUtil::setFrameFromPayload(QString payload, QCanBusFrame *frame)
 {
     if (!payload.isEmpty() && payload.at(0).toUpper() == 'R') {
-        bool validPayloadLength = false;
-
         frame->setFrameType(QCanBusFrame::RemoteRequestFrame);
 
-        if (payload.size() == 1) { // payload = "R"
-            validPayloadLength = true;
-        } else if (payload.size() > 1) { // payload = "R8"
-            payload = payload.mid(1);
-            int rtrFrameLength = payload.toInt(&validPayloadLength);
-
-            if (validPayloadLength && rtrFrameLength >= 0 && rtrFrameLength <= 8) {
-                frame->setPayload(QByteArray(rtrFrameLength, 0));
-            } else if (validPayloadLength) {
-                m_output << "The length must be between 0 and 8 (including)." << endl;
-                validPayloadLength = false;
-            }
-        }
+        if (payload.size() == 1) // payload = "R"
+            return true;
 
-        if (!validPayloadLength) {
-            m_output << "Data field invalid: RTR data frame length not specified/valid." << endl;
+        bool ok = false;
+        int rtrFrameLength = payload.midRef(1).toInt(&ok);
+        if (ok && rtrFrameLength >= 0 && rtrFrameLength <= 8) { // payload = "R8"
+            frame->setPayload(QByteArray(rtrFrameLength, 0));
+            return true;
         }
 
-        return validPayloadLength;
-    } else if (!payload.isEmpty() && payload.at(0) == '#') {
+        m_output << tr("Error: RTR frame length must be between 0 and 8 (including).") << endl;
+        return false;
+    }
+
+    if (!payload.isEmpty() && payload.at(0) == '#') {
         frame->setFlexibleDataRateFormat(true);
-        payload = payload.mid(1);
+        payload.remove(0, 1);
     }
 
     const QRegularExpression re(QStringLiteral("^[0-9A-Fa-f]*$"));
     if (!re.match(payload).hasMatch()) {
-        m_output << "Data field invalid: Only hex numbers allowed." << endl;
+        m_output << tr("Data field invalid: Only hex numbers allowed.") << endl;
         return false;
     }
 
@@ -163,7 +171,7 @@ bool CanBusUtil::setFrameFromPayload(QString payload, QCanBusFrame *frame)
             frame->setErrorStateIndicator(flags & ErrorStateIndicatorFlag);
             payload.remove(0, 1);
         } else {
-            m_output << "Data field invalid: Size is not multiple of two." << endl;
+            m_output << tr("Data field invalid: Size is not multiple of two.") << endl;
             return false;
         }
     }
@@ -172,8 +180,8 @@ bool CanBusUtil::setFrameFromPayload(QString payload, QCanBusFrame *frame)
 
     const int maxSize = frame->hasFlexibleDataRateFormat() ? 64 : 8;
     if (bytes.size() > maxSize) {
-        m_output << "Warning: Truncating payload at max. size of " << maxSize << " bytes." << endl;
-        bytes.truncate(maxSize);
+        m_output << tr("Data field invalid: Size is longer than %1 bytes.").arg(maxSize) << endl;
+        return false;
     }
 
     frame->setPayload(bytes);
@@ -184,20 +192,18 @@ bool CanBusUtil::setFrameFromPayload(QString payload, QCanBusFrame *frame)
 bool CanBusUtil::connectCanDevice()
 {
     if (!m_canBus->plugins().contains(m_pluginName)) {
-        m_output << "Could not find suitable plugin." << endl;
-        m_output << "Available plugins:" << endl;
-        printPlugins();
+        m_output << tr("Cannot find CAN bus plugin '%1'.").arg(m_pluginName) << endl;
         return false;
     }
 
     m_canDevice.reset(m_canBus->createDevice(m_pluginName, m_deviceName));
     if (!m_canDevice) {
-        m_output << "Unable to create QCanBusDevice with device name: " << m_deviceName << endl;
+        m_output << tr("Cannot create CAN bus device: '%1'").arg(m_deviceName) << endl;
         return false;
     }
-    connect(m_canDevice.data(), &QCanBusDevice::errorOccurred, m_readTask, &ReadTask::receiveError);
+    connect(m_canDevice.data(), &QCanBusDevice::errorOccurred, m_readTask, &ReadTask::handleError);
     if (!m_canDevice->connectDevice()) {
-        m_output << "Unable to connect QCanBusDevice with device name: " << m_deviceName << endl;
+        m_output << tr("Cannot create CAN bus device: '%1'").arg(m_deviceName) << endl;
         return false;
     }
 
@@ -210,15 +216,15 @@ bool CanBusUtil::sendData()
     QString payload;
     QCanBusFrame frame;
 
-    if (parseDataField(id, payload) == false)
+    if (!parseDataField(id, payload))
         return false;
 
-    if (setFrameFromPayload(payload, &frame) == false)
+    if (!setFrameFromPayload(payload, &frame))
         return false;
 
     if (id > 0x1FFFFFFF) { // 29 bits
-        id = 0x1FFFFFFF;
-        m_output << "Warning! Id does not fit into Extended Frame Format, setting id to: " << id << endl;
+        m_output << tr("Cannot send invalid frame ID: '%1'").arg(id, 0, 16) << endl;
+        return false;
     }
 
     frame.setFrameId(id);
diff --git a/src/tools/canbusutil/canbusutil.h b/src/tools/canbusutil/canbusutil.h
index 356b3df..e6432a3 100644
--- a/src/tools/canbusutil/canbusutil.h
+++ b/src/tools/canbusutil/canbusutil.h
@@ -37,16 +37,16 @@
 #ifndef CANBUSUTIL_H
 #define CANBUSUTIL_H
 
+#include "readtask.h"
+
 #include <QObject>
-#include <QTextStream>
-#include <QCoreApplication>
 #include <QScopedPointer>
 
-#include "readtask.h"
-
 QT_BEGIN_NAMESPACE
 
 class QCanBusFrame;
+class QCoreApplication;
+class QTextStream;
 
 QT_END_NAMESPACE
 
@@ -59,7 +59,7 @@ public:
     void setShowTimeStamp(bool showTimeStamp);
     void setShowFdFlags(bool showFdFlags);
     bool start(const QString &pluginName, const QString &deviceName, const QString &data = QString());
-    void printPlugins();
+    int  printPlugins();
     int  printDevices(const QString &pluginName);
 
 private:
@@ -69,15 +69,15 @@ private:
     bool sendData();
 
 private:
-    QCanBus *m_canBus;
+    QCanBus *m_canBus = nullptr;
     QTextStream &m_output;
     QCoreApplication &m_app;
-    bool m_listening;
+    bool m_listening = false;
     QString m_pluginName;
     QString m_deviceName;
     QString m_data;
     QScopedPointer<QCanBusDevice> m_canDevice;
-    ReadTask *m_readTask;
+    ReadTask *m_readTask = nullptr;
 };
 
 #endif // CANBUSUTIL_H
diff --git a/src/tools/canbusutil/main.cpp b/src/tools/canbusutil/main.cpp
index 1304f22..6744f19 100644
--- a/src/tools/canbusutil/main.cpp
+++ b/src/tools/canbusutil/main.cpp
@@ -34,6 +34,9 @@
 **
 ****************************************************************************/
 
+#include "canbusutil.h"
+#include "sigtermhandler.h"
+
 #include <QCommandLineParser>
 #include <QCoreApplication>
 #include <QTextStream>
@@ -41,15 +44,10 @@
 
 #include <signal.h>
 
-#include "canbusutil.h"
-#include "sigtermhandler.h"
-
-#define PROGRAMNAME "canbusutil"
-
 int main(int argc, char *argv[])
 {
     QCoreApplication app(argc, argv);
-    QCoreApplication::setApplicationName(QStringLiteral(PROGRAMNAME));
+    QCoreApplication::setApplicationName(QStringLiteral("canbusutil"));
     QCoreApplication::setApplicationVersion(QStringLiteral(QT_VERSION_STR));
 
     QScopedPointer<SigTermHandler> s(SigTermHandler::instance());
@@ -106,10 +104,8 @@ int main(int argc, char *argv[])
 
     parser.process(app);
 
-    if (parser.isSet(listOption)) {
-        util.printPlugins();
-        return 0;
-    }
+    if (parser.isSet(listOption))
+        return util.printPlugins();
 
     QString data;
     const QStringList args = parser.positionalArguments();
@@ -117,16 +113,16 @@ int main(int argc, char *argv[])
         util.setShowTimeStamp(parser.isSet(showTimeStampOption));
         util.setShowFdFlags(parser.isSet(showFdFlagsOption));
     } else if (args.size() == 3) {
-        data = args[2];
+        data = args.at(2);
     } else if (args.size() == 1 && parser.isSet(listDevicesOption)) {
-        return util.printDevices(args[0]);
+        return util.printDevices(args.at(0));
     } else if (args.size() != 2) {
-        fprintf(stderr, "Invalid number of arguments (%d given).\n\n%s",
-            args.size(), qPrintable(parser.helpText()));
+        output << CanBusUtil::tr("Invalid number of arguments (%1 given).").arg(args.size());
+        output << endl << endl << parser.helpText();
         return 1;
     }
 
-    if (!util.start(args[0], args[1], data))
+    if (!util.start(args.at(0), args.at(1), data))
         return -1;
 
     return app.exec();
diff --git a/src/tools/canbusutil/readtask.cpp b/src/tools/canbusutil/readtask.cpp
index fb159b7..2bde509 100644
--- a/src/tools/canbusutil/readtask.cpp
+++ b/src/tools/canbusutil/readtask.cpp
@@ -36,9 +36,9 @@
 
 #include "readtask.h"
 
-ReadTask::ReadTask(QTextStream &output, QObject *parent)
-    : QObject(parent),
-      output(output) { }
+ReadTask::ReadTask(QTextStream &output, QObject *parent) :
+    QObject(parent),
+    m_output(output) { }
 
 void ReadTask::setShowTimeStamp(bool showTimeStamp)
 {
@@ -55,10 +55,10 @@ void ReadTask::setShowFdFlags(bool showFlags)
     m_showFdFlags = showFlags;
 }
 
-void ReadTask::checkMessages() {
+void ReadTask::handleFrames() {
     auto canDevice = qobject_cast<QCanBusDevice *>(QObject::sender());
     if (canDevice == nullptr) {
-        qWarning("ReadTask::checkMessages: Unknown sender");
+        qWarning("ReadTask::handleFrames: Unknown sender.");
         return;
     }
 
@@ -87,16 +87,17 @@ void ReadTask::checkMessages() {
         else
             view += frame.toString();
 
-        output << view << endl;
+        m_output << view << endl;
     }
 }
 
-void ReadTask::receiveError(QCanBusDevice::CanBusError /*error*/) {
+void ReadTask::handleError(QCanBusDevice::CanBusError /*error*/)
+{
     auto canDevice = qobject_cast<QCanBusDevice *>(QObject::sender());
     if (canDevice == nullptr) {
-        qWarning("ReadTask::receiveError: Unknown sender");
+        qWarning("ReadTask::handleError: Unknown sender.");
         return;
     }
 
-    output << "Read error: " << canDevice->errorString() << endl;
+    m_output << tr("Read error: '%1'").arg(canDevice->errorString()) << endl;
 }
diff --git a/src/tools/canbusutil/readtask.h b/src/tools/canbusutil/readtask.h
index e00440a..a866d3d 100644
--- a/src/tools/canbusutil/readtask.h
+++ b/src/tools/canbusutil/readtask.h
@@ -45,17 +45,17 @@ class ReadTask : public QObject
 {
     Q_OBJECT
 public:
-    explicit ReadTask(QTextStream &output, QObject *parent = nullptr);
+    explicit ReadTask(QTextStream &m_output, QObject *parent = nullptr);
     void setShowTimeStamp(bool showStamp);
     bool isShowFdFlags() const;
     void setShowFdFlags(bool isShowFdFlags);
 
 public slots:
-    void checkMessages();
-    void receiveError(QCanBusDevice::CanBusError /*error*/);
+    void handleFrames();
+    void handleError(QCanBusDevice::CanBusError /*error*/);
 
 private:
-    QTextStream &output;
+    QTextStream &m_output;
     bool m_showTimeStamp = false;
     bool m_showFdFlags = false;
 };