3
\$\begingroup\$

I just figured out how to do a building menu with a progression feature in c++ console application.

What this code does:

It simply offers you a choice to build a building and then prints the progression like: 1% - 9% erasing and printing one number at a time (Which is the kind of the core feature that I struggled a lot with).

then prints "Building done".

I would like to have some thoughts and comments on how to:

  1. Make this code simpler in any way .
  2. Did someone else make the same feature and how did you implement it?
  3. General thoughts on the code
  4. How would you structure it?

std::string building;

std::cout << "-CONSTRUCTION MENU-" << std::endl << std::endl;

std::cout << "1. Barrack" << std::endl << std::endl;

std::cout << "Build: ";

std::cin >> building;

if (building == "1" || building == "Barrack" || building == "barrack")
{
    std::string sentence[10] = { "1%", "2%","3%", "4%", "5%", "6%", "7%", "8%", "9%" };

    for (int index = 0; sentence[index] != sentence[9]; index++)
    {
        std::cout << sentence[index] << std::flush;
        std::cout << "\b\b";
        Sleep(150);

        if (index == 8)
        {
            system("cls");
            std::cout << "Building done" << std::endl;
            system("pause");
            break;
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ This kind of implies that 10% means that it's done, or that the remaining 90% progresses instantaneously. \$\endgroup\$ – 200_success Feb 21 at 13:13
  • \$\begingroup\$ "How would you structure it?" Well, that depends on what else you're planning to add. Is this going to be a CLI interface to a Red Alert game? How many buildings are you going to support? Are you going to support more than just buildings? You're in desperate need of some kind of mapping, something to be able to select the next building as well without the program shutting down on you and a decoupling of the input validation to the rest of the program. But at the moment there's not much of a feature yet, so I'm wondering what you mean with point 2. \$\endgroup\$ – Mast Feb 21 at 13:59
  • \$\begingroup\$ I bet you only go up to 9% because you delete exactly two characters. Why not print \b for each character you wrote? std::string has a nice size() function... Also, you should consider sleeping before deleting, now you print and delete immediately, then pause, so you can't really see what was printed. \$\endgroup\$ – Cris Luengo Feb 21 at 19:28
  • \$\begingroup\$ @200_success yes it is supponera to implicerar that 10% is done but can easiky be changed. Was convenient for testing. :) \$\endgroup\$ – Cworm Feb 21 at 21:02
  • \$\begingroup\$ @Mast Hmm im thinking of making a CLI game. But my next thing would be to have the building-progression going on and be able to go back to Other menus and do other stuff at the same time, then go back to the ”building menu” and check the progression. Any suggestions there? ;) \$\endgroup\$ – Cworm Feb 21 at 21:10
5
\$\begingroup\$

There are several things you can do to improve this code.

First of all the code above isn't a valid C++ code. To run it you need to add:

#include <string>
#include <iostream>
#include <Windows.h>

int main()
{
    ... // your code posted above
}

Please next time you ask a question post the full code.

Then your Menu:

std::cout << "-CONSTRUCTION MENU-" << std::endl << std::endl;

std::cout << "1. Barrack" << std::endl << std::endl;

std::cout << "Build: ";

Consider writing it like this:

std::cout << "-CONSTRUCTION MENU-\n\n"
<< "1. Barrack\n\n"
<< "Build: ";
  • \n should be used instead of std::endl. The reason is std::endl not only gives you a new line, it also does an expensive buffer flushing operation which is rarely necessary.

  • No need to call std::cout several times after each other. Statements can be chained.

Then your Percentage display:

std::string sentence[10] = { "1%", "2%","3%", "4%", "5%", "6%", "7%", "8%", "9%" };

for (int index = 0; sentence[index] != sentence[9]; index++)
{
    ...// statements inside
}

Several things are wrong here:

  • You are using a plain C array to hold the sentence variables. This is very bad because a C array does not know about its size. Instead you use the magic number '9' to finish iterating over the strings. Please study the default C++ containers std::array or std::vector. In this case I would go for array since you don't want to add more values at run time.
  • The size is wrong. You declared space for 10 elements but only put in 9.
  • sentence is a very bad name. You don't store sentences. Consider using percentages instead.

Now we can write this instead:

std::array<std::string, 9> percentages = { "1%", "2%","3%", "4%", "5%", "6%", "7%", "8%", "9%" };

for (const auto& sentence : sentences)
{
    ...// statements inside
}

So the world is OK now? Not at all. You shouldn't use an array for numbers in the first place. Consider building the numbers in a loop instead:

for (int i = 1; i <= 9; ++i) {
    std::cout << std::to_string(i) << '%' << "\b\b";
    Sleep(150);
}

I removed the std::flush here since I don't see its purpose.

As a side hint it is a good practice to use ++i instead of i++. If you use objects later instead of ints you save an expensive copy.

Now we come to another dark chapter of your snippet. Your program is mixed with non-portable statements:

Sleep(150);

Consider using std facilities here:

#include <chrono>
#include <thread>

...


std::this_thread::sleep_for(std::chrono::milliseconds(150));

The big advantage here is that this is portable now.

The next candidate is:

system("pause");

Since you just want the program to wait with a message at the end you can do this:

std::cout << "Press any key...";
std::cin.get();

The last statement we get to is this:

system("cls");

No easy answer here. The fact is that the standard does not provide a portable solution here. You could wrap the statement in a function and make it portable for different platforms like this:

void clear_screen()
{
#if defined _WIN32
    std::system("cls");
#elif defined __unix__
    std::system("clear");
#elif defined (__APPLE__)
    std::system("clear");
#endif
}

We should also replace the

#include <windows.h> 

with

#include <cstdlib>

So now also on Linux / MAC the clear should work.

As a last word. A function should only do one thing. You should separate the program in well defined parts. Here is my reworked solution with all the improvements already mentioned, dividing the program into parts:

#include <array>
#include <chrono>
#include <cstdlib>
#include <iostream>
#include <string>
#include <thread>

void clear_screen()
{
#if defined _WIN32
    std::system("cls");
#elif defined (__unix__) 
    std::system("clear");
#elif defined (__APPLE__)
    std::system("clear");
#endif
}

std::string get_user_choice()
{
    std::string choice;
    std::cout << "-CONSTRUCTION MENU-\n\n"
        << "1. Barrack\n\n"
        << "Build: ";
    std::cin >> choice;
    return choice;
}

void print_progress(int start, int end)
{
    for (auto i = start; i <= end; ++i) {
        std::cout << std::to_string(i) << '%' << "\b\b";
        std::this_thread::sleep_for(std::chrono::milliseconds(150));
    }
}

void print_building_completed()
{
    clear_screen();
    std::cout << "Building done\n";
    std::cout << "Press any key...";
    std::cin.get();
}

int main()
{
    auto building = get_user_choice();
    if (building == "1" || building == "Barrack" || building == "barrack"){
        print_progress(0, 100);
        print_building_completed();
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.