I've written a simple numeric menu which displays in the console. When the user types in '1', something happens. When the user hits ENTER, I clear the whole output except for the menu itself. When the user types in '5', the program ends.
However, the code got ugly. It's full of cin.get()
s, cout
s, and other stuff.
int main()
{
using namespace std;
node *root = NULL;
char ch = '0';
while(ch != '5')
{
cout << "1. Add new elements\n"
"2. Display info about the tree\n"
"3. Remove nodes\n"
"4. Export to file\n"
"5. Exit\n";
cin >> ch; cin.get();
switch(ch)
{
case '1': case '3':
{
vector<int> numbers;
string line;
cout << "\nProvide numbers divided with spaces: ";
getline(cin, line);
istringstream in(line, istringstream::in);
int current;
while (in >> current) numbers.push_back(current);
(ch == '1') ? insert(root, numbers) : remove(root, numbers);
cout << "\nClick to return to menu";
cin.get();
break;
}
case '2':
{
displayTreeInfo(root, cout);
cout << "\n\nClick to return to menu";
cin.get();
break;
}
case '4':
{
string name;
cout << "Provide filename: ";
cin >> name;
buildTree(root, loadNumbersFromFile(name));
exportTreeToFile(root, name);
std::cout << "\nData has been exported.\n\n";
cin.get(); cin.get();
break;
}
}
system("cls");
}
exit(EXIT_SUCCESS);
}
How could I improve this code?
1 Answer 1
Using namespace std
: I've already mentioned this in the comments. Although it's not a huge deal for small programs such as this, it's still not a good habit to develop.While
loop: Putstd::cin << ch
both before the loop and after theswitch
. You could also change the loop condition to(ch == '1' || ch == '2' || ch == '3' || ch == '4')
if you really want to make sure that the program will only proceed with a valid input. Other than that, your original condition is okay.
std::cin
: You don't needstd::cin.get()
alongsidestd::cin >>
. Just usestd::cin >>
forchar
inputs. However, you'll need astd::cin.ignore()
before eachgetline
since you're doing achar
input (menu prompt) before astd::string
input in your cases. Forcase
4, keep a consistentgetline
for the filename input and put anignore
before it.Also, you don't need any
cin
s after your cases since yourwhile loop
will always ask for an input forch
. I do see what you're doing with your outputs in your cases, but you don't need to do that. Just let your cases do their things and go straight to the menu and/or prompt again.Filename input: You should provide validation for the filename input. If the user enters in invalid filename, what should happen? Consider implementing a loop asking the user for a valid filename as long as such file cannot be opened. With the provided code here, it's a bit tough for me to give a piece of example code for that.
Functions: You could nicely organize your
switch
statements by putting the body code into functions. Just name them as they appear in your menu. Call each one in your respective cases, along with the breaks. Also, it might be better to have separate procedures for add/remove.Exit: You don't really need
exit(EXIT_SUCCESS)
here if the program will always terminate successfully. You also don't needsystem("cls")
, so just leave it out. If you're doing that just to prevent the menu from appearing before each prompt, just pt it before thewhile
loop.
-
\$\begingroup\$ You're welcome. I did leave out something, so I'll add it in. If you're still curious about the filename input validation, you can always request an idea on Stack Overflow. \$\endgroup\$Jamal– Jamal2013年04月20日 20:20:21 +00:00Commented Apr 20, 2013 at 20:20
using namespace std
inside ofmain
? \$\endgroup\$