Making my central code better

[Momergil]

Hello!

I have a doubt that is not truuuly problematic, but nevertheless would certainly increase my knowledge in programming and make a code of some of my softwares better.

The idea that I have in mind (and I do have a code that makes that work) is that sometimes I want a window (QDialog, normally) to appear before the actual software is shown, or else I want a window (QDialog) to appear where the user will select between a number of MainWindows to use. The first case has a very common example: imagine a software that all times, before it runs, shows a QDialog for the user to put a password in order to use this software. For the second case, here is the code that I'm using to make that work: (since both codes are very similar, I'm gonna show just this second):

int decisao;
bool continuaounao = false;
 
int main(int argc, char *argv[])
{
    QApplication a(argc, argv);
 
    NovoMain NM;
 
inicio: decisao = NM.executa();
        if (decisao == 1)   //Positions
            goto spositions;
        else if (decisao == 2)  //Body
                goto body;
        else if (decisao == 3)  //Outro
                goto context;
        else if (decisao == 4)  //Creditos
                goto creditos;
        else if (decisao == 0)
            return 0;
 
spositions: {
                qDebug() << "Positions";
                SPositions sp;
                sp.setWindowTitle("Positions");
                sp.showMaximized();
                sp.show();
                a.exec();
                if (sp.getSaida() == true)
                    return 0;
                else goto inicio;
            }
 
body:       {
                Body bd;
                bd.setWindowTitle("Body knowledge");
                bd.showMaximized();
                bd.show();
                a.exec();
                if (bd.getSaida() == true)
                    return 0;
                else goto inicio;
            }
context:    {
                Context ct;
                ct.setWindowTitle("Context and details");
                ct.showMaximized();
                ct.show();
                a.exec();
                if (ct.getSaida() == true)
                    return 0;
                else goto inicio;
            }
creditos:   {
                Credits cd;
                cd.setWindowTitle("Credits");
                cd.showMaximized();
                cd.show();
                qDebug() << "Credits";
                a.exec();
                if (cd.getSaida() == true)
                    return 0;
                else goto inicio;
            }
}

int decisao;
bool continuaounao = false;

int main(int argc, char *argv[])
{
    QApplication a(argc, argv);

    NovoMain NM;

inicio: decisao = NM.executa();
        if (decisao == 1)   //Positions
            goto spositions;
        else if (decisao == 2)  //Body
                goto body;
        else if (decisao == 3)  //Outro
                goto context;
        else if (decisao == 4)  //Creditos
                goto creditos;
        else if (decisao == 0)
            return 0;

spositions: {
                qDebug() << "Positions";
                SPositions sp;
                sp.setWindowTitle("Positions");
                sp.showMaximized();
                sp.show();
                a.exec();
                if (sp.getSaida() == true)
                    return 0;
                else goto inicio;
            }

body:       {
                Body bd;
                bd.setWindowTitle("Body knowledge");
                bd.showMaximized();
                bd.show();
                a.exec();
                if (bd.getSaida() == true)
                    return 0;
                else goto inicio;
            }
context:    {
                Context ct;
                ct.setWindowTitle("Context and details");
                ct.showMaximized();
                ct.show();
                a.exec();
                if (ct.getSaida() == true)
                    return 0;
                else goto inicio;
            }
creditos:   {
                Credits cd;
                cd.setWindowTitle("Credits");
                cd.showMaximized();
                cd.show();
                qDebug() << "Credits";
                a.exec();
                if (cd.getSaida() == true)
                    return 0;
                else goto inicio;
            }
}

To copy to clipboard, switch view to plain text mode 

A example of the basic code used in the getSaida() function (the same for all of them):

Context::Context(QWidget *parent) :
    QMainWindow(parent),
    ui(new Ui::Context)
{
    ui->setupUi(this);
    continua = false;
}
 
Context::~Context()
{
    delete ui;
}
 
void Context::on_actionClose_triggered()
{
    close();
}
 
void Context::on_actionExit_triggered()
{
    continua = true;
    close();
}
 
bool Context::getSaida()
{
    return continua;
}

Context::Context(QWidget *parent) :
    QMainWindow(parent),
    ui(new Ui::Context)
{
    ui->setupUi(this);
    continua = false;
}

Context::~Context()
{
    delete ui;
}

void Context::on_actionClose_triggered()
{
    close();
}

void Context::on_actionExit_triggered()
{
    continua = true;
    close();
}

bool Context::getSaida()
{
    return continua;
}

To copy to clipboard, switch view to plain text mode 

Note: the NovoMain is a QDialog with a respective QPushButton for each of the possible MainWindow in the code (SPosition, Body, etc.) and, when one of the QPushButton is clicked, it closes the QDialog and return a specific int catch by "decisao".

My question is: is there a better way to do this? Is there a function or something like that that can replace my goto methodology??



Thanks!

Momergil

[ChrisW67]

You can start by using a switch statement to eliminate the if() and goto collection.

[bah]

As suggested, use a switch statement. I would also use an enumerator instead of raw integers to improve code readability. Something like:

switch (decisao){
    case Decisao::Positions:
         ...
}

switch (decisao){
    case Decisao::Positions:
         ...
}

To copy to clipboard, switch view to plain text mode 

[marcvanriet]

Nowadays, you would get shot, then hanged, and then crucified for using the 'goto' statement.

Use loops with terminating conditions and 'switch' instead.

[Momergil]

Chris,

thanks for the typ.

As suggested, use a switch statement. I would also use an enumerator instead of raw integers to improve code readability. Something like:

switch (decisao){
    case Decisao::Positions:
         ...
}

switch (decisao){
    case Decisao::Positions:
         ...
}

To copy to clipboard, switch view to plain text mode 

bah, could you please give me more details about your suggestion with switch? I know how to use this with int values (which I presume was ChrisW67 idea), but not as your code implements.

marcvanriet, ok, I understood ^^ Normally I do use your suggestions, but some algorithms that are uncommon to me sometimes make me use goto and so forth.


Thanks for everyone,

Momergil.

[bah]

Here's a code sample (there are better enum examples out there), though you might want to change the numbering (I've kept your scheme, though if feels weird starting at 1). The FINAL_DECISAO is there in case you want to use it in a loop somewhere (and should be updated to point to the right enum entry). Also, you should be careful with namespace pollution. Obs: you don't need to explicitly define each value, the enum is supposed to increment by one at each subsequent enum entry (I recommend defining the first one,as some compilers start them at 0 others at 1).

namespace Decisao
{
     // FINAL_DECISAO must match the last enumerator entry
      enum inertiaEnum {
          Positions     =  1,
          Body          =  2,
          Context       =  3,
          Creditos      =  4,
          FINAL_DECISAO =  creditos
      };
//... whatever else is on this namespace
}

namespace Decisao
{
     // FINAL_DECISAO must match the last enumerator entry
      enum inertiaEnum {
          Positions     =  1,
          Body          =  2,
          Context       =  3,
          Creditos      =  4,
          FINAL_DECISAO =  creditos
      };
//... whatever else is on this namespace
}

To copy to clipboard, switch view to plain text mode 

You can then use the enum as you would use the hard coded int, except you won't need to remember what each number means. It will also allow you to change the numbering without refactoring the entire code.

switch (decisao){
    case Decisao::Positions:
         //do what should be done when Positions is selected
    case Decisao::Body:
         //do what should be done when Body is selected
    case Decisao::Context:
         //do what should be done when Context is selected
    case Decisao::Creditos
         //do what should be done when Creditos is selected
    default:
         // do what should be done when none of the others is selected
 
}

switch (decisao){
    case Decisao::Positions:
         //do what should be done when Positions is selected
    case Decisao::Body:
         //do what should be done when Body is selected
    case Decisao::Context:
         //do what should be done when Context is selected
    case Decisao::Creditos
         //do what should be done when Creditos is selected
    default:
         // do what should be done when none of the others is selected

}

To copy to clipboard, switch view to plain text mode 

Qt4 replaced many of Qt3 boolean function parameters for enums so you could actually know what it meant. One such example (I'm pretty sure there was something like this in a blog post about Qt API design somewhere):

// Qt4 replacing a case sensitive == false flag for an enumerator
exampleString.replace("%USER%", user, false);                 // Qt 3
exampleString.replace("%USER%", user, Qt::CaseInsensitive); // Qt 4

// Qt4 replacing a case sensitive == false flag for an enumerator
exampleString.replace("%USER%", user, false);                 // Qt 3
exampleString.replace("%USER%", user, Qt::CaseInsensitive); // Qt 4

To copy to clipboard, switch view to plain text mode 

[boudie]

And don't forget to 'break':

switch (decisao){
    case Decisao::Positions:
         //do what should be done when Positions is selected
         break;              // <<<<<<<<<<<<<<<<<<<<<<
    case Decisao::Body:
    .... etc

switch (decisao){
    case Decisao::Positions:
         //do what should be done when Positions is selected
         break;              // <<<<<<<<<<<<<<<<<<<<<<
    case Decisao::Body:
    .... etc

To copy to clipboard, switch view to plain text mode 

[wysota]

In my opinion each case in the switch should just call a method/function that does a particular job instead of embedding the code directly in the switch.

while(!endOfProgram) {
  int decisiao = NM.executa();
  switch(decisiao) {
    case Positions: endOfProgram = executePositions(); break;
    case Body: endOfProgram = executeBody(); break;
    // ...
  }
}

while(!endOfProgram) {
  int decisiao = NM.executa();
  switch(decisiao) {
    case Positions: endOfProgram = executePositions(); break;
    case Body: endOfProgram = executeBody(); break;
    // ...
  }
}

To copy to clipboard, switch view to plain text mode 

Then each method can return true or false depending on whether the application should exit the loop or not.

[Momergil]

Hello everyone!

Thanks for the help. I used almost all of the typs you gave me and now the code is running fine and, of course, it is much more organized.

Here it is:

int decisao;
bool continuaounao = false;
 
namespace Decisao
{
    enum inertiaEnum
    {
        Positions       = 1,
        Body            = 2,
        Context         = 3,
        Creditos        = 4
    };
}
 
int main(int argc, char *argv[])
{
    QApplication a(argc, argv);
 
    NovoMain NM;
 
    while (continuaounao == false)
    {
        decisao = NM.executa();
 
        switch (decisao)
        {
        case Decisao::Positions:
            {
                SPositions sp;
                sp.setWindowTitle("Positions");
                sp.showMaximized();
                sp.show();
                a.exec();
                continuaounao = sp.getSaida();
            }
                break;
 
        case Decisao::Body:
            {
                Body bd;
                bd.setWindowTitle("Body knowledge");
                bd.showMaximized();
                bd.show();
                a.exec();
                continuaounao = bd.getSaida();
            }
                break;
 
        case Decisao::Context:
            {
                Context ct;
                ct.setWindowTitle("Context and details");
                ct.showMaximized();
                ct.show();
                a.exec();
                continuaounao = ct.getSaida();
            }
                break;
 
        case Decisao::Creditos:
            {
                Credits cd;
                cd.setWindowTitle("Credits");
                cd.showMaximized();
                cd.show();
                a.exec();
                continuaounao = cd.getSaida();
            }
                break;
 
        default:
            return 0;
            break;
        }
    }
    return 0;
}

int decisao;
bool continuaounao = false;

namespace Decisao
{
    enum inertiaEnum
    {
        Positions       = 1,
        Body            = 2,
        Context         = 3,
        Creditos        = 4
    };
}

int main(int argc, char *argv[])
{
    QApplication a(argc, argv);

    NovoMain NM;

    while (continuaounao == false)
    {
        decisao = NM.executa();

        switch (decisao)
        {
        case Decisao::Positions:
            {
                SPositions sp;
                sp.setWindowTitle("Positions");
                sp.showMaximized();
                sp.show();
                a.exec();
                continuaounao = sp.getSaida();
            }
                break;

        case Decisao::Body:
            {
                Body bd;
                bd.setWindowTitle("Body knowledge");
                bd.showMaximized();
                bd.show();
                a.exec();
                continuaounao = bd.getSaida();
            }
                break;

        case Decisao::Context:
            {
                Context ct;
                ct.setWindowTitle("Context and details");
                ct.showMaximized();
                ct.show();
                a.exec();
                continuaounao = ct.getSaida();
            }
                break;

        case Decisao::Creditos:
            {
                Credits cd;
                cd.setWindowTitle("Credits");
                cd.showMaximized();
                cd.show();
                a.exec();
                continuaounao = cd.getSaida();
            }
                break;

        default:
            return 0;
            break;
        }
    }
    return 0;
}

To copy to clipboard, switch view to plain text mode 

Thanks for everyone!


God bless,

Momergil