GUI Number Generator in QT C++





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty{ margin-bottom:0;
}







5












$begingroup$


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



enter image description here



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










share|improve this question







New contributor




sliziky is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







$endgroup$



















    5












    $begingroup$


    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



    enter image description here



    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










    share|improve this question







    New contributor




    sliziky is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
    Check out our Code of Conduct.







    $endgroup$















      5












      5








      5





      $begingroup$


      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



      enter image description here



      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










      share|improve this question







      New contributor




      sliziky is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.







      $endgroup$




      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



      enter image description here



      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







      c++ gui qt






      share|improve this question







      New contributor




      sliziky is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.











      share|improve this question







      New contributor




      sliziky is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.









      share|improve this question




      share|improve this question






      New contributor




      sliziky is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.









      asked yesterday









      slizikysliziky

      283




      283




      New contributor




      sliziky is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.





      New contributor





      sliziky is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.






      sliziky is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.






















          2 Answers
          2






          active

          oldest

          votes


















          2












          $begingroup$

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






          share|improve this answer









          $endgroup$













          • $begingroup$
            Thanks for a tip with QtDesigner, it is really useful tool and it also has shortened my code a lot! What do you think about getting values from spinboxes? Is it "better" to call value() every time I need it or to make separate function extractValues() which will fill my two attributes with these values. Thanks
            $endgroup$
            – sliziky
            20 hours ago





















          4












          $begingroup$

          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.






          share|improve this answer











          $endgroup$









          • 1




            $begingroup$
            Why use std::bind and not just connect(_button, &QPushButton::clicked, this, &Generator::showNumber);?
            $endgroup$
            – GrecKo
            yesterday










          • $begingroup$
            @GrecKo: that would work, too, and should probably be used instead because it's simpler. I don't know why I did it as I did.
            $endgroup$
            – Edward
            yesterday










          • $begingroup$
            I've never known moc to care about the filenames - when building code from Stack Exchange, I normally just name the files using the question number, and that's no problem for it. AFAIK, the filenames are used only when printing error messages, and in the head-of-file comment in the output. Personally, I'd never use upper-case in a filename, except in Java where it is unfortunately required.
            $endgroup$
            – Toby Speight
            17 hours ago












          • $begingroup$
            @TobySpeight: it's specifically the use of moc and CMake, which is the combination I use, that fails unless the filename case is as I indicated. I believe I recall having such problems with other tools (maybe QMake?) but don't recall details. In any case, it's a simple and useful thing to do.
            $endgroup$
            – Edward
            17 hours ago










          • $begingroup$
            Ah, thanks - I use plain Make and pkg-config, and at work I use QMake; the latter with filenames that are lower-case versions of the class names, and that works fine.
            $endgroup$
            – Toby Speight
            17 hours ago












          Your Answer






          StackExchange.ifUsing("editor", function () {
          StackExchange.using("externalEditor", function () {
          StackExchange.using("snippets", function () {
          StackExchange.snippets.init();
          });
          });
          }, "code-snippets");

          StackExchange.ready(function() {
          var channelOptions = {
          tags: "".split(" "),
          id: "196"
          };
          initTagRenderer("".split(" "), "".split(" "), channelOptions);

          StackExchange.using("externalEditor", function() {
          // Have to fire editor after snippets, if snippets enabled
          if (StackExchange.settings.snippets.snippetsEnabled) {
          StackExchange.using("snippets", function() {
          createEditor();
          });
          }
          else {
          createEditor();
          }
          });

          function createEditor() {
          StackExchange.prepareEditor({
          heartbeatType: 'answer',
          autoActivateHeartbeat: false,
          convertImagesToLinks: false,
          noModals: true,
          showLowRepImageUploadWarning: true,
          reputationToPostImages: null,
          bindNavPrevention: true,
          postfix: "",
          imageUploader: {
          brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
          contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
          allowUrls: true
          },
          onDemand: true,
          discardSelector: ".discard-answer"
          ,immediatelyShowMarkdownHelp:true
          });


          }
          });






          sliziky is a new contributor. Be nice, and check out our Code of Conduct.










          draft saved

          draft discarded


















          StackExchange.ready(
          function () {
          StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f217257%2fgui-number-generator-in-qt-c%23new-answer', 'question_page');
          }
          );

          Post as a guest















          Required, but never shown

























          2 Answers
          2






          active

          oldest

          votes








          2 Answers
          2






          active

          oldest

          votes









          active

          oldest

          votes






          active

          oldest

          votes









          2












          $begingroup$

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






          share|improve this answer









          $endgroup$













          • $begingroup$
            Thanks for a tip with QtDesigner, it is really useful tool and it also has shortened my code a lot! What do you think about getting values from spinboxes? Is it "better" to call value() every time I need it or to make separate function extractValues() which will fill my two attributes with these values. Thanks
            $endgroup$
            – sliziky
            20 hours ago


















          2












          $begingroup$

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






          share|improve this answer









          $endgroup$













          • $begingroup$
            Thanks for a tip with QtDesigner, it is really useful tool and it also has shortened my code a lot! What do you think about getting values from spinboxes? Is it "better" to call value() every time I need it or to make separate function extractValues() which will fill my two attributes with these values. Thanks
            $endgroup$
            – sliziky
            20 hours ago
















          2












          2








          2





          $begingroup$

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






          share|improve this answer









          $endgroup$



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







          share|improve this answer












          share|improve this answer



          share|improve this answer










          answered yesterday









          Harald ScheirichHarald Scheirich

          1,436518




          1,436518












          • $begingroup$
            Thanks for a tip with QtDesigner, it is really useful tool and it also has shortened my code a lot! What do you think about getting values from spinboxes? Is it "better" to call value() every time I need it or to make separate function extractValues() which will fill my two attributes with these values. Thanks
            $endgroup$
            – sliziky
            20 hours ago




















          • $begingroup$
            Thanks for a tip with QtDesigner, it is really useful tool and it also has shortened my code a lot! What do you think about getting values from spinboxes? Is it "better" to call value() every time I need it or to make separate function extractValues() which will fill my two attributes with these values. Thanks
            $endgroup$
            – sliziky
            20 hours ago


















          $begingroup$
          Thanks for a tip with QtDesigner, it is really useful tool and it also has shortened my code a lot! What do you think about getting values from spinboxes? Is it "better" to call value() every time I need it or to make separate function extractValues() which will fill my two attributes with these values. Thanks
          $endgroup$
          – sliziky
          20 hours ago






          $begingroup$
          Thanks for a tip with QtDesigner, it is really useful tool and it also has shortened my code a lot! What do you think about getting values from spinboxes? Is it "better" to call value() every time I need it or to make separate function extractValues() which will fill my two attributes with these values. Thanks
          $endgroup$
          – sliziky
          20 hours ago















          4












          $begingroup$

          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.






          share|improve this answer











          $endgroup$









          • 1




            $begingroup$
            Why use std::bind and not just connect(_button, &QPushButton::clicked, this, &Generator::showNumber);?
            $endgroup$
            – GrecKo
            yesterday










          • $begingroup$
            @GrecKo: that would work, too, and should probably be used instead because it's simpler. I don't know why I did it as I did.
            $endgroup$
            – Edward
            yesterday










          • $begingroup$
            I've never known moc to care about the filenames - when building code from Stack Exchange, I normally just name the files using the question number, and that's no problem for it. AFAIK, the filenames are used only when printing error messages, and in the head-of-file comment in the output. Personally, I'd never use upper-case in a filename, except in Java where it is unfortunately required.
            $endgroup$
            – Toby Speight
            17 hours ago












          • $begingroup$
            @TobySpeight: it's specifically the use of moc and CMake, which is the combination I use, that fails unless the filename case is as I indicated. I believe I recall having such problems with other tools (maybe QMake?) but don't recall details. In any case, it's a simple and useful thing to do.
            $endgroup$
            – Edward
            17 hours ago










          • $begingroup$
            Ah, thanks - I use plain Make and pkg-config, and at work I use QMake; the latter with filenames that are lower-case versions of the class names, and that works fine.
            $endgroup$
            – Toby Speight
            17 hours ago
















          4












          $begingroup$

          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.






          share|improve this answer











          $endgroup$









          • 1




            $begingroup$
            Why use std::bind and not just connect(_button, &QPushButton::clicked, this, &Generator::showNumber);?
            $endgroup$
            – GrecKo
            yesterday










          • $begingroup$
            @GrecKo: that would work, too, and should probably be used instead because it's simpler. I don't know why I did it as I did.
            $endgroup$
            – Edward
            yesterday










          • $begingroup$
            I've never known moc to care about the filenames - when building code from Stack Exchange, I normally just name the files using the question number, and that's no problem for it. AFAIK, the filenames are used only when printing error messages, and in the head-of-file comment in the output. Personally, I'd never use upper-case in a filename, except in Java where it is unfortunately required.
            $endgroup$
            – Toby Speight
            17 hours ago












          • $begingroup$
            @TobySpeight: it's specifically the use of moc and CMake, which is the combination I use, that fails unless the filename case is as I indicated. I believe I recall having such problems with other tools (maybe QMake?) but don't recall details. In any case, it's a simple and useful thing to do.
            $endgroup$
            – Edward
            17 hours ago










          • $begingroup$
            Ah, thanks - I use plain Make and pkg-config, and at work I use QMake; the latter with filenames that are lower-case versions of the class names, and that works fine.
            $endgroup$
            – Toby Speight
            17 hours ago














          4












          4








          4





          $begingroup$

          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.






          share|improve this answer











          $endgroup$



          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.







          share|improve this answer














          share|improve this answer



          share|improve this answer








          edited 18 hours ago

























          answered yesterday









          EdwardEdward

          47.9k380213




          47.9k380213








          • 1




            $begingroup$
            Why use std::bind and not just connect(_button, &QPushButton::clicked, this, &Generator::showNumber);?
            $endgroup$
            – GrecKo
            yesterday










          • $begingroup$
            @GrecKo: that would work, too, and should probably be used instead because it's simpler. I don't know why I did it as I did.
            $endgroup$
            – Edward
            yesterday










          • $begingroup$
            I've never known moc to care about the filenames - when building code from Stack Exchange, I normally just name the files using the question number, and that's no problem for it. AFAIK, the filenames are used only when printing error messages, and in the head-of-file comment in the output. Personally, I'd never use upper-case in a filename, except in Java where it is unfortunately required.
            $endgroup$
            – Toby Speight
            17 hours ago












          • $begingroup$
            @TobySpeight: it's specifically the use of moc and CMake, which is the combination I use, that fails unless the filename case is as I indicated. I believe I recall having such problems with other tools (maybe QMake?) but don't recall details. In any case, it's a simple and useful thing to do.
            $endgroup$
            – Edward
            17 hours ago










          • $begingroup$
            Ah, thanks - I use plain Make and pkg-config, and at work I use QMake; the latter with filenames that are lower-case versions of the class names, and that works fine.
            $endgroup$
            – Toby Speight
            17 hours ago














          • 1




            $begingroup$
            Why use std::bind and not just connect(_button, &QPushButton::clicked, this, &Generator::showNumber);?
            $endgroup$
            – GrecKo
            yesterday










          • $begingroup$
            @GrecKo: that would work, too, and should probably be used instead because it's simpler. I don't know why I did it as I did.
            $endgroup$
            – Edward
            yesterday










          • $begingroup$
            I've never known moc to care about the filenames - when building code from Stack Exchange, I normally just name the files using the question number, and that's no problem for it. AFAIK, the filenames are used only when printing error messages, and in the head-of-file comment in the output. Personally, I'd never use upper-case in a filename, except in Java where it is unfortunately required.
            $endgroup$
            – Toby Speight
            17 hours ago












          • $begingroup$
            @TobySpeight: it's specifically the use of moc and CMake, which is the combination I use, that fails unless the filename case is as I indicated. I believe I recall having such problems with other tools (maybe QMake?) but don't recall details. In any case, it's a simple and useful thing to do.
            $endgroup$
            – Edward
            17 hours ago










          • $begingroup$
            Ah, thanks - I use plain Make and pkg-config, and at work I use QMake; the latter with filenames that are lower-case versions of the class names, and that works fine.
            $endgroup$
            – Toby Speight
            17 hours ago








          1




          1




          $begingroup$
          Why use std::bind and not just connect(_button, &QPushButton::clicked, this, &Generator::showNumber);?
          $endgroup$
          – GrecKo
          yesterday




          $begingroup$
          Why use std::bind and not just connect(_button, &QPushButton::clicked, this, &Generator::showNumber);?
          $endgroup$
          – GrecKo
          yesterday












          $begingroup$
          @GrecKo: that would work, too, and should probably be used instead because it's simpler. I don't know why I did it as I did.
          $endgroup$
          – Edward
          yesterday




          $begingroup$
          @GrecKo: that would work, too, and should probably be used instead because it's simpler. I don't know why I did it as I did.
          $endgroup$
          – Edward
          yesterday












          $begingroup$
          I've never known moc to care about the filenames - when building code from Stack Exchange, I normally just name the files using the question number, and that's no problem for it. AFAIK, the filenames are used only when printing error messages, and in the head-of-file comment in the output. Personally, I'd never use upper-case in a filename, except in Java where it is unfortunately required.
          $endgroup$
          – Toby Speight
          17 hours ago






          $begingroup$
          I've never known moc to care about the filenames - when building code from Stack Exchange, I normally just name the files using the question number, and that's no problem for it. AFAIK, the filenames are used only when printing error messages, and in the head-of-file comment in the output. Personally, I'd never use upper-case in a filename, except in Java where it is unfortunately required.
          $endgroup$
          – Toby Speight
          17 hours ago














          $begingroup$
          @TobySpeight: it's specifically the use of moc and CMake, which is the combination I use, that fails unless the filename case is as I indicated. I believe I recall having such problems with other tools (maybe QMake?) but don't recall details. In any case, it's a simple and useful thing to do.
          $endgroup$
          – Edward
          17 hours ago




          $begingroup$
          @TobySpeight: it's specifically the use of moc and CMake, which is the combination I use, that fails unless the filename case is as I indicated. I believe I recall having such problems with other tools (maybe QMake?) but don't recall details. In any case, it's a simple and useful thing to do.
          $endgroup$
          – Edward
          17 hours ago












          $begingroup$
          Ah, thanks - I use plain Make and pkg-config, and at work I use QMake; the latter with filenames that are lower-case versions of the class names, and that works fine.
          $endgroup$
          – Toby Speight
          17 hours ago




          $begingroup$
          Ah, thanks - I use plain Make and pkg-config, and at work I use QMake; the latter with filenames that are lower-case versions of the class names, and that works fine.
          $endgroup$
          – Toby Speight
          17 hours ago










          sliziky is a new contributor. Be nice, and check out our Code of Conduct.










          draft saved

          draft discarded


















          sliziky is a new contributor. Be nice, and check out our Code of Conduct.













          sliziky is a new contributor. Be nice, and check out our Code of Conduct.












          sliziky is a new contributor. Be nice, and check out our Code of Conduct.
















          Thanks for contributing an answer to Code Review Stack Exchange!


          • Please be sure to answer the question. Provide details and share your research!

          But avoid



          • Asking for help, clarification, or responding to other answers.

          • Making statements based on opinion; back them up with references or personal experience.


          Use MathJax to format equations. MathJax reference.


          To learn more, see our tips on writing great answers.




          draft saved


          draft discarded














          StackExchange.ready(
          function () {
          StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f217257%2fgui-number-generator-in-qt-c%23new-answer', 'question_page');
          }
          );

          Post as a guest















          Required, but never shown





















































          Required, but never shown














          Required, but never shown












          Required, but never shown







          Required, but never shown

































          Required, but never shown














          Required, but never shown












          Required, but never shown







          Required, but never shown







          Popular posts from this blog

          GameSpot

          日野市

          Tu-95轟炸機