Qt Number Generator v2

Question

Link to the old question.

I tried to learn some new things from the answers and here's what I did:

1. Used Qt Designer so that in Config.h, all member variables and widget positioning are gone
2. User can't set the lower bound higher than the upper and vice versa
3. Replaced obsolete qrand with generators from the C++11 <random> library
4. Used the Qt5 version of connect
5. Better UI with more options

I like code which can be read as plain English text, that's why I made functions such as _removeLastChar or _correctInputParameters which are basically one-liners but, for me, improve readability a lot.

Code-review IMHO taught me the most about code quality that is why I am asking this "new" version.

Screenshot

generator.h

#ifndef GENERATOR_H
#define GENERATOR_H
#include <QMainWindow>
class QSpinBox;
namespace Ui {
 class Generator;
}
class Generator : public QMainWindow
{
 Q_OBJECT
public:
 explicit Generator(QWidget *parent = nullptr);
 ~Generator();
public slots:
 void generateNumber();
 void clear();
 void saveToFile();
 void setMinValue(int);
 void setMaxValue(int);
private:
 Ui::Generator *ui;
 qint32 _generateNumber();
 QString _getSeparator();
 QString _nums;
 bool _correctInputParameters();
 bool _oneLineOutput();
 void _generateNumbers( int from, int to, bool random );
 void _removeLastChar( QString& string );
};
#endif // GENERATOR_H

generator.cpp

#include "generator.h"
#include "ui_generator.h"
#include <random>
#include <iostream>
#include <QMessageBox>
#include <QTextStream>
#include <QFileDialog>
Generator::Generator(QWidget *parent)
 : QMainWindow(parent)
 , ui(new Ui::Generator)
{
 ui->setupUi(this);
 connect(ui->generateButton, &QPushButton::clicked, this, &Generator::generateNumber);
 connect(ui->clearButton, &QPushButton::clicked, this, &Generator::clear);
 connect(ui->saveButton, &QPushButton::clicked, this, &Generator::saveToFile);
 connect(ui->exitButton, &QPushButton::clicked, this, &QApplication::exit);
 connect(ui->minimumSpinBox, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), this, &Generator::setMinValue);
 connect(ui->maximumSpinBox, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), this, &Generator::setMaxValue);
}
void Generator::generateNumber() {
 clear();
 int numbersCount = ui->numbers->value ();
 _nums = "";
 // random numbers
 if ( ui->random->isChecked () ) {
 _generateNumbers (0, numbersCount, true);
 }
 // sequential numbers
 else {
 int lower = ui->minimumSpinBox->value ();
 int upper = ui->maximumSpinBox->value ();
 _generateNumbers (lower, upper + 1, false);
 }
 ui->textEdit->setText (_nums);
}
void Generator::_generateNumbers( int low, int high, bool random ) {
 QString separator = _getSeparator();
 for ( qint32 i = low; i < high; ++i ) {
 if ( random ) { // random
 _nums += QString::number ( _generateNumber () );
 }
 else { // sequential
 _nums += QString::number( i );
 }
 _nums += separator;
 // output into multiple lines
 if ( !_oneLineOutput () ) {
 _nums += "\n";
 }
 }
 // get rid of the last separator char
 if ( _oneLineOutput () && separator != "" ) { _removeLastChar(_nums);}
}
void Generator::saveToFile () {
 QString filename = QFileDialog::getSaveFileName (this,
 tr("Save numbers"), "",
 tr("Text file (*.txt);;All Files(*)"));
 if ( filename.isEmpty () ) { return; }
 QFile output( filename );
 if ( !output.open(QIODevice::WriteOnly | QIODevice::Text) ) {
 QMessageBox::information( this,
 tr("Unable to open file"),
 output.errorString() );
 return;
 }
 QTextStream ts( &output );
 ts << _nums.toUtf8 ();
 output.close();
}
qint32 Generator::_generateNumber() {
 std::random_device rd;
 std::default_random_engine eng(rd());
 std::uniform_int_distribution< qint32 > distr( ui->minimumSpinBox->value (),
 ui->maximumSpinBox->value () );
 return distr(eng);
}
QString Generator::_getSeparator() {
 auto separator = ui->separator->currentText();
 if ( separator == "(space)" ) return " ";
 if ( separator == "(nothing)" ) return "";
 return separator;
}
void Generator::setMinValue( int newValue ) {
 auto maxValue = ui->maximumSpinBox->value ();
 if ( newValue > maxValue ) { ui->minimumSpinBox->setValue ( maxValue ); }
}
void Generator::setMaxValue ( int newValue ) {
 auto minValue = ui->minimumSpinBox->value ();
 if ( newValue < minValue ) { ui->maximumSpinBox->setValue (minValue); }
}
void Generator::clear (){
 ui->textEdit->clear ();
}
void Generator::_removeLastChar( QString &string ) {
 string.remove ( string.size () - 1, 1 );
}
bool Generator::_correctInputParameters() {
 return ui->minimumSpinBox->value () <= ui->maximumSpinBox->value ();
}
bool Generator::_oneLineOutput() {
 return ui->oneLine->isChecked ();
}
Generator::~Generator() {
 delete ui;
}

• Toby Speight: If you've used Designer, then the UI file you've edited is part of the complete code for review.
• sliziky: Thanks. I originally wanted to show also .ui file but it contains ~500 lines so I decided not to. Should I append the .ui now when there is already the answer?
• Toby Speight: Now that you have a review, you shouldn't add additional code to be reviewed; my comment was intended as a recommendation for future questions.

Answer 1

Seems like you've made a lot of improvements since your previous post! Let's get into the review!

1. General Overview

a. Use the on_<objectName>_<signal> Naming Scheme for Slots

This naming scheme tells the moc to automatically connect a slot with the corresponding <signal> of <objectName> from the UI. We then don't need to call connect(...), saving us a few lines of code.

If we take a look at the clearButton UI object, we can get this auto-connect behaviour by renaming the clear method to on_clearButton_clicked. The implementation doesn't change, only the symbol.

This process of pinpointing the correct slot name is automated from Design mode. First, right-click the object itself or the listing on the object-class tree. Then select the signal to connect and the slot to go to. Qt will automatically generate the on_clearButton_clicked slot in the header and source files (if it doesn't exist yet).

Right-click on the Clear button and select Go to slot...
^{Right-click on your Clear button to bring up the context menu and select Go to slot...}

enter image description here
^{Choose the clicked() signal and click OK.}

Now you no longer need manually connect with connect(...). You can apply this to generateButton, clearButton, saveButton, minimumSpinBox, and maximumSpinBox. Yay, 5 less lines of code! 5 less worries!

(To be clear, static_cast<void (QSpinBox::*)(int)> isn't needed for minimumSpinBox, and maximumSpinBox as the correct overload can be automatically deduced.)

Also note that this naming scheme doesn't have to be used for every slot – it is primarily used for those slots which have a corresponding signal from the UI.

b. Consistency in Order of Methods in Header and Source Files

In your header file, the first four function-like declarations are

public:
 explicit Generator(QWidget *parent = nullptr);
 ~Generator();
public slots:
 void generateNumber();
 void clear();

However, in your source file, the definition for the destructor comes last. This harms readability. Most readers may be expecting the same ordering of methods in both header and source files. Does this mean the header file should conform to the ordering of the source file? Something like below perhaps?

public:
 explicit Generator(QWidget *parent = nullptr);
public slots:
 void generateNumber();
 void clear();
public:
 ~Generator();

Nawww, the source file should conform to the header file. Please, please, please; if you declare the destructor right after the constructor, define the destructor right after the constructor.

Generator::Generator(QWidget *parent)
 : QMainWindow(parent)
 , ui(new Ui::Generator)
{
 ui->setupUi(this);
 connect(ui->generateButton, &QPushButton::clicked, this, &Generator::generateNumber);
 connect(ui->clearButton, &QPushButton::clicked, this, &Generator::clear);
 connect(ui->saveButton, &QPushButton::clicked, this, &Generator::saveToFile);
 connect(ui->exitButton, &QPushButton::clicked, this, &QApplication::exit);
 connect(ui->minimumSpinBox, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), this, &Generator::setMinValue);
 connect(ui->maximumSpinBox, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), this, &Generator::setMaxValue);
}
Generator::~Generator() {
 delete ui;
}
// other methods
// ...

c. Naming

i. _generateNumbers(int ?, int ?, bool random)

A minor issue. You have

void _generateNumbers( int from, int to, bool random );

in your header file but

void Generator::_generateNumbers( int low, int high, bool random ) {

in your source code. Choose either from/to or low/high, but not both.

ii. _correctInputParameters and oneLineOutput

For methods that return bool (also known as predicates), consider starting the method with is or has.

bool _hasCorrectInputParameters();
bool _isOneLineOutput();

Helps with readability. We don't need any special guesswork to infer that these will return bool.

2. Logic

The logic and program flow seems a tad messy, let's try cleaning it up!

a. clear()

What should this clear? Only the text-edit? I'd clear _nums as well.

void Generator::clear() {
 ui->textEdit->clear();
 _nums.clear();
}

The last thing we want is to have the clear method clear only the gui and leave the variable sitting. Clear everything it all at once! Doing so allows us to pinpoint bugs easier – we don't have to spend 30 minutes digging through the entire code to find a lone _nums = "" placed wrongly.

b. generateNumber and _generateNumbers and _generateNumber

First off, these methods could do with better naming. As soon as I type generate, the IDE completer will show these three methods and it all suddenly becomes ambiguous. Be specific with what each method does.

• _generateNumber only generates random numbers, so change it to _generateRandomNumber.
• generateNumber handles the button click, so follow the first section of this answer and change it to on_generateButton_clicked.
• _generateNumbers is a fine name as it is.

Down to the logic. It doesn't really make sense to retrieve values of minimumSpinBox and maximumSpinBox in two places (one, in generateNumber, under the else branch; and two, in _generateNumber). Retrieve it once, then pass it accordingly. By the same principle, since only the random option needs int numbersCount = ui->numbers->value();, this should be placed in _generateNumbers instead.

void Generator::generateNumber() {
 clear();
 // _nums = ""; // moved to clear(); same as _nums.clear()
 int low = ui->minimumSpinBox->value(); // retrieve values from spinboxes ONCE 
 int high = ui->maximumSpinBox->value();
 _generateNumbers(low, high+1, ui->random->isChecked()); // universal, no need for if-else 
 ui->textEdit->setText(_nums);
}

This also means changing _generateNumber to accept parameters so that we can later pass the low and high in generateNumber:

qint32 Generator::_generateNumber(int low, int high) {
 std::random_device rd;
 std::default_random_engine eng(rd());
 std::uniform_int_distribution<qint32> distr(low, high);
 return distr(eng);
}

Currently, _generateNumbers serves two purposes: generating random numbers and generating sequential numbers. However, the arguments used are for completely different purposes, which is... meh... until their names contradict with their purpose which merits another meh. This seems like a big red sign to me:

if ( ui->random->isChecked () ) {
 _generateNumbers (0, numbersCount, true); // low = 0, high = numbersCount ?
}

Does this use case not imply that the generated numbers should be between 0 and numbersCount? Apparently not... Apparently, low and high in that context means to generate high – low number of values.

Since the purposes and use cases are different, it only makes sense to have different implementations, so branch your if-else well ahead of the for-loop(s).

void Generator::_generateNumbers( int low, int high, bool random ) {
 QString separator = _getSeparator();
 if (random) {
 int numbersCount = ui->numbers->value();
 // generate random numbers between low and high
 // for (int i = 0; i < numbersCount; i++)
 // ...
 } else {
 // generate random numbers between low and high
 // ...
 }
 // get rid of the last separator char
 if ( _oneLineOutput () && separator != "" ) { _removeLastChar(_nums);}
}

3. UI

On the UI side, I'd consider removing some borders – they do get in the way, especially the ones around Generate Numbers and the ones around your four buttons.

You should also consider disabling the How many numbers spinbox if the selected number pattern is Sequential as the two are mutually exclusive options.

But then, you could also consider the case where the user selects Sequential and provides a from-value and how-many-numbers-value but doesn't provide a to-value. Sounds like you could make this a third Numbers option: Sequential-n.

It's unfortunate that we don't get the .ui to play around with, but nonetheless, it looks stunning and functional from afar. :-)

• greybeard: Welcome to Code Review! Nice job, first post or not.
• TrebledJ: I realised I missed out a bunch of stuff on my first read. Had to read the code a third time to actually see a large proportion of the inherent problems. :P
• sliziky: Thank you for great review :) How would you solve passing into ( int low, int high, bool random ) if I added the option you mentioned sequential-n , so far I got const QString& mode and then if (mode == "random" ) / if (mode == "sequential") / if (mode == "sequential-n") which for me is more readable than passing int/char and compare with values . Although #define random 1 could "solve" readability
• TrebledJ: @sliziky This is a good use case for enums (enumerated types). These are a tad safer than strings (the compiler will catch any typos) while maintaining readability. Hope this helps ~