Ygtjki

I'm pretty new to Qt but already have a little experience with C++ so the first "project" I wanted to try out was a GUI random number generator.
I am looking for some advices either in naming conventions or OOP architecture, what should I do better and what I'm already doing good. The code is really short so there is not so much to review but I hope you will find something.

1) I know that you should NOT use _ prefix, but I really like it and m_ looks ugly to me.

2) I could not decide whether it's good to write code like this ( I find this better ):

_maxSpinBox->setMinimum ( Config::SpinBox::minimum );
_maxSpinBox->setMaximum ( Config::SpinBox::maximum );
_maxSpinBox->setSingleStep ( Config::SpinBox::single_step );
_maxSpinBox->setValue ( Config::SpinBox::default_value );

Or like this

_maxSpinBox->setMinimum ( Config::SpinBox::minimum );
_maxSpinBox->setMaximum ( Config::SpinBox::maximum );
_maxSpinBox->setSingleStep ( Config::SpinBox::single_step );
_maxSpinBox->setValue ( Config::SpinBox::default_value );

3) I also thought about adding using namespace Config; into generator.cpp because writing Config:: everywhere is really annoying.

Thanks for tips

main.cpp

#include <QApplication>
#include <iostream>
#include "generator.h"
int main(int argc, char **argv)

 QApplication qapp( argc, argv );
 Generator generator;
 generator.show ();
 try 
 qapp.exec();
 
 catch(const std::exception& e) 
 std::cerr << e.what () << std::endl;
 
 return 0;

config.h

#ifndef CONFIG_H
#define CONFIG_H
#include <QFont>
#include <QString>
namespace Config


namespace Window

 constexpr static int height = 150;
 constexpr static int width = 300;
 // Window

namespace Button

 const static QString title = "Generate";
 constexpr static int height = 30;
 constexpr static int width = 80;
 constexpr static int pos_x = Window::width / 2 - width / 2;
 constexpr static int pos_y = Window::height - height - 10;
 // Button

namespace Display

 constexpr static int height = 45;
 constexpr static int width = 90;
 constexpr static int pos_x = Window::width / 2 - width / 2;
 constexpr static int pos_y = 20;
 constexpr static int default_value = 0;
 // Display

namespace Fonts

 const static QFont serifFont( "Times", 10, QFont::Bold );
 const static QFont sansFont( "Helvetica [Cronyx]", 12 );
 // Fonts

namespace SpinBox

 constexpr static int minimum = -30000;
 constexpr static int maximum = 30000;
 constexpr static int single_step = 1;
 constexpr static int default_value = 0;
 // SpinBox

 // Config



#endif // CONFIG_H

generator.h

#ifndef GENERATOR_H
#define GENERATOR_H

#include <QWidget>
#include <exception>
class QPushButton;
class QLabel;
class QSpinBox;
class QGroupBox;
class QVBoxLayout;

struct BadParameters : std::logic_error

 using std::logic_error::logic_error;
;

class Generator : public QWidget

 Q_OBJECT
public:
 explicit Generator( QWidget* parent = nullptr );
public slots:
 void showNumber();
signals:

private:
 QPushButton* _button;
 QLabel* _display;
 QSpinBox* _minSpinBox;
 QSpinBox* _maxSpinBox;
 QGroupBox* _groupBox;
 QVBoxLayout* _layout;
 int _generateNumber( int low, int high );
 void _createSpinBoxes();
 void _createMinSpinBox();
 void _createMaxSpinBox();
 void _createSpinBoxLayout();
 void _createButton();
 void _createDisplay();
 void _init();
;

#endif // GENERATOR_H

generator.cpp

#include <QGroupBox>
#include <QLabel>
#include <QPushButton>
#include <QSpinBox>
#include <QTime>
#include <QVBoxLayout>

#include <random>
#include <qglobal.h>

#include "config.h"
#include "generator.h"


Generator::Generator( QWidget* parent )
 : QWidget( parent )

 _init();
 _createDisplay ();
 _createButton ();
 _createSpinBoxes ();
 connect ( _button, SIGNAL(clicked()), this, SLOT(showNumber()) );

void Generator::_init() 
 QTime time = QTime::currentTime ();
 qsrand( static_cast< uint >( time.msec ()) );
 setFixedSize( Config::Window::width, Config::Window::height );
 setWindowTitle( "Random Number Generator" );

void Generator::_createButton() 
 _button = new QPushButton( Config::Button::title, this );
 _button->setGeometry ( Config::Button::pos_x,
 Config::Button::pos_y,
 Config::Button::width,
 Config::Button::height );

void Generator::_createDisplay() 
 _display = new QLabel( this );
 _display->setFont ( Config::Fonts::sansFont );
 _display->setAlignment ( Qt::AlignCenter);
 _display->setGeometry ( Config::Display::pos_x,
 Config::Display::pos_y,
 Config::Display::width,
 Config::Display::height );

 _display->setNum ( Config::Display::default_value );

void Generator::_createSpinBoxes() 
 _createMinSpinBox();
 _createMaxSpinBox();
 _createSpinBoxLayout();

void Generator::_createSpinBoxLayout()
 _groupBox = new QGroupBox( this );
 _layout = new QVBoxLayout;
 QLabel* labelMin = new QLabel( tr("Minimum: ") );
 QLabel* labelMax = new QLabel( tr("Maximum: ") );

 _layout->addWidget ( labelMin );
 _layout->addWidget ( _minSpinBox );
 _layout->addWidget ( labelMax );
 _layout->addWidget ( _maxSpinBox );
 _groupBox->setLayout ( _layout );

void Generator::_createMaxSpinBox() 
 _maxSpinBox = new QSpinBox ( this );
 _maxSpinBox->setMinimum ( Config::SpinBox::minimum );
 _maxSpinBox->setMaximum ( Config::SpinBox::maximum );
 _maxSpinBox->setSingleStep ( Config::SpinBox::single_step );
 _maxSpinBox->setValue ( Config::SpinBox::default_value );

void Generator::_createMinSpinBox() 
 _minSpinBox = new QSpinBox ( this );
 _minSpinBox->setMinimum ( Config::SpinBox::minimum );
 _minSpinBox->setMaximum ( Config::SpinBox::maximum );
 _minSpinBox->setSingleStep ( Config::SpinBox::single_step );
 _minSpinBox->setValue ( Config::SpinBox::default_value );

int Generator::_generateNumber( int low, int high ) 

 if ( low > high ) 
 throw BadParameters( "Upper bound is NOT higher n" );
 
 return qrand() % (( high + 1) - low) + low;

void Generator::showNumber() 
 _display->setNum( _generateNumber( _minSpinBox->value(),
 _maxSpinBox->value () ));

This

your first statement is that you want advice in naming conventions and in your second line you state

I know that you should NOT use _ prefix, but I really like it and m_ looks ugly to me.

Some conventions are just form and it is up to you to decide what you like better. The reason not to use names in c++ that are prefixed with an underscore is that you are just one keystroke away from writing identifiers that are considered invalid. The standard reserves _<UpperCaseLetter> and __ for internal identifiers. So the advice is stay away from leading underscores.

Where you put your parenthesis is totally up to you, don't get too attached to your style as each project, or each company will have their own style that you should adhere to. My personal advice here, use clang-format or a similar tool and never manually shift things around.

With regard to QT there are differing opinions, my advice is use the designer! Laying UIs out by hand is not a good use of your time. It becomes more and more tedious, these tools are made for a reason and a lot of layout tasks can be done using the QT Designer. This would alleviate your namespace problem for example. And remove most of the code that you wrote dealing with the construction of the UI. It will let you focus on crafting a well thought out and convenient UI rather than writing rote and repetitive code to create the widgets that make up the UI. Some of the following comments are more on the quality of the UI than the code, but I see that as part of the project that you created.

Albeit doing it once is a decent exercise and it looks ok. Although it really doesn't handle ok, If you have the requirement that Minimum is smaller than Maximum your UI should enforce that and not let the user set a minimum that is larger than maximum. Right now your UI will just crash, that is not good. At least an error message would be better, but i'd really expect the UI to enforce any give constraints.

Your question is tagged just c++ and not c++11. If you are just learning C++ that you should learn least c++11, and not start with anything prior. Qt is somewhat orthogonal, and sometimes even blocks some of these features. But it is going to be a lot easier to step back to an older version of c++ than to learn new things. In that vein you should look at this writeup about a variation on how to connect signals and slots.

Keep it up ...

I see a number of things that may help you improve your program.

Use proper case for file names

We all know that Windows is not case sensitive with respect to file names, but pretty much every other operating system out there (Apple's OS X, Linux, BSD, etc.) is. For that reason, when you have a class named Generator it should be in a file named Generator.h rather than generator.h or tools such as moc won't recognize them on any OS other than Windows. By using proper case for file names now, you can avoid all future annoyance in having to port the code to any other OS.

Don't throw from a Qt event handler

Qt does not support throwing an exception from within an event handler, so this code:

int Generator::_generateNumber( int low, int high ) 
 if ( low > high ) 
 throw BadParameters( "Upper bound is NOT higher n" );
 
 return qrand() % (( high + 1) - low) + low;

will generate the following error under Qt5:

Qt has caught an exception thrown from an event handler. Throwing exceptions from an event handler is not supported in Qt. You must not let any exception whatsoever propagate through Qt code. If that is not possible, in Qt 5 you must at least re-implement QCoreApplication::notify() and catch all exceptions there.

See https://stackoverflow.com/questions/10075792/how-to-catch-exceptions-in-qt for more details.

In this case, I'd suggest linking the two spinboxes together to make it impossible for the condition to occur. That is, don't allow high to be set to a value less than low - it's almost always better to prevent exceptions than to try to catch them.

Don't use obsolete functions

The qsrand() and qrand() functions are obsolete and should not be used in new code. I'd suggest using the standard C++ <random> tools instead.

Naming and formatting

The code currently contains these lines:

_minSpinBox->setMinimum ( Config::SpinBox::minimum );
_minSpinBox->setMaximum ( Config::SpinBox::maximum );
_minSpinBox->setSingleStep ( Config::SpinBox::single_step );

There's a lot that could be improved here. First, aligning parentheses like that creates a maintenance headache. If someone adds one single line that happens to be longer than the setSingleStep line, they'd have to adjust every other line to realign. Over the long term, that's a pointless and frustrating battle. Let it go!

Second, you've already noted that the underscore prefix is technically legal but suspect. Personally, I don't bother usually with any particular identifier prefix or suffix and find it easier to both read and write as a result.

Third, rather than making separate calls to setMinimum and setMaximum, one could instead make a single call to setRange instead.

Fourth, this could would be much easier to read without the Config::SpinBox prefix everywhere. I'd suggest rewriting the function like this (in conjunction with the next suggestion):

QSpinBox *Generator::createSpinBox() 
 using namespace Config::SpinBox;
 auto sb = new QSpinBox(this);
 sb->setRange(minimum, maximum);
 sb->setSingleStep(single_step);
 sb->setValue(default_value);
 return sb;

Don't Repeat Yourself (DRY)

If you are creating a lot of almost identical code, you should ask yourself if there is a way to avoid it. This is such common advice that programmers often just use the shorthand and say "DRY up your code". In this case, here's a rewrite of _createSpinBox() that shows how to use the single function above instead of two separate functions:

void Generator::_createSpinBoxes() 
 _minSpinBox = createSpinBox();
 _maxSpinBox = createSpinBox();
 _createSpinBoxLayout();

Accomodate translations

The current code correctly uses tr() in a few places, but not all. For example, the window title and button label. It's very beneficial to get into the habit of making sure that all displayable literal values are translatable. See the Qt translation docs for more details.

Use the new version of connect

Since Qt5, there is a better syntax for connecting slots and signals. So instead of this:

connect ( _button, SIGNAL(clicked()), this, SLOT(showNumber()) );

one could write this:

connect(_button, &QPushButton::clicked, this, &Generator::showNumber);

The version above is thanks to @GrecKo. I had originally written this, but the use of std::bind is really not needed here. I think it was a leftover from an experiment I did on the code.

connect(_button, &QPushButton::clicked, std::bind(&Generator::showNumber, this));

Note that I've used std::bind from <functional> here to allow the correct passing of this.