Why isn't my condition working for my do/while loop? C++

Question

I have my loop set up as seen below:

#include <iostream>
#include <string>
#include <vector>
#include "Agent.h"
#include "Sage.h"
#include "Sova.h"
#include "Reyna.h"

using namespace std;

int main() {
    int choice;
    vector <Agent*> v;


    do
    {
        cout << "Choose an Agent to Reveal Agent Ability" << endl;
        cout << "---------------------------------------" << endl;
        cout << "1. Sage" << endl;
        cout << "2. Sova" << endl;
        cout << "3. Reyna" << endl;
        cout << "4. Display All" << endl;
        cout << "5. Quit" << endl;
        cin >> choice;

        switch (choice)
        {
            case 1:
                v.push_back(new Sage("Healing"));
                break;

            case 2:
                v.push_back(new Sova("Sight"));
                break;

            case 3:
                v.push_back(new Reyna("Blinding"));
                break;
            case 4:
                v.push_back(new Sage("Healing"));
                v.push_back(new Sova("Sight"));
                v.push_back(new Reyna("Blinding"));
                break;
            default:
                cout << "Bad choice! Please try again later.\n";
        }
    } while (choice <=0 || choice >=5);

    for (const auto &Agent : v){
        Agent->action();
    }
    return 0;
}

My condition is while (choice <=0 || choice >=5)

However, when I run this, after I make a choice, the information is output onto the screen and then the program ends. I tried other conditions, but when I selected a choice, the program will loop but will not output any information.

Is this a problem with the position for my for loop?

for (const auto &Agent : v){
    Agent->action();
}

Edit: Here's an example of the output I get when I use something like (choice != 5):

Choose an Agent to Reveal Agent Ability
---------------------------------------
1. Sage
2. Sova
3. Reyna
4. Display All
5. Quit
1
Choose an Agent to Reveal Agent Ability
---------------------------------------
1. Sage
2. Sova
3. Reyna
4. Display All
5. Quit

As you can see, it loops, but it does not display the output.

Did you make a choice that was less than 0 or greater than 5? If you did not the program will end after whatever happens in your range based for loop. — drescherjm, Oct 23 '20 at 21:50
That also. There should be a `case 5: break;` to avoid the bad choice message on exit. — drescherjm, Oct 23 '20 at 21:56
your condition works, `when I run this after I make a choice, the information is output onto the screen and then the program ends`, it's your code — Jérôme Teisseire, Oct 23 '20 at 21:57
totally unrelated: [Look up `std::unique_ptr`](https://en.cppreference.com/w/cpp/memory/unique_ptr) and see if a `vector > v;` is a better fit for your use-case. — user4581301, Oct 23 '20 at 21:57
@drescherjm the thing is, i've already tried something like this and it does loop the menu but it will not output the information i want — mozuna495, Oct 23 '20 at 21:58
Tactical note: `for (const auto &Agent : v){` reuses the identifier `Agent`. You'll get away with it here, but if you need to use `Agent`-the-type, inside the loop, you're in for a nasty surprise. — user4581301, Oct 23 '20 at 22:00
Then you will have to tell the output you want because whatever it is it contradicts the menu you have in your code. In your menu 5. is supposed to end the loop. — drescherjm, Oct 23 '20 at 22:01
You put the output after all menu choices are done. The only way you see the output you have to press 5 and then it will show all of the output of any choices you made before exiting — drescherjm, Oct 23 '20 at 22:03
@drescherjm okay thank you. this is what I assumed it was! i moved the code for the output into my do/while loop and that seemed to solve the problem. — mozuna495, Oct 23 '20 at 22:06
Yes if you want to see the output after each choice you need to move the range based for loop before `} while (choice != 5);` and also clear the vector after the range based loop. — drescherjm, Oct 23 '20 at 22:07
When the user enters a valid choice, you add those items to the `vector` and then exit the menu loop, displaying the contents of the `vector` and exiting the program. As long as the user enters invalid choices, you keep looping the menu. What is the *desired* output supposed to look like for each menu item chosen? Are you hoping to loop back to the menu again after each choice? Display the `vector` on each menu choice? Because that is not how the code is currently written to work. — Remy Lebeau, Oct 23 '20 at 22:08
@drescherjm thank you, that was my next problem and I'm glad you mentioned it. Thanks for the help! — mozuna495, Oct 23 '20 at 22:09

score 0 · Answer 1 · answered Oct 23 '20 at 22:27

You have a couple problems. For one, you don't handle the "quit" case, it just keeps looping then because the loop condition choice >= 5 is satisfied. Also you do not handle input that is not an integer. Additionally, you have a name collision with Agent. Also, you have what could potentially become a memory leak by not deleting the Agents. I know the memory is recouped when the process exits anyway, but it's good practice to delete raw pointers even when you don't have to, and it's even better practice not to use raw pointers in the first place, and to use smart pointers like shared_ptr instead, so the memory is automatically deleted when the objects go out of scope.

#include <iostream>
#include <string>
#include <vector>
#include <memory>
#include "Agent.h"
#include "Sage.h"
#include "Sova.h"
#include "Reyna.h"

using namespace std;

int main() {
    int choice;
    vector <std::shared_ptr<Agent> > v;

    do
    {
        cout << "Choose an Agent to Reveal Agent Ability" << endl;
        cout << "---------------------------------------" << endl;
        cout << "1. Sage" << endl;
        cout << "2. Sova" << endl;
        cout << "3. Reyna" << endl;
        cout << "4. Display All" << endl;
        cout << "5. Quit" << endl;
        
        if (!(cin >> choice)) {
            std::cout << "Please enter an integer." << std::endl;
            choice = -1; // so the loop condition is true and we reloop
            continue;
        }

        switch (choice)
        {
            case 1:
                v.push_back(std::make_shared<Sage>("Healing"));
                break;

            case 2:
                v.push_back(std::make_shared<Sova>("Sight"));
                break;

            case 3:
                v.push_back(std::make_shared<Revna>("Blinding"));
                break;
            case 4:
                v.push_back(std::make_shared<Sage>("Healing"));
                v.push_back(std::make_shared<Sova>("Sight"));
                v.push_back(std::make_shared<Reyna>("Blinding"));
                break;
            case 5:
                std::cout << "Quitting." << std::endl;
                return 0;
                break;
            default:
                cout << "Bad choice! Please try again later.\n";
        }
    } while (choice < 1 || choice > 5);

    for (const auto & agent_ptr : v){
        agent_ptr->action();
    }
    return 0;
}

Good practice is also to not use using namespace std;. See: Why is "using namespace std;" considered bad practice? . You can see where I edited your code, I added in std:: out of force of habit.

From the comments the OP wanted to run the range based for loop inside the other loop to see the result after each step. Also it was desired to clear the vector after printing. — drescherjm, Oct 23 '20 at 23:36
Also `} while (choice != 5);` was the desired condition for the loop. — drescherjm, Oct 23 '20 at 23:37

Why isn't my condition working for my do/while loop? C++

1 Answers1