Replacing the goto Statement

Question

I have a very limited experience with C++ and i wanted to replace the "goto" construct from my code. Any suggestion for refactoring too

int main()
{ 
    int count;
    int countSub = 0;
    int userCount = 0;
    int roleCount = 0;
    int parentGroup;
    cout<<"enter a number of parentGroup"<< endl;
    cin>> parentGroup;
    int subGroup;
    cout<<"enter a number sub Group"<< endl;
    cin>> subGroup;
    int rolePerGroup;
    cout<<"enter a number role per Sub Group"<< endl;
    cin>> rolePerGroup;
    int userPerGroup;
    cout<<"enter a number user per Role"<< endl;
    cin>> userPerGroup;

    do
    {
        if (parentGroup == 0) 
        { 
            cout<<"Error"<<endl;
            exit(EXIT_FAILURE);
        }
        else
        {

            for(count=1;count <= parentGroup; count ++)
            {
                do
                {
                    if(subGroup == 0) goto hello;

                    else
                    {
                        for(countSub = 1;countSub<=subGroup; countSub ++)
                        { 
                            do
                            {
                                hello: 
                                if (rolePerGroup == 0)
                                {
                                    cout<<"Error"<<endl;
                                    exit(EXIT_FAILURE);
                                }

                                else
                                {

                                    for(roleCount = 1; roleCount<=rolePerGroup; roleCount ++)
                                    { 

                                        do
                                        {
                                            if(userPerGroup == 0) goto print;
                                            else
                                            {

                                                    for(userCount = 1; userCount<=userPerGroup; userCount ++)
                                                {

                                                        print:
    cout<<"Parent groups are: "<< count <<" | "<<"Sub group are : "<<countSub<<" | "<<"Role per Sub group are : "<< roleCount <<" | "<<"User per role are : "<< userCount <<endl;
                                                }}
                                            userCount --;
                                        }while(userCount < 0);
                                    }}
                                roleCount --;
                            }while(roleCount < 0);
                        }}
                    countSub --;
                }while(countSub < 0);
            }}
        count --;
    }while(count < 0);
}

My first suggestion would be to split this into multiple functions. That's what 6 nest for loops? — Borgleader, Jul 17 '13 at 16:28
Create a function of whatever is being done at print and call that function instead of `goto`. — Vite Falcon, Jul 17 '13 at 16:29
Just out of morbid curiosity, what are you reading that led you to this? — Bill the Lizard, Jul 17 '13 at 16:33
Wonderful. If subGroup is 0, then you go in the for loop anyway. This way, if subGroup is 0, it's like having subGroup equal to 1. :D — Maxime Chéramy, Jul 17 '13 at 16:33
Why do you have both a do/while loop and a for loop for each counter variable, nested within each other? The do/while loops don't make any sense (they all do something like `do { ... count--; } while (count<0);` which does not seem correct). — interjay, Jul 17 '13 at 16:34
I think this was surely not why Linus Torvalds said "goto is not always wrong"... — , Jul 17 '13 at 16:34
just working on school project that create a group with sub group, role per group under sub group or parent group and users under the role — ephti2000, Jul 17 '13 at 16:36
I think of adding function but more complicated for me too call the function and loop them ..... don't know C++ that much — ephti2000, Jul 17 '13 at 16:38
@Yve Thanks for the advise thats what i'm trying here to have support and idea lol — ephti2000, Jul 17 '13 at 16:42
@interjay my idea of using the do...while is even if the sub group is zero i want the loop to continue only will stop if parentGroup and role per group is 0 — ephti2000, Jul 17 '13 at 16:44

score 0 · Answer 1 · answered Jul 17 '13 at 16:37

One of the goto you want to remove:

if(subGroup == 0) goto hello;
else
{
    for(countSub = 1;countSub<=subGroup; countSub ++) {
        do {
           hello: 
...

So if I understand correctly, you want that if subGroup equals 0 or 1 then you do 1 iteration. The goto is to enter in the for-loop once.

I'd suggest you use a variable I will call N (not a good name) and do:

int N = subGroup == 0 ? 1 : subGroup;
for(countSub = 1;countSub <= N; countSub++) {

I can't see anymore, I won't try to fix the other one but I guess it's the same issue.

score 0 · Accepted Answer · answered Jul 17 '13 at 18:57

Please let me know if I missed something

#include "stdafx.h"
#include <iostream>

using namespace std;


void Print(int count, int countSub, int rolePerGroup, int userCount, int userPerGroup)
{
for(int roleCount = 1; roleCount<=rolePerGroup; roleCount ++)
{ 
    if(userPerGroup == 0) 
    {
        cout<<"Parent groups are: "<< count <<" | "<<"Sub group are :     "<<countSub<<" | "<<"Role per Sub group are : "<< roleCount <<" | "<<"User per role are : "<< userCount <<endl;
        continue;
    }

    for(userCount = 1; userCount<=userPerGroup; userCount ++)
        cout<<"Parent groups are: "<< count <<" | "<<"Sub group are : "<<countSub<<" | "<<"Role per Sub group are : "<< roleCount <<" | "<<"User per role are : "<< userCount <<endl;
}
}

int main()
{ 
int userCount = 0;
int roleCount = 0;
int parentGroup;
cout<<"enter a number of parentGroup"<< endl;
cin>> parentGroup;

if (parentGroup == 0) 
{ 
    cout<<"Parent Group should not be zero"<<endl;
    exit(EXIT_FAILURE);
}

int subGroup;
cout<<"enter a number sub Group"<< endl;
cin>> subGroup;
int rolePerGroup;
cout<<"enter a number role per Sub Group"<< endl;
cin>> rolePerGroup;

if (rolePerGroup == 0)
{
    cout<<"Role per Group should not be zero"<<endl;
    exit(EXIT_FAILURE);
}

int userPerGroup;
cout<<"enter a number user per Role"<< endl;
cin>> userPerGroup;


for(int count=1;count <= parentGroup; count ++)
{
    if(subGroup == 0) 
    {
        Print( count, 0, rolePerGroup, userCount, userPerGroup);
        continue;
    }

    for(int countSub = 1;countSub<=subGroup; countSub ++)
    { 
        Print( count, countSub, rolePerGroup, userCount, userPerGroup);
    }
}
}

no it work perfectly but i want the output for the "user per role are: 1..." instead of printing repeated numbers i want a contentious one , for example if we input 2,2,2,2 i want the user part to be printed 1 to 16 ... can you have any idea how to do it — ephti2000, Jul 29 '13 at 16:34

Replacing the goto Statement

2 Answers2