But then, you could also consider: if 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 add a third Numbers option: Sequential-n.

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.

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.

Seems like you've made a lot of improvements since your previous post! Here are some suggestions:

Name slots which handle UI signals as on_<objectName>_<signal>.

This will tell the moc to automatically connect the slot with the corresponding ui object <objectName>.

This process of pinpointing the correct slot name is automated from Design mode. Firstly, 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.

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

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

Be consistent in the order your methods are declared and defined.

However, in your source file, the definition for the destructor comes last. This harms readability. Most readers will expect the same ordering of methods. Does this mean the header file should conform to the ordering of the source file?

Nawww, the source file should conform to the header file. So pray, if you declare the destructor right after the constructor, define the destructor right after the constructor.

Clearer method names?

You have _generateNumber(), generateNumber(), and _generateNumbers() and when I was first reading your code, I got mixed up between the two and looked at the wrong one until I caught myself. You seem to be giving several meanings to the word "generate" in your code:

1. Handle the Generate button click. (This is what generateNumber does.)
2. Generate a list of numbers. (These may or may not be random. This is what generateNumbers does.)
3. Generate a random number. (This is what _generateNumber does.)

This again, harms readability. The method generateNumber can be changed to on_generateButton_clicked, so that should be settled. The only change I'd make is to change _generateNumber to _generateRandomNumber or something similar. This really clarifies things: "okay, I can expect a random number out of this function."

The 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.

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.

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).

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.)

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

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?

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.

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: if 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 add a third Numbers option: Sequential-n.

Use the on_<objectName>_<signal> naming format for the slots which handle signals from UI objects.

This will tell the moc to automatically connect the slot with the corresponding ui object <objectName>. For example, as the object name for your Clear button is clearButton, you can get the automatic connection by naming your slot on_clearButton_clicked.

This is automated from Design mode by right-clicking the object itself or the listing on the object-class tree. Selecting 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.

Now you no longer need to manually connect using connect(...). You can do this for generateButton, clearButton, and saveButton. Yay, 3 less lines of code!

Use QOverload<int>::of instead of static_cast<void(QSpinBox::*)(int)>

I'll be honest, I was a bit suspicious seeing your code static_casting a signal. Looking at documentation , instead of

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);

use

connect(ui->minimumSpinBox, QOverload<int>::of(&QSpinBox::valueChanged), this, &Generator::setMinValue);
connect(ui->maximumSpinBox, QOverload<int>::of(&QSpinBox::valueChanged), this, &Generator::setMaxValue);

Improves readability, don't ya think? :-)

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

Name slots which handle UI signals as on_<objectName>_<signal>.

This will tell the moc to automatically connect the slot with the corresponding ui object <objectName>.

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. Firstly, 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.

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

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

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